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

[Chris Cunningham]

Adding everyone back on the cc list for a convo with wm4 (meant for
everyone). Expect a new patch shortly to go with this discussion.

Also, just a note for posterity and to make sure I haven't missed anything:
It seems the mp3 TOC is really not a precise tool for seeking, particularly
in large files. I break down what I know about the design in this bug:
https://code.google.com/p/chromium/issues/detail?id=545914#c13 - LMK if I'm
missing something.

On Wed, Nov 18, 2015 at 3:08 AM, wm4 <nfxjfg at gmail.com> wrote:

> On Tue, 17 Nov 2015 14:21:08 -0800
> Chris Cunningham <chcunningham at chromium.org> wrote:
>
> > On Tue, Nov 17, 2015 at 2:38 AM, wm4 <nfxjfg at gmail.com> wrote:
> >
> > > On Mon, 16 Nov 2015 16:58:57 -0800
> > > 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;
> > >
> > > Hm, it's still a bit weird IMHO. Here's a suggestion: if usetoc is
> > > unset (its value is -1), then always set it 0. Other uses of the usetoc
> > > or fast seek flag could be simplified in the code.
> > >
> > > I think this sounds good. Then the behavior becomes:
> > -1 - unset implies default to 0
> > 0 - generic indexing code
> > 1 - toc
> > 2 - goes away, this is now the behavior of 0
> >
> >
> >
> > > Ultimately, the fast seek flag just sets the default mode. Disabling
> > > the flag makes it always use the generic indexing code. Enabling it
> > > used to enable TOC seeking, but we really always want CBR seeking. We
> > > want TOC seeking only if the user explicitly enables it by setting
> > > usetoc. Is that what we want?
> > >
> > >
> > *"We want TOC seeking only if the user explicitly enables it by setting*
> > *usetoc. Is that what we want?"*
> >
> > I think this is the heart of the complexity.
> > Is it useful for users to be able to say things like "fastseek, but never
> > use toc (always use scaling, even for VBR)"?
> > Or alternatively, "fastseek, but always use toc when available (even for
> > CBR)"?
> >
> > I'm not sure. We can support those combinations, but its more complex and
> > harder to reason about.
> >
> > It may be simpler to simply say that fastseek *overrides* usetoc. It
> could
> > cause toc to be used for VBR, and scaling for CBR, with no regard for
> user
> > set values of usetoc (we might detect a user setting and warn that we're
> > ignoring it). I'm happy to take this approach if everyone is on board.
> What
> > do you think? (Also, not sure if you mean to reply just to me, but we'd
> > have to ask Michael too).
>
> No, this was accidental. My mail client selected the wrong reply
> address because the mail had multiple recipients.


I'll merge the threads.


>
> I think there are two things: sane defaults, which apply if usetoc is
> not used, and forcing the demuxer to use a specific method if the user
> passes usetoc.
>
> In my own opinion, there are 3 use-cases:
> - default: seeking is exact, no surprises, but may be slow
>   (what everyone wants)
> - fast: user doesn't care about precision
>   (possibly the API user wants to enable this for big/slow streams)
> - custom: user wants a very specific seeking method to be used
>   (used rarely, maybe only for debugging)
>
> So I think the "usetoc" would fit the "custom" use-case, and we don't
> have to think much about its interaction with the fast seek mode - it
> can simply override it.
>
> This might be what you had in mind anyway. I'm only arguing that it
> makes sense for usetoc to override the fast seek mode.
>

Sounds good to me, the next patch will follow this outline. I'm thinking
I'll do away with the -1 value as well. Seems we want 0 to be the default
and I don't think we have  a use case for telling default apart from user
set value.


>
> >
> > > The failing FATE tests are probably no problem, but they should be
> > > updated with the new seek results.
> > >
> > >
> > Thanks for this tip. How can I get the files needed to run these tests?
> > Right now my checkout doesn't seem to contain the needed mp3?
> >
> > $ make fate-seek-extra-mp3
> > fate-seek-extra-mp3 requires external samples and SAMPLES not specified
>
> The docs state:
>
> make fate-rsync SAMPLES=fate-suite/
> make fate       SAMPLES=fate-suite/
>
> The first one downloads all samples to the given directory. It's about
> 1GB, so it makes sense not to add them to the git repo.
>
> > I'm also surprised these tests aren't running when I just "make fate". I
> > definitely have CONFIG_MP3_DEMUXER 1.
> >
>
> That's strange. I'm fairly certain "fate" includes all tests. Maybe it
> just tried to run all tests that don't require the sample files.
>

Thanks. I missed that step in the docs. Your suspicions are correct, it
just ran what it could without the samples.


>
> >
> > > (Sorry if I'm being a bit pedantic about it, but making this all
> > > simpler and reducing the number of option combinations that interact
> > > with each other is probably a good idea to save our sanity later.)
> > >
> >
> > No worries. I'm with you.
>
>