| Message ID | AS8P250MB074434D09B045AD8BEEB38A38FFAA@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | New API for reference counting and ThreadFrames | expand |
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?
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. 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.) - Andreas
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: > 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
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
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) + pool_free(pool); +} - if (!pool) - return; +static void refstruct_pool_uninit(FFRefStructOpaque unused, void *obj) +{ + FFRefStructPool *pool = obj; + RefCount *entry; pthread_mutex_lock(&pool->mutex); av_assert1(!pool->uninited); @@ -298,11 +302,6 @@ void ff_refstruct_pool_uninit(FFRefStructPool **poolp) pool_free_entry(pool, entry); entry = next; } - - if (atomic_fetch_sub_explicit(&pool->refcount, 1, memory_order_acq_rel) == 1) - pool_free(pool); - - *poolp = NULL; } FFRefStructPool *ff_refstruct_pool_alloc(size_t size, unsigned flags) @@ -317,11 +316,13 @@ FFRefStructPool *ff_refstruct_pool_alloc_ext_c(size_t size, unsigned flags, void (*free_entry_cb)(FFRefStructOpaque opaque, void *obj), void (*free_cb)(FFRefStructOpaque opaque)) { - FFRefStructPool *pool = av_mallocz(sizeof(*pool)); + FFRefStructPool *pool = ff_refstruct_alloc_ext(sizeof(*pool), 0, NULL, + refstruct_pool_uninit); int err; if (!pool) return NULL; + get_refcount(pool)->free = pool_unref; pool->size = size; pool->opaque = opaque; @@ -348,7 +349,9 @@ FFRefStructPool *ff_refstruct_pool_alloc_ext_c(size_t size, unsigned flags, err = pthread_mutex_init(&pool->mutex, NULL); if (err) { - av_free(pool); + // Don't call ff_refstruct_uninit() on pool, as it hasn't been properly + // set up and is just a POD right now. + av_free(get_refcount(pool)); return NULL; } return pool; diff --git a/libavcodec/refstruct.h b/libavcodec/refstruct.h index ce3830977f..d525be61e8 100644 --- a/libavcodec/refstruct.h +++ b/libavcodec/refstruct.h @@ -285,6 +285,9 @@ void *ff_refstruct_pool_get(FFRefStructPool *pool); * @param poolp pointer to a pointer to either NULL or a pool to be freed. * `*poolp` will be set to NULL. */ -void ff_refstruct_pool_uninit(FFRefStructPool **poolp); +static inline void ff_refstruct_pool_uninit(FFRefStructPool **poolp) +{ + ff_refstruct_unref(poolp); +} #endif /* AVCODEC_REFSTRUCT_H */
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(-)