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

[wm4]

On Fri, 4 Sep 2015 13:48:41 -0700
Tsung-Hung Wu <tsunghung at chromium.org> wrote:

> Thanks wm4. I updated the patch according to your comments.
> 
> 
> >* @@ -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.
> 
> Done. Please see the patch. Thanks for your comments.
> 
> > >* /*
> *>* 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.
> 
> Yes, I really need the seek operation fast, and AVFMT_FLAG_FAST_SEEK
> allows to sacrifice the accuracy in exchange for responsiveness. For
> VBR files without TOC, like you said, it's not accurate. For CBR files
> without INFO tag, it's fast and reasonable accurate. Since
> AVFMT_FLAG_FAST_SEEK is set, we should make it fast instead of make it
> safe.
> 
> >*          && (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.
> 
> This tries to refine the timestamp. Because it may seek to middle of a
> frame, mp3_sync() will align the position to a frame boundary. Thus,
> the timestamp may be a little bit off.
> 
> Since it's the only place using mp3->frames in mp3_seek()*, *I think
> we can check it right before using it.
> 
> >*          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);
> *>*      }*

Applied, thanks.