Separate the accurate seek and fast seek into two implementations.

NEW
Assigned to

Status

()

Core
Audio/Video: Playback
P3
normal
2 years ago
2 years ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
The SeekTask class now handles both accurate seek and fast seek. I would like to have separated implementations for each seek operation.
(Assignee)

Updated

2 years ago
Assignee: nobody → tkuo
Blocks: 1235301, 1253184
Depends on: 1261020
(Assignee)

Comment 1

2 years ago
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/
Attachment #8741316 - Flags: review?(jwwang)
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.
(Assignee)

Comment 3

2 years ago
(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?
Flags: needinfo?(jwwang)
I will prefer to wait for Bug 1026330.
Flags: needinfo?(jwwang)
(Assignee)

Updated

2 years ago
Depends on: 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...
Priority: -- → P2
Attachment #8741316 - Flags: review?(jwwang)
(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.