Adding everyone back on the cc list for a convo with wm4 (meant for everyone). Expect a new patch shortly to go with this discussion.
Also, just a note for posterity and to make sure I haven't missed anything: It seems the mp3 TOC is really not a precise tool for seeking, particularly in large files. I break down what I know about the design in this bug: https://code.google.com/p/chromium/issues/detail?id=545914#c13 - LMK if I'm missing something. On Wed, Nov 18, 2015 at 3:08 AM, wm4 <nfx...@gmail.com> wrote: > On Tue, 17 Nov 2015 14:21:08 -0800 > Chris Cunningham <chcunning...@chromium.org> wrote: > > > On Tue, Nov 17, 2015 at 2:38 AM, wm4 <nfx...@gmail.com> wrote: > > > > > On Mon, 16 Nov 2015 16:58:57 -0800 > > > chcunning...@chromium.org wrote: > > > > > > > From: Chris Cunningham <chcunning...@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 when the TOC is > > > > corrupted (e.g. https://crbug.com/545914). > > > > > > > > For VBR, fast seek is not precise, so continue to prefer the TOC > > > > when available. > > > > --- > > > > libavformat/mp3dec.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c > > > > index 32ca00c..e12266c 100644 > > > > --- a/libavformat/mp3dec.c > > > > +++ b/libavformat/mp3dec.c > > > > @@ -515,8 +515,10 @@ static int mp3_seek(AVFormatContext *s, int > > > stream_index, int64_t timestamp, > > > > filesize = size - s->internal->data_offset; > > > > } > > > > > > > > - if ( (mp3->is_cbr || fast_seek) > > > > - && (mp3->usetoc == 0 || !mp3->xing_toc) > > > > + // When fast seeking, prefer to use the TOC when available for > VBR > > > files > > > > + // since av_rescale may not be accurate for VBR. For CBR, > rescaling > > > is > > > > + // always accurate and more direct than a TOC lookup. > > > > + if (fast_seek && (mp3->is_cbr || !mp3->xing_toc) > > > > && st->duration > 0 > > > > && filesize > 0) { > > > > ie = &ie1; > > > > > > Hm, it's still a bit weird IMHO. Here's a suggestion: if usetoc is > > > unset (its value is -1), then always set it 0. Other uses of the usetoc > > > or fast seek flag could be simplified in the code. > > > > > > I think this sounds good. Then the behavior becomes: > > -1 - unset implies default to 0 > > 0 - generic indexing code > > 1 - toc > > 2 - goes away, this is now the behavior of 0 > > > > > > > > > Ultimately, the fast seek flag just sets the default mode. Disabling > > > the flag makes it always use the generic indexing code. Enabling it > > > used to enable TOC seeking, but we really always want CBR seeking. We > > > want TOC seeking only if the user explicitly enables it by setting > > > usetoc. Is that what we want? > > > > > > > > *"We want TOC seeking only if the user explicitly enables it by setting* > > *usetoc. Is that what we want?"* > > > > I think this is the heart of the complexity. > > Is it useful for users to be able to say things like "fastseek, but never > > use toc (always use scaling, even for VBR)"? > > Or alternatively, "fastseek, but always use toc when available (even for > > CBR)"? > > > > I'm not sure. We can support those combinations, but its more complex and > > harder to reason about. > > > > It may be simpler to simply say that fastseek *overrides* usetoc. It > could > > cause toc to be used for VBR, and scaling for CBR, with no regard for > user > > set values of usetoc (we might detect a user setting and warn that we're > > ignoring it). I'm happy to take this approach if everyone is on board. > What > > do you think? (Also, not sure if you mean to reply just to me, but we'd > > have to ask Michael too). > > No, this was accidental. My mail client selected the wrong reply > address because the mail had multiple recipients. I'll merge the threads. > > I think there are two things: sane defaults, which apply if usetoc is > not used, and forcing the demuxer to use a specific method if the user > passes usetoc. > > In my own opinion, there are 3 use-cases: > - default: seeking is exact, no surprises, but may be slow > (what everyone wants) > - fast: user doesn't care about precision > (possibly the API user wants to enable this for big/slow streams) > - custom: user wants a very specific seeking method to be used > (used rarely, maybe only for debugging) > > So I think the "usetoc" would fit the "custom" use-case, and we don't > have to think much about its interaction with the fast seek mode - it > can simply override it. > > This might be what you had in mind anyway. I'm only arguing that it > makes sense for usetoc to override the fast seek mode. > Sounds good to me, the next patch will follow this outline. I'm thinking I'll do away with the -1 value as well. Seems we want 0 to be the default and I don't think we have a use case for telling default apart from user set value. > > > > > > The failing FATE tests are probably no problem, but they should be > > > updated with the new seek results. > > > > > > > > Thanks for this tip. How can I get the files needed to run these tests? > > Right now my checkout doesn't seem to contain the needed mp3? > > > > $ make fate-seek-extra-mp3 > > fate-seek-extra-mp3 requires external samples and SAMPLES not specified > > The docs state: > > make fate-rsync SAMPLES=fate-suite/ > make fate SAMPLES=fate-suite/ > > The first one downloads all samples to the given directory. It's about > 1GB, so it makes sense not to add them to the git repo. > > > I'm also surprised these tests aren't running when I just "make fate". I > > definitely have CONFIG_MP3_DEMUXER 1. > > > > That's strange. I'm fairly certain "fate" includes all tests. Maybe it > just tried to run all tests that don't require the sample files. > Thanks. I missed that step in the docs. Your suspicions are correct, it just ran what it could without the samples. > > > > > > (Sorry if I'm being a bit pedantic about it, but making this all > > > simpler and reducing the number of option combinations that interact > > > with each other is probably a good idea to save our sanity later.) > > > > > > > No worries. I'm with you. > > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel