[FFmpeg-devel] [FFmpeg-cvslog] avformat/mov: make STTS duration unsigned int

[Gyan Doshi]

On 2021-11-23 01:50 am, Michael Niedermayer wrote:
> On Mon, Nov 22, 2021 at 10:23:35PM +0530, Gyan Doshi wrote:
>>
>> On 2021-11-22 09:49 pm, Michael Niedermayer wrote:
>>> On Mon, Nov 22, 2021 at 08:36:30PM +0530, Gyan Doshi wrote:
>>>> On 2021-11-22 07:29 pm, Michael Niedermayer wrote:
>>>>> On Mon, Nov 22, 2021 at 06:57:32PM +0530, Gyan Doshi wrote:
>>>>>> On 2021-11-22 06:50 pm, Michael Niedermayer wrote:
>>>>>>> On Mon, Nov 22, 2021 at 02:17:24PM +0100, Michael Niedermayer wrote:
>>>>>>>> On Mon, Nov 22, 2021 at 09:49:26AM +0000, Gyan Doshi wrote:
>>>>>>>>> ffmpeg | branch: master | Gyan Doshi <ffmpeg at gyani.pro> | Tue Nov 16 19:02:32 2021 +0530| [203b0e3561dea1ec459be226d805abe73e7535e5] | committer: Gyan Doshi
>>>>>>>>>
>>>>>>>>> avformat/mov: make STTS duration unsigned int
>>>>>>>>>
>>>>>>>>> As per 8.6.1.2.2 of ISO/IEC 14496-12:2015(E), STTS sample offsets
>>>>>>>>> are to be always stored as uint32_t. So far, they have been signed ints
>>>>>>>>> which led to desync in files with very large offsets.
>>>>>>>>>
>>>>>>>>> The MOVStts struct was used to store CTTS offsets as well. These can be
>>>>>>>>> negative in version 1. So a new struct MOVCtts was created and all
>>>>>>>>> declarations for CTTS usage changed to MOVCtts.
>>>>>>>>>
>>>>>>>>>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=203b0e3561dea1ec459be226d805abe73e7535e5
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>      libavformat/isom.h   |  9 +++++++--
>>>>>>>>>      libavformat/mov.c    | 20 ++++++++++----------
>>>>>>>>>      libavformat/movenc.c |  2 +-
>>>>>>>>>      3 files changed, 18 insertions(+), 13 deletions(-)
>>>>>>>> This breaks:
>>>>>>>>
>>>>>>>> ./ffmpeg -i ~/videos/mp4-negative-stts-problem.mp4 -c copy  -t 3 -y file-negstts.mov
>>>>>>>>
>>>>>>>> https://samples.ffmpeg.org/mov/mp4-negative-stts-problem.mp4
>>>>>>> failure happens like this:
>>>>>>>
>>>>>>> [mov @ 0x56332ba06800] Application provided duration: 4294966430 is invalid
>>>>>> That's triggered by this code in lavf/movenc.c
>>>>>>
>>>>>> ----
>>>>>>        if (pkt->duration < 0 || pkt->duration > INT_MAX) {
>>>>>>            av_log(s, AV_LOG_ERROR, "Application provided duration: %"PRId64" is
>>>>>> invalid\n", pkt->duration);
>>>>>>            return AVERROR(EINVAL);
>>>>>>        }
>>>>>> ----
>>>>>>
>>>>>> Since the spec allows duration up to uint32, the INT_MAX limit is wrong and
>>>>>> is another separate bug.
>>>>>>
>>>>>> I'll change that when I correct the muxer.
>>>>> this problem seems unrelated to the mov muxer, here framecrc is used instead
>>>>> of mov, it fails too and framecrc is happily accepting the wrong duration
>>>> The original error msg you posted is printed by the mov muxer and is caused
>>>> by the code block I pasted.
>>> I think you misunderstand, Noone complained about the muxer failing to store
>>> a 6 month long audio frame
>>>
>>> The issue is that a file which contains AAC audio with frames of about 10ms
>>> length before 203b0e3561dea1ec459be226d805abe73e7535e5, afterwards returns a frame
>>> with a length of 6 months that frame does not have a duration of 6 month
>>> what it does probably have is a small negative STTS value thats incorrectly
>>> parsed as unsigned. This bug is in the mov demuxer
>> 1) As the start of my commit msg says, ISOBMFF does not allow to store
>> signed sample offsets in the stts box in any version.
>> The demuxer should allow for all valid values to be parsed correctly.
>>
>> 2) If a writer or variant intends a signed representation and then commits
>> an error by writing an unintended negative duration,
>> we can have an option (ideally) or heuristic to detect and adjust such
>> values. This adjustment should not be hardcoded in such  a way
>> so that valid values are also affected. At worst, we can make the
>> pre-203b0e3561 adjudtment the default. Although a heuristic which
>> compares a packet's duration against the average duration in the stream is
>> better.
>>
>> 3) Since the ISOBMFF format does not store timestamps but deltas, the
>> duration does not indicate the playback duration for a sample. It is common
>> for recorders to indicate stream gaps by encoding it via the duration rather
>> than an edit list. The AAC frame will still be rendered in 10ms. It is the
>> PTS
>> of the next packet which is affected.
>>
>> Restoring old behaviour is as simple as adding a demuxer option with default
>> to treat STTS limit as INT_MAX. Would you like that?
> The demuxer should support all real world files without manual intervention
> requiring manual intervention by adjusting an option is problematic.
> We cannot expect from our users to tune thousands of options on a file by
> file basis to adjust for input odities
> Users would dislike that, things should "just work" out of the box
>
> Iam of course not against adding an option but the default should work
> with all real world files. And if it works with all iam not sure we need
> an option.

The default *did not work* with all real world files. It did not work 
for my file.

I'll send a patch for an option.

Regards,
Gyan