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

[Chris Cunningham]

(Fixed CCs)

Thanks for review. Patch coming shortly.

On Thu, Nov 26, 2015 at 10:13 AM, wm4 <nfxjfg at googlemail.com> wrote:

> On Tue, 24 Nov 2015 16:55:03 -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 with TOC precision.
> > (see https://crbug.com/545914#c13)
> >
> > For VBR, fast seek is not precise, so continue to prefer the TOC
> > when available (the lesser of two evils).
> >
> > Also, some re-ordering of the logic in mp3_seek to simplify and
> > give usetoc=1 precedence over fastseek flag.
> > ---
> >  libavformat/mp3dec.c | 45 +++++++++++++++++++++++----------------------
> >  1 file changed, 23 insertions(+), 22 deletions(-)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 32ca00c..a1f21b7 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -115,7 +115,8 @@ static void read_xing_toc(AVFormatContext *s,
> int64_t filesize, int64_t duration
> >  {
> >      int i;
> >      MP3DecContext *mp3 = s->priv_data;
> > -    int fill_index = mp3->usetoc == 1 && duration > 0;
> > +    int fast_seek = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 0;
>
> Nit: the ?1:0 is not really needed.
>

Will fix.

>
> > +    int fill_index = (mp3->usetoc || fast_seek) && duration > 0;
> >
> >      if (!filesize &&
> >          !(filesize = avio_size(s->pb))) {
> > @@ -344,9 +345,6 @@ static int mp3_read_header(AVFormatContext *s)
> >      int ret;
> >      int i;
> >
> > -    if (mp3->usetoc < 0)
> > -        mp3->usetoc = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 2;
> > -
> >      st = avformat_new_stream(s, NULL);
> >      if (!st)
> >          return AVERROR(ENOMEM);
> > @@ -501,40 +499,43 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
> >      MP3DecContext *mp3 = s->priv_data;
> >      AVIndexEntry *ie, ie1;
> >      AVStream *st = s->streams[0];
> > -    int64_t ret  = av_index_search_timestamp(st, timestamp, flags);
> > -    int64_t best_pos;
> >      int fast_seek = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 0;
> >      int64_t filesize = mp3->header_filesize;
> >
> > -    if (mp3->usetoc == 2)
> > -        return -1; // generic index code
> > -
> >      if (filesize <= 0) {
> >          int64_t size = avio_size(s->pb);
> >          if (size > 0 && size > s->internal->data_offset)
> >              filesize = size - s->internal->data_offset;
> >      }
> >
> > -    if (   (mp3->is_cbr || fast_seek)
> > -        && (mp3->usetoc == 0 || !mp3->xing_toc)
> > -        && st->duration > 0
> > -        && filesize > 0) {
> > -        ie = &ie1;
> > -        timestamp = av_clip64(timestamp, 0, st->duration);
> > -        ie->timestamp = timestamp;
> > -        ie->pos       = av_rescale(timestamp, filesize, st->duration) +
> s->internal->data_offset;
> > -    } else if (mp3->xing_toc) {
> > +    if (mp3->xing_toc && (mp3->usetoc || (fast_seek && !mp3->is_cbr))) {
> > +        av_log(s, AV_LOG_ERROR, "XING seeking. filesize = %"PRId64"\n",
> filesize);
> > +        // NOTE: The MP3 TOC is not a precise lookup table. The
> precision is
> > +        // worse closer to the end of the file, especially for large
> files.
> > +        // Seeking to 90% of duration in file of size 4M will be off by
> roughly 1 second.
> > +        if (filesize > 4000000)
> > +            av_log(s, AV_LOG_WARNING, "Using MP3 TOC to seek; may be
> imprecise.\n");
>
> This seems like a rather questionable heuristic. And only to print a
> message? Maybe it would be better to drop it, or always print it
> regardless of file size.
>
> I'll make this always print. Hopefully this helps people avoid TOC.


> > +
> > +        int64_t ret  = av_index_search_timestamp(st, timestamp, flags);
> >          if (ret < 0)
> >              return ret;
> >
> >          ie = &st->index_entries[ret];
> > +    } else if (fast_seek && st->duration > 0 && filesize > 0) {
> > +        if (!mp3->is_cbr)
> > +            av_log(s, AV_LOG_WARNING, "Using scaling to seek VBR MP3;
> may be imprecise.\n");
> > +
> > +        ie = &ie1;
> > +        timestamp = av_clip64(timestamp, 0, st->duration);
> > +        ie->timestamp = timestamp;
> > +        ie->pos       = av_rescale(timestamp, filesize, st->duration) +
> s->internal->data_offset;
> >      } else {
> > -        return -1;
> > +        return -1; // generic index code
> >      }
> >
> > -    best_pos = mp3_sync(s, ie->pos, flags);
> > +    int64_t best_pos = mp3_sync(s, ie->pos, flags);
> >      if (best_pos < 0)
> > -        return best_pos;
> > +      return best_pos;
> >
> >      if (mp3->is_cbr && ie == &ie1 && mp3->frames) {
> >          int frame_duration = av_rescale(st->duration, 1, mp3->frames);
> > @@ -546,7 +547,7 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
> >  }
> >
> >  static const AVOption options[] = {
> > -    { "usetoc", "use table of contents", offsetof(MP3DecContext,
> usetoc), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 2, AV_OPT_FLAG_DECODING_PARAM},
> > +    { "usetoc", "use table of contents", offsetof(MP3DecContext,
> usetoc), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 1, AV_OPT_FLAG_DECODING_PARAM},
> >      { NULL },
> >  };
> >
>
> Other than these minor comments, LGTM.
>
> The fate-seek-extra-mp3 test is supposed to test CBR mode. I suppose
> it's the right thing to switch the test to the fastseek flag. I think
> the fate update should be part of this patch.
>
> Fixed!


> Yes, the generic seek mode is handled via the gapless tests.
>