[FFmpeg-devel] [PATCH] avformat/mp3dec: prefer "fast_seek" to TOC seek for CBR files.

Michael Niedermayer michael at niedermayer.cc
Tue Nov 17 03:39:38 CET 2015


On Mon, Nov 16, 2015 at 05:28:58PM -0800, Chris Cunningham wrote:
> To test this new patch, again use testcount.mp3
> <http://happyworm.com/jPlayerLab/halite/jumptest/testcount.mp3> (CBR,
> corrupt TOC).
> 
> Current ffmpeg (with none of my mp3 patches)
> 
> ./ffplay testcount.mp3 -ss 00:33:20 -usetoc 0
> 
> plays out "1395", which is correct
> 
> 
> ./ffplay testcount.mp3 -ss 00:33:20 -usetoc 1
> plays out "..378, 1389", which is incorrect due to the corrupted TOC.
> 
> ./ffplay testcount.mp3 -ss 00:33:20 -fflags fastseek
> also plays out "..378, 1389" because current fast_seek logic uses the TOC
> whenever available for CBR and VBR alike.
> 
> 
> After applying my *latest* patch:
> 
> ./ffplay testcount.mp3 -ss 00:33:20 -usetoc 0
> is unchanged: plays out "1395", which is correct
> 
> ./ffplay testcount.mp3 -ss 00:33:20 -usetoc 1
> is unchanged: still plays out "378, 1389". This is wrong, but it allows
> users to force using TOC should they prefer.
> 
> ./ffplay testcount.mp3 -ss 00:33:20 -fflags fastseek
> *is changed: *plays out "1395", which is correct. fast seek no longer uses
> the TOC for CBR files.
> 
> 
> 

> What do you think? I personally prefer this way so that usetoc is still
> meaningful for CBR files.

i tend to agree about usetocs meaningfullness but it seems breaking
fate tests:
--- ./tests/ref/seek/extra-mp3  2015-11-16 00:07:55.812389092 +0100
+++ tests/data/fate/seek-extra-mp3      2015-11-17 03:15:54.266446909 +0100
@@ -5,16 +5,14 @@
 ret: 0         st: 0 flags:1 dts: 1.880816 pts: 1.880816 pos:  31544 size:   418
 ret: 0         st: 0 flags:0  ts: 0.788334
 ret: 0         st: 0 flags:1 dts: 0.809796 pts: 0.809796 pos:  14407 size:   418
-ret: 0         st: 0 flags:1  ts:-0.317499
-ret: 0         st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos:   1451 size:   440
+ret:-1         st: 0 flags:1  ts:-0.317499
 ret: 0         st:-1 flags:0  ts: 2.576668
 ret: 0         st: 0 flags:1 dts: 2.586122 pts: 2.586122 pos:  42828 size:   418
 ret: 0         st:-1 flags:1  ts: 1.470835
 ret: 0         st: 0 flags:1 dts: 1.462857 pts: 1.462857 pos:  24856 size:   418
 ret: 0         st: 0 flags:0  ts: 0.365002
 ret: 0         st: 0 flags:1 dts: 0.365714 pts: 0.365714 pos:   7302 size:   418
-ret: 0         st: 0 flags:1  ts:-0.740831
-ret: 0         st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos:   1451 size:   440
+ret:-1         st: 0 flags:1  ts:-0.740831
 ret: 0         st:-1 flags:0  ts: 2.153336
 ret: 0         st: 0 flags:1 dts: 2.168163 pts: 2.168163 pos:  36141 size:   418
 ret: 0         st:-1 flags:1  ts: 1.047503
@@ -41,13 +39,11 @@
 ret: 0         st: 0 flags:1 dts: 1.985306 pts: 1.985306 pos:  33215 size:   418
 ret: 0         st:-1 flags:0  ts: 0.883340
 ret: 0         st: 0 flags:1 dts: 0.888163 pts: 0.888163 pos:  15661 size:   418
-ret: 0         st:-1 flags:1  ts:-0.222493
-ret: 0         st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos:   1451 size:   440
+ret:-1         st:-1 flags:1  ts:-0.222493
 ret: 0         st: 0 flags:0  ts: 2.671674
 ret: 0         st: 0 flags:1 dts: 2.690612 pts: 2.690612 pos:  44500 size:   418
 ret: 0         st: 0 flags:1  ts: 1.565841
-ret: 0         st: 0 flags:1 dts: 1.567347 pts: 1.567347 pos:  26528 size:   418
+ret: 0         st: 0 flags:1 dts: 1.541224 pts: 1.541224 pos:  26110 size:   418
 ret: 0         st:-1 flags:0  ts: 0.460008
 ret: 0         st: 0 flags:1 dts: 0.470204 pts: 0.470204 pos:   8974 size:   418
-ret: 0         st:-1 flags:1  ts:-0.645825
-ret: 0         st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos:   1451 size:   440
+ret:-1         st:-1 flags:1  ts:-0.645825
Test seek-extra-mp3 failed. Look at tests/data/fate/seek-extra-mp3.err for details.
make: *** [fate-seek-extra-mp3] Error 1
make: *** Waiting for unfinished jobs....


> 
> Thanks,
> Chris
> 
> On Mon, Nov 16, 2015 at 4:58 PM, <chcunningham at chromium.org> wrote:
> 
> > From: Chris Cunningham <chcunningham at chromium.org>
> >
> > "Fast seek" uses linear interpolation to find the position of the
> > requested seek time. For CBR this is more direct than using the
> > mp3 TOC and bypassing the TOC avoids problems when the TOC is
> > corrupted (e.g. https://crbug.com/545914).
> >
> > For VBR, fast seek is not precise, so continue to prefer the TOC
> > when available.
> > ---
> >  libavformat/mp3dec.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 32ca00c..e12266c 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -515,8 +515,10 @@ static int mp3_seek(AVFormatContext *s, int
> > stream_index, int64_t timestamp,
> >              filesize = size - s->internal->data_offset;
> >      }
> >
> > -    if (   (mp3->is_cbr || fast_seek)
> > -        && (mp3->usetoc == 0 || !mp3->xing_toc)
> > +    // When fast seeking, prefer to use the TOC when available for VBR
> > files
> > +    // since av_rescale may not be accurate for VBR. For CBR, rescaling is
> > +    // always accurate and more direct than a TOC lookup.
> > +    if (fast_seek && (mp3->is_cbr || !mp3->xing_toc)
> >          && st->duration > 0
> >          && filesize > 0) {
> >          ie = &ie1;
> > --
> > 2.6.0.rc2.230.g3dd15c0
> >
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151117/d7a6edcf/attachment.sig>


More information about the ffmpeg-devel mailing list