Always transition to DECODING when SEEKING is done

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Depends on 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(7 attachments)

http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/dom/media/MediaDecoderStateMachine.cpp#1497

For now, MDSM transitions to COMPLETED and notifies 'playbackEnded' if the new position equals the duration. However, this is not reliable because the metadata might lie about the duration.

We should let MDSM decode to the end to decide the end of playback. This also simplify the logic because there will be only one place to notify 'playbackEnded' which is the COMPLETED state.

Note this bug is also required to fix bug 1317570.
Assignee: nobody → jwwang
Blocks: 1317570
Priority: -- → P3
Attachment #8811115 - Flags: review?(cpearce)
Attachment #8811116 - Flags: review?(cpearce)
Attachment #8811117 - Flags: review?(cpearce)
Attachment #8811118 - Flags: review?(cpearce)
Attachment #8811119 - Flags: review?(cpearce)
Attachment #8811120 - Flags: review?(cpearce)
Attachment #8811121 - Flags: review?(cpearce)
Comment on attachment 8811115 [details]
Bug 1317576. Part 1 - add some helper functions.

https://reviewboard.mozilla.org/r/93338/#review93358
Attachment #8811115 - Flags: review?(cpearce) → review+
Comment on attachment 8811116 [details]
Bug 1317576. Part 2 - always transition to DECODING when seek is done.

https://reviewboard.mozilla.org/r/93340/#review93360
Attachment #8811116 - Flags: review?(cpearce) → review+
Comment on attachment 8811117 [details]
Bug 1317576. Part 3 - remove unused parameter/member.

https://reviewboard.mozilla.org/r/93342/#review93362

::: dom/media/MediaDecoder.h:61
(Diff revision 1)
>  #endif
>  
>  class MediaDecoder : public AbstractMediaDecoder
>  {
>  public:
>    struct SeekResolveValue {

I guess you still need the empty struct so that SeekPromise has a type?
Attachment #8811117 - Flags: review?(cpearce) → review+
Comment on attachment 8811118 [details]
Bug 1317576. Part 4 - remove unused MediaDecoder::SeekResolveValue.

https://reviewboard.mozilla.org/r/93344/#review93364

::: dom/media/MediaDecoder.h
(Diff revision 1)
>  #endif
>  
>  class MediaDecoder : public AbstractMediaDecoder
>  {
>  public:
> -  struct SeekResolveValue {

Ah there you go, removing SeekResolveValue. :)
Attachment #8811118 - Flags: review?(cpearce) → review+
Comment on attachment 8811119 [details]
Bug 1317576. Part 5 - run Step() in CompletedState::Enter() without scheduling an additional cycle.

https://reviewboard.mozilla.org/r/93346/#review93366
Attachment #8811119 - Flags: review?(cpearce) → review+
Comment on attachment 8811120 [details]
Bug 1317576. Part 6 - ensure 'playbackEnded' is notified when seeking to the end on a paused media element.

https://reviewboard.mozilla.org/r/93348/#review93368
Attachment #8811120 - Flags: review?(cpearce) → review+
Comment on attachment 8811121 [details]
Bug 1317576. Part 7 - remove the assertion that doesn't make sense anymore.

https://reviewboard.mozilla.org/r/93350/#review93370
Attachment #8811121 - Flags: review?(cpearce) → review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/860e550daed6
Part 1 - add some helper functions. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/aafde76b36ae
Part 2 - always transition to DECODING when seek is done. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/8f2dc4f621b5
Part 3 - remove unused parameter/member. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/efdc26b14ffa
Part 4 - remove unused MediaDecoder::SeekResolveValue. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/16b9dfb4ca47
Part 5 - run Step() in CompletedState::Enter() without scheduling an additional cycle. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/6f39dfa71498
Part 6 - ensure 'playbackEnded' is notified when seeking to the end on a paused media element. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/1f9d289e8272
Part 7 - remove the assertion that doesn't make sense anymore. r=cpearce
Depends on: 1349097
Depends on: 1386489
You need to log in before you can comment on or make changes to this bug.