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

[Dale Curtis]

On Thu, Nov 23, 2017 at 3:54 PM, Carl Eugen Hoyos 
wrote:
>
> @Dale:
> Could you do that?
>

Thanks to John for putting out a patch for this.

- dale
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Carl Eugen Hoyos]

2017-11-23 19:45 GMT+01:00 John Stebbins :
> On 11/22/2017 05:26 PM, Carl Eugen Hoyos wrote:
>> 2017-11-23 1:30 GMT+01:00 John Stebbins :
>>> On 11/22/2017 02:36 PM, Carl Eugen Hoyos wrote:
 2017-08-24 0:39 GMT+02:00 Dale Curtis :

> -sc->ctts_data[ctts_count].count= count;
> -sc->ctts_data[ctts_count].duration = duration;
> -ctts_count++;
> +/* Expand entries such that we have a 1-1 mapping with samples. 
> */
> +for (j = 0; j < count; j++)
> +add_ctts_entry(>ctts_data, _count, 
> >ctts_allocated_size, 1, duration);
 count is a 32bit value read from the file, so this hunk makes
 the demuxer allocate huge amount of memories for some
 files.

 Is there an upper limit for count?
>>> In practice, if a valid mp4 blows up due to this ctts allocation,
>>> it's also going to blow up when AVIndexEntries is allocated
>>> for the samples.
>>> An invalid mp4 can do anything of course.
>> This is about invalid files allocating >1GB.
>
> Ah, ok.  The practical limit would be the number of samples 
> (sc->sample_count).
> But you can't be certain this is set before the ctts box is parsed (the value 
> is
> determined when parsing stsz box).  You can be certain it is set before
> mov_build_index is called.  So perhaps revert this part and then add code to
> mov_build_index to expand the ctts_data entries there.  This would solve the
> invalid mp4 alloc issues while still preserving the fix for trampling of ctts.

@Dale:
Could you do that?

Thank you, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[John Stebbins]

On 11/22/2017 05:26 PM, Carl Eugen Hoyos wrote:
> 2017-11-23 1:30 GMT+01:00 John Stebbins :
>> On 11/22/2017 02:36 PM, Carl Eugen Hoyos wrote:
>>> 2017-08-24 0:39 GMT+02:00 Dale Curtis :
>>>
 -sc->ctts_data[ctts_count].count= count;
 -sc->ctts_data[ctts_count].duration = duration;
 -ctts_count++;
 +/* Expand entries such that we have a 1-1 mapping with samples. */
 +for (j = 0; j < count; j++)
 +add_ctts_entry(>ctts_data, _count, 
 >ctts_allocated_size, 1, duration);
>>> count is a 32bit value read from the file, so this hunk makes
>>> the demuxer allocate huge amount of memories for some
>>> files.
>>>
>>> Is there an upper limit for count?
>> In practice, if a valid mp4 blows up due to this ctts allocation,
>> it's also going to blow up when AVIndexEntries is allocated
>> for the samples.
>> An invalid mp4 can do anything of course.
> This is about invalid files allocating >1GB.
>
>

Ah, ok.  The practical limit would be the number of samples (sc->sample_count). 
 But you can't be certain this is set
before the ctts box is parsed (the value is determined when parsing stsz box).  
You can be certain it is set before
mov_build_index is called.  So perhaps revert this part and then add code to 
mov_build_index to expand the ctts_data
entries there.  This would solve the invalid mp4 alloc issues while still 
preserving the fix for trampling of ctts.

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Carl Eugen Hoyos]

2017-11-23 1:30 GMT+01:00 John Stebbins :
> On 11/22/2017 02:36 PM, Carl Eugen Hoyos wrote:
>> 2017-08-24 0:39 GMT+02:00 Dale Curtis :
>>
>>> -sc->ctts_data[ctts_count].count= count;
>>> -sc->ctts_data[ctts_count].duration = duration;
>>> -ctts_count++;
>>> +/* Expand entries such that we have a 1-1 mapping with samples. */
>>> +for (j = 0; j < count; j++)
>>> +add_ctts_entry(>ctts_data, _count, 
>>> >ctts_allocated_size, 1, duration);
>> count is a 32bit value read from the file, so this hunk makes
>> the demuxer allocate huge amount of memories for some
>> files.
>>
>> Is there an upper limit for count?
>
> In practice, if a valid mp4 blows up due to this ctts allocation,
> it's also going to blow up when AVIndexEntries is allocated
> for the samples.

> An invalid mp4 can do anything of course.

This is about invalid files allocating >1GB.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[John Stebbins]

On 11/22/2017 02:36 PM, Carl Eugen Hoyos wrote:
> 2017-08-24 0:39 GMT+02:00 Dale Curtis :
>
>> -sc->ctts_data[ctts_count].count= count;
>> -sc->ctts_data[ctts_count].duration = duration;
>> -ctts_count++;
>> +/* Expand entries such that we have a 1-1 mapping with samples. */
>> +for (j = 0; j < count; j++)
>> +add_ctts_entry(>ctts_data, _count, 
>> >ctts_allocated_size, 1, duration);
> count is a 32bit value read from the file, so this hunk makes
> the demuxer allocate huge amount of memories for some
> files.
>
> Is there an upper limit for count?
>
>

In practice, if a valid mp4 blows up due to this ctts allocation, it's also 
going to blow up when AVIndexEntries is
allocated for the samples.  An invalid mp4 can do anything of course.

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Carl Eugen Hoyos]

2017-08-24 0:39 GMT+02:00 Dale Curtis :

> -sc->ctts_data[ctts_count].count= count;
> -sc->ctts_data[ctts_count].duration = duration;
> -ctts_count++;
> +/* Expand entries such that we have a 1-1 mapping with samples. */
> +for (j = 0; j < count; j++)
> +add_ctts_entry(>ctts_data, _count, 
> >ctts_allocated_size, 1, duration);

count is a 32bit value read from the file, so this hunk makes
the demuxer allocate huge amount of memories for some
files.

Is there an upper limit for count?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

On Fri, Aug 25, 2017 at 10:18 AM, Michael Niedermayer <
mich...@niedermayer.cc> wrote:
>
> hmm
>
> maybe i misunderstand but assuming we insert a block of dummy blank
> entries (without breaking monotonicity) and keep a pointer to that
> block
>
> then add entries with a av_add_index_entry()
> and in case of failure remove the blank entries
>
> this would result in the same as now and seems relativly simple
> it would not need memmove and in general would not need a log n
> search if each falls on the first spot of the block
>
> or am i missing something ?
>

This patch does what you're suggesting, but I'm not confident it's right in
all cases.

- dale


ctts_fast.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Daniel Glöckner]

On Thu, Aug 24, 2017 at 10:57:43AM +0200, Michael Niedermayer wrote:
> > From b4b49b6b584b33e1da95a5d72b05fd9134ab28f9 Mon Sep 17 00:00:00 2001
> > From: Dale Curtis 
> > Date: Mon, 17 Jul 2017 17:38:09 -0700
> > Subject: [PATCH] Fix trampling of ctts during seeks when sidx support is
> >  enabled.
> 
> applied
> 
> thanks

