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

[Anton Khirnov <anton@khirnov.net>]

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.

-- 
Anton Khirnov
_______________________________________________
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".