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

[Gyan Doshi <ffmpeg@gyani.pro>]

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