On Fri, 4 Sep 2015 13:48:41 -0700 Tsung-Hung Wu <tsungh...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel