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 953F94C31D for ; Wed, 24 Jul 2024 19:53:50 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id AD5F068D701; Wed, 24 Jul 2024 22:53:47 +0300 (EEST) Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id D0E2B68D674 for ; Wed, 24 Jul 2024 22:53:40 +0300 (EEST) Received: by mail.gandi.net (Postfix) with ESMTPSA id 37055FF803 for ; Wed, 24 Jul 2024 19:53:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc; s=gm1; t=1721850820; 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=1ucKwwCPfb1zCLCs2PcPudS37j5FcMn3VeE6YAnzhiE=; b=CUfMqUv8cX/ox25D5IgAbbKBIT9h1TwGBfz2U8Jg87u/0BdXzIPVZaXXrskEdqTWo0JOgl H4cSMllRZn36PDEpl5x9315YD4xd+wiXiYQ1q+hw001wRwGvtlJTdtZ56c8WUF0/SlosG2 6wkfOHEbP+MPtm2jyLHwYrwmogZGcDJGPZoYYAUlnhicUhO98m1vTnWRRv8nITQwoNfdaP oMlWHV0nKzL7GPM4+dLxWVjYinVpMBqcnsakIQm0t90WcyF5fg3Mv/z9Kvq3Xg8RdVI/uA eCGYVKr4BZLbT4dWQ6mDO3E4QgDZ1lGaHCGQ+7rhptKBrYZnbjEHsSoh4BdRMQ== Date: Wed, 24 Jul 2024 21:53:39 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20240724195339.GG4991@pb2> References: <20240716171155.31838-1-anton@khirnov.net> <20240716171155.31838-27-anton@khirnov.net> MIME-Version: 1.0 In-Reply-To: <20240716171155.31838-27-anton@khirnov.net> X-GND-Sasl: michael@niedermayer.cc Subject: Re: [FFmpeg-devel] [PATCH 27/39] lavc/ffv1: change FFV1SliceContext.plane into a RefStruct object 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="===============3735017551081511727==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============3735017551081511727== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="PrI1xm21m7BS25/2" Content-Disposition: inline --PrI1xm21m7BS25/2 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 16, 2024 at 07:11:42PM +0200, Anton Khirnov wrote: > Frame threading in the FFV1 decoder works in a very unusual way - the > state that needs to be propagated from the previous frame is not decoded > pixels(=B9), but each slice's entropy coder state after decoding the slic= e. >=20 > For that purpose, the decoder's update_thread_context() callback stores > a pointer to the previous frame thread's private data. Then, when > decoding each slice, the frame thread uses the standard progress > mechanism to wait for the corresponding slice in the previous frame to > be completed, then copies the entropy coder state from the > previously-stored pointer. >=20 > This approach is highly dubious, as update_thread_context() should be > the only point where frame-thread contexts come into direct contact. > There are no guarantees that the stored pointer will be valid at all, or > will contain any particular data after update_thread_context() finishes. >=20 > More specifically, this code can break due to the fact that keyframes > reset entropy coder state and thus do not need to wait for the previous > frame. As an example, consider a decoder process with 2 frame threads - > thread 0 with its context 0, and thread 1 with context 1 - decoding a > previous frame P, current frame F, followed by a keyframe K. Then > consider concurrent execution consistent with the following sequence of > events: > * thread 0 starts decoding P > * thread 0 reads P's slice header, then calls > ff_thread_finish_setup() allowing next frame thread to start > * main thread calls update_thread_context() to transfer state from > context 0 to context 1; context 1 stores a pointer to context 0's priva= te > data > * thread 1 starts decoding F > * thread 1 reads F's slice header, then calls > ff_thread_finish_setup() allowing the next frame thread to start > decoding > * thread 0 finishes decoding P > * thread 0 starts decoding K; since K is a keyframe, it does not > wait for F and reallocates the arrays holding entropy coder state > * thread 0 finishes decoding K > * thread 1 reads entropy coder state from its stored pointer to context > 0, however it finds state from K rather than from P >=20 > This execution is currently prevented by special-casing FFV1 in the > generic frame threading code, however that is supremely ugly. It also > involves unnecessary copies of the state arrays, when in fact they can > only be used by one thread at a time. >=20 > This commit addresses these deficiencies by changing the array of > PlaneContext (each of which contains the allocated state arrays) > embedded in FFV1SliceContext into a RefStruct object. This object can > then be propagated across frame threads in standard manner. Since the > code structure guarantees only one thread accesses it at a time, no > copies are necessary. It is also re-created for keyframes, solving the > above issue cleanly. >=20 > Special-casing of FFV1 in the generic frame threading code will be > removed in a later commit. >=20 > (=B9) except in the case of a damaged slice, when previous frame's pixels > are used directly > --- > libavcodec/ffv1.c | 30 ++++++++++++++++++++++++------ > libavcodec/ffv1.h | 4 +++- > libavcodec/ffv1dec.c | 33 +++++++++------------------------ > 3 files changed, 36 insertions(+), 31 deletions(-) LGTM thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If a bugfix only changes things apparently unrelated to the bug with no further explanation, that is a good sign that the bugfix is wrong. --PrI1xm21m7BS25/2 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZqFbwwAKCRBhHseHBAsP qxoNAJsFd7VX6mVYTpwR0NF6DRKemaQizQCfSADS3zS0PUdfwKZiWLc7hiN3wd0= =PBZy -----END PGP SIGNATURE----- --PrI1xm21m7BS25/2-- --===============3735017551081511727== 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". --===============3735017551081511727==--