[FFmpeg-devel] [PATCH 4/4] lavf/mov: Add support for edit list parsing.

Wed Aug 31 04:37:22 EEST 2016

On Sun, Aug 28, 2016 at 3:10 AM, Michael Niedermayer <michael at niedermayer.cc
> wrote:

> On Sat, Aug 27, 2016 at 03:30:24PM -0700, Sasi Inguva wrote:
> > On Fri, Aug 26, 2016 at 5:55 PM, Michael Niedermayer
> <michael at niedermayer.cc
> > > wrote:
> >
> > > On Fri, Aug 26, 2016 at 12:49:19PM -0700, Sasi Inguva wrote:
> > > > I think there is some bug in mp3 decoder which is making skip
> > > > samples -1431655766 for ~/tickets/5528/fire.mp3 . For now I have
> removed
> > > > the assert from the 3rd commit.
> > > > For the file one.mov , I think the audio has edit list with start
> time
> > > > correspending to the second sample - (which should be media time
> 1024 if
> > > I
> > > > guess correctly). This indicates that the first sample be used for
> > > encoder
> > > > delay.
> > > >  Previously without edit  list parsing , we used to offset the
> start_dts
> > > by
> > > > -1024 to make the second sample timestamp start from zero.
> > > >              sc->time_offset = start_time - empty_duration;
> > > > -            current_dts = -sc->time_offset;
> > > >              if (sc->ctts_count>0 && sc->stts_count>0 &&
> > > >
> > > > But now edit list parsing handles the rebasing of timestamps to zero,
> > > > because it will  assign increasing timestamps  starting from zero, to
> > > > samples present in the edit list.
> > >
> > > > Because the first sample is not in the
> > > > edit list, we mark it as DISCARD, which flag av_decode_audio4 will
> look
> > > at
> > > > and decode-and-discard that frame. So it wouldn't matter what the
> first
> > > > sample timestamp should be assigned because it is anyway discarded
> after
> > > > decode.
> > >
> > > current applications using libavformat have no knowledge of the
> > > discard flag you can add the DISCARD flag, you can set it on packets
> but
> > > applications written or built for libavformat 57 dont have to know
> > > this flag and can treat the packets like any other packet.
> > >
> >
> > Yes. they can treat the packet like any other packet. They don't have to
> > know about the discard flag.
> >
> > Adding this feature without a major version bump requires things to
> > > still work reasonable with the old semantics that apps are build
> > > around. That should be possible unless iam missing something
> >
> >
> > As I have pointed out earlier this code will change the timestamps of the
> > packets. In the case of multiple edit lists, the code will also repeat
> some
> > packets, and might make the timestamps non-monotonous. I don't know if
> the
> > last behavior is not  an acceptable expectation from av_read_frame.
>
> if timestamps repeat then it will not be possible to seek in the file
> by timestamp (to all positions) and i suspect also not by byte position.
> How would one seek then ?
> or do i misunderstand ?
>
>
In case of MOV  container, the seek is done using av_index_search_timestamp
function
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/mov.c;h=f4999068519f1f06f6b5d84ca007148e74e5a82e;hb=HEAD#l5419
.

i) In case of single edit list , the timestamps will only be repeated but
not non-monotonous. In that case av_index_search_timestamp will still work
correctly, only that it will seek to any one of the packets with the same
timestamp. However when we decode the file, then all of the discarded
packets with similar timestamps should be discarded and only the
non-discarded packet will bet output. So in short,
./ffprobe -read_intervals 0.0  -show_frames -print_format compact
one-editlist-audio.mov
<https://drive.google.com/file/d/0Bz6XfEJZ-9N3ejNkMW9yU0k4ZFE/view?usp=sharing>
should
give exactly the same behavior as before while -show_packets will show more
discarded packets at the start.  I had to change the patch (4) a bit to
make the audio-seek on MOV to work  correctly, so please reapply the
attached patch to test.

ii) In case of multiple edit lists , timestamps might be non monotonous  so
existing av_index_search_timestamp implementation won't be correct, since
it assumes sorted timestamps. However making it work for discarded packets
is not that hard. I have attached a 5th patch that changes av_index_search
function. This fixes the issue in (i) also

> > However as I've pointed out, we are already showing the wrong packets for
> > videos with multiple edit lists by not parsing the edit lists currently,
> > which will introduce AVSync issues. So this patch wont make things any
> > worse for those cases in my view.
>
> Is it difficult to adjust the timestamps of discarded packets to avoid
> timestamp discontinuities ? (for the cases where this is possible of
> course only)
> the timestamps for discarded packets i would assume are meaningless in
> the new semenatics but they matter for the old semantics
> again, please correct me if iam wrong
>
> The way fix_index is implemented it is difficult to change the timestamps
to avoid discotinuities and still have the timeline the same as MOV edit
lists would intend.
The timestamps for discarded packets are meaningless to av_decode_*
functions because they parse the DISCARD flag and ignore the packets. I am
not sure, what you mean by semantics though, because I don't think I am
changing any user expectations defined by the mov_read_frame mov_seek_frame
functions . If by semantics, you mean that user expects to see
monotonically increasing timestamps for the "demuxed " packets then yes
that expectation changes to " monotonically increasing timestamps for the
"demuxed and non-discarded" packets" and user has to parse the discard
flag. And to reiterate , this semantic is only affected in the case of
files with multiple edit lists which are very less in number , since in
case of single edit list videos timestamps will only be repeated and still
are monotonically increasing.

As of now with 5th patch the seek also works with edit lists.  I can hide
this functionality under a flag like "support_edit_liist" in MOV demuxer,
and turn it off by default, if u think that is better.

> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you drop bombs on a foreign country and kill a hundred thousand
> innocent people, expect your government to call the consequence
> "unprovoked inhuman terrorist attacks" and use it to justify dropping
> more bombs and killing more people. The technology changed, the idea is
> old.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avutil-frame-Add-a-flag-to-discard-frame-after-decod.patch
Type: text/x-patch
Size: 1670 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160830/64d4aed4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-lavf-mov-Add-support-for-edit-list-parsing.patch
Type: text/x-patch
Size: 117703 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160830/64d4aed4/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-lavc-Add-a-flag-in-AVPacket-to-discard-packet-after-.patch
Type: text/x-patch
Size: 2921 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160830/64d4aed4/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-avformat-avframe.h-Add-a-flag-in-AVIndexEntry-to-dis.patch
Type: text/x-patch
Size: 1818 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160830/64d4aed4/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-lavf-utils-Support-av_index_search_timestamp-in-case.patch
Type: text/x-patch
Size: 1113 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160830/64d4aed4/attachment-0004.bin>