Re: [FFmpeg-devel] [PATCH] avformat/mov: correct 0 valued entries in stts

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 7379 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".