With this commit I can no longer reproduce the problem described in
ticket 6560, but I now get lots of

[mov,mp4,m4a,3gp,3g2,mj2 @ 0x9942020] Failed to add index entry

messages. These messages vanish if I also apply the patch that I posted
three weeks ago ( https://patchwork.ffmpeg.org/patch/4632/ ) and that
has so far received no comments.

Best regards,

  Daniel

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

On Thu, Aug 24, 2017 at 05:06:16PM -0700, Dale Curtis wrote:
> On Thu, Aug 24, 2017 at 2:27 AM, Michael Niedermayer  > wrote:
> 
> >
> > 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)
> >
> 
> Unfortunately this is hard / impossible to do if I understand what you're
> asking for correctly. Here's my response to the same suggestion from Rodger
> above: "We could speculatively move all entries based on the first insert
> and total entries count, but their are several conditionals in
> av_add_index_entry() which may cause a bail out and such failure would be
> unrecoverable (maybe painfully?) if we moved everything ahead of time."

hmm

maybe i misunderstand but assuming we insert a block of dummy blank
entries (without breaking monotonicity) and keep a pointer to that
block

then add entries with a av_add_index_entry()
and in case of failure remove the blank entries

this would result in the same as now and seems relativly simple
it would not need memmove and in general would not need a log n
search if each falls on the first spot of the block

or am i missing something ?





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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

On Thu, Aug 24, 2017 at 2:27 AM, Michael Niedermayer  wrote:

>
> 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)
>

Unfortunately this is hard / impossible to do if I understand what you're
asking for correctly. Here's my response to the same suggestion from Rodger
above: "We could speculatively move all entries based on the first insert
and total entries count, but their are several conditionals in
av_add_index_entry() which may cause a bail out and such failure would be
unrecoverable (maybe painfully?) if we moved everything ahead of time."

- dale
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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  > 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 
> 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 


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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  > 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 
> Date: Mon, 17 Jul 2017 17:38:09 -0700
> Subject: [PATCH] Fix trampling of ctts during seeks when sidx support is
>  enabled.

applied

thanks

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

On Mon, Aug 07, 2017 at 12:32:16PM -0700, Dale Curtis wrote:
> On Fri, Aug 4, 2017 at 4:38 PM, Michael Niedermayer 
> wrote:
> 
> > so theres no easy way to create a smaller file then 64mb ?
> >
> 
> Ah no, I didn't realize you had a size limit; I just meant none of the
> existing clips were large enough / worked. I've truncated the clip at
> http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4 to 180 seconds
> and the seek test passes and fails correctly.

file uploded

thx

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

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

On Sat, Aug 19, 2017 at 3:50 PM, Michael Niedermayer  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


ctts_fix_v9.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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.

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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


> 
> - dale
> 
> On Thu, Aug 10, 2017 at 1:02 PM, Dale Curtis 
> wrote:
> 
> > On Tue, Aug 8, 2017 at 6:48 PM, Michael Niedermayer <
> > mich...@niedermayer.cc> wrote:
> >
> >>
> >> the fate test seems to fail:
> >>
> >> did i do something silly ?
> >>
> >
> > Ah no, I did when I remuxed the test file. Updated expectations and test
> > clip at http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4
> >
> > - dale
> >
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

Anything else here? It'd be nice to get this landed soon if no one has any
other comments.

- dale

On Thu, Aug 10, 2017 at 1:02 PM, Dale Curtis 
wrote:

> On Tue, Aug 8, 2017 at 6:48 PM, Michael Niedermayer <
> mich...@niedermayer.cc> wrote:
>
>>
>> the fate test seems to fail:
>>
>> did i do something silly ?
>>
>
> Ah no, I did when I remuxed the test file. Updated expectations and test
> clip at http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4
>
> - dale
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

On Tue, Aug 8, 2017 at 6:48 PM, Michael Niedermayer 
wrote:

>
> the fate test seems to fail:
>
> did i do something silly ?
>

Ah no, I did when I remuxed the test file. Updated expectations and test
clip at http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4

- dale
From 6e77ff3deaa6e0ac04fb4e51dba1d4a0e69e9d5d Mon Sep 17 00:00:00 2001
From: Dale Curtis 
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.

Signed-off-by: Dale Curtis 
---
 libavformat/isom.h   |   1 +
 libavformat/mov.c|  79 ++--
 tests/fate/seek.mak  |   3 ++
 tests/ref/seek/extra-mp4 | 134 +++
 4 files changed, 189 insertions(+), 28 deletions(-)
 create mode 100644 tests/ref/seek/extra-mp4

diff --git a/libavformat/isom.h b/libavformat/isom.h
index ff009b0896..fdd98c28f5 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -137,6 +137,7 @@ typedef struct MOVStreamContext {
 unsigned int stts_count;
 MOVStts *stts_data;
 unsigned int ctts_count;
+unsigned int ctts_allocated_size;
 MOVStts *ctts_data;
 unsigned int stsc_count;
 MOVStsc *stsc_data;
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 63f84be782..85377f39a9 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -74,6 +74,8 @@ typedef struct MOVParseTableEntry {
 
 static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom);
 static int mov_read_mfra(MOVContext *c, AVIOContext *f);
+static int64_t add_ctts_entry(MOVStts** ctts_data, unsigned int* ctts_count, unsigned int* allocated_size,
+  int count, int duration);
 
 static int mov_metadata_track_or_disc_number(MOVContext *c, AVIOContext *pb,
  unsigned len, const char *key)
@@ -2708,7 +2710,7 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
 AVStream *st;
 MOVStreamContext *sc;
-unsigned int i, entries, ctts_count = 0;
+unsigned int i, j, entries, ctts_count = 0;
 
 if (c->fc->nb_streams < 1)
 return 0;
@@ -2726,7 +2728,7 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 if (entries >= UINT_MAX / sizeof(*sc->ctts_data))
 return AVERROR_INVALIDDATA;
 av_freep(>ctts_data);
-sc->ctts_data = av_realloc(NULL, entries * sizeof(*sc->ctts_data));
+sc->ctts_data = av_fast_realloc(NULL, >ctts_allocated_size, entries * sizeof(*sc->ctts_data));
 if (!sc->ctts_data)
 return AVERROR(ENOMEM);
 
@@ -2741,9 +2743,9 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 continue;
 }
 
-sc->ctts_data[ctts_count].count= count;
-sc->ctts_data[ctts_count].duration = duration;
-ctts_count++;
+/* Expand entries such that we have a 1-1 mapping with samples. */
+for (j = 0; j < count; j++)
+add_ctts_entry(>ctts_data, _count, >ctts_allocated_size, 1, duration);
 
 av_log(c->fc, AV_LOG_TRACE, "count=%d, duration=%d\n",
 count, duration);
