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

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:
a) The unnecessary allocations (and ensuing error checks)
when using the API. Besides the need for runtime checks it
imposes upon the developer the burden of thinking through
what happens in case an error happens. Furthermore, these
error paths are typically not covered by FATE.
b) The AVBuffer API is designed with buffers and not with
objects in mind: The type for the actual buffers used
is uint8_t*; it pretends to be able to make buffers
writable, but this is wrong in case the buffer is not a POD.
Another instance of this thinking is the lack of a reset
callback in the AVBufferPool API.
c) The AVBuffer API incurs unnecessary indirections by
going through the AVBufferRef.data pointer. In case the user
tries to avoid this indirection and stores a pointer to
AVBuffer.data separately (which also allows to use the correct
type), the user has to keep these two pointers in sync
in case they can change (and in any case has two pointers
occupying space in the containing context). See the following
commit using this API for H.264 parameter sets for an example
of the removal of such syncing code as well as the casts
involved in the parts where only the AVBufferRef* pointer
was stored.
d) Given that the AVBuffer API allows custom allocators,
creating refcounted objects with dedicated free functions
often involves a lot of boilerplate like this:
obj = av_mallocz(sizeof(*obj));
ref = av_buffer_create((uint8_t*)obj, sizeof(*obj), free_func, opaque, 0);
if (!ref) {
    av_free(obj);
    return AVERROR(ENOMEM);
}
(There is also a corresponding av_free() at the end of free_func().)
This is now just
obj = ff_refstruct_alloc_ext(sizeof(*obj), 0, opaque, free_func);
if (!obj)
    return AVERROR(ENOMEM);
See the subsequent patch for the framepool (i.e. get_buffer.c)
for an example.

This API does things differently; it is designed to be lightweight*
as well as geared to the common case where the allocator of the
underlying object does not matter as long as it is big enough and
suitably aligned. This allows to allocate the user data together
with the API's bookkeeping data which avoids an allocation as well
as the need for separate pointers to the user data and the API's
bookkeeping data. This entails that the actual allocation of the
object is performed by refstruct, not the user. This is responsible
for avoiding the boilerplate code mentioned in d).

As a downside, custom allocators are not supported, but it will
become apparent in subsequent commits that there are enough
usecases to make it worthwhile.

Another advantage of this API is that one only needs to include
the relevant header if one uses the API and not when one includes
the header or some other component that uses it. This is because there
is no refstruct type analog of AVBufferRef. 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).

Finally, this API supports const-qualified opaque pointees;
this will allow to avoid casting const away by the CBS code.

*: Basically the only exception to the you-only-pay-for-what-you-use
rule is that it always uses atomics for the refcount.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
I am the most unsure about whether to use FFRefStructOpaque
at all or not just a void*; the only beneficiary of this
is CBS where it saves casting one const away.

I am also open to other naming suggestions like RefObject
(RefObj?) for this API.

 libavcodec/Makefile    |   1 +
 libavcodec/refstruct.c | 139 +++++++++++++++++++++++++++++++++++++++
 libavcodec/refstruct.h | 145 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 285 insertions(+)
 create mode 100644 libavcodec/refstruct.c
 create mode 100644 libavcodec/refstruct.h

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.

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.

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.

Regards,

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

