| Message ID | GV1P250MB07370D546BF8AB62620CC4088F002@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 2135a40b1c0a16dc6d69c9d4df998a9912bb66a4 |
| Headers | show |
| Series | [FFmpeg-devel,v2,01/27] avcodec/threadprogress: Add new API for frame-threaded progress | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Mon, Apr 08, 2024 at 10:13:40PM +0200, Andreas Rheinhardt wrote: > Frame-threaded decoders with inter-frame dependencies > use the ThreadFrame API for syncing. It works as follows: > > During init each thread allocates an AVFrame for every > ThreadFrame. > > Thread A reads the header of its packet and allocates > a buffer for an AVFrame with ff_thread_get_ext_buffer() > (which also allocates a small structure that is shared > with other references to this frame) and sets its fields, > including side data. Then said thread calls ff_thread_finish_setup(). > From that moment onward it is not allowed to change any > of the AVFrame fields at all any more, but it may change > fields which are an indirection away, like the content of > AVFrame.data or already existing side data. > > After thread A has called ff_thread_finish_setup(), > another thread (the user one) calls the codec's update_thread_context > callback which in turn calls ff_thread_ref_frame() which > calls av_frame_ref() which reads every field of A's > AVFrame; hence the above restriction on modifications > of the AVFrame (as any modification of the AVFrame by A after > ff_thread_finish_setup() would be a data race). Of course, > this av_frame_ref() also incurs allocations and therefore > needs to be checked. ff_thread_ref_frame() also references > the small structure used for communicating progress. > > This av_frame_ref() makes it awkward to propagate values that > only become known during decoding to later threads (in case of > frame reordering or other mechanisms of delayed output (like > show-existing-frames) it's not the decoding thread, but a later > thread that returns the AVFrame). E.g. for VP9 when exporting video > encoding parameters as side data the number of blocks only > becomes known during decoding, so one can't allocate the side data > before ff_thread_finish_setup(). It is currently being done afterwards > and this leads to a data race in the vp9-encparams test when using > frame-threading. Returning decode_error_flags is also complicated > by this. > > To perform this exchange a buffer shared between the references > is needed (notice that simply giving the later threads a pointer > to the original AVFrame does not work, because said AVFrame will > be reused lateron when thread A decodes the next packet given to it). > One could extend the buffer already used for progress for this > or use a new one (requiring yet another allocation), yet both > of these approaches have the drawback of being unnatural, ugly > and requiring quite a lot of ad-hoc code. E.g. in case of the VP9 > side data mentioned above one could not simply use the helper > that allocates and adds the side data to an AVFrame in one go. > > The ProgressFrame API meanwhile offers a different solution to all > of this. It is based around the idea that the most natural > shared object for sharing information about an AVFrame between > decoding threads is the AVFrame itself. To actually implement this > the AVFrame needs to be reference counted. This is achieved by > putting a (ownership) pointer into a shared (and opaque) structure > that is managed by the RefStruct API and which also contains > the stuff necessary for progress reporting. > The users get a pointer to this AVFrame with the understanding > that the owner may set all the fields until it has indicated > that it has finished decoding this AVFrame; then the users are > allowed to read everything. Every decoder may of course employ > a different contract than the one outlined above. > > Given that there is no underlying av_frame_ref(), creating > references to a ProgressFrame can't fail. Only > ff_thread_progress_get_buffer() can fail, but given that > it will replace calls to ff_thread_get_ext_buffer() it is > at places where errors are already expected and properly > taken care of. > > The ProgressFrames are empty (i.e. the AVFrame pointer is NULL > and the AVFrames are not allocated during init at all) > while not being in use; ff_thread_progress_get_buffer() both > sets up the actual ProgressFrame and already calls > ff_thread_get_buffer(). So instead of checking for > ThreadFrame.f->data[0] or ThreadFrame.f->buf[0] being NULL > for "this reference frame is non-existing" one should check for > ProgressFrame.f. > > This also implies that one can only set AVFrame properties > after having allocated the buffer. This restriction is not deep: > if it becomes onerous for any codec, ff_thread_progress_get_buffer() > can be broken up. The user would then have to get a buffer > himself. > > In order to avoid unnecessary allocations, the shared structure > is pooled, so that both the structure as well as the AVFrame > itself are reused. This means that there won't be lots of > unnecessary allocations in case of non-frame-threaded decoding. > It might even turn out to have fewer than the current code > (the current code allocates AVFrames for every DPB slot, but > these are often excessively large and not completely used; > the new code allocates them on demand). Pooling relies on the > reset function of the RefStruct pool API, it would be impossible > to implement with the AVBufferPool API. > > Finally, ProgressFrames have no notion of owner; they are built > on top of the ThreadProgress API which also lacks such a concept. > Instead every ThreadProgress and every ProgressFrame contains > its own mutex and condition variable, making it completely independent > of pthread_frame.c. Just like the ThreadFrame API it is simply > presumed that only the actual owner/producer of a frame reports > progress on said frame. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/Makefile | 1 + > libavcodec/avcodec.c | 1 + > libavcodec/codec_internal.h | 4 ++ > libavcodec/decode.c | 122 +++++++++++++++++++++++++++++++++ > libavcodec/internal.h | 2 + > libavcodec/progressframe.h | 133 ++++++++++++++++++++++++++++++++++++ > libavcodec/pthread_frame.c | 1 + > libavcodec/tests/avcodec.c | 3 +- > 8 files changed, 266 insertions(+), 1 deletion(-) > create mode 100644 libavcodec/progressframe.h breaks build without threads ./configure --disable-pthreads --cc='ccache gcc' && make -j32 ... libavcodec/threadprogress.c: In function ‘ff_thread_progress_report’: libavcodec/threadprogress.c:54:5: error: implicit declaration of function ‘pthread_mutex_lock’; did you mean ‘ff_mutex_lock’? [-Werror=implicit-function-declaration] pthread_mutex_lock(&pro->progress_mutex); ^~~~~~~~~~~~~~~~~~ ff_mutex_lock libavcodec/threadprogress.c:55:5: error: implicit declaration of function ‘pthread_cond_broadcast’; did you mean ‘ff_cond_broadcast’? [-Werror=implicit-function-declaration] pthread_cond_broadcast(&pro->progress_cond); ^~~~~~~~~~~~~~~~~~~~~~ ff_cond_broadcast libavcodec/threadprogress.c:56:5: error: implicit declaration of function ‘pthread_mutex_unlock’; did you mean ‘ff_mutex_unlock’? [-Werror=implicit-function-declaration] pthread_mutex_unlock(&pro->progress_mutex); ^~~~~~~~~~~~~~~~~~~~ ff_mutex_unlock libavcodec/threadprogress.c: In function ‘ff_thread_progress_await’: libavcodec/threadprogress.c:71:9: error: implicit declaration of function ‘pthread_cond_wait’; did you mean ‘ff_cond_wait’? [-Werror=implicit-function-declaration] pthread_cond_wait(&pro->progress_cond, &pro->progress_mutex); ^~~~~~~~~~~~~~~~~ ff_cond_wait cc1: some warnings being treated as errors ffbuild/common.mak:81: recipe for target 'libavcodec/threadprogress.o' failed make: *** [libavcodec/threadprogress.o] Error 1 make: *** Waiting for unfinished jobs.... [...]
Michael Niedermayer: > On Mon, Apr 08, 2024 at 10:13:40PM +0200, Andreas Rheinhardt wrote: >> Frame-threaded decoders with inter-frame dependencies >> use the ThreadFrame API for syncing. It works as follows: >> >> During init each thread allocates an AVFrame for every >> ThreadFrame. >> >> Thread A reads the header of its packet and allocates >> a buffer for an AVFrame with ff_thread_get_ext_buffer() >> (which also allocates a small structure that is shared >> with other references to this frame) and sets its fields, >> including side data. Then said thread calls ff_thread_finish_setup(). >> From that moment onward it is not allowed to change any >> of the AVFrame fields at all any more, but it may change >> fields which are an indirection away, like the content of >> AVFrame.data or already existing side data. >> >> After thread A has called ff_thread_finish_setup(), >> another thread (the user one) calls the codec's update_thread_context >> callback which in turn calls ff_thread_ref_frame() which >> calls av_frame_ref() which reads every field of A's >> AVFrame; hence the above restriction on modifications >> of the AVFrame (as any modification of the AVFrame by A after >> ff_thread_finish_setup() would be a data race). Of course, >> this av_frame_ref() also incurs allocations and therefore >> needs to be checked. ff_thread_ref_frame() also references >> the small structure used for communicating progress. >> >> This av_frame_ref() makes it awkward to propagate values that >> only become known during decoding to later threads (in case of >> frame reordering or other mechanisms of delayed output (like >> show-existing-frames) it's not the decoding thread, but a later >> thread that returns the AVFrame). E.g. for VP9 when exporting video >> encoding parameters as side data the number of blocks only >> becomes known during decoding, so one can't allocate the side data >> before ff_thread_finish_setup(). It is currently being done afterwards >> and this leads to a data race in the vp9-encparams test when using >> frame-threading. Returning decode_error_flags is also complicated >> by this. >> >> To perform this exchange a buffer shared between the references >> is needed (notice that simply giving the later threads a pointer >> to the original AVFrame does not work, because said AVFrame will >> be reused lateron when thread A decodes the next packet given to it). >> One could extend the buffer already used for progress for this >> or use a new one (requiring yet another allocation), yet both >> of these approaches have the drawback of being unnatural, ugly >> and requiring quite a lot of ad-hoc code. E.g. in case of the VP9 >> side data mentioned above one could not simply use the helper >> that allocates and adds the side data to an AVFrame in one go. >> >> The ProgressFrame API meanwhile offers a different solution to all >> of this. It is based around the idea that the most natural >> shared object for sharing information about an AVFrame between >> decoding threads is the AVFrame itself. To actually implement this >> the AVFrame needs to be reference counted. This is achieved by >> putting a (ownership) pointer into a shared (and opaque) structure >> that is managed by the RefStruct API and which also contains >> the stuff necessary for progress reporting. >> The users get a pointer to this AVFrame with the understanding >> that the owner may set all the fields until it has indicated >> that it has finished decoding this AVFrame; then the users are >> allowed to read everything. Every decoder may of course employ >> a different contract than the one outlined above. >> >> Given that there is no underlying av_frame_ref(), creating >> references to a ProgressFrame can't fail. Only >> ff_thread_progress_get_buffer() can fail, but given that >> it will replace calls to ff_thread_get_ext_buffer() it is >> at places where errors are already expected and properly >> taken care of. >> >> The ProgressFrames are empty (i.e. the AVFrame pointer is NULL >> and the AVFrames are not allocated during init at all) >> while not being in use; ff_thread_progress_get_buffer() both >> sets up the actual ProgressFrame and already calls >> ff_thread_get_buffer(). So instead of checking for >> ThreadFrame.f->data[0] or ThreadFrame.f->buf[0] being NULL >> for "this reference frame is non-existing" one should check for >> ProgressFrame.f. >> >> This also implies that one can only set AVFrame properties >> after having allocated the buffer. This restriction is not deep: >> if it becomes onerous for any codec, ff_thread_progress_get_buffer() >> can be broken up. The user would then have to get a buffer >> himself. >> >> In order to avoid unnecessary allocations, the shared structure >> is pooled, so that both the structure as well as the AVFrame >> itself are reused. This means that there won't be lots of >> unnecessary allocations in case of non-frame-threaded decoding. >> It might even turn out to have fewer than the current code >> (the current code allocates AVFrames for every DPB slot, but >> these are often excessively large and not completely used; >> the new code allocates them on demand). Pooling relies on the >> reset function of the RefStruct pool API, it would be impossible >> to implement with the AVBufferPool API. >> >> Finally, ProgressFrames have no notion of owner; they are built >> on top of the ThreadProgress API which also lacks such a concept. >> Instead every ThreadProgress and every ProgressFrame contains >> its own mutex and condition variable, making it completely independent >> of pthread_frame.c. Just like the ThreadFrame API it is simply >> presumed that only the actual owner/producer of a frame reports >> progress on said frame. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/Makefile | 1 + >> libavcodec/avcodec.c | 1 + >> libavcodec/codec_internal.h | 4 ++ >> libavcodec/decode.c | 122 +++++++++++++++++++++++++++++++++ >> libavcodec/internal.h | 2 + >> libavcodec/progressframe.h | 133 ++++++++++++++++++++++++++++++++++++ >> libavcodec/pthread_frame.c | 1 + >> libavcodec/tests/avcodec.c | 3 +- >> 8 files changed, 266 insertions(+), 1 deletion(-) >> create mode 100644 libavcodec/progressframe.h > > breaks build without threads > > ./configure --disable-pthreads --cc='ccache gcc' && make -j32 > ... > libavcodec/threadprogress.c: In function ‘ff_thread_progress_report’: > libavcodec/threadprogress.c:54:5: error: implicit declaration of function ‘pthread_mutex_lock’; did you mean ‘ff_mutex_lock’? [-Werror=implicit-function-declaration] > pthread_mutex_lock(&pro->progress_mutex); > ^~~~~~~~~~~~~~~~~~ > ff_mutex_lock > libavcodec/threadprogress.c:55:5: error: implicit declaration of function ‘pthread_cond_broadcast’; did you mean ‘ff_cond_broadcast’? [-Werror=implicit-function-declaration] > pthread_cond_broadcast(&pro->progress_cond); > ^~~~~~~~~~~~~~~~~~~~~~ > ff_cond_broadcast > libavcodec/threadprogress.c:56:5: error: implicit declaration of function ‘pthread_mutex_unlock’; did you mean ‘ff_mutex_unlock’? [-Werror=implicit-function-declaration] > pthread_mutex_unlock(&pro->progress_mutex); > ^~~~~~~~~~~~~~~~~~~~ > ff_mutex_unlock > libavcodec/threadprogress.c: In function ‘ff_thread_progress_await’: > libavcodec/threadprogress.c:71:9: error: implicit declaration of function ‘pthread_cond_wait’; did you mean ‘ff_cond_wait’? [-Werror=implicit-function-declaration] > pthread_cond_wait(&pro->progress_cond, &pro->progress_mutex); > ^~~~~~~~~~~~~~~~~ > ff_cond_wait > cc1: some warnings being treated as errors > ffbuild/common.mak:81: recipe for target 'libavcodec/threadprogress.o' failed > make: *** [libavcodec/threadprogress.o] Error 1 > make: *** Waiting for unfinished jobs.... > Thanks for testing; fixed by v3 of patch #1. - Andreas
Quoting Andreas Rheinhardt (2024-04-08 22:13:40) > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > index fd356bd190..6b2c4312e0 100644 > --- a/libavcodec/pthread_frame.c > +++ b/libavcodec/pthread_frame.c > @@ -779,6 +779,7 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free, > if (!copy->internal) > return AVERROR(ENOMEM); > copy->internal->thread_ctx = p; > + copy->internal->progress_frame_pool = avctx->internal->progress_frame_pool; I'd still prefer every thread to have its own reference. Looks good otherwise.
Anton Khirnov: > Quoting Andreas Rheinhardt (2024-04-08 22:13:40) >> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c >> index fd356bd190..6b2c4312e0 100644 >> --- a/libavcodec/pthread_frame.c >> +++ b/libavcodec/pthread_frame.c >> @@ -779,6 +779,7 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free, >> if (!copy->internal) >> return AVERROR(ENOMEM); >> copy->internal->thread_ctx = p; >> + copy->internal->progress_frame_pool = avctx->internal->progress_frame_pool; > > I'd still prefer every thread to have its own reference. > > Looks good otherwise. > The opaque of this pool is the main AVCodecContext; if the main AVCodecContext is destroyed, the pool is in a state where one can no longer get new entries from it. So giving every thread its own reference is pretending to make it an equal co-owner of the pool, but it is not as the pool must not outlive the main AVCodecContext. - Andreas
Quoting Andreas Rheinhardt (2024-04-10 09:09:00) > Anton Khirnov: > > Quoting Andreas Rheinhardt (2024-04-08 22:13:40) > >> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > >> index fd356bd190..6b2c4312e0 100644 > >> --- a/libavcodec/pthread_frame.c > >> +++ b/libavcodec/pthread_frame.c > >> @@ -779,6 +779,7 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free, > >> if (!copy->internal) > >> return AVERROR(ENOMEM); > >> copy->internal->thread_ctx = p; > >> + copy->internal->progress_frame_pool = avctx->internal->progress_frame_pool; > > > > I'd still prefer every thread to have its own reference. > > > > Looks good otherwise. > > > > The opaque of this pool is the main AVCodecContext; if the main > AVCodecContext is destroyed, the pool is in a state where one can no > longer get new entries from it. So giving every thread its own reference > is pretending to make it an equal co-owner of the pool, but it is not as > the pool must not outlive the main AVCodecContext. But the only use of that opaque is checking whether frame threading is in use, which is a constant during decoder lifetime. Might be cleaner to avoid using AVCodecContext as opaque. In any case, this is not important, feel free to leave it as is.
Anton Khirnov: > Quoting Andreas Rheinhardt (2024-04-10 09:09:00) >> Anton Khirnov: >>> Quoting Andreas Rheinhardt (2024-04-08 22:13:40) >>>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c >>>> index fd356bd190..6b2c4312e0 100644 >>>> --- a/libavcodec/pthread_frame.c >>>> +++ b/libavcodec/pthread_frame.c >>>> @@ -779,6 +779,7 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free, >>>> if (!copy->internal) >>>> return AVERROR(ENOMEM); >>>> copy->internal->thread_ctx = p; >>>> + copy->internal->progress_frame_pool = avctx->internal->progress_frame_pool; >>> >>> I'd still prefer every thread to have its own reference. >>> >>> Looks good otherwise. >>> >> >> The opaque of this pool is the main AVCodecContext; if the main >> AVCodecContext is destroyed, the pool is in a state where one can no >> longer get new entries from it. So giving every thread its own reference >> is pretending to make it an equal co-owner of the pool, but it is not as >> the pool must not outlive the main AVCodecContext. > > But the only use of that opaque is checking whether frame threading is > in use, which is a constant during decoder lifetime. Might be cleaner to > avoid using AVCodecContext as opaque. > In any case, this is not important, feel free to leave it as is. > But whether frame threading is in use is only determined after ff_decode_preinit(). - Andreas
Quoting Andreas Rheinhardt (2024-04-10 10:02:55) > Anton Khirnov: > > Quoting Andreas Rheinhardt (2024-04-10 09:09:00) > >> Anton Khirnov: > >>> Quoting Andreas Rheinhardt (2024-04-08 22:13:40) > >>>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > >>>> index fd356bd190..6b2c4312e0 100644 > >>>> --- a/libavcodec/pthread_frame.c > >>>> +++ b/libavcodec/pthread_frame.c > >>>> @@ -779,6 +779,7 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free, > >>>> if (!copy->internal) > >>>> return AVERROR(ENOMEM); > >>>> copy->internal->thread_ctx = p; > >>>> + copy->internal->progress_frame_pool = avctx->internal->progress_frame_pool; > >>> > >>> I'd still prefer every thread to have its own reference. > >>> > >>> Looks good otherwise. > >>> > >> > >> The opaque of this pool is the main AVCodecContext; if the main > >> AVCodecContext is destroyed, the pool is in a state where one can no > >> longer get new entries from it. So giving every thread its own reference > >> is pretending to make it an equal co-owner of the pool, but it is not as > >> the pool must not outlive the main AVCodecContext. > > > > But the only use of that opaque is checking whether frame threading is > > in use, which is a constant during decoder lifetime. Might be cleaner to > > avoid using AVCodecContext as opaque. > > In any case, this is not important, feel free to leave it as is. > > > > But whether frame threading is in use is only determined after > ff_decode_preinit(). I realize, which is why I'm not insisting you change this. But it could be handled, e.g. by allocating the pool later, or splitting off the part of thread init that determines active_thread_type.
Anton Khirnov: > Quoting Andreas Rheinhardt (2024-04-10 10:02:55) >> Anton Khirnov: >>> Quoting Andreas Rheinhardt (2024-04-10 09:09:00) >>>> Anton Khirnov: >>>>> Quoting Andreas Rheinhardt (2024-04-08 22:13:40) >>>>>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c >>>>>> index fd356bd190..6b2c4312e0 100644 >>>>>> --- a/libavcodec/pthread_frame.c >>>>>> +++ b/libavcodec/pthread_frame.c >>>>>> @@ -779,6 +779,7 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free, >>>>>> if (!copy->internal) >>>>>> return AVERROR(ENOMEM); >>>>>> copy->internal->thread_ctx = p; >>>>>> + copy->internal->progress_frame_pool = avctx->internal->progress_frame_pool; >>>>> >>>>> I'd still prefer every thread to have its own reference. >>>>> >>>>> Looks good otherwise. >>>>> >>>> >>>> The opaque of this pool is the main AVCodecContext; if the main >>>> AVCodecContext is destroyed, the pool is in a state where one can no >>>> longer get new entries from it. So giving every thread its own reference >>>> is pretending to make it an equal co-owner of the pool, but it is not as >>>> the pool must not outlive the main AVCodecContext. >>> >>> But the only use of that opaque is checking whether frame threading is >>> in use, which is a constant during decoder lifetime. Might be cleaner to >>> avoid using AVCodecContext as opaque. >>> In any case, this is not important, feel free to leave it as is. >>> >> >> But whether frame threading is in use is only determined after >> ff_decode_preinit(). > > I realize, which is why I'm not insisting you change this. > > But it could be handled, e.g. by allocating the pool later, or splitting > off the part of thread init that determines active_thread_type. > Which is IMO less elegant than simply doing what I did here. - Andreas
diff --git a/libavcodec/Makefile b/libavcodec/Makefile index 7f6de4470e..472568bd6c 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -56,6 +56,7 @@ OBJS = ac3_parser.o \ qsv_api.o \ raw.o \ refstruct.o \ + threadprogress.o \ utils.o \ version.o \ vlc.o \ diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c index 525fe516bd..888dd76228 100644 --- a/libavcodec/avcodec.c +++ b/libavcodec/avcodec.c @@ -441,6 +441,7 @@ av_cold void ff_codec_close(AVCodecContext *avctx) av_frame_free(&avci->recon_frame); ff_refstruct_unref(&avci->pool); + ff_refstruct_pool_uninit(&avci->progress_frame_pool); ff_hwaccel_uninit(avctx); diff --git a/libavcodec/codec_internal.h b/libavcodec/codec_internal.h index d6757e2def..832e6440d7 100644 --- a/libavcodec/codec_internal.h +++ b/libavcodec/codec_internal.h @@ -62,6 +62,10 @@ * Codec initializes slice-based threading with a main function */ #define FF_CODEC_CAP_SLICE_THREAD_HAS_MF (1 << 5) +/** + * The decoder might make use of the ProgressFrame API. + */ +#define FF_CODEC_CAP_USES_PROGRESSFRAMES (1 << 11) /* * The codec supports frame threading and has inter-frame dependencies, so it * uses ff_thread_report/await_progress(). diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 255347766a..f18f85c33c 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -49,8 +49,10 @@ #include "hwconfig.h" #include "internal.h" #include "packet_internal.h" +#include "progressframe.h" #include "refstruct.h" #include "thread.h" +#include "threadprogress.h" typedef struct DecodeContext { AVCodecInternal avci; @@ -1668,6 +1670,116 @@ int ff_reget_buffer(AVCodecContext *avctx, AVFrame *frame, int flags) return ret; } +typedef struct ProgressInternal { + ThreadProgress progress; + struct AVFrame *f; +} ProgressInternal; + +static void check_progress_consistency(const ProgressFrame *f) +{ + av_assert1(!!f->f == !!f->progress); + av_assert1(!f->progress || f->progress->f == f->f); +} + +static int progress_frame_get(AVCodecContext *avctx, ProgressFrame *f) +{ + FFRefStructPool *pool = avctx->internal->progress_frame_pool; + + av_assert1(!f->f && !f->progress); + + f->progress = ff_refstruct_pool_get(pool); + if (!f->progress) + return AVERROR(ENOMEM); + + f->f = f->progress->f; + return 0; +} + +int ff_progress_frame_get_buffer(AVCodecContext *avctx, ProgressFrame *f, int flags) +{ + int ret; + + ret = progress_frame_get(avctx, f); + if (ret < 0) + return ret; + + ret = ff_thread_get_buffer(avctx, f->progress->f, flags); + if (ret < 0) { + f->f = NULL; + ff_refstruct_unref(&f->progress); + return ret; + } + return 0; +} + +void ff_progress_frame_ref(ProgressFrame *dst, const ProgressFrame *src) +{ + av_assert1(src->progress && src->f && src->f == src->progress->f); + av_assert1(!dst->f && !dst->progress); + dst->f = src->f; + dst->progress = ff_refstruct_ref(src->progress); +} + +void ff_progress_frame_unref(ProgressFrame *f) +{ + check_progress_consistency(f); + f->f = NULL; + ff_refstruct_unref(&f->progress); +} + +void ff_progress_frame_replace(ProgressFrame *dst, const ProgressFrame *src) +{ + if (dst == src) + return; + ff_progress_frame_unref(dst); + check_progress_consistency(src); + if (src->f) + ff_progress_frame_ref(dst, src); +} + +void ff_progress_frame_report(ProgressFrame *f, int n) +{ + ff_thread_progress_report(&f->progress->progress, n); +} + +void ff_progress_frame_await(const ProgressFrame *f, int n) +{ + ff_thread_progress_await(&f->progress->progress, n); +} + +static av_cold int progress_frame_pool_init_cb(FFRefStructOpaque opaque, void *obj) +{ + const AVCodecContext *avctx = opaque.nc; + ProgressInternal *progress = obj; + int ret; + + ret = ff_thread_progress_init(&progress->progress, avctx->active_thread_type & FF_THREAD_FRAME); + if (ret < 0) + return ret; + + progress->f = av_frame_alloc(); + if (!progress->f) + return AVERROR(ENOMEM); + + return 0; +} + +static void progress_frame_pool_reset_cb(FFRefStructOpaque unused, void *obj) +{ + ProgressInternal *progress = obj; + + ff_thread_progress_reset(&progress->progress); + av_frame_unref(progress->f); +} + +static av_cold void progress_frame_pool_free_entry_cb(FFRefStructOpaque opaque, void *obj) +{ + ProgressInternal *progress = obj; + + ff_thread_progress_destroy(&progress->progress); + av_frame_free(&progress->f); +} + int ff_decode_preinit(AVCodecContext *avctx) { AVCodecInternal *avci = avctx->internal; @@ -1766,6 +1878,16 @@ int ff_decode_preinit(AVCodecContext *avctx) if (!avci->in_pkt || !avci->last_pkt_props) return AVERROR(ENOMEM); + if (ffcodec(avctx->codec)->caps_internal & FF_CODEC_CAP_USES_PROGRESSFRAMES) { + avci->progress_frame_pool = + ff_refstruct_pool_alloc_ext(sizeof(ProgressInternal), + FF_REFSTRUCT_POOL_FLAG_FREE_ON_INIT_ERROR, + avctx, progress_frame_pool_init_cb, + progress_frame_pool_reset_cb, + progress_frame_pool_free_entry_cb, NULL); + if (!avci->progress_frame_pool) + return AVERROR(ENOMEM); + } ret = decode_bsfs_init(avctx); if (ret < 0) return ret; diff --git a/libavcodec/internal.h b/libavcodec/internal.h index 64fe0122c8..bc20a797ae 100644 --- a/libavcodec/internal.h +++ b/libavcodec/internal.h @@ -61,6 +61,8 @@ typedef struct AVCodecInternal { struct FramePool *pool; + struct FFRefStructPool *progress_frame_pool; + void *thread_ctx; /** diff --git a/libavcodec/progressframe.h b/libavcodec/progressframe.h new file mode 100644 index 0000000000..dc841f30d2 --- /dev/null +++ b/libavcodec/progressframe.h @@ -0,0 +1,133 @@ +/* + * Copyright (c) 2022 Andreas Rheinhardt <andreas.rheinhardt@outlook.com> + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef AVCODEC_PROGRESSFRAME_H +#define AVCODEC_PROGRESSFRAME_H + +/** + * ProgressFrame is an API to easily share frames without an underlying + * av_frame_ref(). Its main usecase is in frame-threading scenarios, + * yet it could also be used for purely single-threaded decoders that + * want to keep multiple references to the same frame. + * + * The underlying principle behind the API is that all that is needed + * to share a frame is a reference count and a contract between all parties. + * The ProgressFrame provides the reference count and the frame is unreferenced + * via ff_thread_release_buffer() when the reference count reaches zero. + * + * In order to make this API also usable for frame-threaded decoders it also + * provides a way of exchanging simple information about the state of + * decoding the frame via ff_thread_progress_report() and + * ff_thread_progress_await(). + * + * The typical contract for frame-threaded decoders is as follows: + * Thread A initializes a ProgressFrame via ff_thread_progress_get_buffer() + * (which already allocates the AVFrame's data buffers), calls + * ff_thread_finish_setup() and starts decoding the frame. Later threads + * receive a reference to this frame, which means they get a pointer + * to the AVFrame and the internal reference count gets incremented. + * Later threads whose frames use A's frame as reference as well as + * the thread that will eventually output A's frame will wait for + * progress on said frame reported by A. As soon as A has reported + * that it has finished decoding its frame, it must no longer modify it + * (neither its data nor its properties). + * + * Because creating a reference with this API does not involve reads + * from the actual AVFrame, the decoding thread may modify the properties + * (i.e. non-data fields) until it has indicated to be done with this + * frame. This is important for e.g. propagating decode_error_flags; + * it also allows to add side-data late. + */ + +struct AVCodecContext; + +typedef struct ProgressFrame { + struct AVFrame *f; + struct ProgressInternal *progress; +} ProgressFrame; + +/** + * Notify later decoding threads when part of their reference frame is ready. + * Call this when some part of the frame is finished decoding. + * Later calls with lower values of progress have no effect. + * + * @param f The frame being decoded. + * @param progress Value, in arbitrary units, of how much of the frame has decoded. + * + * @warning Calling this on a blank ProgressFrame causes undefined behaviour + */ +void ff_progress_frame_report(ProgressFrame *f, int progress); + +/** + * Wait for earlier decoding threads to finish reference frames. + * Call this before accessing some part of a frame, with a given + * value for progress, and it will return after the responsible decoding + * thread calls ff_thread_progress_report() with the same or + * higher value for progress. + * + * @param f The frame being referenced. + * @param progress Value, in arbitrary units, to wait for. + * + * @warning Calling this on a blank ProgressFrame causes undefined behaviour + */ +void ff_progress_frame_await(const ProgressFrame *f, int progress); + +/** + * This function sets up the ProgressFrame, i.e. gets ProgressFrame.f + * and also calls ff_thread_get_buffer() on the frame. + * + * @note: This must only be called by codecs with the + * FF_CODEC_CAP_USES_PROGRESSFRAMES internal cap. + */ +int ff_progress_frame_get_buffer(struct AVCodecContext *avctx, + ProgressFrame *f, int flags); + +/** + * Give up a reference to the underlying frame contained in a ProgressFrame + * and reset the ProgressFrame, setting all pointers to NULL. + * + * @note: This implies that when using this API the check for whether + * a frame exists is by checking ProgressFrame.f and not + * ProgressFrame.f->data[0] or ProgressFrame.f->buf[0]. + */ +void ff_progress_frame_unref(ProgressFrame *f); + +/** + * Set dst->f to src->f and make dst a co-owner of src->f. + * dst can then be used to wait on progress of the underlying frame. + * + * @note: There is no underlying av_frame_ref() here. dst->f and src->f + * really point to the same AVFrame. Typically this means that + * the decoding thread is allowed to set all the properties of + * the AVFrame until it has indicated to have finished decoding. + * Afterwards later threads may read all of these fields. + * Access to the frame's data is governed by + * ff_thread_progress_report/await(). + */ +void ff_progress_frame_ref(ProgressFrame *dst, const ProgressFrame *src); + +/** + * Do nothing if dst and src already refer to the same AVFrame; + * otherwise unreference dst and if src is not blank, put a reference + * to src's AVFrame in its place (in case src is not blank). + */ +void ff_progress_frame_replace(ProgressFrame *dst, const ProgressFrame *src); + +#endif /* AVCODEC_PROGRESSFRAME_H */ diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index fd356bd190..6b2c4312e0 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -779,6 +779,7 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free, if (!copy->internal) return AVERROR(ENOMEM); copy->internal->thread_ctx = p; + copy->internal->progress_frame_pool = avctx->internal->progress_frame_pool; copy->delay = avctx->delay; diff --git a/libavcodec/tests/avcodec.c b/libavcodec/tests/avcodec.c index 08ca507bf0..58f54cac74 100644 --- a/libavcodec/tests/avcodec.c +++ b/libavcodec/tests/avcodec.c @@ -141,7 +141,8 @@ int main(void){ ret = 1; } } - if (codec2->caps_internal & (FF_CODEC_CAP_ALLOCATE_PROGRESS | + if (codec2->caps_internal & (FF_CODEC_CAP_USES_PROGRESSFRAMES | + FF_CODEC_CAP_ALLOCATE_PROGRESS | FF_CODEC_CAP_SETS_PKT_DTS | FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM | FF_CODEC_CAP_EXPORTS_CROPPING |
Frame-threaded decoders with inter-frame dependencies use the ThreadFrame API for syncing. It works as follows: During init each thread allocates an AVFrame for every ThreadFrame. Thread A reads the header of its packet and allocates a buffer for an AVFrame with ff_thread_get_ext_buffer() (which also allocates a small structure that is shared with other references to this frame) and sets its fields, including side data. Then said thread calls ff_thread_finish_setup(). From that moment onward it is not allowed to change any of the AVFrame fields at all any more, but it may change fields which are an indirection away, like the content of AVFrame.data or already existing side data. After thread A has called ff_thread_finish_setup(), another thread (the user one) calls the codec's update_thread_context callback which in turn calls ff_thread_ref_frame() which calls av_frame_ref() which reads every field of A's AVFrame; hence the above restriction on modifications of the AVFrame (as any modification of the AVFrame by A after ff_thread_finish_setup() would be a data race). Of course, this av_frame_ref() also incurs allocations and therefore needs to be checked. ff_thread_ref_frame() also references the small structure used for communicating progress. This av_frame_ref() makes it awkward to propagate values that only become known during decoding to later threads (in case of frame reordering or other mechanisms of delayed output (like show-existing-frames) it's not the decoding thread, but a later thread that returns the AVFrame). E.g. for VP9 when exporting video encoding parameters as side data the number of blocks only becomes known during decoding, so one can't allocate the side data before ff_thread_finish_setup(). It is currently being done afterwards and this leads to a data race in the vp9-encparams test when using frame-threading. Returning decode_error_flags is also complicated by this. To perform this exchange a buffer shared between the references is needed (notice that simply giving the later threads a pointer to the original AVFrame does not work, because said AVFrame will be reused lateron when thread A decodes the next packet given to it). One could extend the buffer already used for progress for this or use a new one (requiring yet another allocation), yet both of these approaches have the drawback of being unnatural, ugly and requiring quite a lot of ad-hoc code. E.g. in case of the VP9 side data mentioned above one could not simply use the helper that allocates and adds the side data to an AVFrame in one go. The ProgressFrame API meanwhile offers a different solution to all of this. It is based around the idea that the most natural shared object for sharing information about an AVFrame between decoding threads is the AVFrame itself. To actually implement this the AVFrame needs to be reference counted. This is achieved by putting a (ownership) pointer into a shared (and opaque) structure that is managed by the RefStruct API and which also contains the stuff necessary for progress reporting. The users get a pointer to this AVFrame with the understanding that the owner may set all the fields until it has indicated that it has finished decoding this AVFrame; then the users are allowed to read everything. Every decoder may of course employ a different contract than the one outlined above. Given that there is no underlying av_frame_ref(), creating references to a ProgressFrame can't fail. Only ff_thread_progress_get_buffer() can fail, but given that it will replace calls to ff_thread_get_ext_buffer() it is at places where errors are already expected and properly taken care of. The ProgressFrames are empty (i.e. the AVFrame pointer is NULL and the AVFrames are not allocated during init at all) while not being in use; ff_thread_progress_get_buffer() both sets up the actual ProgressFrame and already calls ff_thread_get_buffer(). So instead of checking for ThreadFrame.f->data[0] or ThreadFrame.f->buf[0] being NULL for "this reference frame is non-existing" one should check for ProgressFrame.f. This also implies that one can only set AVFrame properties after having allocated the buffer. This restriction is not deep: if it becomes onerous for any codec, ff_thread_progress_get_buffer() can be broken up. The user would then have to get a buffer himself. In order to avoid unnecessary allocations, the shared structure is pooled, so that both the structure as well as the AVFrame itself are reused. This means that there won't be lots of unnecessary allocations in case of non-frame-threaded decoding. It might even turn out to have fewer than the current code (the current code allocates AVFrames for every DPB slot, but these are often excessively large and not completely used; the new code allocates them on demand). Pooling relies on the reset function of the RefStruct pool API, it would be impossible to implement with the AVBufferPool API. Finally, ProgressFrames have no notion of owner; they are built on top of the ThreadProgress API which also lacks such a concept. Instead every ThreadProgress and every ProgressFrame contains its own mutex and condition variable, making it completely independent of pthread_frame.c. Just like the ThreadFrame API it is simply presumed that only the actual owner/producer of a frame reports progress on said frame. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/Makefile | 1 + libavcodec/avcodec.c | 1 + libavcodec/codec_internal.h | 4 ++ libavcodec/decode.c | 122 +++++++++++++++++++++++++++++++++ libavcodec/internal.h | 2 + libavcodec/progressframe.h | 133 ++++++++++++++++++++++++++++++++++++ libavcodec/pthread_frame.c | 1 + libavcodec/tests/avcodec.c | 3 +- 8 files changed, 266 insertions(+), 1 deletion(-) create mode 100644 libavcodec/progressframe.h