Re: [FFmpeg-devel] [PATCH 24/42] avcodec/refstruct: Allow to share pools

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2023-10-12 15:51:18)
>> Anton Khirnov:
>>> Quoting Andreas Rheinhardt (2023-09-19 21:57:16)
>>>> To do this, make FFRefStructPool itself refcounted according
>>>> to the RefStruct API.
>>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>>> ---
>>>>  libavcodec/refstruct.c | 29 ++++++++++++++++-------------
>>>>  libavcodec/refstruct.h |  5 ++++-
>>>>  2 files changed, 20 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/libavcodec/refstruct.c b/libavcodec/refstruct.c
>>>> index f8d040874d..2108ff8163 100644
>>>> --- a/libavcodec/refstruct.c
>>>> +++ b/libavcodec/refstruct.c
>>>> @@ -187,7 +187,7 @@ static void pool_free(FFRefStructPool *pool)
>>>>      pthread_mutex_destroy(&pool->mutex);
>>>>      if (pool->free_cb)
>>>>          pool->free_cb(pool->opaque);
>>>> -    av_free(pool);
>>>> +    av_free(get_refcount(pool));
>>>>  }
>>>>  
>>>>  static void pool_free_entry(FFRefStructPool *pool, RefCount *ref)
>>>> @@ -278,13 +278,17 @@ void *ff_refstruct_pool_get(FFRefStructPool *pool)
>>>>      return ret;
>>>>  }
>>>>  
>>>> -void ff_refstruct_pool_uninit(FFRefStructPool **poolp)
>>>> +static void pool_unref(void *ref)
>>>>  {
>>>> -    FFRefStructPool *pool = *poolp;
>>>> -    RefCount *entry;
>>>> +    FFRefStructPool *pool = get_userdata(ref);
>>>> +    if (atomic_fetch_sub_explicit(&pool->refcount, 1, memory_order_acq_rel) == 1)
>>>
>>> Is there a reason you cannot fold pool->refcount into the pool's
>>> containing RefStruct?
>>>
>>
>> If I simply incremented the pool's refcount for every entry currently in
>> use by users, then the entries would only be freed when the last entry
>> has been returned and all the references to the pool unreferenced.
> 
> Ok, can you please mention this in a comment somewhere? It's quite
> non-obvious why do both pool_unref() and refstruct_pool_uninit() exist.
> 

Right now I could move everything from pool_unref() at the end of
refstruct_pool_uninit(), leaving pool_unref() empty, but existing. The
actual reason that pool_unref() and refstruct_pool_uninit() are
different are:
1. The pool must not be freed when its refcount goes to zero, hence I
need to override its free callback.
2. I pondered adding support for weak references in RefStruct (in this
case the unref callback would be called when there is no strong
reference and the free function when there is no reference at all any
more). In this case I could give every pool entry in use a weak
reference to the pool; FFRefStructPool.refcount would then be
unnecessary, as the pool's weak refcount would take over its function.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".