Andreas Rheinhardt:
> 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:
> a) The unnecessary allocations (and ensuing error checks)
> when using the API. Besides the need for runtime checks it
> imposes upon the developer the burden of thinking through
> what happens in case an error happens. Furthermore, these
> error paths are typically not covered by FATE.
> b) The AVBuffer API is designed with buffers and not with
> objects in mind: The type for the actual buffers used
> is uint8_t*; it pretends to be able to make buffers
> writable, but this is wrong in case the buffer is not a POD.
> Another instance of this thinking is the lack of a reset
> callback in the AVBufferPool API.
> c) The AVBuffer API incurs unnecessary indirections by
> going through the AVBufferRef.data pointer. In case the user
> tries to avoid this indirection and stores a pointer to
> AVBuffer.data separately (which also allows to use the correct
> type), the user has to keep these two pointers in sync
> in case they can change (and in any case has two pointers
> occupying space in the containing context). See the following
> commit using this API for H.264 parameter sets for an example
> of the removal of such syncing code as well as the casts
> involved in the parts where only the AVBufferRef* pointer
> was stored.
> d) Given that the AVBuffer API allows custom allocators,
> creating refcounted objects with dedicated free functions
> often involves a lot of boilerplate like this:
> obj = av_mallocz(sizeof(*obj));
> ref = av_buffer_create((uint8_t*)obj, sizeof(*obj), free_func, opaque, 0);
> if (!ref) {
>     av_free(obj);
>     return AVERROR(ENOMEM);
> }
> (There is also a corresponding av_free() at the end of free_func().)
> This is now just
> obj = ff_refstruct_alloc_ext(sizeof(*obj), 0, opaque, free_func);
> if (!obj)
>     return AVERROR(ENOMEM);
> See the subsequent patch for the framepool (i.e. get_buffer.c)
> for an example.
> 
> This API does things differently; it is designed to be lightweight*
> as well as geared to the common case where the allocator of the
> underlying object does not matter as long as it is big enough and
> suitably aligned. This allows to allocate the user data together
> with the API's bookkeeping data which avoids an allocation as well
> as the need for separate pointers to the user data and the API's
> bookkeeping data. This entails that the actual allocation of the
> object is performed by refstruct, not the user. This is responsible
> for avoiding the boilerplate code mentioned in d).
> 
> As a downside, custom allocators are not supported, but it will
> become apparent in subsequent commits that there are enough
> usecases to make it worthwhile.
> 
> Another advantage of this API is that one only needs to include
> the relevant header if one uses the API and not when one includes
> the header or some other component that uses it. This is because there
> is no refstruct type analog of AVBufferRef. 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).
> 
> Finally, this API supports const-qualified opaque pointees;
> this will allow to avoid casting const away by the CBS code.
> 
> *: Basically the only exception to the you-only-pay-for-what-you-use
> rule is that it always uses atomics for the refcount.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> I am the most unsure about whether to use FFRefStructOpaque
> at all or not just a void*; the only beneficiary of this
> is CBS where it saves casting one const away.
> 
> I am also open to other naming suggestions like RefObject
> (RefObj?) for this API.
> 
>  libavcodec/Makefile    |   1 +
>  libavcodec/refstruct.c | 139 +++++++++++++++++++++++++++++++++++++++
>  libavcodec/refstruct.h | 145 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 285 insertions(+)
>  create mode 100644 libavcodec/refstruct.c
>  create mode 100644 libavcodec/refstruct.h
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index bf3b0a93f9..7541f38535 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -55,6 +55,7 @@ OBJS = ac3_parser.o                                                     \
>         profiles.o                                                       \
>         qsv_api.o                                                        \
>         raw.o                                                            \
> +       refstruct.o                                                      \
>         utils.o                                                          \
>         version.o                                                        \
>         vlc.o                                                            \
> diff --git a/libavcodec/refstruct.c b/libavcodec/refstruct.c
> new file mode 100644
> index 0000000000..917cf6b7ac
> --- /dev/null
> +++ b/libavcodec/refstruct.c
> @@ -0,0 +1,139 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdatomic.h>
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "internal.h"
> +#include "refstruct.h"
> +
> +#include "libavutil/macros.h"
> +#include "libavutil/mem.h"
> +
> +typedef struct RefCount {
> +    /**
> +     * An uintptr_t is big enough to hold the address of every reference,
> +     * so no overflow can happen when incrementing the refcount as long as
> +     * the user does not throw away references.
> +     */
> +    atomic_uintptr_t  refcount;
> +    FFRefStructOpaque opaque;
> +    void (*free_cb)(FFRefStructOpaque opaque, void *obj);
> +} RefCount;
> +
> +#if __STDC_VERSION__ >= 201112L
> +#define REFCOUNT_OFFSET FFALIGN(sizeof(RefCount), FFMAX3(STRIDE_ALIGN, 16, _Alignof(max_align_t)))
> +#else
> +#define REFCOUNT_OFFSET FFALIGN(sizeof(RefCount), FFMAX(STRIDE_ALIGN, 16))
> +#endif
> +
> +static RefCount *get_refcount(void *obj)
> +{
> +    return (RefCount*)((char*)obj - REFCOUNT_OFFSET);
> +}
> +
> +static void *get_userdata(void *buf)
> +{
> +    return (char*)buf + REFCOUNT_OFFSET;
> +}
> +
> +static void refcount_init(RefCount *ref, FFRefStructOpaque opaque,
> +                          void (*free_cb)(FFRefStructOpaque opaque, void *obj))
> +{
> +    atomic_init(&ref->refcount, 1);
> +    ref->opaque  = opaque;
> +    ref->free_cb = free_cb;
> +}
> +
> +void *ff_refstruct_alloc_ext_c(size_t size, unsigned flags, FFRefStructOpaque opaque,
> +                               void (*free_cb)(FFRefStructOpaque opaque, void *obj))
> +{
> +    void *buf, *obj;
> +
> +    if (size > SIZE_MAX - REFCOUNT_OFFSET)
> +        return NULL;
> +    buf = av_malloc(size + REFCOUNT_OFFSET);
> +    if (!buf)
> +        return NULL;
> +    refcount_init(buf, opaque, free_cb);
> +    obj = get_userdata(buf);
> +    if (!(flags & FF_REFSTRUCT_FLAG_NO_ZEROING))
> +        memset(obj, 0, size);
> +
> +    return obj;
> +}
> +
> +void *ff_refstruct_allocz(size_t size)
> +{
> +    return ff_refstruct_alloc_ext(size, 0, NULL, NULL);
> +}
> +
> +void ff_refstruct_unref(void *objp)
> +{
> +    void *obj;
> +    RefCount *ref;
> +
> +    memcpy(&obj, objp, sizeof(obj));
> +    if (!obj)
> +        return;
> +    memcpy(objp, &(void *){ NULL }, sizeof(obj));
> +
> +    ref = get_refcount(obj);
> +    if (atomic_fetch_sub_explicit(&ref->refcount, 1, memory_order_acq_rel) == 1) {
> +        if (ref->free_cb)
> +            ref->free_cb(ref->opaque, obj);
> +        av_free(ref);
> +    }
> +
> +    return;
> +}
> +
> +void *ff_refstruct_ref(void *obj)
> +{
> +    RefCount *ref = get_refcount(obj);
> +
> +    atomic_fetch_add_explicit(&ref->refcount, 1, memory_order_relaxed);
> +
> +    return obj;
> +}
> +
> +const void *ff_refstruct_ref_c(const void *obj)
> +{
> +    /* Casting const away here is fine, as it is only supposed
> +     * to apply to the user's data and not our bookkeeping data. */
> +    RefCount *ref = get_refcount((void*)obj);
> +
> +    atomic_fetch_add_explicit(&ref->refcount, 1, memory_order_relaxed);
> +
> +    return obj;
> +}
> +
> +void ff_refstruct_replace(void *dstp, const void *src)
> +{
> +    const void *dst;
> +    memcpy(&dst, dstp, sizeof(dst));
> +
> +    if (src == dst)
> +        return;
> +    ff_refstruct_unref(dstp);
> +    if (src) {
> +        dst = ff_refstruct_ref_c(src);
> +        memcpy(dstp, &dst, sizeof(dst));
> +    }
> +}
> diff --git a/libavcodec/refstruct.h b/libavcodec/refstruct.h
> new file mode 100644
> index 0000000000..0086717c17
> --- /dev/null
> +++ b/libavcodec/refstruct.h
> @@ -0,0 +1,145 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVCODEC_REFSTRUCT_H
> +#define AVCODEC_REFSTRUCT_H
> +
> +#include <stddef.h>
> +
> +/**
> + * RefStruct is an API for creating reference-counted objects
> + * with minimal overhead. The API is designed for objects,
> + * not buffers like the AVBuffer API. The main differences
> + * to the AVBuffer API are as follows:
> + *
> + * - It uses void* instead of uint8_t* as its base type due to
> + *   its focus on objects.
> + * - There are no equivalents of AVBuffer and AVBufferRef.
> + *   E.g. there is no way to get the usable size of the object:
> + *   The user is supposed to know what is at the other end of
> + *   the pointer. It also avoids one level of indirection.
> + * - Custom allocators are not supported. This allows to simplify
> + *   the implementation and reduce the amount of allocations.
> + * - It also has the advantage that the user's free callback need
> + *   only free the resources owned by the object, but not the
> + *   object itself.
> + * - Because referencing (and replacing) an object managed by the
> + *   RefStruct API does not involve allocations, they can not fail
> + *   and therefore need not be checked.
> + *
> + * @note Referencing and unreferencing the buffers is thread-safe and thus
> + * may be done from multiple threads simultaneously without any need for
> + * additional locking.
> + */
> +
> +/**
> + * This union is used for all opaque parameters in this API to spare the user
> + * to cast const away in case the opaque to use is const-qualified.
> + *
> + * The functions provided by this API with an FFRefStructOpaque come in pairs
> + * named foo_c and foo. The foo function accepts void* as opaque and is just
> + * a wrapper around the foo_c function; "_c" means "(potentially) const".
> + */
> +typedef union {
> +    void *nc;
> +    const void *c;
> +} FFRefStructOpaque;
> +
> +/**
> + * If this flag is set in ff_refstruct_alloc_ext_c(), the object will not
> + * be initially zeroed.
> + */
> +#define FF_REFSTRUCT_FLAG_NO_ZEROING (1 << 0)
> +
> +/**
> + * Allocate a refcounted object of usable size `size` managed via
> + * the RefStruct API.
> + *
> + * By default (in the absence of flags to the contrary),
> + * the returned object is initially zeroed.
> + *
> + * @param size    Desired usable size of the returned object.
> + * @param flags   A bitwise combination of FF_REFSTRUCT_FLAG_* flags.
> + * @param opaque  A pointer that will be passed to the free_cb callback.
> + * @param free_cb A callback for freeing this object's content
> + *                when its reference count reaches zero;
> + *                it must not free the object itself.
> + * @return A pointer to an object of the desired size or NULL on failure.
> + */
> +void *ff_refstruct_alloc_ext_c(size_t size, unsigned flags, FFRefStructOpaque opaque,
> +                               void (*free_cb)(FFRefStructOpaque opaque, void *obj));
> +
> +/**
> + * A wrapper around ff_refstruct_alloc_ext_c() for the common case
> + * of a non-const qualified opaque.
> + *
> + * @see ff_refstruct_alloc_ext_c()
> + */
> +static inline
> +void *ff_refstruct_alloc_ext(size_t size, unsigned flags, void *opaque,
> +                             void (*free_cb)(FFRefStructOpaque opaque, void *obj))
> +{
> +    return ff_refstruct_alloc_ext_c(size, flags, (FFRefStructOpaque){.nc = opaque},
> +                                    free_cb);
> +}
> +
> +/**
> + * Equivalent to ff_refstruct_alloc_ext(size, 0, NULL, NULL)
> + */
> +void *ff_refstruct_allocz(size_t size);
> +
> +/**
> + * Decrement the reference count of the underlying object and automatically
> + * free the object if there are no more references to it.
> + *
> + * `*objp == NULL` is legal and a no-op.
> + *
> + * @param objp Pointer to a pointer that is either NULL or points to an object
> + *             managed via this API. `*objp` is set to NULL on return.
> + */
> +void ff_refstruct_unref(void *objp);
> +
> +/**
> + * Create a new reference to an object managed via this API,
> + * i.e. increment the reference count of the underlying object
> + * and return obj.
> + * @return a pointer equal to obj.
> + */
> +void *ff_refstruct_ref(void *obj);
> +
> +/**
> + * Analog of ff_refstruct_ref(), but for constant objects.
> + * @see ff_refstruct_ref()
> + */
> +const void *ff_refstruct_ref_c(const void *obj);
> +
> +/**
> + * Ensure `*dstp` refers to the same object as src.
> + *
> + * If `*dstp` is already equal to src, do nothing. Otherwise unreference `*dstp`
> + * and replace it with a new reference to src in case `src != NULL` (this
> + * involves incrementing the reference count of src's underlying object) or
> + * with NULL otherwise.
> + *
> + * @param dstp Pointer to a pointer that is either NULL or points to an object
> + *             managed via this API.
> + * @param src  A pointer to an object managed via this API or NULL.
> + */
> +void ff_refstruct_replace(void *dstp, const void *src);
> +
> +#endif /* AVCODEC_REFSTRUCT_H */

