Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2017-07-17 Thread Dale Curtis 
Okay the following patch should fix this issue. Chromium/reviewable version
here https://chromium-review.googlesource.com/c/572585/

- dale

On Fri, Jul 14, 2017 at 6:31 PM, Dale Curtis 
wrote:

> On Fri, Jul 14, 2017 at 5:38 PM, Michael Niedermayer <
> mich...@niedermayer.cc> wrote:
>
>>
>> I dont think CTTS is the only affected atom.
>>
>>
> Thanks for the response. Yes, I think that's likely true. The ctts one is
> just likely the most important since it controls timestamp ordering.
>
>
>> what you describe sounds like an option. but i might be missing something,
>> possibly even something significant. I dont feel that confident with
>> this code after quickly looking at it.
>>
>
> Here are my experiments thus far if you have further thoughts:
> https://chromium-review.googlesource.com/c/572730/
> https://chromium-review.googlesource.com/c/572585/
>
> Both fixes and the existing code seem to suffer from assuming that the
> sample number in the AVIndex and that of the ctts_data are the same; which
> seems questionable. The second experiment should be viable with the
> addition of memmove() when inserting in the middle of the array. Will look
> at it more next week.
>
> - dale
>
>
>
From 4460d4b16472d1fd201818df6c18ba3e515941c4 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 
---
 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;
-

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2017-07-14 Thread Dale Curtis 
On Fri, Jul 14, 2017 at 5:38 PM, Michael Niedermayer  wrote:

>
> I dont think CTTS is the only affected atom.
>
>
Thanks for the response. Yes, I think that's likely true. The ctts one is
just likely the most important since it controls timestamp ordering.


> what you describe sounds like an option. but i might be missing something,
> possibly even something significant. I dont feel that confident with
> this code after quickly looking at it.
>

Here are my experiments thus far if you have further thoughts:
https://chromium-review.googlesource.com/c/572730/
https://chromium-review.googlesource.com/c/572585/

Both fixes and the existing code seem to suffer from assuming that the
sample number in the AVIndex and that of the ctts_data are the same; which
seems questionable. The second experiment should be viable with the
addition of memmove() when inserting in the middle of the array. Will look
at it more next week.

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

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2017-07-14 Thread Michael Niedermayer 
Hi

On Fri, Jul 14, 2017 at 02:52:23PM -0700, Dale Curtis wrote:
> After looking at this some more. I don't think the way ctts entries are
> stored is correct unless you assume all ctts entries are known prior to any
> seeks taking place. Either through reading all trun boxes ahead of time or
> content having a ctts atom.
> 
> The current code is broken in the presence of seeks since seeks can not
> know the proper ctts index unless all trun boxes have been read. Instead it
> performs the seek+root switch and then ends up writing ctts entries into
> the wrong slots of the global ctts index as trun boxes are encountered
> after the seek.
> 
> To fix this I think the code needs to only use the global ctts index if the
> ctts atom is present. If the ctts data is just being pulled from the trun
> boxes, then it should probably be stored in along with each
> MOVFragmentIndex.
> 
> Is anyone else familiar enough with this code to have opinions on direction
> here? Rodger seems MIA.

I dont think CTTS is the only affected atom.

what you describe sounds like an option. but i might be missing something,
possibly even something significant. I dont feel that confident with
this code after quickly looking at it.


> 
> - dale
> 
> On Fri, Jun 30, 2017 at 2:56 PM, Dale Curtis 
> wrote:
> 
> > Hmm, finally got around to looking into this again and this still isn't
> > fixed. Just seeking a few times in ffplay can trigger this issue with the
> > clip linked in my original message:
> >
> > http://storage.googleapis.com/dalecurtis-shared/buck480p30_na.mp4
> >
> > ./ffplay -v debug -drp 1 ~/Downloads/buck480p30_na.mp4
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> > 14583000
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> > 14586000
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] found fragment index for track 1
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] found fragment index entry for
> > track 1 and moof_offset 16686198
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] found frag time 14589000, using
> > it for dts
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> > 14607000
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> > 1461
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> > 14622000
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> > 14631000
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> > 14634000
> > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> > 14643000
> >
> > Disabled sidx processing resolves this issue:
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 63f84be782..919475f12f 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -5497,7 +5497,7 @@ static const MOVParseTableEntry
> > mov_default_parse_table[] = {
> >  { MKTAG('a','l','a','c'), mov_read_alac }, /* alac specific atom */
> >  { MKTAG('a','v','c','C'), mov_read_glbl },
> >  { MKTAG('p','a','s','p'), mov_read_pasp },
> > -{ MKTAG('s','i','d','x'), mov_read_sidx },
> > +// { MKTAG('s','i','d','x'), mov_read_sidx },
> >
> > Rodger, are you able to still look into this?
> >
> > - dale
> >
> > On Fri, Feb 12, 2016 at 2:21 AM, Rodger Combs 
> > wrote:
> >
> >> This issue is fixed by this patch, but I'm unsure of possible
> >> implications on other files. It passes FATE, at least.
> >>
> >> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >> index 149e3b4..c5e0a1e 100644
> >> --- a/libavformat/mov.c
> >> +++ b/libavformat/mov.c
> >> @@ -3609,7 +3609,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext
> >> *pb, MOVAtom atom)
> >>  }
> >>  av_log(c->fc, AV_LOG_DEBUG, "calculated into dts
> >> %"PRId64"\n", dts);
> >>  } else {
> >> -dts = frag->time;
> >> +dts = frag->time - sc->time_offset;
> >>  av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64
> >>  ", using it for dts\n", dts);
> >>  }
> >>
> >>
> >> > On Jan 15, 2016, at 16:57, Michael Niedermayer 
> >> wrote:
> >> >
> >> > On Fri, Jan 15, 2016 at 10:24:43PM +, Dan Sanders wrote:
> >> >> Michael, I wanted to check if you have you looked into this playback
> >> issue,
> >> >> or were planning to?
> >> >
> >> > i didnt look into it, i had thought rodger would look into it as it
> >> > was his patch ...
> >> >
> >> > rodger, did you look into this ?
> >> >
> >> > [...]
> >> > --
> >> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >> >
> >> > Rewriting code that is poorly written but fully understood is good.
> >> > Rewriting code that one doesnt understand is a sign that one is less
> >> smart
> >> > then the original author, trying to rewrite it will not make it better.
> >>
> >>
> >
>

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2017-07-14 Thread Dale Curtis 
After looking at this some more. I don't think the way ctts entries are
stored is correct unless you assume all ctts entries are known prior to any
seeks taking place. Either through reading all trun boxes ahead of time or
content having a ctts atom.

The current code is broken in the presence of seeks since seeks can not
know the proper ctts index unless all trun boxes have been read. Instead it
performs the seek+root switch and then ends up writing ctts entries into
the wrong slots of the global ctts index as trun boxes are encountered
after the seek.

To fix this I think the code needs to only use the global ctts index if the
ctts atom is present. If the ctts data is just being pulled from the trun
boxes, then it should probably be stored in along with each
MOVFragmentIndex.

Is anyone else familiar enough with this code to have opinions on direction
here? Rodger seems MIA.

- dale

On Fri, Jun 30, 2017 at 2:56 PM, Dale Curtis 
wrote:

