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 6C7B140A7B for ; Sat, 5 Mar 2022 19:44:46 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 12E1468B0DB; Sat, 5 Mar 2022 21:44:44 +0200 (EET) Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 63E2268AB64 for ; Sat, 5 Mar 2022 21:44:37 +0200 (EET) Received: from localhost (213-47-68-29.cable.dynamic.surfer.at [213.47.68.29]) (Authenticated sender: michael@niedermayer.cc) by mail.gandi.net (Postfix) with ESMTPSA id 818E960002 for ; Sat, 5 Mar 2022 19:44:36 +0000 (UTC) Date: Sat, 5 Mar 2022 20:44:35 +0100 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20220305194435.GH2829255@pb2> References: MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH v3] avcodec/ffv1dec: Don't set ThreadFrame properties, fix race 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="===============3235570964774370078==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============3235570964774370078== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="08JdqxoOYh0nnW3b" Content-Disposition: inline --08JdqxoOYh0nnW3b Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 04, 2022 at 04:40:54AM +0100, Andreas Rheinhardt wrote: > Each FFV1 slice has its own SAR and picture structure encoded; > when a slice header was parsed, the relevant fields of a ThreadFrame's > AVFrame were directly set based upon the parsed values. This is > a data race in case slice threading is in use because of the concurrent > writes. In case of frame threading, it is also a data race because > the writes happen after ff_thread_finish_setup(), so that the reads > performed by ff_thread_ref_frame() are unsynchronized with the writes > performed when parsing the header. >=20 > This commit fixes these issues by not writing to the ThreadFrame at all; > instead the raw data is read into the each SliceContext first; after > decoding the current frame and creating the actual output frame these > values are compared to each other. If they are valid and coincide, the > derived value is written directly to the output frame, not to the > ThreadFrame, thereby avoiding data races; in case they are not valid > or inconsistent the most commonly used valid value is used instead. >=20 > This fixes most FFV1 FATE-tests completely when using slice threading; > the exceptions are fate-vsynth3-ffv1, vsynth3-ffv1-v3-yuv420p and > vsynth3-ffv1-v3-yuv422p10. (In these tests the partitioning into slices > does not honour chroma subsampling; as a result, chroma pixels at slice > borders get set by more than one thread without any synchronization.) >=20 > Signed-off-by: Andreas Rheinhardt > --- > libavcodec/ffv1.h | 4 ++ > libavcodec/ffv1dec.c | 130 ++++++++++++++++++++++++++++++++++++------- > 2 files changed, 114 insertions(+), 20 deletions(-) >=20 > diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h > index ac80fa85ce..f640d5a710 100644 > --- a/libavcodec/ffv1.h > +++ b/libavcodec/ffv1.h > @@ -91,6 +91,8 @@ typedef struct FFV1Context { > struct FFV1Context *fsrc; > =20 > AVFrame *cur; > + int picture_structure; > + AVRational sample_aspect_ratio; > int plane_count; > int ac; ///< 1=3Drange coder <-> 0=3Dgo= lomb rice > int ac_byte_count; ///< number of bytes used for A= C coding > @@ -132,6 +134,8 @@ typedef struct FFV1Context { > int slice_coding_mode; > int slice_rct_by_coef; > int slice_rct_ry_coef; > + > + AVRational slice_sample_aspect_ratios[MAX_SLICES]; > } FFV1Context; > =20 > int ff_ffv1_common_init(AVCodecContext *avctx); > diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c > index 201630167d..8a8ab90b2b 100644 > --- a/libavcodec/ffv1dec.c > +++ b/libavcodec/ffv1dec.c > @@ -167,7 +167,7 @@ static int decode_slice_header(const FFV1Context *f, = FFV1Context *fs) > { > RangeCoder *c =3D &fs->c; > uint8_t state[CONTEXT_SIZE]; > - unsigned ps, i, context_count; > + unsigned i, context_count; > memset(state, 128, sizeof(state)); > =20 > av_assert0(f->version > 2); > @@ -205,25 +205,17 @@ static int decode_slice_header(const FFV1Context *f= , FFV1Context *fs) > p->context_count =3D context_count; > } > =20 > - ps =3D get_symbol(c, state, 0); > - if (ps =3D=3D 1) { > - f->cur->interlaced_frame =3D 1; > - f->cur->top_field_first =3D 1; > - } else if (ps =3D=3D 2) { > - f->cur->interlaced_frame =3D 1; > - f->cur->top_field_first =3D 0; > - } else if (ps =3D=3D 3) { > - f->cur->interlaced_frame =3D 0; > - } > - f->cur->sample_aspect_ratio.num =3D get_symbol(c, state, 0); > - f->cur->sample_aspect_ratio.den =3D get_symbol(c, state, 0); > - > - if (av_image_check_sar(f->width, f->height, > - f->cur->sample_aspect_ratio) < 0) { > + fs->picture_structure =3D get_symbol(c, state, 0); > + fs->sample_aspect_ratio.num =3D get_symbol(c, state, 0); > + fs->sample_aspect_ratio.den =3D get_symbol(c, state, 0); > + /* Num or den being zero means unknown for FFV1; our unknown is 0 / = 1. */ > + if (fs->sample_aspect_ratio.num =3D=3D 0 || fs->sample_aspect_ratio.= den =3D=3D 0) { > + fs->sample_aspect_ratio =3D (AVRational) { 0, 1 }; > + } else if (av_image_check_sar(f->width, f->height, > + fs->sample_aspect_ratio) < 0) { > av_log(f->avctx, AV_LOG_WARNING, "ignoring invalid SAR: %u/%u\n", > - f->cur->sample_aspect_ratio.num, > - f->cur->sample_aspect_ratio.den); > - f->cur->sample_aspect_ratio =3D (AVRational){ 0, 1 }; > + fs->sample_aspect_ratio.num, fs->sample_aspect_ratio.den); > + fs->sample_aspect_ratio =3D (AVRational) { 0, 0 }; > } > =20 > if (fs->version > 3) { > @@ -251,6 +243,9 @@ static int decode_slice(AVCodecContext *c, void *arg) > AVFrame * const p =3D f->cur; > int i, si; > =20 > + fs->picture_structure =3D 0; > + fs->sample_aspect_ratio =3D (AVRational){ 0, 0 }; > + > for( si=3D0; fs !=3D f->slice_context[si]; si ++) > ; > =20 > @@ -831,6 +826,28 @@ static av_cold int decode_init(AVCodecContext *avctx) > return 0; > } > =20 > +/* Macro to simplify comparisons of the rational values we deal with her= e. > + * get_symbol() ensures that these fit into 32bits, so that one can just > + * compare them in 64bits; they are also actually unsigned, so cast to t= hat. > + * Notice that av_image_check_sar() checks for these values to be > + * positive integers. */ > +#define CMP_Q(x, y) (FFDIFFSIGN((unsigned)(x).num * (uint64_t)(unsigned)= (y).den,\ > + (unsigned)(y).num * (uint64_t)(unsigned)= (x).den)) > +static int compare_sar(const void *a, const void *b) > +{ > + const AVRational x =3D *(const AVRational*)a; > + const AVRational y =3D *(const AVRational*)b; > + int cmp; > + > + /* 0/0 means invalid and 0/1 means unknown. > + * Order the fractions so that the former are at the end. */ > + if (x.den =3D=3D 0 || y.den =3D=3D 0) > + return y.den - x.den; > + cmp =3D CMP_Q(x, y); > + /* For definiteness, order equivalent fractions by "lower terms last= ". */ > + return cmp ? cmp : y.num - x.num; > +} Can you explain why you compare them "by value". There should not be=20 2 slices one with 4/3 and one with 8/6. That would be broken, these headers should be identical. A encoder should write the same values in each slice. if two differ that implies damage occured to one of them or they are not slices of the same frame Based on this i assumed that the most common pair is the most likely undama= ged not requireing interpretation of the fractions Not against how you do it, it just feels unneeded but maybe iam missing som= ething if you want it to be this way because you prefer it thats fine too. I am ju= st trying to understand why we seem to see a different solution as better here thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Whats the most studid thing your enemy could do ? Blow himself up Whats the most studid thing you could do ? Give up your rights and freedom because your enemy blew himself up. --08JdqxoOYh0nnW3b Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCYiO9oAAKCRBhHseHBAsP q6U/AJ9CVKWOe/rKabHlzxogQ7aMHQCa1gCfXmWNwBNW1BtGlLEuBWTxk6ELiMo= =7Ey9 -----END PGP SIGNATURE----- --08JdqxoOYh0nnW3b-- --===============3235570964774370078== 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". --===============3235570964774370078==--