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

[Tsung-Hung Wu]

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);
*>*      }*
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avformat-mp3dec-Make-MP3-seek-fast.patch
Type: text/x-patch
Size: 2950 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150904/4daca014/attachment.bin>