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 E178A40D4F for ; Thu, 30 Dec 2021 14:16:56 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 312C168AFD1; Thu, 30 Dec 2021 16:16:42 +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 6948F68AFCC for ; Thu, 30 Dec 2021 16:16:36 +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 5E99C200007 for ; Thu, 30 Dec 2021 14:16:35 +0000 (UTC) Date: Thu, 30 Dec 2021 15:16:34 +0100 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20211230141634.GC2829255@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> <20211229180850.GB2829255@pb2> <62684d93-1f0f-bf70-e7b8-37f887e9fc5c@gyani.pro> MIME-Version: 1.0 In-Reply-To: <62684d93-1f0f-bf70-e7b8-37f887e9fc5c@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="===============9150810095210036746==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============9150810095210036746== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Fe0foNkF5QatIl+Q" Content-Disposition: inline --Fe0foNkF5QatIl+Q Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Dec 30, 2021 at 10:07:21AM +0530, Gyan Doshi wrote: >=20 >=20 > On 2021-12-29 11:38 pm, Michael Niedermayer wrote: > > On Wed, Dec 29, 2021 at 09:39:34PM +0530, Gyan Doshi wrote: > > >=20 > > > On 2021-12-29 05:58 pm, Michael Niedermayer wrote: > > > > On Tue, Dec 28, 2021 at 10:26:42PM +0530, Gyan Doshi wrote: > > > > > 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 exce= pt 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(MOVContex= t *c, AVIOContext *pb, MOVAtom atom) > > > > > > > > > av_log(c->fc, AV_LOG_TRACE, "sample_count=3D= %d, sample_duration=3D%d\n", > > > > > > > > > sample_count, sample_duration); > > > > > > > > > + if (!sample_count) { > > > > > > > > > + av_log(c->fc, AV_LOG_WARNING, "invalid sample co= unt 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 de= lta 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 p= atch) > > > > > > > >=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/CodecCopy= Failing.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 specifi= cation > > > > > > > Just to clarify, it did not break that file. That file uses s= tts in an > > > > > > > unusual way. > > > > > > > Before 2015, lavf exported packets with the same desync as th= e 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 fil= es 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 specifi= cation" > > > > > > > > more. The code above directly and intentionally changes val= ues. > > > > > > > > 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 differ= ent - 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 li= ke sync issues, > > > > > > > I'll see if I can restore it. > > > > > > First we need to find cases that broke. I certainly will not fi= nd every > > > > > >=20 > > > > > > If a patch is writen with the goal "dont break any file" it wou= ld be easy > > > > > > But you said that changes are "expected, intended" so then my q= uestion > > > > > > 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 unl= ess > > > > > > someone else finds such a file and reports it. (if its not in s= pec) > > > > > > 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 regre= ssion > > > > > > 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 m= uch as we > > > > > can. > > > > Sounds good but that alone doesnt work well > > > > The codebase is iteratively written, the demuxers move in steps tow= ards > > > > 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 9= 99 > > > > 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 thro= wn > > > > out and thats just bad. Bad for users, bad for the people who did t= hat work > > > It depends if all files beyond the first 500 are out-of-spec files. I= f they > > > are it suggests a spec widely ignored. > > >=20 > > > In this specific case however, I haven't seen any complaints about br= oken > > > MP4 demuxing outside of your samples. > > > I am on Stack Overflow and other popular support forums where users t= end to > > > first go to ask or complain. > > > FATE which is meant to catch undesirable behaviour passes. Both those= things > > > tell me that funky files whose demuxing > > > has changed constitute a very tiny set of files at most. And my lates= t set > > have you considered using https://samples.ffmpeg.org/ as a set of files= to > > test ? That should be a broader set than fate >=20 > If some behaviour change is to be checked for, it should be in FATE. That= 's > the point of a regression test suite. i do agree, and i very much welcome everyone adding tests to fate to improve its coverage. >=20 > I see dozens, if not hundreds, of files in /mov and /ffmpeg-bugs There should be around a thousand files that are parseable by the mov demux= er in there, the whole archive is something between 100-200gb (not just such f= iles) > Is there anything specifc you have in mind? yes if a patch adds support for unsigned STTS, NO file thats not identified as unsigned STTS should change.=20 If a patch changes files, then the explanation cannot be "its convenient for the implementation" without good evidence that this convenience doesnt worsen the functionality. That requires probably extensive testing, finding files that are handled differently should be easy to do automatically. when i did some tests, i kept finding issues in your patches, this is very inefficient. You have to post the patch i have to apply it and test then reply with an explanation of the failure. We arent online at the same time / working at the same time so theres a delay and this is surely inconvenient for both of us as we have to start and stop working on this. It should be much easier if you test your demuxer changes yourself and only post them once they do not worsen some file Or of course if your demuxer changes do not change the handling of existing unrelated files then you wouldnt have to test all these unrelated files also being before a release is another factor where a quicker turnaround time for you on found bugs and thus testing yourself would be better thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you fake or manipulate statistics in a paper in physics you will never get a job again. If you fake or manipulate statistics in a paper in medicin you will get a job for life at the pharma industry. --Fe0foNkF5QatIl+Q Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCYc2/PgAKCRBhHseHBAsP qySMAJ9MK2iYlg7ujnlDeiBpVYUqDXbUkgCeOrMYg1yaA25L75q/AH1McQuGGnc= =k5fH -----END PGP SIGNATURE----- --Fe0foNkF5QatIl+Q-- --===============9150810095210036746== 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". --===============9150810095210036746==--