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

[Michael Niedermayer]

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

Of course maybe iam wrong but as long as the output of
./ffmpeg -i ~/videos/mp4-negative-stts-problem.mp4 -c copy  -t 3 -y file-negstts.mov
doesnt play with complete and synced audio and video theres a regression 
that needs to be fixed

The fix for this is may be to compare unsigned vs signed interpretation
a less than 1 second negative value is probably more likely correct than a
more than 1 month positive one.
There very well may be something else detectable, maybe theres some format
variant that has it signed.
But this file does not have a unsigned STTS as the recent commit assumes

So please either
1. convince me that iam wrong which is very possible
2. fix the stts parsing code
3. revert the commit
4. just tell me you have no time and ill look into it

thx

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/2073540d/attachment.sig>