Message ID | AS8P250MB074487CAD933FE4F6325C8EB8FFAA@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
---|---|
Headers | show |
Series | New API for reference counting and ThreadFrames | expand |
On Tue, Sep 19, 2023 at 09:57:09PM +0200, Andreas Rheinhardt wrote: > Very similar to the AVBufferPool API, but with some differences: > 1. Reusing an already existing entry does not incur an allocation > at all any more (the AVBufferPool API needs to allocate an AVBufferRef). > 2. The tasks done while holding the lock are smaller; e.g. > allocating new entries is now performed without holding the lock. > The same goes for freeing. > 3. The entries are freed as soon as possible (the AVBufferPool API > frees them in two batches: The first in av_buffer_pool_uninit() and > the second immediately before the pool is freed when the last > outstanding entry is returned to the pool). > 4. The API is designed for objects and not naked buffers and > therefore has a reset callback. This is called whenever an object > is returned to the pool. > 5. Just like with the RefStruct API, custom allocators are not > supported. > > (If desired, the FFRefStructPool struct itself could be made > reference counted via the RefStruct API; an FFRefStructPool > would then be freed via ff_refstruct_unref().) > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/refstruct.c | 194 ++++++++++++++++++++++++++++++++++++++++- > libavcodec/refstruct.h | 128 +++++++++++++++++++++++++++ > 2 files changed, 321 insertions(+), 1 deletion(-) seems to break building on ppc (--disable-pthreads) CC libavcodec/refstruct.o src/libavcodec/refstruct.c: In function ‘pool_free’: src/libavcodec/refstruct.c:187:5: error: implicit declaration of function ‘pthread_mutex_destroy’; did you mean ‘ff_mutex_destroy’? [-Werror=implicit-function-declaration] pthread_mutex_destroy(&pool->mutex); ^~~~~~~~~~~~~~~~~~~~~ ff_mutex_destroy src/libavcodec/refstruct.c: In function ‘pool_return_entry’: src/libavcodec/refstruct.c:205:5: error: implicit declaration of function ‘pthread_mutex_lock’; did you mean ‘ff_mutex_lock’? [-Werror=implicit-function-declaration] pthread_mutex_lock(&pool->mutex); ^~~~~~~~~~~~~~~~~~ ff_mutex_lock src/libavcodec/refstruct.c:211:5: error: implicit declaration of function ‘pthread_mutex_unlock’; did you mean ‘ff_mutex_unlock’? [-Werror=implicit-function-declaration] pthread_mutex_unlock(&pool->mutex); ^~~~~~~~~~~~~~~~~~~~ ff_mutex_unlock src/libavcodec/refstruct.c: In function ‘ff_refstruct_pool_alloc_ext_c’: src/libavcodec/refstruct.c:339:11: error: implicit declaration of function ‘pthread_mutex_init’; did you mean ‘ff_mutex_init’? [-Werror=implicit-function-declaration] err = pthread_mutex_init(&pool->mutex, NULL); ^~~~~~~~~~~~~~~~~~ ff_mutex_init cc1: some warnings being treated as errors /home/michael/ffmpeg-git/ffmpeg/ffbuild/common.mak:81: recipe for target 'libavcodec/refstruct.o' failed make: *** [libavcodec/refstruct.o] Error 1 make: Target 'all' not remade because of errors. [...]
Michael Niedermayer: > On Tue, Sep 19, 2023 at 09:57:09PM +0200, Andreas Rheinhardt wrote: >> Very similar to the AVBufferPool API, but with some differences: >> 1. Reusing an already existing entry does not incur an allocation >> at all any more (the AVBufferPool API needs to allocate an AVBufferRef). >> 2. The tasks done while holding the lock are smaller; e.g. >> allocating new entries is now performed without holding the lock. >> The same goes for freeing. >> 3. The entries are freed as soon as possible (the AVBufferPool API >> frees them in two batches: The first in av_buffer_pool_uninit() and >> the second immediately before the pool is freed when the last >> outstanding entry is returned to the pool). >> 4. The API is designed for objects and not naked buffers and >> therefore has a reset callback. This is called whenever an object >> is returned to the pool. >> 5. Just like with the RefStruct API, custom allocators are not >> supported. >> >> (If desired, the FFRefStructPool struct itself could be made >> reference counted via the RefStruct API; an FFRefStructPool >> would then be freed via ff_refstruct_unref().) >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/refstruct.c | 194 ++++++++++++++++++++++++++++++++++++++++- >> libavcodec/refstruct.h | 128 +++++++++++++++++++++++++++ >> 2 files changed, 321 insertions(+), 1 deletion(-) > > seems to break building on ppc (--disable-pthreads) > > CC libavcodec/refstruct.o > src/libavcodec/refstruct.c: In function ‘pool_free’: > src/libavcodec/refstruct.c:187:5: error: implicit declaration of function ‘pthread_mutex_destroy’; did you mean ‘ff_mutex_destroy’? [-Werror=implicit-function-declaration] > pthread_mutex_destroy(&pool->mutex); > ^~~~~~~~~~~~~~~~~~~~~ > ff_mutex_destroy > src/libavcodec/refstruct.c: In function ‘pool_return_entry’: > src/libavcodec/refstruct.c:205:5: error: implicit declaration of function ‘pthread_mutex_lock’; did you mean ‘ff_mutex_lock’? [-Werror=implicit-function-declaration] > pthread_mutex_lock(&pool->mutex); > ^~~~~~~~~~~~~~~~~~ > ff_mutex_lock > src/libavcodec/refstruct.c:211:5: error: implicit declaration of function ‘pthread_mutex_unlock’; did you mean ‘ff_mutex_unlock’? [-Werror=implicit-function-declaration] > pthread_mutex_unlock(&pool->mutex); > ^~~~~~~~~~~~~~~~~~~~ > ff_mutex_unlock > src/libavcodec/refstruct.c: In function ‘ff_refstruct_pool_alloc_ext_c’: > src/libavcodec/refstruct.c:339:11: error: implicit declaration of function ‘pthread_mutex_init’; did you mean ‘ff_mutex_init’? [-Werror=implicit-function-declaration] > err = pthread_mutex_init(&pool->mutex, NULL); > ^~~~~~~~~~~~~~~~~~ > ff_mutex_init > cc1: some warnings being treated as errors > /home/michael/ffmpeg-git/ffmpeg/ffbuild/common.mak:81: recipe for target 'libavcodec/refstruct.o' failed > make: *** [libavcodec/refstruct.o] Error 1 > make: Target 'all' not remade because of errors. Sorry for not having tested --disable-pthreads. Will switch to our ff_mutex-wrappers. - Andreas
Quoting Andreas Rheinhardt (2023-09-19 21:57:06) >avcodec/hevcdec: Use RefStruct API for RefPicListTap buffer ^ b > Given that the RefStruct API relies on the user to know > the size of the objects and does not provide a way to get it, Is there a reason you decided not to provide it, or there just wasn't any code that would be improved by it? > we need to store the number of elements allocated ourselves; > but this is actually better than deriving it from the size > in bytes. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/hevc_refs.c | 14 ++++++++------ > libavcodec/hevcdec.c | 5 ++--- > libavcodec/hevcdec.h | 3 ++- > 3 files changed, 12 insertions(+), 10 deletions(-) The actual code looks good.
Quoting Andreas Rheinhardt (2023-09-19 21:57:08) > Avoids allocations and error checks as well as the boilerplate > code for creating an AVBuffer with a custom free callback. > Also increases type safety. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/nvdec.c | 50 ++++++++++++++++++---------------------------- > libavcodec/nvdec.h | 4 ++-- > 2 files changed, 21 insertions(+), 33 deletions(-) Looks ok.
Quoting Andreas Rheinhardt (2023-09-19 21:57:07) > Avoids allocations and error checks and allows to remove > cleanup code for earlier allocations. Also avoids casts > and indirections. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > I actually intend to remove the ThreadFrame API in the not-so > long-term. > > libavcodec/h264_mb.c | 4 ++-- > libavcodec/pthread_frame.c | 23 ++++++++++++----------- > libavcodec/threadframe.h | 4 +--- > libavcodec/utils.c | 14 ++++---------- > 4 files changed, 19 insertions(+), 26 deletions(-) LGTM
Anton Khirnov: > Quoting Andreas Rheinhardt (2023-09-19 21:57:06) >> avcodec/hevcdec: Use RefStruct API for RefPicListTap buffer > ^ > b > >> Given that the RefStruct API relies on the user to know >> the size of the objects and does not provide a way to get it, > > Is there a reason you decided not to provide it, or there just wasn't > any code that would be improved by it? > In this example, there would be no improvement if RefStruct recorded the size (in bytes) and provided a getter for it. In general, the philosophy of the RefStruct API is that the user and not the API knows what is in the user data. So the user has to keep track of stuff like this in cases where the user doesn't know it. The only scenario where keeping track of the size would be advantageous would be for using arrays-of-whatever if the callbacks provided the size as a parameter (to be ignored by most users), because one could then derive the number of elements from it. I pondered this, but given that the AVBuffer API doesn't do so either, I didn't do it. Shall I change this? >> we need to store the number of elements allocated ourselves; >> but this is actually better than deriving it from the size >> in bytes. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/hevc_refs.c | 14 ++++++++------ >> libavcodec/hevcdec.c | 5 ++--- >> libavcodec/hevcdec.h | 3 ++- >> 3 files changed, 12 insertions(+), 10 deletions(-) > > The actual code looks good. >
Sep 19, 2023, 21:58 by andreas.rheinhardt@outlook.com: > Avoids allocations, error checks and indirections. > Also increases type-safety. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/vulkan_av1.c | 2 +- > libavcodec/vulkan_decode.c | 49 ++++++++++++++++---------------------- > libavcodec/vulkan_decode.h | 2 +- > libavcodec/vulkan_h264.c | 2 +- > libavcodec/vulkan_hevc.c | 2 +- > 5 files changed, 24 insertions(+), 33 deletions(-) > Like the other patch, LGTM
Quoting Andreas Rheinhardt (2023-10-02 13:07:14) > Anton Khirnov: > > Quoting Andreas Rheinhardt (2023-09-19 21:57:06) > >> avcodec/hevcdec: Use RefStruct API for RefPicListTap buffer > > ^ > > b > > > >> Given that the RefStruct API relies on the user to know > >> the size of the objects and does not provide a way to get it, > > > > Is there a reason you decided not to provide it, or there just wasn't > > any code that would be improved by it? > > > > In this example, there would be no improvement if RefStruct recorded the > size (in bytes) and provided a getter for it. > > In general, the philosophy of the RefStruct API is that the user and not > the API knows what is in the user data. So the user has to keep track of > stuff like this in cases where the user doesn't know it. > > The only scenario where keeping track of the size would be advantageous > would be for using arrays-of-whatever if the callbacks provided the size > as a parameter (to be ignored by most users), because one could then > derive the number of elements from it. I pondered this, but given that > the AVBuffer API doesn't do so either, I didn't do it. Shall I change this? I'm leaning towards no, it's overly specific and the advantage probably small.
Quoting Andreas Rheinhardt (2023-09-19 21:57:09) > Very similar to the AVBufferPool API, but with some differences: > 1. Reusing an already existing entry does not incur an allocation > at all any more (the AVBufferPool API needs to allocate an AVBufferRef). > 2. The tasks done while holding the lock are smaller; e.g. > allocating new entries is now performed without holding the lock. > The same goes for freeing. > 3. The entries are freed as soon as possible (the AVBufferPool API > frees them in two batches: The first in av_buffer_pool_uninit() and > the second immediately before the pool is freed when the last > outstanding entry is returned to the pool). > 4. The API is designed for objects and not naked buffers and > therefore has a reset callback. This is called whenever an object > is returned to the pool. > 5. Just like with the RefStruct API, custom allocators are not > supported. > > (If desired, the FFRefStructPool struct itself could be made > reference counted via the RefStruct API; an FFRefStructPool > would then be freed via ff_refstruct_unref().) Considering that it's intended to be used from multiple threads, that seems like the better option. Though I have not seen the following patches yet, so maybe the advantage is not as big as I'd think. > +/** > + * If this flag is not set, every object in the pool will be zeroed before > + * the init callback is called or before it is turned over to the user > + * for the first time if no init callback has been provided. > + */ > +#define FF_REFSTRUCT_POOL_FLAG_NO_ZEROING FF_REFSTRUCT_FLAG_NO_ZEROING Do you think using the same namespace really improves things? It does not seem so to me.
Anton Khirnov: > Quoting Andreas Rheinhardt (2023-09-19 21:57:09) >> Very similar to the AVBufferPool API, but with some differences: >> 1. Reusing an already existing entry does not incur an allocation >> at all any more (the AVBufferPool API needs to allocate an AVBufferRef). >> 2. The tasks done while holding the lock are smaller; e.g. >> allocating new entries is now performed without holding the lock. >> The same goes for freeing. >> 3. The entries are freed as soon as possible (the AVBufferPool API >> frees them in two batches: The first in av_buffer_pool_uninit() and >> the second immediately before the pool is freed when the last >> outstanding entry is returned to the pool). >> 4. The API is designed for objects and not naked buffers and >> therefore has a reset callback. This is called whenever an object >> is returned to the pool. >> 5. Just like with the RefStruct API, custom allocators are not >> supported. >> >> (If desired, the FFRefStructPool struct itself could be made >> reference counted via the RefStruct API; an FFRefStructPool >> would then be freed via ff_refstruct_unref().) > > Considering that it's intended to be used from multiple threads, that > seems like the better option. Though I have not seen the following > patches yet, so maybe the advantage is not as big as I'd think. > It is implemented in patch #24. >> +/** >> + * If this flag is not set, every object in the pool will be zeroed before >> + * the init callback is called or before it is turned over to the user >> + * for the first time if no init callback has been provided. >> + */ >> +#define FF_REFSTRUCT_POOL_FLAG_NO_ZEROING FF_REFSTRUCT_FLAG_NO_ZEROING > > Do you think using the same namespace really improves things? It does > not seem so to me. > Actually, the namespaces are separate. The pool API uses FF_REFSTRUCT_POOL_FLAG_*, the ordinary RefStruct API uses FF_REFSTRUCT_FLAG_*. That some pool flags are simply mapped to non-pool flags is an implementation detail and does not join the namespaces. - Andreas
Quoting Andreas Rheinhardt (2023-09-19 21:57:10) > It involves less allocations and therefore has the nice property > that deriving a reference from a reference can't fail. > This allows for considerable simplifications in > ff_h264_(ref|replace)_picture(). > Switching to the RefStruct API also allows to make H264Picture > smaller, because some AVBufferRef* pointers could be removed > without replacement. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/h264_picture.c | 72 +++++++++++---------------------------- > libavcodec/h264_slice.c | 44 ++++++++++++------------ > libavcodec/h264dec.c | 19 ++++++----- > libavcodec/h264dec.h | 23 ++++++------- > 4 files changed, 62 insertions(+), 96 deletions(-) LGTM
Quoting Andreas Rheinhardt (2023-09-19 21:57:34) > Avoids implicit av_frame_ref() and therefore allocations > and error checks. It also avoids explicitly allocating > the AVFrames (done implicitly when getting the buffer). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/ffv1.c | 1 - > libavcodec/ffv1.h | 4 +-- > libavcodec/ffv1dec.c | 83 +++++++++++++++++++------------------------- > 3 files changed, 37 insertions(+), 51 deletions(-) Looks ok