Will apply patches 2-16 (i.e. up to the pool API) tomorrow unless there
are objections.

- Andreas

Andreas Rheinhardt (12023-10-06):
> Will apply patches 2-16 (i.e. up to the pool API) tomorrow unless there
> are objections.

Have you given some thought to using a template to make the API
type-safe directly?

Regards,

Nicolas George:
> Andreas Rheinhardt (12023-10-06):
>> Will apply patches 2-16 (i.e. up to the pool API) tomorrow unless there
>> are objections.
> 
> Have you given some thought to using a template to make the API
> type-safe directly?

Do you mean a template that generates functions like

HEVCVPS *ff_hevc_vps_ref(HEVCVPS *vps)
{
    return ff_refstruct_ref(vps);
}

automatically? I don't think that's worth it.

- Andreas

Andreas Rheinhardt (12023-10-06):
> Do you mean a template that generates functions like
> 
> HEVCVPS *ff_hevc_vps_ref(HEVCVPS *vps)
> {
>     return ff_refstruct_ref(vps);
> }
> 
> automatically?

Yes, but better, directly:

void ff_hevc_vps_unref(HEVCVPS *vps)
{
    if (atomic_fetch_sub_explicit(&(*vps)->AVRC_FIELD, 1, memory_order_acq_rel) == 1) {
        AVRC_FREE(*vps);
        *vps = NULL;
    }
}

