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

[wm4]

On Tue,  1 Dec 2015 10:54:38 -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    | 39 +++++++++++++++++++--------------------
>  libavformat/seek-test.c |  8 +++++---
>  tests/fate/seek.mak     |  2 +-
>  3 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 32ca00c..04c9861 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;
> +    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,35 +499,36 @@ 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;
> +    int fast_seek = s->flags & AVFMT_FLAG_FAST_SEEK;
>      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))) {
> +        // NOTE: The MP3 TOC is not a precise lookup table. Accuracy is worse
> +        // for bigger files.
> +        av_log(s, AV_LOG_WARNING, "Using MP3 TOC to seek; may be imprecise.\n");
> +
> +        int64_t ret = av_index_search_timestamp(st, timestamp, flags);

I moved this line above the av_log() to avoid
declaration-after-statement.

>          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);
> @@ -546,7 +545,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 },
>  };
>  
> diff --git a/libavformat/seek-test.c b/libavformat/seek-test.c
> index 1926f21..bfd06db 100644
> --- a/libavformat/seek-test.c
> +++ b/libavformat/seek-test.c
> @@ -56,7 +56,7 @@ static void ts_str(char buffer[60], int64_t ts, AVRational base)
>  int main(int argc, char **argv)
>  {
>      const char *filename;
> -    AVFormatContext *ic = NULL;
> +    AVFormatContext *ic = avformat_alloc_context();
>      int i, ret, stream_id;
>      int j;
>      int64_t timestamp;
> @@ -76,8 +76,10 @@ int main(int argc, char **argv)
>              frame_count = atoi(argv[i+1]);
>          } else if(!strcmp(argv[i], "-duration")){
>              duration = atoi(argv[i+1]);
> -        } else if(!strcmp(argv[i], "-usetoc")) {
> -            av_dict_set(&format_opts, "usetoc", argv[i+1], 0);
> +        } else if(!strcmp(argv[i], "-fastseek")) {
> +            if (atoi(argv[i+1])) {
> +                ic->flags |= AVFMT_FLAG_FAST_SEEK;
> +            }
>          } else {
>              argc = 1;
>          }
> diff --git a/tests/fate/seek.mak b/tests/fate/seek.mak
> index dfb2e84..a229e72 100644
> --- a/tests/fate/seek.mak
> +++ b/tests/fate/seek.mak
> @@ -244,7 +244,7 @@ FATE_SEEK += $(FATE_SEEK_LAVF-yes:%=fate-seek-lavf-%)
>  # extra files
>  
>  FATE_SEEK_EXTRA-$(CONFIG_MP3_DEMUXER)   += fate-seek-extra-mp3
> -fate-seek-extra-mp3:  CMD = run libavformat/seek-test$(EXESUF) $(TARGET_SAMPLES)/gapless/gapless.mp3 -usetoc 0
> +fate-seek-extra-mp3:  CMD = run libavformat/seek-test$(EXESUF) $(TARGET_SAMPLES)/gapless/gapless.mp3 -fastseek 1
>  FATE_SEEK_EXTRA += $(FATE_SEEK_EXTRA-yes)
>  
>  

Applied, with the change I mentioned.