On Wed, Dec 29, 2021 at 09:39:34PM +0530, Gyan Doshi wrote: > > > 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 except for > > > > > > > the last entry. > > > > > > > > > > > > > > In addition, also catch 0 value for sample count. > > > > > > > --- > > > > > > > libavformat/mov.c | 12 ++++++++++++ > > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > > > 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=%d, sample_duration=%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 = sample_count = 1; > > > > > > > + } > > > > > > > + > > > > > > > + if (!sample_duration && i != 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 = sample_duration = 1; > > > > > > > + } > > > > > > > + > > > > > > > duration+=(int64_t)sample_duration*(uint64_t)sample_count; > > > > > > > total_sample_count+=sample_count; > > > > > > This does not produce the same output > > > > > > tickets/2096/m.f4v > > > > > > > > > > > > videos/stretch.mov (2344 matches for "invalid" after this patch) > > > > > > > > > > > > tickets/976/CodecCopyFailing.mp4 > > > > > > > > > > > > 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/CodecCopyFailing.mp4 > > > > https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2096/m.f4v > > > > > > > > i failed to find the 3rd online > > > > > > > > > > > > > > Taking a step back, the problem started with > > > > > > 203b0e3561dea1ec459be226d805abe73e7535e5 > > > > > > which broke a real world file which was outside the specification > > > > > 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. > > > > > > > > > > > you then suggested a fix which crashed with some fuzzed files which > > > > > > where outside the specification > > > > > > > > > > > > and now this fix on top which changes real world files which > > > > > > are outside the specification > > > > > > > > > > > > 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 ? > > > > > 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. > > > > > > > > > > If there's a specific / "correct" playback for these files like sync issues, > > > > > 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 > > > > 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 > > > > 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 > > It depends if all files beyond the first 500 are out-of-spec files. If they > are it suggests a spec widely ignored. > > 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 things > 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 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 [...] -- 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