> Hmm, finally got around to looking into this again and this still isn't
> fixed. Just seeking a few times in ffplay can trigger this issue with the
> clip linked in my original message:
>
> http://storage.googleapis.com/dalecurtis-shared/buck480p30_na.mp4
>
> ./ffplay -v debug -drp 1 ~/Downloads/buck480p30_na.mp4
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> 14583000
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> 14586000
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] found fragment index for track 1
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] found fragment index entry for
> track 1 and moof_offset 16686198
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] found frag time 14589000, using
> it for dts
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> 14607000
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> 1461
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> 14622000
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> 14631000
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> 14634000
> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
> 14643000
>
> Disabled sidx processing resolves this issue:
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 63f84be782..919475f12f 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -5497,7 +5497,7 @@ static const MOVParseTableEntry
> mov_default_parse_table[] = {
>  { MKTAG('a','l','a','c'), mov_read_alac }, /* alac specific atom */
>  { MKTAG('a','v','c','C'), mov_read_glbl },
>  { MKTAG('p','a','s','p'), mov_read_pasp },
> -{ MKTAG('s','i','d','x'), mov_read_sidx },
> +// { MKTAG('s','i','d','x'), mov_read_sidx },
>
> Rodger, are you able to still look into this?
>
> - dale
>
> On Fri, Feb 12, 2016 at 2:21 AM, Rodger Combs 
> wrote:
>
>> This issue is fixed by this patch, but I'm unsure of possible
>> implications on other files. It passes FATE, at least.
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 149e3b4..c5e0a1e 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -3609,7 +3609,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext
>> *pb, MOVAtom atom)
>>  }
>>  av_log(c->fc, AV_LOG_DEBUG, "calculated into dts
>> %"PRId64"\n", dts);
>>  } else {
>> -dts = frag->time;
>> +dts = frag->time - sc->time_offset;
>>  av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64
>>  ", using it for dts\n", dts);
>>  }
>>
>>
>> > On Jan 15, 2016, at 16:57, Michael Niedermayer 
>> wrote:
>> >
>> > On Fri, Jan 15, 2016 at 10:24:43PM +, Dan Sanders wrote:
>> >> Michael, I wanted to check if you have you looked into this playback
>> issue,
>> >> or were planning to?
>> >
>> > i didnt look into it, i had thought rodger would look into it as it
>> > was his patch ...
>> >
>> > rodger, did you look into this ?
>> >
>> > [...]
>> > --
>> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>> >
>> > Rewriting code that is poorly written but fully understood is good.
>> > Rewriting code that one doesnt understand is a sign that one is less
>> smart
>> > then the original author, trying to rewrite it will not make it better.
>>
>>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2017-06-30 Thread Dale Curtis 
Hmm, finally got around to looking into this again and this still isn't
fixed. Just seeking a few times in ffplay can trigger this issue with the
clip linked in my original message:

http://storage.googleapis.com/dalecurtis-shared/buck480p30_na.mp4

./ffplay -v debug -drp 1 ~/Downloads/buck480p30_na.mp4
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
14583000
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
14586000
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] found fragment index for track 1
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] found fragment index entry for
track 1 and moof_offset 16686198
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] found frag time 14589000, using
it for dts
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
14607000
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
1461
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
14622000
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
14631000
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
14634000
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce8c0] invalid dts/pts combination
14643000

Disabled sidx processing resolves this issue:

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 63f84be782..919475f12f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -5497,7 +5497,7 @@ static const MOVParseTableEntry
mov_default_parse_table[] = {
 { MKTAG('a','l','a','c'), mov_read_alac }, /* alac specific atom */
 { MKTAG('a','v','c','C'), mov_read_glbl },
 { MKTAG('p','a','s','p'), mov_read_pasp },
-{ MKTAG('s','i','d','x'), mov_read_sidx },
+// { MKTAG('s','i','d','x'), mov_read_sidx },

Rodger, are you able to still look into this?

- dale

On Fri, Feb 12, 2016 at 2:21 AM, Rodger Combs 
wrote:

> This issue is fixed by this patch, but I'm unsure of possible implications
> on other files. It passes FATE, at least.
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 149e3b4..c5e0a1e 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -3609,7 +3609,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
>  }
>  av_log(c->fc, AV_LOG_DEBUG, "calculated into dts
> %"PRId64"\n", dts);
>  } else {
> -dts = frag->time;
> +dts = frag->time - sc->time_offset;
>  av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64
>  ", using it for dts\n", dts);
>  }
>
>
> > On Jan 15, 2016, at 16:57, Michael Niedermayer 
> wrote:
> >
> > On Fri, Jan 15, 2016 at 10:24:43PM +, Dan Sanders wrote:
> >> Michael, I wanted to check if you have you looked into this playback
> issue,
> >> or were planning to?
> >
> > i didnt look into it, i had thought rodger would look into it as it
> > was his patch ...
> >
> > rodger, did you look into this ?
> >
> > [...]
> > --
> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Rewriting code that is poorly written but fully understood is good.
> > Rewriting code that one doesnt understand is a sign that one is less
> smart
> > then the original author, trying to rewrite it will not make it better.
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2016-02-12 Thread Rodger Combs 
This issue is fixed by this patch, but I'm unsure of possible implications on 
other files. It passes FATE, at least.

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 149e3b4..c5e0a1e 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -3609,7 +3609,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 }
 av_log(c->fc, AV_LOG_DEBUG, "calculated into dts %"PRId64"\n", 
dts);
 } else {
-dts = frag->time;
+dts = frag->time - sc->time_offset;
 av_log(c->fc, AV_LOG_DEBUG, "found frag time %"PRId64
 ", using it for dts\n", dts);
 }


