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 A51E340963 for ; Fri, 4 Mar 2022 00:03:18 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 198BF68B0A4; Fri, 4 Mar 2022 02:03:16 +0200 (EET) 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 C299668A8BE for ; Fri, 4 Mar 2022 02:03:09 +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 C9734FF802 for ; Fri, 4 Mar 2022 00:03:08 +0000 (UTC) Date: Fri, 4 Mar 2022 01:03:07 +0100 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20220304000307.GC2829255@pb2> References: MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH v2] 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="===============5442560756264343793==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============5442560756264343793== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="YMMAg1GRa9D1x0WF" Content-Disposition: inline --YMMAg1GRa9D1x0WF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 03, 2022 at 08:55:08AM +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 | 119 +++++++++++++++++++++++++++++++++++-------- > 2 files changed, 103 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..bda8a5a2f1 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. */ 0/0 is unknown in FFV1, 0/1 and 1/0 are treated as unknown because thats the only reasonable thing one really could do if they are encountered > + 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,21 @@ static av_cold int decode_init(AVCodecContext *avctx) > return 0; > } > =20 > +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 ratios so that these values are at the end. */ > + if (x.num =3D=3D 0 || y.num =3D=3D 0) > + return y.num - x.num; > + cmp =3D av_cmp_q(x, y); > + /* For definiteness, order equivalent fractions by "lower terms last= ". */ > + return cmp ? cmp : y.num - x.num; > +} you can probably simplify this if you map the 32/32 fraction into a 64bit i= nt also should be faster i also wonder a bit if it would make sense to split this "find the most com= mon element" code out or if that makes no sense. > + > static int decode_frame(AVCodecContext *avctx, void *data, int *got_fram= e, AVPacket *avpkt) > { > uint8_t *buf =3D avpkt->data; > @@ -969,9 +979,78 @@ static int decode_frame(AVCodecContext *avctx, void = *data, int *got_frame, AVPac > =20 > if (f->last_picture.f) > ff_thread_release_ext_buffer(avctx, &f->last_picture); > - if ((ret =3D av_frame_ref(data, f->picture.f)) < 0) > + p =3D data; > + if ((ret =3D av_frame_ref(p, f->picture.f)) < 0) > return ret; > =20 > + if (f->version > 2) { > + AVRational sar =3D f->slice_context[0]->sample_aspect_ratio; > + int pic_structures[4] =3D { 0 }, picture_structure; > + int inconsistent_sar =3D 0, has_sar =3D 0; > + > + for (int i =3D 0; i < f->slice_count; i++) { > + const FFV1Context *const fs =3D f->slice_context[i]; > + int slice_picture_structure =3D (unsigned)fs->picture_struct= ure > 3 ? > + 0 : fs->picture_stru= cture; > + > + pic_structures[slice_picture_structure]++; > + > + if (fs->sample_aspect_ratio.num !=3D sar.num || > + fs->sample_aspect_ratio.den !=3D sar.den) > + inconsistent_sar =3D 1; > + sar =3D fs->sample_aspect_ratio; > + if (sar.num =3D=3D 0) > + inconsistent_sar =3D 1; > + else > + has_sar =3D 1; > + f->slice_sample_aspect_ratios[i] =3D sar; > + } > + if (has_sar) { > + if (inconsistent_sar) { > + AVRational last_sar =3D (AVRational){ 0, 0 }; > + qsort(f->slice_sample_aspect_ratios, f->slice_count, > + sizeof(f->slice_sample_aspect_ratios[0]), compare_= sar); > + > + for (int i =3D 0, current_run =3D 0, longest_run =3D 0; > + i < f->slice_count; i++) { > + AVRational cur_sar =3D f->slice_sample_aspect_ratios= [i]; > + > + if (cur_sar.num =3D=3D 0) > + break; > + if (av_cmp_q(cur_sar, last_sar)) > + current_run =3D 1; > + else > + current_run++; > + if (current_run > longest_run) { > + longest_run =3D current_run; > + sar =3D cur_sar; > + } > + last_sar =3D cur_sar; > + } > + av_log(avctx, AV_LOG_WARNING, "SAR inconsistent, using %= u/%u\n", > + sar.num, sar.den); > + } > + p->sample_aspect_ratio =3D sar; > + } if there are 10 slices and 9 say aspect unknown and one says 5000:3 i would belive the 9 more, this code looks a bit like it would always favor known over unknown [...] thx --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB What does censorship reveal? It reveals fear. -- Julian Assange --YMMAg1GRa9D1x0WF Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCYiFXOAAKCRBhHseHBAsP q6jhAJ9LjqaUTOQDL02+dPaYXhlFNG66OQCfSNjdmBphamsVQA3KjkZgqV8juPs= =de/S -----END PGP SIGNATURE----- --YMMAg1GRa9D1x0WF-- --===============5442560756264343793== 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". --===============5442560756264343793==--