Always transition to DECODING when SEEKING is done

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year 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)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
Assignee: nobody → jwwang
Blocks: 1317570
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
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 8

2 years ago
mozreview-review
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 9

2 years ago
mozreview-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 10

2 years ago
mozreview-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 11

2 years ago
mozreview-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 12

2 years ago
mozreview-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 13

2 years ago
mozreview-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 14

2 years ago
mozreview-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+
(Assignee)

Comment 15

2 years ago
Thanks for the review!

Comment 16

2 years ago
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

Updated

2 years ago
Depends on: 1349097
(Assignee)

Updated

a year ago
Depends on: 1386489
You need to log in before you can comment on or make changes to this bug.