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

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

[Gyan Doshi]

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;
     }
-- 
2.33.0

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

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

["zhilizhao(赵志立)"]

> On Dec 27, 2021, at 1:57 PM, Gyan Doshi <ffmpeg@gyani.pro> 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);

No, zero is a valid value, for example, fragmented mp4.

> +        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;
>     }
> -- 
> 2.33.0
> 
> _______________________________________________
> 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".

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

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

["zhilizhao(赵志立)"]

> On Dec 27, 2021, at 2:38 PM, zhilizhao(赵志立) <quinkblack@foxmail.com> wrote:
> 
>> On Dec 27, 2021, at 1:57 PM, Gyan Doshi <ffmpeg@gyani.pro> 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);
> 
> No, zero is a valid value, for example, fragmented mp4.

Ok, it’s sample_count, not entry_count.

> 
>> +        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;
>>    }
>> -- 
>> 2.33.0
>> 
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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".

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

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

[Michael Niedermayer]

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

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


Taking a step back, the problem started with 
203b0e3561dea1ec459be226d805abe73e7535e5
which broke a real world file which was outside the specification

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 ?
The change refers to the specification but the specification
will not help me when it to comes to how to handle all the wierd
and wonderful files the exist out there ...

thx

PS: also if you want to write fate tests for some of the odd files we find
in the process here, this may be a good idea and might simplify future work

[...]

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

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

[Gyan Doshi]

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?

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

Regards,
Gyan
_______________________________________________
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".

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

[Michael Niedermayer]

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

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 ?

PS: if you need random testfiles for testing arbitrary changes 
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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.

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

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

[Gyan Doshi]

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.

So if you revert 203b0e3561d  and do nothing else, lavf can then fail to 
parse some valid files.
The 'regression' then is the loss to accommodate some out-of-spec files.

I've now seen the two files you linked to. It turns out that they appear 
to use 0 delta values for
a similar purpose as mp4-negative-stts-problem.mp4. And I can 
accommodate that use in my patches.

Updated patches have been sent.

I'm able to remux both CodecCopyFailing.mp4 and m.f4v. Their playback 
looks acceptably similar and in sync.
Any individual timestamp changes don't accumulate and cause drift.

Regards,
Gyan
_______________________________________________
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".

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

[Michael Niedermayer]

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

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

thx

[...]
-- 
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.

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

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

[Gyan Doshi]

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 of patches restore the special handling
needed for the set of files you've highlighted.

Regards,
Gyan

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

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

[Michael Niedermayer]

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

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

[Gyan Doshi]

On 2021-12-29 11:38 pm, Michael Niedermayer wrote:
> 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

If some behaviour change is to be checked for, it should be in FATE. 
That's the point of a regression test suite.

I see dozens, if not hundreds, of files in /mov and /ffmpeg-bugs
Is there anything specifc you have in mind?

Regards,
Gyan

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

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

[Michael Niedermayer]

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

On Thu, Dec 30, 2021 at 10:07:21AM +0530, Gyan Doshi wrote:
> 
> 
> On 2021-12-29 11:38 pm, Michael Niedermayer wrote:
> > 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
> 
> 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.


> 
> 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 demuxer
in there, the whole archive is something between 100-200gb (not just such files)


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

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

[...]
-- 
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.

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

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

[Gyan Doshi]

On 2021-12-30 07:46 pm, Michael Niedermayer wrote:
> On Thu, Dec 30, 2021 at 10:07:21AM +0530, Gyan Doshi wrote:
>>
>> On 2021-12-29 11:38 pm, Michael Niedermayer wrote:
>>> 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
>> 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.
>
>
>> 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 demuxer
> in there, the whole archive is something between 100-200gb (not just such files)
>
>
>> 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.
This is what my original patch for max_stts_delta did. It used a default 
value of INT_MAX so all deltas identified as negative pre-option
would still be treated as negative post-option. But you asked for a 
different default value.


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

I'll survey all mov demuxer files in the sample suite over the weekend. 
But note the 0-value adjustment patch is a separate and detachable issue 
from max_stts_delta.
You can test the latest version of max_stts_delta patch with the current 
default value or the original default of INT_MAX and it should not break 
demux or sync relative to 203b0e3561~

Regards,
Gyan
_______________________________________________
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".

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

[Michael Niedermayer]

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

On Thu, Dec 30, 2021 at 10:39:05PM +0530, Gyan Doshi wrote:
> 
> 
> On 2021-12-30 07:46 pm, Michael Niedermayer wrote:
> > On Thu, Dec 30, 2021 at 10:07:21AM +0530, Gyan Doshi wrote:
> > > 
> > > On 2021-12-29 11:38 pm, Michael Niedermayer wrote:
> > > > 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
> > > 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.
> > 
> > 
> > > 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 demuxer
> > in there, the whole archive is something between 100-200gb (not just such files)
> > 
> > 
> > > 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.
> This is what my original patch for max_stts_delta did. It used a default
> value of INT_MAX so all deltas identified as negative pre-option
> would still be treated as negative post-option. But you asked for a
> different default value.
> 
> 
> > 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.
> 
> I'll survey all mov demuxer files in the sample suite over the weekend. But
> note the 0-value adjustment patch is a separate and detachable issue from
> max_stts_delta.

> You can test the latest version of max_stts_delta patch with the current
> default value or the original default of INT_MAX and it should not break
> demux or sync relative to 203b0e3561~

v4 seems to work, i have just now tested it
thanks


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.

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

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

[Gyan Doshi]

On 2021-12-30 11:25 pm, Michael Niedermayer wrote:
> On Thu, Dec 30, 2021 at 10:39:05PM +0530, Gyan Doshi wrote:
>>
>> On 2021-12-30 07:46 pm, Michael Niedermayer wrote:
>>> On Thu, Dec 30, 2021 at 10:07:21AM +0530, Gyan Doshi wrote:
>>>> On 2021-12-29 11:38 pm, Michael Niedermayer wrote:
>>>>> 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
>>>> 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.
>>>
>>>
>>>> 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 demuxer
>>> in there, the whole archive is something between 100-200gb (not just such files)
>>>
>>>
>>>> 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.
>> This is what my original patch for max_stts_delta did. It used a default
>> value of INT_MAX so all deltas identified as negative pre-option
>> would still be treated as negative post-option. But you asked for a
>> different default value.
>>
>>
>>> 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.
>> I'll survey all mov demuxer files in the sample suite over the weekend. But
>> note the 0-value adjustment patch is a separate and detachable issue from
>> max_stts_delta.
>> You can test the latest version of max_stts_delta patch with the current
>> default value or the original default of INT_MAX and it should not break
>> demux or sync relative to 203b0e3561~
> v4 seems to work, i have just now tested it
> thanks

So, I'll push max_stts_delta soon and test the 0 value patch on the weekend.

Regards,
Gyan
_______________________________________________
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".