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

Sat Sep 3 02:15:14 EEST 2016

On Fri, Sep 02, 2016 at 01:00:59PM -0700, Sasi Inguva wrote:
> On Aug 31, 2016 5:23 AM, "Michael Niedermayer" <michael at niedermayer.cc>
> wrote:
> >
> > On Tue, Aug 30, 2016 at 06:37:22PM -0700, Sasi Inguva wrote:
> > > 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-9N3ejNkMW9yU0k4ZF
> E/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.
> >
> > My first question is, entirely independant of the implementation from
> > the patches. What is the correct output ? (my primary focus is on
> > the timestamps)
> >
> >
> > Also if there are discontinuities, AVFMT_TS_DISCONT is normally set
> > (and this never happens for files with indexes but only for files
> >  that dont have indexes like mpeg*)
> > players will generally seek by file position if AVFMT_TS_DISCONT is
> > set (because timestamps are ambigous in that case)
> >
> > iam not sure how to seek reliably by file position in a mov
> > with edit lists and stll have the file positions actually be file
> > positions of the frames, so this direction gets tricky too ...
> >
> I'll try do describe the behavior of the timestamps and seek after all
> these patches are applied.
> Reading:
> i) Decoding (using av_codec_video* API) - Non-repeated monotonically
> increasing timestamps.

> ii) Just demuxing - Repeated timestamps in case of Single edit list .

can you explain why you insist on this ?
why do you assign invalid timestamps to discarded packets ?
These packets are just normal packets in the current API, applications
will treat them like any other packet and the timestamps would likely
cause problems, confusion and bugreports

> Non-monotonic timestamps in case of multiple edit lists.

can you elaborate on how exactly they are non monotonic
you said above that they are monotonic after the decoder, i would
have expected that they would still be non monotonic after the
decoder for multiple edit lists
so maybe i misunderstood

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160903/2d57a949/attachment.sig>