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 90B1041D54 for ; Wed, 15 Dec 2021 22:23:53 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A1B9C68AD39; Thu, 16 Dec 2021 00:23:50 +0200 (EET) Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id CED5C687F96 for ; Thu, 16 Dec 2021 00:23:43 +0200 (EET) Received: from localhost (213-47-68-29.cable.dynamic.surfer.at [213.47.68.29]) (Authenticated sender: michael@niedermayer.cc) by relay12.mail.gandi.net (Postfix) with ESMTPSA id 5B7D7200005; Wed, 15 Dec 2021 22:23:42 +0000 (UTC) Date: Wed, 15 Dec 2021 23:23:40 +0100 From: Michael Niedermayer To: FFmpeg development discussions and patches , Gyan Doshi Message-ID: <20211215222340.GX2829255@pb2> References: <20211123124507.8634-1-ffmpeg@gyani.pro> <20211123131106.8652-1-ffmpeg@gyani.pro> <20211123194630.GR2829255@pb2> <6d7ad8df-75a2-3da9-d30a-8d27717a9289@gyani.pro> <20211124192632.GS2829255@pb2> <9807e11a-c70e-512a-ce59-a408787ecefc@gyani.pro> <20211125170318.GB2829255@pb2> MIME-Version: 1.0 In-Reply-To: <20211125170318.GB2829255@pb2> Subject: Re: [FFmpeg-devel] [PATCH v2] avformat/mov: add option max_stts_delta 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="===============6632199459896918003==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============6632199459896918003== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="DYxtdHSupNSdWAwr" Content-Disposition: inline --DYxtdHSupNSdWAwr Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 25, 2021 at 06:03:18PM +0100, Michael Niedermayer wrote: > On Thu, Nov 25, 2021 at 10:51:15AM +0530, Gyan Doshi wrote: > >=20 > >=20 > > On 2021-11-25 12:56 am, Michael Niedermayer wrote: > > > On Wed, Nov 24, 2021 at 10:58:00AM +0530, Gyan Doshi wrote: > > > >=20 > > > > On 2021-11-24 01:16 am, Michael Niedermayer wrote: > > > > > On Tue, Nov 23, 2021 at 06:41:06PM +0530, Gyan Doshi wrote: > > > > > > Very high stts sample deltas may occasionally be intended but u= sually > > > > > > they are written in error or used to store a negative value for= dts correction > > > > > > when treated as signed 32-bit integers. > > > > > >=20 > > > > > > This option lets the user set an upper limit, beyond which the = delta > > > > > > is clamped to 1. Negative values of under 1 second are used to = adjust > > > > > > dts. > > > > > >=20 > > > > > > Unit is the track time scale. Default is INT_MAX which maintain= s current handling. > > > > > > --- > > > > > > doc/demuxers.texi | 6 ++++++ > > > > > > libavformat/isom.h | 1 + > > > > > > libavformat/mov.c | 14 +++++++++----- > > > > > > 3 files changed, 16 insertions(+), 5 deletions(-) > > > > > >=20 > > > > > > diff --git a/doc/demuxers.texi b/doc/demuxers.texi > > > > > > index cab8a7072c..15078b9b1b 100644 > > > > > > --- a/doc/demuxers.texi > > > > > > +++ b/doc/demuxers.texi > > > > > > @@ -713,6 +713,12 @@ specify. > > > > > > @item decryption_key > > > > > > 16-byte key, in hex, to decrypt files encrypted using ISO Co= mmon Encryption (CENC/AES-128 CTR; ISO/IEC 23001-7). > > > > > > + > > > > > > +@item max_stts_delta > > > > > > +The sample offsets stored in a track's stts box are 32-bit uns= igned integers. However, very large values usually indicate > > > > > > +a value written by error or a storage of a small negative valu= e as a way to correct accumulated DTS delay. > > > > > > +Range is 0 to UINT_MAX. Default is INT_MAX. > > > > > > + > > > > > > @end table > > > > > > @subsection Audible AAX > > > > > > diff --git a/libavformat/isom.h b/libavformat/isom.h > > > > > > index ef8f19b18c..625dea8421 100644 > > > > > > --- a/libavformat/isom.h > > > > > > +++ b/libavformat/isom.h > > > > > > @@ -305,6 +305,7 @@ typedef struct MOVContext { > > > > > > int32_t movie_display_matrix[3][3]; ///< display matrix = =66rom mvhd > > > > > > int have_read_mfra_size; > > > > > > uint32_t mfra_size; > > > > > > + uint32_t max_stts_delta; > > > > > > } MOVContext; > > > > > > int ff_mp4_read_descr_len(AVIOContext *pb); > > > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c > > > > > > index 451cb78bbf..bbda07ac42 100644 > > > > > > --- a/libavformat/mov.c > > > > > > +++ b/libavformat/mov.c > > > > > > @@ -3965,14 +3965,17 @@ static void mov_build_index(MOVContext = *mov, AVStream *st) > > > > > > current_offset +=3D sample_size; > > > > > > stream_size +=3D sample_size; > > > > > > - /* A negative sample duration is invalid based= on the spec, > > > > > > - * but some samples need it to correct the DTS= =2E */ > > > > > > - if (sc->stts_data[stts_index].duration < 0) { > > > > > > + /* STTS sample offsets are uint32 but some fil= es store it as int32 > > > > > > + * with negative values used to correct DTS de= lays. > > > > > > + There may be abnormally large values as wel= l. */ > > > > > > + if (sc->stts_data[stts_index].duration > mov->= max_stts_delta) { > > > > > > + // assume high delta is a negative correct= ion if less than 1 second > > > > > > + int32_t delta_magnitude =3D *((int32_t *)&= sc->stts_data[stts_index].duration); > > > > > > av_log(mov->fc, AV_LOG_WARNING, > > > > > > - "Invalid SampleDelta %d in STTS, at= %d st:%d\n", > > > > > > + "Correcting too large SampleDelta %= u in STTS, at %d st:%d.\n", > > > > > > sc->stts_data[stts_index].duratio= n, stts_index, > > > > > > st->index); > > > > > > - dts_correction +=3D sc->stts_data[stts_ind= ex].duration - 1; > > > > > > + dts_correction +=3D (delta_magnitude < 0 &= & FFABS(delta_magnitude) < sc->time_scale ? delta_magnitude - 1 : 0); > > > > > > sc->stts_data[stts_index].duration =3D 1; > > > > > > } > > > > > > current_dts +=3D sc->stts_data[stts_index].d= uration; > > > > > > @@ -8566,6 +8569,7 @@ static const AVOption mov_options[] =3D { > > > > > > { "decryption_key", "The media decryption key (hex)", OF= FSET(decryption_key), AV_OPT_TYPE_BINARY, .flags =3D AV_OPT_FLAG_DECODING_P= ARAM }, > > > > > > { "enable_drefs", "Enable external track support.", OFFS= ET(enable_drefs), AV_OPT_TYPE_BOOL, > > > > > > {.i64 =3D 0}, 0, 1, FLAGS }, > > > > > > + { "max_stts_delta", "treat offsets above this value as inv= alid", OFFSET(max_stts_delta), AV_OPT_TYPE_INT, {.i64 =3D INT_MAX}, 0, UINT= _MAX, .flags =3D AV_OPT_FLAG_DECODING_PARAM }, > > > > > > { NULL }, > > > > > > }; > > > > > The stts is used in other places, the value parsed as unsigned wi= ll cause > > > > > problems there too > > > > I see stts duration used in 3 places in mov.c > > > >=20 > > > > mov_read_stts(), which my earlier patch changed. > > > >=20 > > > > mov_build_index(), which this patch changes, > > > >=20 > > > > mov_read_trak() where frame rate is populated for CFR streams and i= s called > > > > very shortly after mov_build_index(). > > > > I don't think that can break for non-malformed files as the only du= ration > > > > entry in the stream is not likely to be > INT_MAX > > > > In any case, after this patch, with default option value, it will s= ee the > > > > same values as earlier. > > > >=20 > > > If iam not mistaken there is code that adds the values up to initiliz= e some > > > duration. this works with 64bit so the small negative values interpre= ted as > > > unsigned will result in a wrong result i would expect > > > that may very well not be the only issue > >=20 > > Yeah, I see the stream duration being set in mov_read_stts(). > > Interestingly,=A0 after 153639cb9cf, the unadjusted deltas were being a= dded, > > which would have included all negative deltas, small or large. >=20 > did you check which variant leads to the correct duration ? > i naively thought that as it was before was correct but i didnt check > that so i can very well be wrong >=20 >=20 > >=20 > > 153639cb9cf=A0 shifted the original negative delta adjustment code from > > mov_read_stts to mov_build_index. Was that necessary? >=20 > thats andreas commit, 5 years ago, i cannot awnser this=20 >=20 >=20 > >=20 > >=20 > > > > > the cast is also fragile as it will break when someone tries to c= hange it > > > > > to int64 > > > > In what circumstances would that happen? > > > when someone tries to change the code. > > > having both signed and unsigned 32bit in a 32bit element screams > >=20 > > Originally, negative values never left mov_read_stts. Maybe that's best. > >=20 > > Anyway, FATE passes, so it may only affect edge cases if it does. But I= will > > walk through the code to check. > >=20 > >=20 > > > [...] > > > > > also please select the default so that it works with all real wor= ld files > > > > No single value can all work for all files. Hence the option. > > > but what i wrote was about all real world files. > > > Sure you can make a file that fails but so far the files i have seen = and > > > the ones you told me about all would work with 49% of all possibly de= faults > > > why do you pick a default that doesnt work with them and then argue a= bout > > > that ? > > > Lets try to fix the code not win the argument please > >=20 > > I picked the default to keep the old behaviour. 99% of files I see have= all > > valid offsets. > > Among the rest, large values are mostly errors. Very few are intended. > > What default do you suggest? >=20 > I would suggest something that treats small negative ones as negative > signed but leaves all the rest as unsigned. I dont really know what > "small" should be here but what the file here used was around -10ms > so if we take a common timebase and 10seconds that should hopefully > catch all these negative cases and leave >99% of the space for unsigned Whats the status of this ? Do you have time and interrest to work on a fix ? If not this should be reverted until someone has time I think we should neither break one file to fix one nor should a per file user action like tuning an option be needed for normal non crafted files. Thanks [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras --DYxtdHSupNSdWAwr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCYbpq6AAKCRBhHseHBAsP q2PwAKCEJC5puJy/8eQrUmPwopIm3nPB7QCeMPEjqD/Me87H8rCs+v4Bzxc1s94= =Gl07 -----END PGP SIGNATURE----- --DYxtdHSupNSdWAwr-- --===============6632199459896918003== 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". --===============6632199459896918003==--