|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
MozReview Request: Bug 1264171 - seperate the fast seek's logic out from accurate seek's logic; r?jwwang
58 bytes, text/x-review-board-request
|Details | Review|
The SeekTask class now handles both accurate seek and fast seek. I would like to have separated implementations for each seek operation.
Created attachment 8741316 [details] MozReview Request: Bug 1264171 - seperate the fast seek's logic out from accurate seek's logic; r?jwwang Review commit: https://reviewboard.mozilla.org/r/46393/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46393/
FastSeekTask is basically SeekTask which contains all the logic about fast seek and accurate seek. I can't see how this change improve/simplify the code.
(In reply to JW Wang [:jwwang] PTO 4/7 ~ 4/15 from comment #2) > FastSeekTask is basically SeekTask which contains all the logic about fast > seek and accurate seek. I can't see how this change improve/simplify the > code. Hmm..., basically FastSeek shares almost the same logic of AccuractSeek, the only difference is while receiving decoded audio/video data from reader (a.k.a. the OnAudioDecoded()/OnVideoDecoded()). However, FastSeek has the chance to fall back to AccurateSeek (https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#656). That is the main reason why FastSeek needs to contain all what AccurateSeek has. Another way is letting FastSeek comprises a SeekTask instance and delegate the seeking operation to it if needed. Or, we can just postpone this bug until Bug 1026330 is resolved?
I will prefer to wait for Bug 1026330.
I would prefer to remove fastSeek. No other browsers implement it. We added telemetry to Firefox 47 to report when it's used, and we've had no reports yet. So it's looking like no-one uses it.
Actually it looks like Safari implements fastSeek: https://bugs.webkit.org/show_bug.cgi?id=124262 I'm still not convinced it's worth keeping.
It used to benefit B2G, but now we may not need it...
I think we should wait for the telemetry  that reports how often fastSeek is used hits our release channel, and if we don't have any/many uses, consider removing fastSeeek.  https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-04-16&keys=__none__!__none__!__none__!__none__&max_channel_version=nightly%252F48&measure=VIDEO_FASTSEEK_USED&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-03-06&table=0&trim=1&use_submission_date=0
(In reply to Chris Pearce (:cpearce) from comment #5) > I would prefer to remove fastSeek. No other browsers implement it. We added > telemetry to Firefox 47 to report when it's used, and we've had no reports > yet. So it's looking like no-one uses it. We can use fast seek to implement recovery seek when video decode is resumed from being suspended, if the video has no audio.
Mass change P2 -> P3
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.