Apart from the type safety, I see a significant benefit in having the
free function hard-coded in the unref function rather than having to
spare a pointer for it.

Regards,

Nicolas George:
> Andreas Rheinhardt (12023-10-06):
>> Do you mean a template that generates functions like
>>
>> HEVCVPS *ff_hevc_vps_ref(HEVCVPS *vps)
>> {
>>     return ff_refstruct_ref(vps);
>> }
>>
>> automatically?
> 
> Yes, but better, directly:
> 
> void ff_hevc_vps_unref(HEVCVPS *vps)
> {
>     if (atomic_fetch_sub_explicit(&(*vps)->AVRC_FIELD, 1, memory_order_acq_rel) == 1) {
>         AVRC_FREE(*vps);
>         *vps = NULL;
>     }
> }
> 
> Apart from the type safety, I see a significant benefit in having the
> free function hard-coded in the unref function rather than having to
> spare a pointer for it.
> 

And as I have already explained, I do not think that this benefit is
significant and you have provided no evidence (i.e. an example) that it
is so. Additionally, the flexibility provided by function pointers is
useful for stuff like CBS.

- Andreas

Andreas Rheinhardt (12023-10-06):
> And as I have already explained, I do not think that this benefit is
> significant and you have provided no evidence (i.e. an example) that it
> is so. Additionally, the flexibility provided by function pointers is
> useful for stuff like CBS.

