[FFmpeg-devel] [PATCH] avformat/mp3dec: Make MP3 seek fast

[wm4]

On Tue, 1 Sep 2015 11:35:17 -0700
Tsung-Hung Wu <tsunghung at chromium.org> wrote:

> Hi FFmpeg developers,
> 
> I am trying to solve an issue: seeking a large MP3 file is slow. This
> happens when play Podcast audio, for example
> http://podcastdownload.npr.org/anon.npr-podcasts/podcast/344098539/430765119/npr_430765119.mp3
> It works not so good in less computing power mobile devices, especially
> under slow network environment.
> My goal is to improve the usability, although it may sacrifice the accuracy.
> 
> I noticed that FFmpeg has AVFMT_FLAG_FAST_SEEK flag. That's exactly what I
> need, sacrifices the accuracy of playback time in exchange for
> responsiveness. However, it does not work as my expectation. I made some
> changes to fulfill my needs. Please find the attachment for the patch.
> Just to be clear, I put some comments below for discussions, which are not
> in the patch.
> 
> Could you please kindly review the attached patch? Let me know if you have
> any comments. Thank you so much for your help.
> 
> Best Regards,
> Andy
> 
> ---
>  libavformat/mp3dec.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 007c6ea..7f675ab 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -342,7 +342,7 @@ static int mp3_read_header(AVFormatContext *s)
>      int i;
> 
>      if (mp3->usetoc < 0)
> -        mp3->usetoc = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 0 : 2;
> +        mp3->usetoc = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 2;

ok.

> 
> /*
> Should usetoc be 0 when AVFMT_FLAG_FAST_SEEK is enabled? When usetoc is 0,
> seeking a VBR mp3 is slow, even if it contains a TOC.
> Thus, I think 1 is a more proper selection.
> */
> 
>      st = avformat_new_stream(s, NULL);
>      if (!st)
> @@ -489,19 +489,21 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
>      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 > 0 ? mp3->header_filesize
> +                            : avio_size(s->pb) - s->internal->data_offset;

ok. But maybe the avio_size() return value should be checked. It can
return an error.

> 
> /*
> Many MP3 files in Podcast does not have VBRI, LAME, or XING tag, so
> header_filesize is not always available. (file size - data offset) could be
> a good estimation.
> */
> 
> 
>      if (mp3->usetoc == 2)
>          return -1; // generic index code
> 
> -    if (   mp3->is_cbr
> +    if (   (mp3->is_cbr || fast_seek)

Not sure why you need this. If the file is VBR and has no TOC, then all
odds are off. But I guess this is ok on the AVFMT_FLAG_FAST_SEEK code
path, if you think it's more useful this way.

>          && (mp3->usetoc == 0 || !mp3->xing_toc)
>          && st->duration > 0
> -        && mp3->header_filesize > s->internal->data_offset
> -        && mp3->frames) {
> +        && filesize > 0) {
> /*
> Not sure why (mp3->header_filesize > s->internal->data_offset) has to be
> true. What if a MP3 file contains a short audio and a large picture?
> Again, mp3->frames is not always available. Do we really need it here? I
> move the check to below.
> */

I can't think of any reason either. Maybe the original intention of
this code was actually to check whether header_size is a sane value at
all? There are lots of weird corner cases, like incomplete or
concatenated mp3 files.

>          ie = &ie1;
>          timestamp = av_clip64(timestamp, 0, st->duration);
>          ie->timestamp = timestamp;
> -        ie->pos       = av_rescale(timestamp, mp3->header_filesize,
> st->duration) + s->internal->data_offset;
> +        ie->pos       = av_rescale(timestamp, filesize, st->duration) +
> s->internal->data_offset;
>      } else if (mp3->xing_toc) {
>          if (ret < 0)
>              return ret;
> @@ -515,7 +517,7 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
>      if (best_pos < 0)
>          return best_pos;
> 
> -    if (mp3->is_cbr && ie == &ie1) {
> +    if (mp3->is_cbr && ie == &ie1 && mp3->frames) {

Not sure what this does after all.

>          int frame_duration = av_rescale(st->duration, 1, mp3->frames);
>          ie1.timestamp = frame_duration * av_rescale(best_pos -
> s->internal->data_offset, mp3->frames, mp3->header_filesize);
>      }