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.
> 
>> In fact, when I did this, I pondered two things: Shall I make
>> ff_refstruct_pool_uninit() free all the currently available buffers and
>> then unreference the caller's reference or shall I just make it a
>> wrapper to ff_refstruct_unref() to decrement the pool's refcount? The
>> latter is very simple and I did it; the former could be advantageous in
>> particular in case of frame-threading in case the dimensions change. (In
>> this scenario, no user will ever create new entries after the first user
>> unreferences a pool.)
> 
> But in other scenarios you might want to get rid of some pool references
> while still using the others, so maybe that should be a flag if
> anything.
> 

If I added a non-inlined ff_refstruct_pool_uninit(), I could simply
still allow to call ff_refstruct_unref() on pool-references, so that one
can get both behaviours. (Notice that this allows a malicious owner of a
pool-reference to drain the pool ad libitum, but you can do way worse
stuff with a reference than this.)

- 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".