I must have missed that reply, sorry.

Regards,

On 9/19/2023 4:56 PM, Andreas Rheinhardt wrote:
> +
> +/**
> + * Allocate a refcounted object of usable size `size` managed via
> + * the RefStruct API.
> + *
> + * By default (in the absence of flags to the contrary),
> + * the returned object is initially zeroed.
> + *
> + * @param size    Desired usable size of the returned object.
> + * @param flags   A bitwise combination of FF_REFSTRUCT_FLAG_* flags.
> + * @param opaque  A pointer that will be passed to the free_cb callback.
> + * @param free_cb A callback for freeing this object's content
> + *                when its reference count reaches zero;
> + *                it must not free the object itself.
> + * @return A pointer to an object of the desired size or NULL on failure.
> + */
> +void *ff_refstruct_alloc_ext_c(size_t size, unsigned flags, FFRefStructOpaque opaque,
> +                               void (*free_cb)(FFRefStructOpaque opaque, void *obj));
> +
> +/**
> + * A wrapper around ff_refstruct_alloc_ext_c() for the common case
> + * of a non-const qualified opaque.
> + *
> + * @see ff_refstruct_alloc_ext_c()
> + */
> +static inline
> +void *ff_refstruct_alloc_ext(size_t size, unsigned flags, void *opaque,
> +                             void (*free_cb)(FFRefStructOpaque opaque, void *obj))
> +{
> +    return ff_refstruct_alloc_ext_c(size, flags, (FFRefStructOpaque){.nc = opaque},
> +                                    free_cb);
> +}
> +
> +/**
> + * Equivalent to ff_refstruct_alloc_ext(size, 0, NULL, NULL)

Why is this not inlined, then?

> + */
> +void *ff_refstruct_allocz(size_t size);