[FFmpeg-devel] [PATCH 02/42] avcodec/refstruct: Add simple API for refcounted objects

[Andreas Rheinhardt]

Nicolas George:
> Andreas Rheinhardt (12023-09-19):
>> For now, this API is supposed to replace all the internal uses
>> of reference counted objects in libavcodec; "internal" here
>> means that the object is created in libavcodec and is never
>> put directly in the hands of anyone outside of it.
>>
>> It is intended to be made public eventually, but for now
>> I enjoy the ability to modify it freely.
>>
>> Several shortcomings of the AVBuffer API motivated this API:
> 
> <snip>
> 
> Thanks for stating qualms that I have had for a long time about this
> API.
> 
> An API like this one would have been really useful when the new channel
> layout structure was integrated. On a brighter note, it will be really
> useful if we finally turn the library's global state into a structure
> that can exist in multiple instances.
> 
>> 					      This brings with it
>> one further downside: It is not apparent from the pointer itself
>> whether the underlying object is managed by the refstruct API
>> or whether this pointer is a reference to it (or merely a pointer
>> to it).
> 
> I do not count it as a downside: it is just how the C language is
> supposed to work. When we get a pointer, there is nothing written on it
> that says whether we are supposed to free(), av_free(), g_object_unref()
> it or just do nothing. People who cannot track static pointer ownership
> in their sleep should do Java.
> 

I agree that this is not a major issue; I just wanted to be honest and
mention it in the commit message.

> Also, you probably do not remember, three years ago I started proposing
> a similar idea, and it got bikeshedded to death:
> 
> http://ffmpeg.org/pipermail/ffmpeg-devel/2020-June/265227.html
> 
> The difference is:
> 
> You make this API universal using void pointers and callbacks, and you
> make it convenient by hiding the internals using pointer arithmetic.
> 
> I achieve more or less the same using a template to generate code, with
> the extra benefit that it is type-safe.
> 

1. I do not agree that your approach simplifies creating refcounted
structures.
Think of e.g. the H.264 and HEVC parameter sets patches in this series:
Using your approach I would need to include this header five times, each
time preceded by custom defines. I would also need to give the
referencing and unreferencing functions external linkage and therefore
add them to the headers (this is not true for init_ref_count and
get_ref_count (which would be unused in this scenario), so your template
would need to be updated to enable this usecase, further complicating
it). This is more boilerplate code than both current master and vastly
more than my approach.
(Also notice that in this case the references are of type pointer to
const, so the ref and unref functions would need to accept such
pointers, yet the init_ref_count and free functions would not, so this
means your template would need to be a bit more complex again (although
with sane defaults this can be made to affect only users that
const-references).)
2. Your solution lacks universality and this makes it even more unusable
for e.g. CBS: To support it with your approach one would basically have
to add a structure like from Anton's counterproposal (or a structure
like RefCount from refstruct.c) to the beginning of every CBS unit
context, which would be very instrusive and ugly (unless you hide it
like RefStruct does).
3. Your solution to the indirection caused by the custom free callbacks
basically amounts to "Add custom unref function for every refcounted
structure and inline the freeing code into it". I don't think that this
indirection matters and agree with Anton's "If you constantly alloc and
free objects in tight loops then you have way bigger problems than a
single pointer dereference", but I also want to add that this adds more
code into the binary.
4. I assume that if you extended your proposal to pools, we would end up
with typed pools where every callback is inlined and lots of code in the
binary will be duplicated. I don't like this result.
5. With both your and Anton's approach one would need to allocate a
structure oneself, followed by initializing the refcount. In this API
this is only one action, making every creation simpler.
6. I also like that RefStruct completely hides its internals, whereas
your solution needs an atomic integer in the structure.

To sum it up: I don't consider indirection to be an issue and the
type-unsafety is also not really worrisome. The gain in type-safety from
custom referencing and unreferencing functions would be far outweighed
by the boilerplate code to create them as well as the space said
boilerplate code will take up in the binary.

> I see a few places in the code below where you pay the price for your
> design choice. I think you should consider taking a few ideas from my
> version.

Could you be more specific? Is it just the inherent type-unsafety and
the indirection caused by function pointers?

- Andreas