| 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
This patchset initially grew out of my dissatisfaction with the fact that the AVBuffer API contains implicit allocations in av_buffer_ref() that need to be checked. Later this combined with my dissatisfaction with the current ThreadFrame API (which contains an implicit av_frame_ref()) which forbids decoders to set any AVFrame field of a frame that is used for syncing after having called ff_thread_finish_setup(). This leads to a race in the vp9-encparams test that is fixed in this patchset. Andreas Rheinhardt (42): tests/fate-run: Ensure that THREADS=random is actually random avcodec/refstruct: Add simple API for refcounted objects avcodec/get_buffer: Use RefStruct API for FramePool avcodec/h264_ps: Use RefStruct API for SPS/PPS avcodec/hevc_ps: Use RefStruct API for parameter sets avcodec/vp8: Use RefStruct API for seg_map avcodec/wavpack: Use RefStruct API for DSD context avcodec/dovi_rpu: Use RefStruct API for Vdr data avcodec/refstruct: Allow checking for exclusive ownership avcodec/cbs: Use RefStruct-API for unit content avcodec/cbs_sei: Use RefStruct API for SEI messages avcodec/decode: Use RefStruct API for hwaccel_picture_private avcodec/vulkan_decode: Use RefStruct API for shared_ref avcodec/hevcdec: Use RefStruct API for RefPicListTap buffer avcodec/pthread_frame: Use RefStruct API for ThreadFrame.progress avcodec/nvdec: Use RefStruct API for decoder_ref avcodec/refstruct: Add RefStruct pool API avcodec/h264dec: Use RefStruct-pool API instead of AVBufferPool API avcodec/hevcdec: Use RefStruct-pool API instead of AVBufferPool API avcodec/nvdec: Use RefStruct-pool API for decoder pool avcodec/refstruct: Allow to always return zeroed pool entries avcodec/vp9: Use RefStruct-pool API for extradata avcodec/vaapi_encode: Use RefStruct pool API, stop abusing AVBuffer API avcodec/refstruct: Allow to share pools avcodec/vp9: Join extradata buffer pools avcodec/refstruct: Allow to use a dynamic opaque avcodec/pthread_frame: Add new progress API avcodec/mimic: Switch to ProgressFrames avcodec/vp3: Switch to ProgressFrames avcodec/vp9: Switch to ProgressFrames avcodec/vp9: Fix race when attaching side-data for show-existing frame avcodec/vp9: Reduce wait times avcodec/vp9: Simplify replacing VP9Frame avcodec/vp9: Replace atomic_store() by atomic_init() avcodec/threadprogress: Add new API for frame-threaded progress avcodec/wavpack: Use ThreadProgress API avcodec/vp8: Convert to ProgressFrame API avcodec/codec_internal: Remove FF_CODEC_CAP_ALLOCATE_PROGRESS avcodec/hevcdec: Move collocated_ref to HEVCContext avcodec/hevcdec: Switch to ProgressFrames avcodec/pngdec: Switch to ProgressFrames avcodec/ffv1dec: Switch to ProgressFrames doc/multithreading.txt | 8 +- libavcodec/Makefile | 1 + libavcodec/av1dec.c | 64 +--- libavcodec/av1dec.h | 14 +- libavcodec/avcodec.c | 4 +- libavcodec/cbs.c | 103 +++---- libavcodec/cbs.h | 12 +- libavcodec/cbs_av1.c | 32 +- libavcodec/cbs_av1.h | 3 +- libavcodec/cbs_h264.h | 6 +- libavcodec/cbs_h2645.c | 113 +++---- libavcodec/cbs_h265.h | 9 +- libavcodec/cbs_h266.h | 11 +- libavcodec/cbs_h266_syntax_template.c | 10 +- libavcodec/cbs_internal.h | 7 +- libavcodec/cbs_sei.c | 61 ++-- libavcodec/cbs_sei.h | 20 +- libavcodec/cbs_sei_syntax_template.c | 5 + libavcodec/codec_internal.h | 7 +- libavcodec/decode.c | 146 +++++++-- libavcodec/decode.h | 10 +- libavcodec/dovi_rpu.c | 54 ++-- libavcodec/dovi_rpu.h | 4 +- libavcodec/dxva2_vp9.c | 4 +- libavcodec/ffv1.c | 1 - libavcodec/ffv1.h | 4 +- libavcodec/ffv1dec.c | 82 +++--- libavcodec/get_buffer.c | 44 +-- libavcodec/h264_mb.c | 4 +- libavcodec/h264_parser.c | 9 +- libavcodec/h264_picture.c | 91 ++---- libavcodec/h264_ps.c | 57 ++-- libavcodec/h264_ps.h | 13 +- libavcodec/h264_refs.c | 2 +- libavcodec/h264_sei.c | 2 +- libavcodec/h264_slice.c | 88 +++--- libavcodec/h264dec.c | 21 +- libavcodec/h264dec.h | 25 +- libavcodec/hevc_filter.c | 8 +- libavcodec/hevc_mvs.c | 8 +- libavcodec/hevc_parser.c | 8 +- libavcodec/hevc_ps.c | 80 +++-- libavcodec/hevc_ps.h | 10 +- libavcodec/hevc_refs.c | 64 ++-- libavcodec/hevc_sei.c | 5 +- libavcodec/hevcdec.c | 113 +++---- libavcodec/hevcdec.h | 22 +- libavcodec/hwaccel_internal.h | 3 +- libavcodec/internal.h | 4 +- libavcodec/mediacodecdec.c | 10 +- libavcodec/mimic.c | 60 ++-- libavcodec/mpeg4videodec.c | 3 +- libavcodec/mpegpicture.c | 18 +- libavcodec/mpegpicture.h | 1 - libavcodec/nvdec.c | 97 +++--- libavcodec/nvdec.h | 10 +- libavcodec/pngdec.c | 67 ++--- libavcodec/progressframe.h | 134 +++++++++ libavcodec/progressframe_internal.h | 32 ++ libavcodec/pthread_frame.c | 127 ++++++-- libavcodec/refstruct.c | 407 ++++++++++++++++++++++++++ libavcodec/refstruct.h | 360 +++++++++++++++++++++++ libavcodec/rv30.c | 1 - libavcodec/rv40.c | 1 - libavcodec/tests/avcodec.c | 10 +- libavcodec/threadframe.h | 4 +- libavcodec/threadprogress.h | 37 +++ libavcodec/utils.c | 14 +- libavcodec/vaapi_encode.c | 60 ++-- libavcodec/vaapi_encode.h | 5 +- libavcodec/videotoolbox.c | 4 +- libavcodec/vp3.c | 148 ++++------ libavcodec/vp8.c | 127 +++----- libavcodec/vp8.h | 11 +- libavcodec/vp9.c | 195 +++++------- libavcodec/vp9_mc_template.c | 2 +- libavcodec/vp9block.c | 5 +- libavcodec/vp9dec.h | 7 +- libavcodec/vp9lpf.c | 1 + libavcodec/vp9mvs.c | 4 +- libavcodec/vp9recon.c | 19 +- libavcodec/vp9shared.h | 14 +- libavcodec/vulkan_av1.c | 11 +- libavcodec/vulkan_decode.c | 49 ++-- libavcodec/vulkan_decode.h | 2 +- libavcodec/vulkan_h264.c | 15 +- libavcodec/vulkan_hevc.c | 27 +- libavcodec/wavpack.c | 125 ++++---- libavcodec/webp.c | 3 +- tests/fate-run.sh | 2 +- 90 files changed, 2165 insertions(+), 1540 deletions(-) create mode 100644 libavcodec/progressframe.h create mode 100644 libavcodec/progressframe_internal.h create mode 100644 libavcodec/refstruct.c create mode 100644 libavcodec/refstruct.h create mode 100644 libavcodec/threadprogress.h