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

[Gyan Doshi <ffmpeg@gyani.pro>]

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