@@ -3046,7 +3048,6 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
 int64_t index;
 int64_t index_ctts_count;
 int flags;
-unsigned int ctts_allocated_size = 0;
 int64_t start_dts = 0;
 int64_t edit_list_media_time_dts = 0;
 int64_t edit_list_start_encountered = 0;
@@ -3081,6 +3082,7 @@ static void 

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

[Michael Niedermayer]

On Mon, Aug 07, 2017 at 12:31:01PM -0700, Dale Curtis wrote:
> On Fri, Aug 4, 2017 at 4:40 PM, Rodger Combs  wrote:
> 
> > +sc->ctts_data = av_fast_realloc(sc->ctts_data,
> > >ctts_allocated_size, request_size);
> > ^ this line is incorrect; setting realloc's first arg to its return value
> > leaks the existing allocation in the OOM case. Since you're doing your own
> > calculation for the desired new size here, you may want to use av_reallocp
> > (which frees the original allocation on failure).
> >
> 
> Thanks, fixed; kept av_fast_realloc() for size setting simplicity, but
> reused function-scope ctts_data as temporary.
> 
> 
> >
> > When reading a trun that requires this sort of realloc, is it common for
> > there to be any existing entries in the range we're writing? Would it be
> > safe to remove+replace them? Could we do a single realloc + single memmove,
> > and then fill the newly-opened space, rather than repeating the memmove for
> > each entry?
> >
> 
> Yes it's common for their to be existing entries. To be clear this occurs
> with standard files that are of sufficient length that a seek can occur
> before all trun boxes are read. We could speculatively move all entries
> based on the first insert and total entries count, but their are several
> conditionals in av_add_index_entry() which may cause a bail out and such
> failure would be unrecoverable (maybe painfully?) if we moved everything
> ahead of time.
> 
> - dale

>  libavformat/isom.h   |1 
>  libavformat/mov.c|   79 +--
>  tests/fate/seek.mak  |3 +
>  tests/ref/seek/extra-mp4 |  134 
> +++
>  4 files changed, 189 insertions(+), 28 deletions(-)
> a2c3a9c29cc60b76508cc7fd9168bdffd60c6ead  ctts_fix_v7.patch
> From 4938dac2d9f3b40c62822d9129046edbde44468d Mon Sep 17 00:00:00 2001
> From: Dale Curtis 
> 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.
> 
> Signed-off-by: Dale Curtis 
> ---
>  libavformat/isom.h   |   1 +
>  libavformat/mov.c|  79 ++--
>  tests/fate/seek.mak  |   3 ++
>  tests/ref/seek/extra-mp4 | 134 
> +++
>  4 files changed, 189 insertions(+), 28 deletions(-)
>  create mode 100644 tests/ref/seek/extra-mp4

the fate test seems to fail:

did i do something silly ?

TESTseek-extra-mp4
--- ./tests/ref/seek/extra-mp4  2017-08-09 03:46:04.041659416 +0200
+++ tests/data/fate/seek-extra-mp4  2017-08-09 03:46:18.565659589 +0200
@@ -1,134 +1,134 @@
-ret: 0 st: 0 flags:1 dts:-0.03 pts: 0.00 pos:   4174 size:   
147
-ret: 0 st: 0 flags:0 dts: 0.00 pts: 0.03 pos:   4321 size:
24
-ret: 0 st: 0 flags:0 dts: 0.03 pts: 0.07 pos:   4345 size:  
6779
-ret: 0 st: 0 flags:0 dts: 0.07 pts: 0.10 pos:  11124 size: 
11041
+ret: 0 st: 0 flags:1 dts:-0.03 pts: 0.00 pos: 48 size:   
147
+ret: 0 st: 0 flags:0 dts: 0.00 pts: 0.03 pos:195 size:
24
+ret: 0 st: 0 flags:0 dts: 0.03 pts: 0.07 pos:219 size:  
6779
+ret: 0 st: 0 flags:0 dts: 0.07 pts: 0.10 pos:   6998 size: 
11041
 ret: 0 st:-1 flags:0  ts:-1.00
-ret: 0 st: 0 flags:1 dts:-0.03 pts: 0.00 pos:   4174 size:   
147
-ret: 0 st: 0 flags:0 

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

[Dale Curtis]

On Fri, Aug 4, 2017 at 4:40 PM, Rodger Combs  wrote:

> +sc->ctts_data = av_fast_realloc(sc->ctts_data,
> >ctts_allocated_size, request_size);
> ^ this line is incorrect; setting realloc's first arg to its return value
> leaks the existing allocation in the OOM case. Since you're doing your own
> calculation for the desired new size here, you may want to use av_reallocp
> (which frees the original allocation on failure).
>

Thanks, fixed; kept av_fast_realloc() for size setting simplicity, but
reused function-scope ctts_data as temporary.


>
> When reading a trun that requires this sort of realloc, is it common for
> there to be any existing entries in the range we're writing? Would it be
> safe to remove+replace them? Could we do a single realloc + single memmove,
> and then fill the newly-opened space, rather than repeating the memmove for
> each entry?
>

Yes it's common for their to be existing entries. To be clear this occurs
with standard files that are of sufficient length that a seek can occur
before all trun boxes are read. We could speculatively move all entries
based on the first insert and total entries count, but their are several
conditionals in av_add_index_entry() which may cause a bail out and such
failure would be unrecoverable (maybe painfully?) if we moved everything
ahead of time.

- dale
From 4938dac2d9f3b40c62822d9129046edbde44468d Mon Sep 17 00:00:00 2001
From: Dale Curtis 
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.

Signed-off-by: Dale Curtis 
---
 libavformat/isom.h   |   1 +
 libavformat/mov.c|  79 ++--
 tests/fate/seek.mak  |   3 ++
 tests/ref/seek/extra-mp4 | 134 +++
 4 files changed, 189 insertions(+), 28 deletions(-)
 create mode 100644 tests/ref/seek/extra-mp4

