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 8048A40C75 for ; Wed, 29 Dec 2021 18:09:00 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1D27C68A2F6; Wed, 29 Dec 2021 20:08:58 +0200 (EET) Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 2A380680923 for ; Wed, 29 Dec 2021 20:08:52 +0200 (EET) Received: from localhost (213-47-68-29.cable.dynamic.surfer.at [213.47.68.29]) (Authenticated sender: michael@niedermayer.cc) by relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 4D97B240002 for ; Wed, 29 Dec 2021 18:08:50 +0000 (UTC) Date: Wed, 29 Dec 2021 19:08:50 +0100 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20211229180850.GB2829255@pb2> References: <20211227055711.63060-1-ffmpeg@gyani.pro> <20211227190841.GW2829255@pb2> <1df7ec4a-45cd-a6b7-2096-afaa19d09d08@gyani.pro> <20211227234803.GX2829255@pb2> <66581536-ca84-4c2f-7dd9-f39ea2389244@gyani.pro> <20211229122829.GZ2829255@pb2> <566297af-bbf2-d2c1-7279-84596b2a69d1@gyani.pro> MIME-Version: 1.0 In-Reply-To: <566297af-bbf2-d2c1-7279-84596b2a69d1@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="===============4568453601758337221==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============4568453601758337221== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="GMq1OpGuLUulEJi3" Content-Disposition: inline --GMq1OpGuLUulEJi3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Dec 29, 2021 at 09:39:34PM +0530, Gyan Doshi wrote: >=20 >=20 > On 2021-12-29 05:58 pm, Michael Niedermayer wrote: > > On Tue, Dec 28, 2021 at 10:26:42PM +0530, Gyan Doshi wrote: > > >=20 > > > On 2021-12-28 05:18 am, Michael Niedermayer wrote: > > > > On Tue, Dec 28, 2021 at 01:33:54AM +0530, Gyan Doshi wrote: > > > > > 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 f= or > > > > > > > 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= , AVIOContext *pb, MOVAtom atom) > > > > > > > av_log(c->fc, AV_LOG_TRACE, "sample_count=3D%d, s= ample_duration=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)sa= mple_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 > > > > > Where do I find these files? > > > > https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket976/CodecCopyFail= ing.mp4 > > > > https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2096/m.f4v > > > >=20 > > > > i failed to find the 3rd online > > > >=20 > > > >=20 > > > > > > Taking a step back, the problem started with > > > > > > 203b0e3561dea1ec459be226d805abe73e7535e5 > > > > > > which broke a real world file which was outside the specificati= on > > > > > 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 ot= her > > > > > demuxers do so till today. > > > > > Andreas' patch added a hack to make it play in sync. My patch 203= b0e356 > > > > > 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 w= hich > > > > > > 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 specificati= on" > > > > > > 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 ? > > > > > 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 s= ync issues, > > > > > I'll see if I can restore it. > > > > First we need to find cases that broke. I certainly will not find e= very > > > >=20 > > > > If a patch is writen with the goal "dont break any file" it would b= e easy > > > > But you said that changes are "expected, intended" so then my quest= ion > > > > 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 > > > >=20 > > > > Here it seems you dont care what happens with changed files unless > > > > 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 > > > >=20 > > > > We are before a release and id like to fix the regression > > > > ATM objectively the only option i have is reverting > > > > 203b0e3561dea1ec459be226d805abe73e7535e5 > > > > can you provide another option ? something that fixes the regression > > > > without breaking something else ? > > > 1) The first priority ought to be to not mishandle compliant files > > > 2) Subject to 1, we should accommodate for out-of-spec files as much = as we > > > can. > > Sounds good but that alone doesnt work well > > The codebase is iteratively written, the demuxers move in steps towards > > fewer bugs, more wide file support, better maintainability. > > if you start with a demuxer which supports 500 files and then > > it supports 550 then 700 then 800 and so on eventually it reaches 999 > > and now someone finds a spec compliance bug and fixes it so 1 more file > > is supported that developer has to try to not break past efforts of > > supporting other files. Because otherwise alot of past work is thrown > > out and thats just bad. Bad for users, bad for the people who did that = work >=20 > It depends if all files beyond the first 500 are out-of-spec files. If th= ey > are it suggests a spec widely ignored. >=20 > In this specific case however, I haven't seen any complaints about broken > MP4 demuxing outside of your samples. > I am on Stack Overflow and other popular support forums where users tend = to > first go to ask or complain. > FATE which is meant to catch undesirable behaviour passes. Both those thi= ngs > tell me that funky files whose demuxing > has changed constitute a very tiny set of files at most. And my latest set have you considered using https://samples.ffmpeg.org/ as a set of files to test ? That should be a broader set than fate it can be downloaded with rsync if you have enough space A patch which changes some files and the amount affected is unknown=20 vs. A patch which changes some files and was tested against all mov/mp4 files from samples and changed 5 where the change improved av-sync These 2 hypothetical commit messages give a totally different feeling to a reviewer thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact --GMq1OpGuLUulEJi3 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCYcykLAAKCRBhHseHBAsP qw7OAJ47XMl7YkbLX0Bm6Sda4jfq3/zahwCfWJ9a82GKxcIGws90sI2VX1fQizg= =7puj -----END PGP SIGNATURE----- --GMq1OpGuLUulEJi3-- --===============4568453601758337221== 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". --===============4568453601758337221==--