| Message ID | CAPUDrwecNn9A+RNc8U8NT5qg+77cB-Wc6fqWY2n91J9e-BC_og@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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 <dalecurtis@chromium.org> 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 @ 0x7fbce00008c0] invalid dts/pts combination > 14583000 > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > 14586000 > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found fragment index for track 1 > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found fragment index entry for > track 1 and moof_offset 16686198 > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found frag time 14589000, using > it for dts > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > 14607000 > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > 14610000 > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > 14622000 > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > 14631000 > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > 14634000 > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] 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 <rodger.combs@gmail.com> > 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 <michael@niedermayer.cc> >> wrote: >> > >> > On Fri, Jan 15, 2016 at 10:24:43PM +0000, 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. >> >> >
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 <dalecurtis@chromium.org> > 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 @ 0x7fbce00008c0] invalid dts/pts combination > > 14583000 > > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > > 14586000 > > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found fragment index for track 1 > > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found fragment index entry for > > track 1 and moof_offset 16686198 > > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] found frag time 14589000, using > > it for dts > > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > > 14607000 > > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > > 14610000 > > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > > 14622000 > > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > > 14631000 > > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] invalid dts/pts combination > > 14634000 > > [mov,mp4,m4a,3gp,3g2,mj2 @ 0x7fbce00008c0] 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 <rodger.combs@gmail.com> > > 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 <michael@niedermayer.cc> > >> wrote: > >> > > >> > On Fri, Jan 15, 2016 at 10:24:43PM +0000, 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
On Fri, Jul 14, 2017 at 5:38 PM, Michael Niedermayer <michael@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
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?