diff --git a/libavformat/isom.h b/libavformat/isom.h
index ff009b0896..fdd98c28f5 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -137,6 +137,7 @@ typedef struct MOVStreamContext {
 unsigned int stts_count;
 MOVStts *stts_data;
 unsigned int ctts_count;
+unsigned int ctts_allocated_size;
 MOVStts *ctts_data;
 unsigned int stsc_count;
 MOVStsc *stsc_data;
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 63f84be782..85377f39a9 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -74,6 +74,8 @@ typedef struct MOVParseTableEntry {
 
 static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom);
 static int mov_read_mfra(MOVContext *c, AVIOContext *f);
+static int64_t add_ctts_entry(MOVStts** ctts_data, unsigned int* ctts_count, unsigned int* allocated_size,
+  int count, int duration);
 
 static int mov_metadata_track_or_disc_number(MOVContext *c, AVIOContext *pb,
  unsigned len, const char *key)
@@ -2708,7 +2710,7 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
 AVStream *st;
 MOVStreamContext *sc;
-unsigned int i, entries, ctts_count = 0;
+unsigned int i, j, entries, ctts_count = 0;
 
 if (c->fc->nb_streams < 1)
 return 0;
@@ -2726,7 +2728,7 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 if (entries >= UINT_MAX / sizeof(*sc->ctts_data))
 return AVERROR_INVALIDDATA;
 av_freep(>ctts_data);
-sc->ctts_data = av_realloc(NULL, entries * sizeof(*sc->ctts_data));
+

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

[Dale Curtis]

On Fri, Aug 4, 2017 at 4:38 PM, Michael Niedermayer 
wrote:

> so theres no easy way to create a smaller file then 64mb ?
>

Ah no, I didn't realize you had a size limit; I just meant none of the
existing clips were large enough / worked. I've truncated the clip at
http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4 to 180 seconds
and the seek test passes and fails correctly.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rodger Combs]

+sc->ctts_data = av_fast_realloc(sc->ctts_data, 
>ctts_allocated_size, request_size);
^ this line is incorrect; setting realloc's first arg to its return value leaks 
the existing allocation in the OOM case. Since you're doing your own 
calculation for the desired new size here, you may want to use av_reallocp 
(which frees the original allocation on failure).

When reading a trun that requires this sort of realloc, is it common for there 
to be any existing entries in the range we're writing? Would it be safe to 
remove+replace them? Could we do a single realloc + single memmove, and then 
fill the newly-opened space, rather than repeating the memmove for each entry?

> On Aug 4, 2017, at 18:02, Dale Curtis  wrote:
> 
> Any comments here? +rodger who wrote the original sidx processing for review.
> 
> - dale
> 
> On Mon, Jul 31, 2017 at 3:01 PM, Dale Curtis  > wrote:
> Whoops, that patch accidentally reverted to an earlier version. Here's the 
> fixed one that works with the mpg sample mentioned above.
> 
> - dale
> 
> On Mon, Jul 31, 2017 at 2:29 PM, Dale Curtis  > wrote:
> Here's an updated patch with a fate test attached. You'll need to add 
> http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4 
>  to the 
> fate-suite/mov for this test. This is licensed under creative commons 
> attribution, so it should be fine for tests: https://peach.blender.org/about/ 
>  I tried to use the existing samples, but 
> you need a clip long enough that the entire index isn't generated from the 
> start.
> 
> - dale
> 
> On Tue, Jul 25, 2017 at 1:03 PM, Michael Niedermayer  > wrote:
> On Mon, Jul 24, 2017 at 02:32:41PM -0700, Dale Curtis wrote:
> > On Thu, Jul 20, 2017 at 5:00 AM, Michael Niedermayer  > > wrote:
> >
> > > Hi
> > >
> > > On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote:
> > > > Thanks will take a look. Is this test not part of fate? make fate passed
> > >
> > > no, we should have tests for all (fixed) tickets in fate ideally
> > > but in reality most tickets lack a corresponding test
> > > I tried both in outreachy and as well in GSoC to improve this situation
> > > with student projects but both only moved this forward by a small
> > > step. Its a large amount of work to create robust, portable and
> > > practical tests for "all" tickets and everything else.
> > > The way out to get this actually done would be to pay a developer to
> > > create tests for "all" tickets in fate. I belive carl would be the
> > > ideal one to do this work as he has since a very long time always tested
> > > and kept track of all our tickets.
> > > I did suggest a while ago to someone at google that funding such
> > > project would make sense but IIRC i never heared back.
> > > if some company would fund something like this, i belive this would be
> > > very usefull in the long run for code quality
> > >
> >
> > I think it'd be pretty hard to get someone to go through and create tests
> > for every issue ever seen. Even Chromium has trouble with this, but making
> 
> yes, unless theres someone who enjoys doing this kind of work.
> 
> 
> > it part of the culture helps. I.e. reviewers should ask for every change to
> > include a test.
> 
> yes though theres already
> 1.8 patch submission checklist
> ...
> 26. Consider adding a regression test for your code.
> on http://ffmpeg.org/developer.html 
> 
> 
> > I'm happy to add one to fate for this change. Or can do so
> > in a followup patch if you prefer.
> 
> please do (any variant is fine)
> 
> thx
> 
> >
> > Does anyone have comments on this change specifically? We've already rolled
> > this into Chrome and it's working fine and passing all regression tests.
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org 
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel 
> > 
> 
> --
> 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
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org 
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel 
> 
> 
> 
> 
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

On Mon, Jul 31, 2017 at 02:29:29PM -0700, Dale Curtis wrote:
> Here's an updated patch with a fate test attached. You'll need to add
> http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4 to the
> fate-suite/mov for this test. This is licensed under creative commons
> attribution, so it should be fine for tests:
> https://peach.blender.org/about/ I tried to use the existing samples, but
> you need a clip long enough that the entire index isn't generated from the
> start.

so theres no easy way to create a smaller file then 64mb ?

iam trying to keep the size of the fate samples small,
imagening 64mb * thousands of tests and each fate client needs a copy
of this...


[...]

-- 
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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

Any comments here? +rodger who wrote the original sidx processing for
review.

- dale

On Mon, Jul 31, 2017 at 3:01 PM, Dale Curtis 
wrote:

> Whoops, that patch accidentally reverted to an earlier version. Here's the
> fixed one that works with the mpg sample mentioned above.
>
> - dale
>
> On Mon, Jul 31, 2017 at 2:29 PM, Dale Curtis 
> wrote:
>
>> Here's an updated patch with a fate test attached. You'll need to add
>> http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4 to the
>> fate-suite/mov for this test. This is licensed under creative commons
>> attribution, so it should be fine for tests: https://peach.blender.o
>> rg/about/ I tried to use the existing samples, but you need a clip long
>> enough that the entire index isn't generated from the start.
>>
>> - dale
>>
>> On Tue, Jul 25, 2017 at 1:03 PM, Michael Niedermayer <
>> mich...@niedermayer.cc> wrote:
>>
>>> On Mon, Jul 24, 2017 at 02:32:41PM -0700, Dale Curtis wrote:
>>> > On Thu, Jul 20, 2017 at 5:00 AM, Michael Niedermayer
>>> >> > > wrote:
>>> >
>>> > > Hi
>>> > >
>>> > > On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote:
>>> > > > Thanks will take a look. Is this test not part of fate? make fate
>>> passed
>>> > >
>>> > > no, we should have tests for all (fixed) tickets in fate ideally
>>> > > but in reality most tickets lack a corresponding test
>>> > > I tried both in outreachy and as well in GSoC to improve this
>>> situation
>>> > > with student projects but both only moved this forward by a small
>>> > > step. Its a large amount of work to create robust, portable and
>>> > > practical tests for "all" tickets and everything else.
>>> > > The way out to get this actually done would be to pay a developer to
>>> > > create tests for "all" tickets in fate. I belive carl would be the
>>> > > ideal one to do this work as he has since a very long time always
>>> tested
>>> > > and kept track of all our tickets.
>>> > > I did suggest a while ago to someone at google that funding such
>>> > > project would make sense but IIRC i never heared back.
>>> > > if some company would fund something like this, i belive this would
>>> be
>>> > > very usefull in the long run for code quality
>>> > >
>>> >
>>> > I think it'd be pretty hard to get someone to go through and create
>>> tests
>>> > for every issue ever seen. Even Chromium has trouble with this, but
>>> making
>>>
>>> yes, unless theres someone who enjoys doing this kind of work.
>>>
>>>
>>> > it part of the culture helps. I.e. reviewers should ask for every
>>> change to
>>> > include a test.
>>>
>>> yes though theres already
>>> 1.8 patch submission checklist
>>> ...
>>> 26. Consider adding a regression test for your code.
>>> on http://ffmpeg.org/developer.html
>>>
>>>
>>> > I'm happy to add one to fate for this change. Or can do so
>>> > in a followup patch if you prefer.
>>>
>>> please do (any variant is fine)
>>>
>>> thx
>>>
>>> >
>>> > Does anyone have comments on this change specifically? We've already
>>> rolled
>>> > this into Chrome and it's working fine and passing all regression
>>> tests.
>>> > ___
>>> > ffmpeg-devel mailing list
>>> > ffmpeg-devel@ffmpeg.org
>>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> --
>>> 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
>>>
>>> ___
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>>
>>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

