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

[Hendrik Leppkes]

On Tue, Nov 23, 2021 at 6:19 AM Gyan Doshi <ffmpeg at gyani.pro> wrote:
>
>
>
> 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.
>

Options are useless when noone tells me which option I need to flip
for any given file to play properly.
Detecting special cases and handling them would be a far better choice.

- Hendrik