1317576 - Always transition to DECODING when SEEKING is done

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: Bug 1317576. Part 1 - add some helper functions.

Review commit: https://reviewboard.mozilla.org/r/93338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93338/

Comment 2

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

Attachment: Bug 1317576. Part 2 - always transition to DECODING when seek is done.

DecodingState::Enter() will check whether decoding is completed and transition to COMPLETED.

Review commit: https://reviewboard.mozilla.org/r/93340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93340/

Comment 3

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

Attachment: Bug 1317576. Part 3 - remove unused parameter/member.

Review commit: https://reviewboard.mozilla.org/r/93342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93342/

Comment 4

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

Attachment: Bug 1317576. Part 4 - remove unused MediaDecoder::SeekResolveValue.

Review commit: https://reviewboard.mozilla.org/r/93344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93344/

Comment 5

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

Attachment: Bug 1317576. Part 5 - run Step() in CompletedState::Enter() without scheduling an additional cycle.

MDSM might have reached the end of playback. We want to notify 'playbackEnded' ASAP.

Review commit: https://reviewboard.mozilla.org/r/93346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93346/

Comment 6

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

Attachment: Bug 1317576. Part 6 - ensure 'playbackEnded' is notified when seeking to the end on a paused media element.

Review commit: https://reviewboard.mozilla.org/r/93348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93348/

Comment 7

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

Attachment: Bug 1317576. Part 7 - remove the assertion that doesn't make sense anymore.

It is possible to call AudioEndTime() in CompletedState::Step() with MedisSink
not started at all. This happens when seeking to the end on a video only file.

Review commit: https://reviewboard.mozilla.org/r/93350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93350/

Comment 8

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

Comment on attachment 8811115 [details]
Bug 1317576. Part 1 - add some helper functions.

https://reviewboard.mozilla.org/r/93338/#review93358

Comment 9

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

Comment on attachment 8811116 [details]
Bug 1317576. Part 2 - always transition to DECODING when seek is done.

https://reviewboard.mozilla.org/r/93340/#review93360

Comment 10

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

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?

Comment 11

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

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. :)

Comment 12

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

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

Comment 13

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

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

Comment 14

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

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

Comment 15

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

Thanks for the review!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/860e550daed6
https://hg.mozilla.org/mozilla-central/rev/aafde76b36ae
https://hg.mozilla.org/mozilla-central/rev/8f2dc4f621b5
https://hg.mozilla.org/mozilla-central/rev/efdc26b14ffa
https://hg.mozilla.org/mozilla-central/rev/16b9dfb4ca47
https://hg.mozilla.org/mozilla-central/rev/6f39dfa71498
https://hg.mozilla.org/mozilla-central/rev/1f9d289e8272