Whoops, that patch accidentally reverted to an earlier version. Here's the
fixed one that works with the mpg sample mentioned above.

- dale

On Mon, Jul 31, 2017 at 2:29 PM, Dale Curtis 
wrote:

> Here's an updated patch with a fate test attached. You'll need to add
> http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4 to the
> fate-suite/mov for this test. This is licensed under creative commons
> attribution, so it should be fine for tests: https://peach.blender.o
> rg/about/ I tried to use the existing samples, but you need a clip long
> enough that the entire index isn't generated from the start.
>
> - dale
>
> On Tue, Jul 25, 2017 at 1:03 PM, Michael Niedermayer <
> mich...@niedermayer.cc> wrote:
>
>> On Mon, Jul 24, 2017 at 02:32:41PM -0700, Dale Curtis wrote:
>> > On Thu, Jul 20, 2017 at 5:00 AM, Michael Niedermayer
>> > > > wrote:
>> >
>> > > Hi
>> > >
>> > > On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote:
>> > > > Thanks will take a look. Is this test not part of fate? make fate
>> passed
>> > >
>> > > no, we should have tests for all (fixed) tickets in fate ideally
>> > > but in reality most tickets lack a corresponding test
>> > > I tried both in outreachy and as well in GSoC to improve this
>> situation
>> > > with student projects but both only moved this forward by a small
>> > > step. Its a large amount of work to create robust, portable and
>> > > practical tests for "all" tickets and everything else.
>> > > The way out to get this actually done would be to pay a developer to
>> > > create tests for "all" tickets in fate. I belive carl would be the
>> > > ideal one to do this work as he has since a very long time always
>> tested
>> > > and kept track of all our tickets.
>> > > I did suggest a while ago to someone at google that funding such
>> > > project would make sense but IIRC i never heared back.
>> > > if some company would fund something like this, i belive this would be
>> > > very usefull in the long run for code quality
>> > >
>> >
>> > I think it'd be pretty hard to get someone to go through and create
>> tests
>> > for every issue ever seen. Even Chromium has trouble with this, but
>> making
>>
>> yes, unless theres someone who enjoys doing this kind of work.
>>
>>
>> > it part of the culture helps. I.e. reviewers should ask for every
>> change to
>> > include a test.
>>
>> yes though theres already
>> 1.8 patch submission checklist
>> ...
>> 26. Consider adding a regression test for your code.
>> on http://ffmpeg.org/developer.html
>>
>>
>> > I'm happy to add one to fate for this change. Or can do so
>> > in a followup patch if you prefer.
>>
>> please do (any variant is fine)
>>
>> thx
>>
>> >
>> > Does anyone have comments on this change specifically? We've already
>> rolled
>> > this into Chrome and it's working fine and passing all regression tests.
>> > ___
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel@ffmpeg.org
>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> --
>> 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
>>
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>>
>
From 72f1211befcd169fbf343b902185995aece926df Mon Sep 17 00:00:00 2001
From: Dale Curtis 
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 

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

[Dale Curtis]

Here's an updated patch with a fate test attached. You'll need to add
http://storage.googleapis.com/dalecurtis/buck480p30_na.mp4 to the
fate-suite/mov for this test. This is licensed under creative commons
attribution, so it should be fine for tests:
https://peach.blender.org/about/ I tried to use the existing samples, but
you need a clip long enough that the entire index isn't generated from the
start.

- dale

On Tue, Jul 25, 2017 at 1:03 PM, Michael Niedermayer  wrote:

> On Mon, Jul 24, 2017 at 02:32:41PM -0700, Dale Curtis wrote:
> > On Thu, Jul 20, 2017 at 5:00 AM, Michael Niedermayer
>  > > wrote:
> >
> > > Hi
> > >
> > > On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote:
> > > > Thanks will take a look. Is this test not part of fate? make fate
> passed
> > >
> > > no, we should have tests for all (fixed) tickets in fate ideally
> > > but in reality most tickets lack a corresponding test
> > > I tried both in outreachy and as well in GSoC to improve this situation
> > > with student projects but both only moved this forward by a small
> > > step. Its a large amount of work to create robust, portable and
> > > practical tests for "all" tickets and everything else.
> > > The way out to get this actually done would be to pay a developer to
> > > create tests for "all" tickets in fate. I belive carl would be the
> > > ideal one to do this work as he has since a very long time always
> tested
> > > and kept track of all our tickets.
> > > I did suggest a while ago to someone at google that funding such
> > > project would make sense but IIRC i never heared back.
> > > if some company would fund something like this, i belive this would be
> > > very usefull in the long run for code quality
> > >
> >
> > I think it'd be pretty hard to get someone to go through and create tests
> > for every issue ever seen. Even Chromium has trouble with this, but
> making
>
> yes, unless theres someone who enjoys doing this kind of work.
>
>
> > it part of the culture helps. I.e. reviewers should ask for every change
> to
> > include a test.
>
> yes though theres already
> 1.8 patch submission checklist
> ...
> 26. Consider adding a regression test for your code.
> on http://ffmpeg.org/developer.html
>
>
> > I'm happy to add one to fate for this change. Or can do so
> > in a followup patch if you prefer.
>
> please do (any variant is fine)
>
> thx
>
> >
> > Does anyone have comments on this change specifically? We've already
> rolled
> > this into Chrome and it's working fine and passing all regression tests.
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> --
> 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
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
From 8c99024ff723853d8d8f83d9862b884e57fe2b83 Mon Sep 17 00:00:00 2001
From: Dale Curtis 
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.

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.

