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 5411740B69 for ; Mon, 27 Dec 2021 23:48:13 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id B07AC68B0AD; Tue, 28 Dec 2021 01:48:11 +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 9278968AF50 for ; Tue, 28 Dec 2021 01:48:05 +0200 (EET) Received: from localhost (213-47-68-29.cable.dynamic.surfer.at [213.47.68.29]) (Authenticated sender: michael@niedermayer.cc) by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id DE589FF802 for ; Mon, 27 Dec 2021 23:48:04 +0000 (UTC) Date: Tue, 28 Dec 2021 00:48:03 +0100 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20211227234803.GX2829255@pb2> References: <20211227055711.63060-1-ffmpeg@gyani.pro> <20211227190841.GW2829255@pb2> <1df7ec4a-45cd-a6b7-2096-afaa19d09d08@gyani.pro> MIME-Version: 1.0 In-Reply-To: <1df7ec4a-45cd-a6b7-2096-afaa19d09d08@gyani.pro> Subject: Re: [FFmpeg-devel] [PATCH] avformat/mov: correct 0 valued entries in stts 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="===============6402461378141273602==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============6402461378141273602== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="dVUrXUjE+ke2Ara+" Content-Disposition: inline --dVUrXUjE+ke2Ara+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Dec 28, 2021 at 01:33:54AM +0530, Gyan Doshi wrote: >=20 >=20 > On 2021-12-28 12:38 am, Michael Niedermayer wrote: > > On Mon, Dec 27, 2021 at 11:27:10AM +0530, Gyan Doshi wrote: > > > As per ISO 14496-12, sample duration of 0 is invalid except for > > > the last entry. > > >=20 > > > In addition, also catch 0 value for sample count. > > > --- > > > libavformat/mov.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > >=20 > > > diff --git a/libavformat/mov.c b/libavformat/mov.c > > > index 2aed6e80ef..fb7406cdd6 100644 > > > --- a/libavformat/mov.c > > > +++ b/libavformat/mov.c > > > @@ -2968,6 +2968,18 @@ static int mov_read_stts(MOVContext *c, AVIOCo= ntext *pb, MOVAtom atom) > > > av_log(c->fc, AV_LOG_TRACE, "sample_count=3D%d, sample_dura= tion=3D%d\n", > > > sample_count, sample_duration); > > > + if (!sample_count) { > > > + av_log(c->fc, AV_LOG_WARNING, "invalid sample count of 0 in = stts for st %d at entry %u; changing to 1.\n", > > > + c->fc->nb_streams-1, i); > > > + sc->stts_data[i].count =3D sample_count =3D 1; > > > + } > > > + > > > + if (!sample_duration && i !=3D entries-1) { > > > + av_log(c->fc, AV_LOG_WARNING, "invalid sample delta of 0 in = stts for st %d at entry %u; changing to 1.\n", > > > + c->fc->nb_streams-1, i); > > > + sc->stts_data[i].duration =3D sample_duration =3D 1; > > > + } > > > + > > > duration+=3D(int64_t)sample_duration*(uint64_t)sample_count; > > > total_sample_count+=3Dsample_count; > > This does not produce the same output > > tickets/2096/m.f4v > >=20 > > videos/stretch.mov (2344 matches for "invalid" after this patch) > >=20 > > tickets/976/CodecCopyFailing.mp4 > >=20 > > But there are many more, some maybe even generated by FFmpeg >=20 > Where do I find these files? https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket976/CodecCopyFailing.mp4 https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2096/m.f4v i failed to find the 3rd online >=20 > > Taking a step back, the problem started with > > 203b0e3561dea1ec459be226d805abe73e7535e5 > > which broke a real world file which was outside the specification >=20 > Just to clarify, it did not break that file. That file uses stts in an > unusual way. > Before 2015, lavf exported packets with the same desync as the other > demuxers do so till today. > Andreas' patch added a hack to make it play in sync. My patch 203b0e356 > broke that hack. > The patch for max_stts_delta is a way to restore it back. >=20 > > you then suggested a fix which crashed with some fuzzed files which > > where outside the specification > >=20 > > and now this fix on top which changes real world files which > > are outside the specification > >=20 > > I think, maybe you should consider the "outside the specification" > > more. The code above directly and intentionally changes values. > > So as a reviewer i have to ask the obvious, is that change a > > bugfix or a bug ? >=20 > Not surprising that the output of out-of-spec files is different - that's > expected, intended and trivial. > It would be a bug if in-spec files were treated differently. FATE passes. >=20 > If there's a specific / "correct" playback for these files like sync issu= es, > I'll see if I can restore it. First we need to find cases that broke. I certainly will not find every If a patch is writen with the goal "dont break any file" it would be easy But you said that changes are "expected, intended" so then my question as reviewer would be what about these expected changes ? Did it change any real files output? did it fix a bug? What is the idea behind the change ? please correct me if iam wrong but Here it seems you dont care what happens with changed files unless=20 someone else finds such a file and reports it. (if its not in spec) And the idea seems that 0 is inconvenient so you change it to 1 Its not that 0 could fundamentally not be intended to mean 0 We are before a release and id like to fix the regression ATM objectively the only option i have is reverting=20 203b0e3561dea1ec459be226d805abe73e7535e5 can you provide another option ? something that fixes the regression without breaking something else ? PS: if you need random testfiles for testing arbitrary changes=20 samples.ffmpeg.org should have alot and is also accessible with rsync so you dont need to wait and hope someone will spot an issue. thanks [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB You can kill me, but you cannot change the truth. --dVUrXUjE+ke2Ara+ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCYcpQsAAKCRBhHseHBAsP qzxeAJ9tdjF7FNfWhkRmzVvNqRQHO5NhiQCfcSuTe2Dj9HZ+uqZ2n2hU4yXfoXg= =CxbP -----END PGP SIGNATURE----- --dVUrXUjE+ke2Ara+-- --===============6402461378141273602== 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". --===============6402461378141273602==--