[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