Additionally since ctts samples from trun boxes always have a count
of 1, there's probably an opportunity to avoid the post-seek fixup
that iterates O(n) over all samples with an O(1) when no non-1 count
samples are present.

Signed-off-by: Dale Curtis 
---
 

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

[Michael Niedermayer]

On Mon, Jul 24, 2017 at 02:32:41PM -0700, Dale Curtis wrote:
> On Thu, Jul 20, 2017 at 5:00 AM, Michael Niedermayer  > wrote:
> 
> > Hi
> >
> > On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote:
> > > Thanks will take a look. Is this test not part of fate? make fate passed
> >
> > no, we should have tests for all (fixed) tickets in fate ideally
> > but in reality most tickets lack a corresponding test
> > I tried both in outreachy and as well in GSoC to improve this situation
> > with student projects but both only moved this forward by a small
> > step. Its a large amount of work to create robust, portable and
> > practical tests for "all" tickets and everything else.
> > The way out to get this actually done would be to pay a developer to
> > create tests for "all" tickets in fate. I belive carl would be the
> > ideal one to do this work as he has since a very long time always tested
> > and kept track of all our tickets.
> > I did suggest a while ago to someone at google that funding such
> > project would make sense but IIRC i never heared back.
> > if some company would fund something like this, i belive this would be
> > very usefull in the long run for code quality
> >
> 
> I think it'd be pretty hard to get someone to go through and create tests
> for every issue ever seen. Even Chromium has trouble with this, but making

yes, unless theres someone who enjoys doing this kind of work.


> it part of the culture helps. I.e. reviewers should ask for every change to
> include a test.

yes though theres already
1.8 patch submission checklist
...
26. Consider adding a regression test for your code.
on http://ffmpeg.org/developer.html


> I'm happy to add one to fate for this change. Or can do so
> in a followup patch if you prefer.

please do (any variant is fine)

thx

> 
> Does anyone have comments on this change specifically? We've already rolled
> this into Chrome and it's working fine and passing all regression tests.
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

On Thu, Jul 20, 2017 at 5:00 AM, Michael Niedermayer  wrote:

> Hi
>
> On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote:
> > Thanks will take a look. Is this test not part of fate? make fate passed
>
> no, we should have tests for all (fixed) tickets in fate ideally
> but in reality most tickets lack a corresponding test
> I tried both in outreachy and as well in GSoC to improve this situation
> with student projects but both only moved this forward by a small
> step. Its a large amount of work to create robust, portable and
> practical tests for "all" tickets and everything else.
> The way out to get this actually done would be to pay a developer to
> create tests for "all" tickets in fate. I belive carl would be the
> ideal one to do this work as he has since a very long time always tested
> and kept track of all our tickets.
> I did suggest a while ago to someone at google that funding such
> project would make sense but IIRC i never heared back.
> if some company would fund something like this, i belive this would be
> very usefull in the long run for code quality
>

I think it'd be pretty hard to get someone to go through and create tests
for every issue ever seen. Even Chromium has trouble with this, but making
it part of the culture helps. I.e. reviewers should ask for every change to
include a test. I'm happy to add one to fate for this change. Or can do so
in a followup patch if you prefer.

Does anyone have comments on this change specifically? We've already rolled
this into Chrome and it's working fine and passing all regression tests.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

Hi

On Wed, Jul 19, 2017 at 07:30:01PM -0700, Dale Curtis wrote:
> Thanks will take a look. Is this test not part of fate? make fate passed

no, we should have tests for all (fixed) tickets in fate ideally
but in reality most tickets lack a corresponding test
I tried both in outreachy and as well in GSoC to improve this situation
with student projects but both only moved this forward by a small
step. Its a large amount of work to create robust, portable and
practical tests for "all" tickets and everything else.
The way out to get this actually done would be to pay a developer to
create tests for "all" tickets in fate. I belive carl would be the
ideal one to do this work as he has since a very long time always tested
and kept track of all our tickets.
I did suggest a while ago to someone at google that funding such
project would make sense but IIRC i never heared back.
if some company would fund something like this, i belive this would be
very usefull in the long run for code quality



> for me. The attached patch fixes this; the issue was that the index entries
> are 1 to 1 with ctts values. When samples are added without ctts entries
> we'd just initialize a single ctts entry with a count of 5. This left a gap
> in the ctts table; the fix is to use only 1-count entries when this case is
> hit.
> 
> Note: This made me realize the presence of a ctts box and a trun box with
> ctts samples has always been broken. If the ctts box comes second it'll
> wipe the trun's generated table, but if the trun box comes after the ctts
> box it will try to insert samples at incorrect positions. Prior to my patch
> they would be looked up at incorrect positions, so there shouldn't be any
> new bad behavior here.
> 
> - dale

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Dale Curtis]

I found some samples with ctts and trun boxes, so I've updated the patch to
handle these cases since I don't know how common they are in the wild and
it's an easy fix. I'll send a followup patch if this one is accepted to
remove support for > 1 count ctts entries.

- dale

On Wed, Jul 19, 2017 at 7:30 PM, Dale Curtis 
wrote:

