[FFmpeg-devel] [mov] Fix trampling of ctts during seeks when sidx support is enabled.

[Michael Niedermayer]

On Wed, Aug 23, 2017 at 03:39:01PM -0700, Dale Curtis wrote:
> On Sat, Aug 19, 2017 at 3:50 PM, Michael Niedermayer <michael at niedermayer.cc
> > wrote:
> 
> > On Sun, Aug 20, 2017 at 12:48:30AM +0200, Michael Niedermayer wrote:
> > > On Fri, Aug 18, 2017 at 03:57:45PM -0700, Dale Curtis wrote:
> > > > Anything else here? It'd be nice to get this landed soon if no one has
> > any
> > > > other comments.
> > >
> > > it appears to not apply cleanly anymore
> >
> > seems caused by f4544163b27615ecfff1b42d6acdb3672ac92399.
> >
> 
> Thanks rebased on ToT after chatting with Jacob to ensure my patch didn't
> regress his issue.
> 
> - dale

>  libavformat/isom.h       |    1 
>  libavformat/mov.c        |   92 +++++++++++++++++++-------------
>  tests/fate/seek.mak      |    3 +
>  tests/ref/seek/extra-mp4 |  134 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 195 insertions(+), 35 deletions(-)
> 98449b2f1a0464b474c3b17ee29850daa1b0081a  ctts_fix_v9.patch
> From b4b49b6b584b33e1da95a5d72b05fd9134ab28f9 Mon Sep 17 00:00:00 2001
> From: Dale Curtis <dalecurtis at chromium.org>
> Date: Mon, 17 Jul 2017 17:38:09 -0700
> Subject: [PATCH] Fix trampling of ctts during seeks when sidx support is
>  enabled.
> 
> When sidx box support is enabled, the code will skip reading all
> trun boxes (each containing ctts entries for samples inthat box).
> 
> If seeks are attempted before all ctts values are known, the old
> code would dump ctts entries into the wrong location. These are
> then used to compute pts values which leads to out of order and
> incorrectly timestamped packets.
> 
> This patch fixes ctts processing by always using the index returned
> by av_add_index_entry() as the ctts_data index. When the index gains
> new entries old values are reshuffled as appropriate.
> 
> This approach makes sense since the mov demuxer is already relying
> on the mapping of AVIndex entries to samples for correct demuxing.
> 
> As a result of this all ctts entries are now 1-count. A followup
> change will be submitted to remove support for > 1 count entries
> which will simplify seeking.
> 
> Notes for future improvement:
> Probably there are other boxes (stts, stsc, etc) that are impacted
> by this issue... this patch only attempts to fix ctts since it
> completely breaks packet timestamping.
> 

> This patch continues using an array for the ctts data, which is not
> the most ideal given the rearrangement that needs to happen (via
> memmove as new entries are read in). Ideally AVIndex and the ctts
> data would be set-type structures so addition is always worst case
> O(lg(n)) instead of the O(n^2) that exists now; this slowdown is
> noticeable during seeks.

can the insertions be done in groups instead of one at a time ?
so that it basically merges 2 lists (O(n)) instead of inserting
one at a time O(n^2)
?
This would significantly improve the worst case while not needing
to change the data structures
(of course iam not against changing the data structures if someone wants
to do the work)

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20170824/b4622f6c/attachment.sig>