From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id E8DB349301 for ; Tue, 9 Apr 2024 01:44:59 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 77FA268D3BD; Tue, 9 Apr 2024 04:44:56 +0300 (EEST) Received: from mslow1.mail.gandi.net (mslow1.mail.gandi.net [217.70.178.240]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 6EF5E68D128 for ; Tue, 9 Apr 2024 04:44:50 +0300 (EEST) Received: from relay2-d.mail.gandi.net (unknown [IPv6:2001:4b98:dc4:8::222]) by mslow1.mail.gandi.net (Postfix) with ESMTP id 738C1C0EA6 for ; Tue, 9 Apr 2024 01:40:14 +0000 (UTC) Received: by mail.gandi.net (Postfix) with ESMTPSA id DC69540002 for ; Tue, 9 Apr 2024 01:40:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc; s=gm1; t=1712626808; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=YYwd8aIGDoSpgkn2fejMGI/kf7of45xFeVmy5yMvQR4=; b=UIFEkGlia9KvsjoYNJq1M9UPAIexLhBctDDiQCf7y7G3Ak9VwFUjTQhO7MC4GPiZ40geqa YJjH7uDzLEozbP3hnSLHJ4UtWizaMvnb98OQeoi0I4D+qgZk77aMBvnFj5PAz1wHj9VXO2 icaAmbHXIPZ0sl23lTU+CVFh7ID4anhICvbJgs4SjwHCxYK+ayYR5Gia4Dqp/TsdgXf43y GuNojK2FoleTOinrQde2fwd/q3DWCjyDCQsrdJaTF4H+Hu9gzgavvlrM+Rcev4aRJNtukS BhdpciRtoSSEe9JlmDqtUN2Cpbyh613r4Sa5qebhbHwjDBYLqgm3go01UYad0g== Date: Tue, 9 Apr 2024 03:40:07 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20240409014007.GK6420@pb2> References: MIME-Version: 1.0 In-Reply-To: X-GND-Sasl: michael@niedermayer.cc Subject: Re: [FFmpeg-devel] [PATCH v2 02/27] avcodec/decode: Add new ProgressFrame API X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: multipart/mixed; boundary="===============8332697971199010105==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============8332697971199010105== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="/T0IDKhzUL1ElznN" Content-Disposition: inline --/T0IDKhzUL1ElznN Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > During init each thread allocates an AVFrame for every > ThreadFrame. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > Signed-off-by: Andreas Rheinhardt > --- > 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 =2E/configure --disable-pthreads --cc=3D'ccache gcc' && make -j32 =2E.. libavcodec/threadprogress.c: In function =E2=80=98ff_thread_progress_report= =E2=80=99: libavcodec/threadprogress.c:54:5: error: implicit declaration of function = =E2=80=98pthread_mutex_lock=E2=80=99; did you mean =E2=80=98ff_mutex_lock= =E2=80=99? [-Werror=3Dimplicit-function-declaration] pthread_mutex_lock(&pro->progress_mutex); ^~~~~~~~~~~~~~~~~~ ff_mutex_lock libavcodec/threadprogress.c:55:5: error: implicit declaration of function = =E2=80=98pthread_cond_broadcast=E2=80=99; did you mean =E2=80=98ff_cond_bro= adcast=E2=80=99? [-Werror=3Dimplicit-function-declaration] pthread_cond_broadcast(&pro->progress_cond); ^~~~~~~~~~~~~~~~~~~~~~ ff_cond_broadcast libavcodec/threadprogress.c:56:5: error: implicit declaration of function = =E2=80=98pthread_mutex_unlock=E2=80=99; did you mean =E2=80=98ff_mutex_unlo= ck=E2=80=99? [-Werror=3Dimplicit-function-declaration] pthread_mutex_unlock(&pro->progress_mutex); ^~~~~~~~~~~~~~~~~~~~ ff_mutex_unlock libavcodec/threadprogress.c: In function =E2=80=98ff_thread_progress_await= =E2=80=99: libavcodec/threadprogress.c:71:9: error: implicit declaration of function = =E2=80=98pthread_cond_wait=E2=80=99; did you mean =E2=80=98ff_cond_wait=E2= =80=99? [-Werror=3Dimplicit-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' fail= ed make: *** [libavcodec/threadprogress.o] Error 1 make: *** Waiting for unfinished jobs.... [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The smallest minority on earth is the individual. Those who deny=20 individual rights cannot claim to be defenders of minorities. - Ayn Rand --/T0IDKhzUL1ElznN Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZhSccwAKCRBhHseHBAsP q3E9AJ4pMAbagvb4NjsStNdFFJZPxz0mvgCghytqMhA8qyCrtuT5cZK78PbrDGE= =j57F -----END PGP SIGNATURE----- --/T0IDKhzUL1ElznN-- --===============8332697971199010105== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". --===============8332697971199010105==--