Quoting Andreas Rheinhardt (2023-09-19 21:57:19) > 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.
Do we really need an owner? I never liked this notion for ThreadFrames. Might as well make the mutex and the cond per-ProgressFrame and drop the debug logs - I never found them useful. Then this API could be entirely independent of the frame threading implementation and could potentially be used elsewhere. > +#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. nit: the name is not ideal for these other use cases. You could call it something like SharedFrame instead. > +typedef struct ProgressFrame { > + struct AVFrame *f; > + struct ProgressInternal *progress; > +} ProgressFrame; > + > +/** > + * Notify later decoding threads when part of their reference picture is > ready. s/picture/frame/ everywhere "picture" has a special meaning for interlaced video and might confuse readers. > diff --git a/libavcodec/progressframe_internal.h > b/libavcodec/progressframe_internal.h > new file mode 100644 > index 0000000000..1101207106 > --- /dev/null > +++ b/libavcodec/progressframe_internal.h > @@ -0,0 +1,32 @@ > +/* > + * Copyright (c) 2022 Andreas Rheinhardt <andreas.rheinha...@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_INTERNAL_H > +#define AVCODEC_PROGRESSFRAME_INTERNAL_H Mention who this header is for, so clue-free decoder writers don't start including it. > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > index 282f3fad58..9e827f0606 100644 > --- a/libavcodec/pthread_frame.c > +++ b/libavcodec/pthread_frame.c > @@ -34,6 +34,8 @@ > #include "hwaccel_internal.h" > #include "hwconfig.h" > #include "internal.h" > +#include "progressframe.h" > +#include "progressframe_internal.h" > #include "pthread_internal.h" > #include "refstruct.h" > #include "thread.h" > @@ -795,6 +797,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; It feels cleaner to me to make each of those a separate reference, especially since it's pretty much free. -- Anton Khirnov _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".