On Tue, Nov 23, 2021 at 6:19 AM Gyan Doshi <ffm...@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 <ffm...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