> Thanks will take a look. Is this test not part of fate? make fate passed
> for me. The attached patch fixes this; the issue was that the index entries
> are 1 to 1 with ctts values. When samples are added without ctts entries
> we'd just initialize a single ctts entry with a count of 5. This left a gap
> in the ctts table; the fix is to use only 1-count entries when this case is
> hit.
>
> Note: This made me realize the presence of a ctts box and a trun box with
> ctts samples has always been broken. If the ctts box comes second it'll
> wipe the trun's generated table, but if the trun box comes after the ctts
> box it will try to insert samples at incorrect positions. Prior to my patch
> they would be looked up at incorrect positions, so there shouldn't be any
> new bad behavior here.
>
> - dale
>
> On Wed, Jul 19, 2017 at 3:28 PM, Michael Niedermayer <
> mich...@niedermayer.cc> wrote:
>
>> On Tue, Jul 18, 2017 at 11:49:26AM -0700, Dale Curtis wrote:
>> > Updated patch that fixes other ctts modification code to use the new
>> > ctts_allocated_size value; I've verified this passes fate.
>> >
>> > - dale
>> >
>> > On Tue, Jul 18, 2017 at 9:53 AM, Dale Curtis 
>> > wrote:
>> >
>> > > Resending as it's own message per contribution rules. I've also
>> attached
>> > > the patch in case the formatting gets trashed.
>> > >
>> > > 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.
>> > >
>> > > 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.
>> > >
>> > > Additionally since ctts samples from trun boxes always have a count
>> > > of 1, there's probably an opportunity to avoid the post-seek fixup
>> > > that iterates O(n) over all samples with an O(1) when no non-1 count
>> > > samples are present.
>> > >
>> > > Signed-off-by: Dale Curtis 
>> > > ---
>> > >  libavformat/isom.h |  1 +
>> > >  libavformat/mov.c  | 46 ++
>> +---
>> > >  2 files changed, 32 insertions(+), 15 deletions(-)
>> > >
>> > > diff --git a/libavformat/isom.h b/libavformat/isom.h
>> > > index ff009b0896..fdd98c28f5 100644
>> > > --- a/libavformat/isom.h
>> > > +++ b/libavformat/isom.h
>> > > @@ -137,6 +137,7 @@ typedef struct MOVStreamContext {
>> > >  unsigned int stts_count;
>> > >  MOVStts *stts_data;
>> > >  unsigned int ctts_count;
>> > > +unsigned int ctts_allocated_size;
>> > >  MOVStts *ctts_data;
>> > >  unsigned int stsc_count;
>> > >  MOVStsc *stsc_data;
>> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
>> > > index 63f84be782..412290b435 100644
>> > > --- a/libavformat/mov.c
>> > > +++ b/libavformat/mov.c
>> > > @@ -4297,11 +4297,6 @@ static int mov_read_trun(MOVContext *c,
>> AVIOContext
>> > > *pb, MOVAtom atom)
>> > >  }
>> > >  if ((uint64_t)entries+sc->ctts_count >=
>> > > UINT_MAX/sizeof(*sc->ctts_data))
>> > >  return AVERROR_INVALIDDATA;
>> > > -if ((err = av_reallocp_array(>ctts_data, entries +
>> > > sc->ctts_count,
>> > > - sizeof(*sc->ctts_data))) < 0) {
>> > > -sc->ctts_count = 0;
>> > > -return err;
>> > > -}
>> > >  if (flags & MOV_TRUN_DATA_OFFSET)data_offset=
>> > > avio_rb32(pb);
>> > >  if (flags & MOV_TRUN_FIRST_SAMPLE_FLAGS) first_sample_flags =
>> > > avio_rb32(pb);
>> > >  

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

[Dale Curtis]

Thanks will take a look. Is this test not part of fate? make fate passed
for me. The attached patch fixes this; the issue was that the index entries
are 1 to 1 with ctts values. When samples are added without ctts entries
we'd just initialize a single ctts entry with a count of 5. This left a gap
in the ctts table; the fix is to use only 1-count entries when this case is
hit.

Note: This made me realize the presence of a ctts box and a trun box with
ctts samples has always been broken. If the ctts box comes second it'll
wipe the trun's generated table, but if the trun box comes after the ctts
box it will try to insert samples at incorrect positions. Prior to my patch
they would be looked up at incorrect positions, so there shouldn't be any
new bad behavior here.

- dale

On Wed, Jul 19, 2017 at 3:28 PM, Michael Niedermayer  wrote:

> On Tue, Jul 18, 2017 at 11:49:26AM -0700, Dale Curtis wrote:
> > Updated patch that fixes other ctts modification code to use the new
> > ctts_allocated_size value; I've verified this passes fate.
> >
> > - dale
> >
> > On Tue, Jul 18, 2017 at 9:53 AM, Dale Curtis 
> > wrote:
> >
> > > Resending as it's own message per contribution rules. I've also
> attached
> > > the patch in case the formatting gets trashed.
> > >
> > > 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.
> > >
> > > 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.
> > >
> > > Additionally since ctts samples from trun boxes always have a count
> > > of 1, there's probably an opportunity to avoid the post-seek fixup
> > > that iterates O(n) over all samples with an O(1) when no non-1 count
> > > samples are present.
> > >
> > > Signed-off-by: Dale Curtis 
> > > ---
> > >  libavformat/isom.h |  1 +
> > >  libavformat/mov.c  | 46 ++
> +---
> > >  2 files changed, 32 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/libavformat/isom.h b/libavformat/isom.h
> > > index ff009b0896..fdd98c28f5 100644
> > > --- a/libavformat/isom.h
> > > +++ b/libavformat/isom.h
> > > @@ -137,6 +137,7 @@ typedef struct MOVStreamContext {
> > >  unsigned int stts_count;
> > >  MOVStts *stts_data;
> > >  unsigned int ctts_count;
> > > +unsigned int ctts_allocated_size;
> > >  MOVStts *ctts_data;
> > >  unsigned int stsc_count;
> > >  MOVStsc *stsc_data;
> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > index 63f84be782..412290b435 100644
> > > --- a/libavformat/mov.c
> > > +++ b/libavformat/mov.c
> > > @@ -4297,11 +4297,6 @@ static int mov_read_trun(MOVContext *c,
> AVIOContext
> > > *pb, MOVAtom atom)
> > >  }
> > >  if ((uint64_t)entries+sc->ctts_count >=
> > > UINT_MAX/sizeof(*sc->ctts_data))
> > >  return AVERROR_INVALIDDATA;
> > > -if ((err = av_reallocp_array(>ctts_data, entries +
> > > sc->ctts_count,
> > > - sizeof(*sc->ctts_data))) < 0) {
> > > -sc->ctts_count = 0;
> > > -return err;
> > > -}
> > >  if (flags & MOV_TRUN_DATA_OFFSET)data_offset=
> > > avio_rb32(pb);
> > >  if (flags & MOV_TRUN_FIRST_SAMPLE_FLAGS) first_sample_flags =
> > > avio_rb32(pb);
> > >  dts= sc->track_end - sc->time_offset;
> > > @@ -4312,26 +4307,28 @@ static int mov_read_trun(MOVContext *c,
> > > AVIOContext *pb, MOVAtom atom)
> > >  unsigned sample_size = frag->size;
> > >  int sample_flags = i ? frag->flags : first_sample_flags;
> > >  unsigned sample_duration = frag->duration;
> > > +unsigned ctts_duration = 0;
> > >  int keyframe = 0;
> > > +int ctts_index = 0;
> > > +int 

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

[Michael Niedermayer]

