1264171 - Separate the accurate seek and fast seek into two implementations.

Status: NEW

(Core :: Audio/Video: Playback, defect, P3)

Description

[Tzuhao Kuo [:kaku]]

The SeekTask class now handles both accurate seek and fast seek. I would like to have separated implementations for each seek operation.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: 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/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 3

[Tzuhao Kuo [:kaku]]

(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?

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

I will prefer to wait for Bug 1026330.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 7

[Blake Wu [:bwu][:blakewu]]

It used to benefit B2G, but now we may not need it...

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

I think we should wait for the telemetry [1] that reports how often fastSeek is used hits our release channel, and if we don't have any/many uses, consider removing fastSeeek.

[1] 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

Comment 9

[u480271]

(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.

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.