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

Michael Niedermayer michael at niedermayer.cc
Mon Nov 22 22:20:44 EET 2021


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.

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Homeopathy is like voting while filling the ballot out with transparent ink.
Sometimes the outcome one wanted occurs. Rarely its worse than filling out
a ballot properly.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20211122/4b54f435/attachment.sig>


More information about the ffmpeg-devel mailing list