> On Jan 15, 2016, at 16:57, Michael Niedermayer  wrote:
> 
> On Fri, Jan 15, 2016 at 10:24:43PM +, Dan Sanders wrote:
>> Michael, I wanted to check if you have you looked into this playback issue,
>> or were planning to?
> 
> i didnt look into it, i had thought rodger would look into it as it
> was his patch ...
> 
> rodger, did you look into this ?
> 
> [...]
> -- 
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Rewriting code that is poorly written but fully understood is good.
> Rewriting code that one doesnt understand is a sign that one is less smart
> then the original author, trying to rewrite it will not make it better.

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

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2016-01-15 Thread Michael Niedermayer 
On Fri, Jan 15, 2016 at 10:24:43PM +, Dan Sanders wrote:
> Michael, I wanted to check if you have you looked into this playback issue,
> or were planning to?

i didnt look into it, i had thought rodger would look into it as it
was his patch ...

rodger, did you look into this ?

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

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.


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

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2016-01-15 Thread Dan Sanders 
Michael, I wanted to check if you have you looked into this playback issue,
or were planning to?

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

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2015-12-12 Thread Dan Sanders 
I took a look at the structure of this file, and the encoding appears to be
correct. The frame order in the moof boxes matches the H.264 stream, and
the durations of the moof boxes match with the sidx.

Some notes to help with analysis: There are no per-frame durations
specified, every frame is exactly 3000 time units long (=1s/30). The first
frame PTS is 3000, but there is an edit list to shift that to movie time 0.
The fragments contain 152 frames each, and the base times of the fragments
reflect that correctly.

On Fri, Dec 11, 2015 at 2:39 PM Dale Curtis  wrote:

> This patch seems to be causing some issues with some h264 video-only
> content. Attempting to play the following in ffplay with "-v debug -drp 1"
> (disables pts/dts fixups and purely uses pts) will cause jerky playback
> after seeking around a few times due to dropped out of order frames. These
> are shown in the log as "invalid dts/pts combination..." Skipping sidx
> parsing for this file yields smooth playback. I haven't figured out yet if
> this is bad encoding or something is wrong with the patch, but FYI.
>
> http://storage.googleapis.com/dalecurtis-shared/buck480p30_na.mp4
>
> - dale
>
> On Tue, Oct 6, 2015 at 9:18 AM, Michael Niedermayer <
> mich...@niedermayer.cc> wrote:
>
>> On Tue, Oct 06, 2015 at 03:50:23AM -0500, Rodger Combs wrote:
>> > Fixes trac #3842
>> > ---
>> >  libavformat/isom.h |   2 +
>> >  libavformat/mov.c  | 245
>> -
>> >  2 files changed, 208 insertions(+), 39 deletions(-)
>>
>> i think this should be applied unless someone has more comments
>>
>> [...]
>
>
>> --
>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> If you think the mosad wants you dead since a long time then you are
>> either
>> wrong or dead since a long time.
>>
>> ___
>> 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] [PATCH] lavf/mov: add support for sidx fragment indexes

2015-12-11 Thread Dale Curtis 
This patch seems to be causing some issues with some h264 video-only
content. Attempting to play the following in ffplay with "-v debug -drp 1"
(disables pts/dts fixups and purely uses pts) will cause jerky playback
after seeking around a few times due to dropped out of order frames. These
are shown in the log as "invalid dts/pts combination..." Skipping sidx
parsing for this file yields smooth playback. I haven't figured out yet if
this is bad encoding or something is wrong with the patch, but FYI.

http://storage.googleapis.com/dalecurtis-shared/buck480p30_na.mp4

- dale

On Tue, Oct 6, 2015 at 9:18 AM, Michael Niedermayer 
wrote:

> On Tue, Oct 06, 2015 at 03:50:23AM -0500, Rodger Combs wrote:
> > Fixes trac #3842
> > ---
> >  libavformat/isom.h |   2 +
> >  libavformat/mov.c  | 245
> -
> >  2 files changed, 208 insertions(+), 39 deletions(-)
>
> i think this should be applied unless someone has more comments
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you think the mosad wants you dead since a long time then you are either
> wrong or dead since a long time.
>
> ___
> 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] [PATCH] lavf/mov: add support for sidx fragment indexes

