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 80C1E476D8 for ; Sat, 21 Oct 2023 10:34:50 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3BB1B68CAD1; Sat, 21 Oct 2023 13:34:47 +0300 (EEST) Received: from mail0.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 74B1C68CA22 for ; Sat, 21 Oct 2023 13:34:40 +0300 (EEST) Received: from localhost (localhost [IPv6:::1]) by mail0.khirnov.net (Postfix) with ESMTP id 1A2DE240498; Sat, 21 Oct 2023 12:34:40 +0200 (CEST) Received: from mail0.khirnov.net ([IPv6:::1]) by localhost (mail0.khirnov.net [IPv6:::1]) (amavis, port 10024) with ESMTP id bhBBO7uwFqfP; Sat, 21 Oct 2023 12:34:35 +0200 (CEST) Received: from lain.khirnov.net (lain.khirnov.net [IPv6:2001:67c:1138:4306::3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "lain.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail0.khirnov.net (Postfix) with ESMTPS id 23DAC2400FF; Sat, 21 Oct 2023 12:34:35 +0200 (CEST) Received: by lain.khirnov.net (Postfix, from userid 1000) id 21FC21601B9; Sat, 21 Oct 2023 12:34:34 +0200 (CEST) From: Anton Khirnov To: FFmpeg development discussions and patches In-Reply-To: References: Mail-Followup-To: FFmpeg development discussions and patches , Andreas Rheinhardt Date: Sat, 21 Oct 2023 12:34:34 +0200 Message-ID: <169788447411.30698.2593801220456381390@lain.khirnov.net> User-Agent: alot/0.8.1 MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH 27/42] avcodec/pthread_frame: Add new progress 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 Cc: Andreas Rheinhardt Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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 > + * > + * 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-request@ffmpeg.org with subject "unsubscribe".