On Tue, Jul 18, 2017 at 11:49:26AM -0700, Dale Curtis wrote:
> Updated patch that fixes other ctts modification code to use the new
> ctts_allocated_size value; I've verified this passes fate.
> 
> - dale
> 
> On Tue, Jul 18, 2017 at 9:53 AM, Dale Curtis 
> wrote:
> 
> > Resending as it's own message per contribution rules. I've also attached
> > the patch in case the formatting gets trashed.
> >
> > 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.
> >
> > 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.
> >
> > Additionally since ctts samples from trun boxes always have a count
> > of 1, there's probably an opportunity to avoid the post-seek fixup
> > that iterates O(n) over all samples with an O(1) when no non-1 count
> > samples are present.
> >
> > Signed-off-by: Dale Curtis 
> > ---
> >  libavformat/isom.h |  1 +
> >  libavformat/mov.c  | 46 +++---
> >  2 files changed, 32 insertions(+), 15 deletions(-)
> >
> > diff --git a/libavformat/isom.h b/libavformat/isom.h
> > index ff009b0896..fdd98c28f5 100644
> > --- a/libavformat/isom.h
> > +++ b/libavformat/isom.h
> > @@ -137,6 +137,7 @@ typedef struct MOVStreamContext {
> >  unsigned int stts_count;
> >  MOVStts *stts_data;
> >  unsigned int ctts_count;
> > +unsigned int ctts_allocated_size;
> >  MOVStts *ctts_data;
> >  unsigned int stsc_count;
> >  MOVStsc *stsc_data;
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 63f84be782..412290b435 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -4297,11 +4297,6 @@ static int mov_read_trun(MOVContext *c, AVIOContext
> > *pb, MOVAtom atom)
> >  }
> >  if ((uint64_t)entries+sc->ctts_count >=
> > UINT_MAX/sizeof(*sc->ctts_data))
> >  return AVERROR_INVALIDDATA;
> > -if ((err = av_reallocp_array(>ctts_data, entries +
> > sc->ctts_count,
> > - sizeof(*sc->ctts_data))) < 0) {
> > -sc->ctts_count = 0;
> > -return err;
> > -}
> >  if (flags & MOV_TRUN_DATA_OFFSET)data_offset=
> > avio_rb32(pb);
> >  if (flags & MOV_TRUN_FIRST_SAMPLE_FLAGS) first_sample_flags =
> > avio_rb32(pb);
> >  dts= sc->track_end - sc->time_offset;
> > @@ -4312,26 +4307,28 @@ static int mov_read_trun(MOVContext *c,
> > AVIOContext *pb, MOVAtom atom)
> >  unsigned sample_size = frag->size;
> >  int sample_flags = i ? frag->flags : first_sample_flags;
> >  unsigned sample_duration = frag->duration;
> > +unsigned ctts_duration = 0;
> >  int keyframe = 0;
> > +int ctts_index = 0;
> > +int old_nb_index_entries = st->nb_index_entries;
> >
> >  if (flags & MOV_TRUN_SAMPLE_DURATION) sample_duration =
> > avio_rb32(pb);
> >  if (flags & MOV_TRUN_SAMPLE_SIZE) sample_size =
> > avio_rb32(pb);
> >  if (flags & MOV_TRUN_SAMPLE_FLAGS)sample_flags=
> > avio_rb32(pb);
> > -sc->ctts_data[sc->ctts_count].count = 1;
> > -sc->ctts_data[sc->ctts_count].duration = (flags &
> > MOV_TRUN_SAMPLE_CTS) ?
> > -  avio_rb32(pb) : 0;
> > -mov_update_dts_shift(sc, sc->ctts_data[sc->ctts_count].duration);
> > +if (flags & MOV_TRUN_SAMPLE_CTS)  ctts_duration   =
> > avio_rb32(pb);
> > +
> > +mov_update_dts_shift(sc, ctts_duration);
> >  if (frag->time != AV_NOPTS_VALUE) {
> >  if (c->use_mfra_for == FF_MOV_FLAG_MFRA_PTS) {
> >  int64_t pts = frag->time;
> >  av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64
> >  " sc->dts_shift %d ctts.duration %d"
> > 

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

[Dale Curtis]

Updated patch that fixes other ctts modification code to use the new
ctts_allocated_size value; I've verified this passes fate.

- dale

On Tue, Jul 18, 2017 at 9:53 AM, Dale Curtis 
wrote:

> Resending as it's own message per contribution rules. I've also attached
> the patch in case the formatting gets trashed.
>
> 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.
>
> 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.
>
> Additionally since ctts samples from trun boxes always have a count
> of 1, there's probably an opportunity to avoid the post-seek fixup
> that iterates O(n) over all samples with an O(1) when no non-1 count
> samples are present.
>
> Signed-off-by: Dale Curtis 
> ---
>  libavformat/isom.h |  1 +
>  libavformat/mov.c  | 46 +++---
>  2 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index ff009b0896..fdd98c28f5 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -137,6 +137,7 @@ typedef struct MOVStreamContext {
>  unsigned int stts_count;
>  MOVStts *stts_data;
>  unsigned int ctts_count;
> +unsigned int ctts_allocated_size;
>  MOVStts *ctts_data;
>  unsigned int stsc_count;
>  MOVStsc *stsc_data;
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 63f84be782..412290b435 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4297,11 +4297,6 @@ static int mov_read_trun(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
>  }
>  if ((uint64_t)entries+sc->ctts_count >=
> UINT_MAX/sizeof(*sc->ctts_data))
>  return AVERROR_INVALIDDATA;
> -if ((err = av_reallocp_array(>ctts_data, entries +
> sc->ctts_count,
> - sizeof(*sc->ctts_data))) < 0) {
> -sc->ctts_count = 0;
> -return err;
> -}
>  if (flags & MOV_TRUN_DATA_OFFSET)data_offset=
> avio_rb32(pb);
>  if (flags & MOV_TRUN_FIRST_SAMPLE_FLAGS) first_sample_flags =
> avio_rb32(pb);
>  dts= sc->track_end - sc->time_offset;
> @@ -4312,26 +4307,28 @@ static int mov_read_trun(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
>  unsigned sample_size = frag->size;
>  int sample_flags = i ? frag->flags : first_sample_flags;
>  unsigned sample_duration = frag->duration;
> +unsigned ctts_duration = 0;
>  int keyframe = 0;
> +int ctts_index = 0;
> +int old_nb_index_entries = st->nb_index_entries;
>
>  if (flags & MOV_TRUN_SAMPLE_DURATION) sample_duration =
> avio_rb32(pb);
>  if (flags & MOV_TRUN_SAMPLE_SIZE) sample_size =
> avio_rb32(pb);
>  if (flags & MOV_TRUN_SAMPLE_FLAGS)sample_flags=
> avio_rb32(pb);
> -sc->ctts_data[sc->ctts_count].count = 1;
> -sc->ctts_data[sc->ctts_count].duration = (flags &
> MOV_TRUN_SAMPLE_CTS) ?
> -  avio_rb32(pb) : 0;
> -mov_update_dts_shift(sc, sc->ctts_data[sc->ctts_count].duration);
> +if (flags & MOV_TRUN_SAMPLE_CTS)  ctts_duration   =
> avio_rb32(pb);
> +
> +mov_update_dts_shift(sc, ctts_duration);
>  if (frag->time != AV_NOPTS_VALUE) {
>  if (c->use_mfra_for == FF_MOV_FLAG_MFRA_PTS) {
>  int64_t pts = frag->time;
>  av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64
>  " sc->dts_shift %d ctts.duration %d"
>  " sc->time_offset %"PRId64" flags &
> MOV_TRUN_SAMPLE_CTS %d\n", pts,
> -sc->dts_shift, sc->ctts_data[sc->ctts_count].
> duration,
> +sc->dts_shift, ctts_duration,
>  sc->time_offset, flags &