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

[Gyan Doshi]

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?

Regards,
Gyan