1314884 - Remove SeekTarget::mEventVisibility

Status: RESOLVED FIXED

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

Description

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

This member is used by SeekingState::Enter() only. We can pass an additional parameter to SeekingState::Enter() to indicate whether to publish events during seeking.

Comment 1

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

Attachment: Bug 1314884. Part 1 - move the calculation of seek target into DormantState::Enter() so we don't need to pass a SeekJob to the function.

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

Comment 2

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

Attachment: Bug 1314884. Part 2 - Let DORMANT transition to SEEKING when play state changes to PLAYING since we need to seek anyway whenever exiting dormant.

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

Comment 3

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

Attachment: Bug 1314884. Part 3 - remove the call to |mPendingSeek.RejectIfExists(__func__)| from DormantState::HandleSeek() since it will be rejected in Exit() before transitioning to SEEKING.

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

Comment 4

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

Attachment: Bug 1314884. Part 4 - extract the code of HandleSeek() to the parent class.

DormantState, DecodingState, SeekingState, BufferingState and CompletedState have exactly the same implementation of HandleSeek().
We can extract the code to avoid duplication.

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

Comment 5

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

Attachment: Bug 1314884. Part 5 - pass StateObject::EventVisibility to SeekingState::Enter() to indicate whether to publish events during seek.

Note EventVisibility::Suppressed is used by internal seeks only like video-only seek or exit-dormant seek.

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

Comment 6

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

Attachment: Bug 1314884. Part 6 - remove SeekTarget::mEventVisibility.

Note it is fine to always assume MediaDecoderEventVisibility::Observable in SeekJob::Resolve()
because no one will wait for the promise of internal seeks (where EventVisibility::Suppressed is passed).

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

Comment 7

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

Attachment: Bug 1314884. Part 7 - remove SeekResolveValue::mEventVisibility for we always assume MediaDecoderEventVisibility::Observable in P6.

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

Comment 8

[Tzuhao Kuo [:kaku]]

Comment on attachment 8808457 [details]
Bug 1314884. Part 1 - move the calculation of seek target into DormantState::Enter() so we don't need to pass a SeekJob to the function.

https://reviewboard.mozilla.org/r/91278/#review91498

Comment 9

[Tzuhao Kuo [:kaku]]

Comment on attachment 8808458 [details]
Bug 1314884. Part 2 - Let DORMANT transition to SEEKING when play state changes to PLAYING since we need to seek anyway whenever exiting dormant.

https://reviewboard.mozilla.org/r/91280/#review91500

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8808459 [details]
Bug 1314884. Part 3 - remove the call to |mPendingSeek.RejectIfExists(__func__)| from DormantState::HandleSeek() since it will be rejected in Exit() before transitioning to SEEKING.

https://reviewboard.mozilla.org/r/91282/#review91502

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8808460 [details]
Bug 1314884. Part 4 - extract the code of HandleSeek() to the parent class.

https://reviewboard.mozilla.org/r/91284/#review91504

Comment 12

[Tzuhao Kuo [:kaku]]

Comment on attachment 8808461 [details]
Bug 1314884. Part 5 - pass StateObject::EventVisibility to SeekingState::Enter() to indicate whether to publish events during seek.

https://reviewboard.mozilla.org/r/91286/#review91506

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8808462 [details]
Bug 1314884. Part 6 - remove SeekTarget::mEventVisibility.

https://reviewboard.mozilla.org/r/91288/#review91508

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8808463 [details]
Bug 1314884. Part 7 - remove SeekResolveValue::mEventVisibility for we always assume MediaDecoderEventVisibility::Observable in P6.

https://reviewboard.mozilla.org/r/91290/#review91510

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/268128dac277
Part 1 - move the calculation of seek target into DormantState::Enter() so we don't need to pass a SeekJob to the function. r=kaku
https://hg.mozilla.org/integration/autoland/rev/3bcc70c0259f
Part 2 - Let DORMANT transition to SEEKING when play state changes to PLAYING since we need to seek anyway whenever exiting dormant. r=kaku
https://hg.mozilla.org/integration/autoland/rev/32223527f7f8
Part 3 - remove the call to |mPendingSeek.RejectIfExists(__func__)| from DormantState::HandleSeek() since it will be rejected in Exit() before transitioning to SEEKING. r=kaku
https://hg.mozilla.org/integration/autoland/rev/f443222a39c8
Part 4 - extract the code of HandleSeek() to the parent class. r=kaku
https://hg.mozilla.org/integration/autoland/rev/c2d1763308e6
Part 5 - pass StateObject::EventVisibility to SeekingState::Enter() to indicate whether to publish events during seek. r=kaku
https://hg.mozilla.org/integration/autoland/rev/392e20da0963
Part 6 - remove SeekTarget::mEventVisibility. r=kaku
https://hg.mozilla.org/integration/autoland/rev/3a67958e318a
Part 7 - remove SeekResolveValue::mEventVisibility for we always assume MediaDecoderEventVisibility::Observable in P6. r=kaku

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/268128dac277
https://hg.mozilla.org/mozilla-central/rev/3bcc70c0259f
https://hg.mozilla.org/mozilla-central/rev/32223527f7f8
https://hg.mozilla.org/mozilla-central/rev/f443222a39c8
https://hg.mozilla.org/mozilla-central/rev/c2d1763308e6
https://hg.mozilla.org/mozilla-central/rev/392e20da0963
https://hg.mozilla.org/mozilla-central/rev/3a67958e318a