2015-10-06 Thread Michael Niedermayer 
On Tue, Oct 06, 2015 at 03:50:23AM -0500, Rodger Combs wrote:
> Fixes trac #3842
> ---
>  libavformat/isom.h |   2 +
>  libavformat/mov.c  | 245 
> -
>  2 files changed, 208 insertions(+), 39 deletions(-)

i think this should be applied unless someone has more comments

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

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.


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

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2015-10-05 Thread wm4 
On Sat,  3 Oct 2015 11:57:03 -0500
Rodger Combs  wrote:

> Fixes trac #3842
> ---
>  libavformat/isom.h |   2 +
>  libavformat/mov.c  | 250 
> -
>  2 files changed, 213 insertions(+), 39 deletions(-)
> 
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index aee9d6e..6e921c0 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -103,6 +103,7 @@ typedef struct MOVSbgp {
>  typedef struct MOVFragmentIndexItem {
>  int64_t moof_offset;
>  int64_t time;
> +int headers_read;
>  } MOVFragmentIndexItem;
>  
>  typedef struct MOVFragmentIndex {
> @@ -197,6 +198,7 @@ typedef struct MOVContext {
>  int has_looked_for_mfra;
>  MOVFragmentIndex** fragment_index_data;
>  unsigned fragment_index_count;
> +int fragment_index_complete;
>  int atom_depth;
>  unsigned int aax_mode;  ///< 'aax' file has been detected
>  uint8_t file_key[20];
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index da170a6..5aa7491 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -3349,7 +3349,7 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  MOVFragment *frag = >fragment;
>  MOVTrackExt *trex = NULL;
>  MOVFragmentIndex* index = NULL;
> -int flags, track_id, i;
> +int flags, track_id, i, found = 0;
>  
>  avio_r8(pb); /* version */
>  flags = avio_rb24(pb);
> @@ -3367,15 +3367,6 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  av_log(c->fc, AV_LOG_ERROR, "could not find corresponding trex\n");
>  return AVERROR_INVALIDDATA;
>  }
> -for (i = 0; i < c->fragment_index_count; i++) {
> -MOVFragmentIndex* candidate = c->fragment_index_data[i];
> -if (candidate->track_id == frag->track_id) {
> -av_log(c->fc, AV_LOG_DEBUG,
> -   "found fragment index for track %u\n", frag->track_id);
> -index = candidate;
> -break;
> -}
> -}
>  
>  frag->base_data_offset = flags & MOV_TFHD_BASE_DATA_OFFSET ?
>   avio_rb64(pb) : flags & 
> MOV_TFHD_DEFAULT_BASE_IS_MOOF ?
> @@ -3389,23 +3380,32 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  frag->flags= flags & MOV_TFHD_DEFAULT_FLAGS ?
>   avio_rb32(pb) : trex->flags;
>  frag->time = AV_NOPTS_VALUE;
> -if (index) {
> -int i, found = 0;
> -for (i = index->current_item; i < index->item_count; i++) {
> -if (frag->implicit_offset == index->items[i].moof_offset) {
> -av_log(c->fc, AV_LOG_DEBUG, "found fragment index entry "
> -"for track %u and moof_offset %"PRId64"\n",
> -frag->track_id, index->items[i].moof_offset);
> -frag->time = index->items[i].time;
> -index->current_item = i + 1;
> -found = 1;
> +for (i = 0; i < c->fragment_index_count; i++) {
> +int j;
> +MOVFragmentIndex* candidate = c->fragment_index_data[i];
> +if (candidate->track_id == frag->track_id) {
> +av_log(c->fc, AV_LOG_DEBUG,
> +   "found fragment index for track %u\n", frag->track_id);
> +index = candidate;
> +for (j = index->current_item; j < index->item_count; j++) {
> +if (frag->implicit_offset == index->items[j].moof_offset) {
> +av_log(c->fc, AV_LOG_DEBUG, "found fragment index entry "
> +"for track %u and moof_offset %"PRId64"\n",
> +frag->track_id, index->items[j].moof_offset);
> +frag->time = index->items[j].time;
> +index->current_item = j + 1;
> +found = 1;
> +break;
> +}
>  }
> +if (found)
> +break;
>  }
> -if (!found) {
> -av_log(c->fc, AV_LOG_WARNING, "track %u has a fragment index "
> -   "but it doesn't have an (in-order) entry for moof_offset "
> -   "%"PRId64"\n", frag->track_id, frag->implicit_offset);
> -}
> +}
> +if (index && !found) {
> +av_log(c->fc, AV_LOG_DEBUG, "track %u has a fragment index but "
> +   "it doesn't have an (in-order) entry for moof_offset "
> +   "%"PRId64"\n", frag->track_id, frag->implicit_offset);
>  }
>  av_log(c->fc, AV_LOG_TRACE, "frag flags 0x%x\n", frag->flags);
>  return 0;
> @@ -3596,7 +3596,99 @@ static int mov_read_trun(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  return AVERROR_EOF;
>  
>  frag->implicit_offset = offset;
> -st->duration = sc->track_end = dts + sc->time_offset;
> +
> +sc->track_end = dts + sc->time_offset;
> +if (st->duration < sc->track_end)
> +

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2015-10-05 Thread Yusuke Nakamura 
2015-10-04 1:57 GMT+09:00 Rodger Combs :

> Fixes trac #3842
> ---
>  libavformat/isom.h |   2 +
>  libavformat/mov.c  | 250
> -
>  2 files changed, 213 insertions(+), 39 deletions(-)
>
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index aee9d6e..6e921c0 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -103,6 +103,7 @@ typedef struct MOVSbgp {
>  typedef struct MOVFragmentIndexItem {
>  int64_t moof_offset;
>  int64_t time;
> +int headers_read;
>  } MOVFragmentIndexItem;
>
>  typedef struct MOVFragmentIndex {
> @@ -197,6 +198,7 @@ typedef struct MOVContext {
>  int has_looked_for_mfra;
>  MOVFragmentIndex** fragment_index_data;
>  unsigned fragment_index_count;
> +int fragment_index_complete;
>  int atom_depth;
>  unsigned int aax_mode;  ///< 'aax' file has been detected
>  uint8_t file_key[20];
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index da170a6..5aa7491 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -3349,7 +3349,7 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
>  MOVFragment *frag = >fragment;
>  MOVTrackExt *trex = NULL;
>  MOVFragmentIndex* index = NULL;
> -int flags, track_id, i;
> +int flags, track_id, i, found = 0;
>
>  avio_r8(pb); /* version */
>  flags = avio_rb24(pb);
> @@ -3367,15 +3367,6 @@ static int mov_read_tfhd(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
>  av_log(c->fc, AV_LOG_ERROR, "could not find corresponding
> trex\n");
>  return AVERROR_INVALIDDATA;
>  }
> -for (i = 0; i < c->fragment_index_count; i++) {
> -MOVFragmentIndex* candidate = c->fragment_index_data[i];
> -if (candidate->track_id == frag->track_id) {
> -av_log(c->fc, AV_LOG_DEBUG,
> -   "found fragment index for track %u\n", frag->track_id);
> -index = candidate;
> -break;
> -}
> -}
>
>  frag->base_data_offset = flags & MOV_TFHD_BASE_DATA_OFFSET ?
>   avio_rb64(pb) : flags &
> MOV_TFHD_DEFAULT_BASE_IS_MOOF ?
> @@ -3389,23 +3380,32 @@ static int mov_read_tfhd(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
>  frag->flags= flags & MOV_TFHD_DEFAULT_FLAGS ?
>   avio_rb32(pb) : trex->flags;
>  frag->time = AV_NOPTS_VALUE;
> -if (index) {
> -int i, found = 0;
> -for (i = index->current_item; i < index->item_count; i++) {
> -if (frag->implicit_offset == index->items[i].moof_offset) {
> -av_log(c->fc, AV_LOG_DEBUG, "found fragment index entry "
> -"for track %u and moof_offset %"PRId64"\n",
> -frag->track_id, index->items[i].moof_offset);
> -frag->time = index->items[i].time;
> -index->current_item = i + 1;
> -found = 1;
> +for (i = 0; i < c->fragment_index_count; i++) {
> +int j;
> +MOVFragmentIndex* candidate = c->fragment_index_data[i];
> +if (candidate->track_id == frag->track_id) {
> +av_log(c->fc, AV_LOG_DEBUG,
> +   "found fragment index for track %u\n", frag->track_id);
> +index = candidate;
> +for (j = index->current_item; j < index->item_count; j++) {
> +if (frag->implicit_offset == index->items[j].moof_offset)
> {
> +av_log(c->fc, AV_LOG_DEBUG, "found fragment index
> entry "
> +"for track %u and moof_offset %"PRId64"\n",
> +frag->track_id, index->items[j].moof_offset);
> +frag->time = index->items[j].time;
> +index->current_item = j + 1;
> +found = 1;
> +break;
> +}
>  }
> +if (found)
> +break;
>  }
> -if (!found) {
> -av_log(c->fc, AV_LOG_WARNING, "track %u has a fragment index "
> -   "but it doesn't have an (in-order) entry for
> moof_offset "
> -   "%"PRId64"\n", frag->track_id, frag->implicit_offset);
> -}
> +}
> +if (index && !found) {
> +av_log(c->fc, AV_LOG_DEBUG, "track %u has a fragment index but "
> +   "it doesn't have an (in-order) entry for moof_offset "
> +   "%"PRId64"\n", frag->track_id, frag->implicit_offset);
>  }
>  av_log(c->fc, AV_LOG_TRACE, "frag flags 0x%x\n", frag->flags);
>  return 0;
> @@ -3596,7 +3596,99 @@ static int mov_read_trun(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
>  return AVERROR_EOF;
>
>  frag->implicit_offset = offset;
> -st->duration = sc->track_end = dts + sc->time_offset;
> +
> +sc->track_end = dts + sc->time_offset;
> +if (st->duration < sc->track_end)
> +st->duration =

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2015-09-09 Thread Michael Niedermayer 
On Wed, Sep 09, 2015 at 10:01:32AM -0500, Rodger Combs wrote:
> The logic in mov_seek_fragment for matching track_ids to AVStream ids is
> almost certainly wrong, and should be corrected (by someone who knows more
> about the relevant structures) before this is merged.
> 
> Fixes trac #3842
> ---
>  libavformat/isom.h |   1 +
>  libavformat/mov.c  | 181 
> -
>  2 files changed, 168 insertions(+), 14 deletions(-)

breaks fate

 wmapro-ism
TESTwmavoice-7k
TESTwmavoice-11k
stddev: 7200.16 PSNR: 19.18 MAXDIFF:33041 bytes:   884736/   679936
MAXDIFF: |33041 - 0| >= 1
size: |884736 - 679936| >= 0
Test wmapro-ism failed. Look at tests/data/fate/wmapro-ism.err for details.
make: *** [fate-wmapro-ism] Error 1
TESTwmavoice-19k
TESTwmav1-encode
TESTwmav2-encode
TESTxvid-custom-matrix
TESTxvid-idct
TESTalac-16-level-0
TESTalac-16-level-1
TESTalac-16-level-2
--
+0, 60, 60,1,37440, 0xd0ac588a
+0, 61, 61,1,37440, 0xcf824bed
+0, 62, 62,1,37440, 0xc162522a
+0, 63, 63,1,37440, 0xa1703f3f
+0, 64, 64,1,37440, 0x7cd93e6e
+0, 65, 65,1,37440, 0xd6b731b0
+0, 66, 66,1,37440, 0x367b1dd0
Test vc1-ism failed. Look at tests/data/fate/vc1-ism.err for details.
make: *** [fate-vc1-ism] Error 1
TESTqt-ima4-mono
TESTqt-ima4-stereo
TESTqt-mac3-mono

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

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch


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

Re: [FFmpeg-devel] [PATCH] lavf/mov: add support for sidx fragment indexes

2015-07-20 Thread Michael Niedermayer 
On Mon, Jul 20, 2015 at 03:04:55PM -0500, Rodger Combs wrote:
 The logic in mov_seek_fragment for matching track_ids to AVStream ids is
 almost certainly wrong, and should be corrected (by someone who knows more
 about the relevant structures) before this is merged.
 
 Fixes trac #3842
 ---
  libavformat/isom.h |   1 +
  libavformat/mov.c  | 181 
 -
  2 files changed, 168 insertions(+), 14 deletions(-)

breaks fate
stddev: 7200.16 PSNR: 19.18 MAXDIFF:33041 bytes:   884736/   679936
MAXDIFF: |33041 - 0| = 1
size: |884736 - 679936| = 0
Test wmapro-ism failed. Look at tests/data/fate/wmapro-ism.err for details.
make: *** [fate-wmapro-ism] Error 1


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

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus


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