1307725 - Move MDSM::mIsPrerolling into DecodingState

Status: RESOLVED FIXED

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

Description

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

Prerolling is the concern of DecodingState. Other states don't need to worry about it.

Comment 1

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

Attachment: Bug 1307725. Part 1 - add StateObject::HandleAudioCaptured() to handle audio capture.

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

Comment 2

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

Attachment: Bug 1307725. Part 2 - handle prerolling in DecodingState::HandleEndOfStream() and move it to the public section.

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

Comment 3

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

Attachment: Bug 1307725. Part 3 - add StateObject::HandleWaitingForData to handle NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA.

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

Comment 4

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

Attachment: Bug 1307725. Part 4 - move MaybeStopPrerolling() into DecodingState.

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

Comment 5

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

Attachment: Bug 1307725. Part 5 - move the check of mIsPrerolling out of MaybeStartPlayback().

Note CompletedState doesn't need to check this flag because prerolling should be done when entering the COMPLETED state.

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

Comment 6

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

Attachment: Bug 1307725. Part 6 - move mIsPrerolling out of MDSM::DumpDebugInfo().

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

Comment 7

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

Attachment: Bug 1307725. Part 7 - move mIsPrerolling into DecodingState.

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

Comment 8

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

Attachment: Bug 1307725. Part 8 - move DonePrerolling{Audio,Video} into DecodingState.

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

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8799590 [details]
Bug 1307725. Part 1 - add StateObject::HandleAudioCaptured() to handle audio capture.

https://reviewboard.mozilla.org/r/84712/#review83364

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 1)
> -  ScheduleStateMachine();
>  
>    // Don't buffer as much when audio is captured because we don't need to worry
>    // about high latency audio devices.
>    mAmpleAudioThresholdUsecs = mAudioCaptured ?
>                                detail::AMPLE_AUDIO_USECS / 2 :
>                                detail::AMPLE_AUDIO_USECS;
>  
> -  MaybeStopPrerolling();
> +  mStateObj->HandleAudioCaptured();

The logic seems inequivalent to me here.
It's possible for |SetAudioCapture| being invoked in any state. Then by a call to |ScheduleStateMachine|, Further actions are expected if MDSM is entering either "Decodeing" or "Complete" state in the next cycle.

But now, you're assuming |HandleAudioCaptured| should do nothing (return false) in states other than "Decoding" and "Complete".

Would you elobarte on this ?

Comment 10

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

Comment on attachment 8799590 [details]
Bug 1307725. Part 1 - add StateObject::HandleAudioCaptured() to handle audio capture.

https://reviewboard.mozilla.org/r/84712/#review83372

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 1)
> -  ScheduleStateMachine();
>  
>    // Don't buffer as much when audio is captured because we don't need to worry
>    // about high latency audio devices.
>    mAmpleAudioThresholdUsecs = mAudioCaptured ?
>                                detail::AMPLE_AUDIO_USECS / 2 :
>                                detail::AMPLE_AUDIO_USECS;
>  
> -  MaybeStopPrerolling();
> +  mStateObj->HandleAudioCaptured();

ScheduleStateMachine() is called to start the media sink when the state is DECODING or COMPLETED. For other states, there is nothing to do. That is why the default implementation of HandleAudioCaptured() returns false and does nothing.

As you can see, ScheduleStateMachine() only makes sense for the state objects which implement Step(), e.g. BufferingState, DecodingState and CompletedState. And BufferingState doesn't care about switching media sinks.

Comment 11

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8799590 [details]
Bug 1307725. Part 1 - add StateObject::HandleAudioCaptured() to handle audio capture.

https://reviewboard.mozilla.org/r/84712/#review83392

Comment 12

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8799591 [details]
Bug 1307725. Part 2 - handle prerolling in DecodingState::HandleEndOfStream() and move it to the public section.

https://reviewboard.mozilla.org/r/84714/#review83434

Comment 13

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8799594 [details]
Bug 1307725. Part 5 - move the check of mIsPrerolling out of MaybeStartPlayback().

https://reviewboard.mozilla.org/r/84720/#review83444

Comment 14

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8799595 [details]
Bug 1307725. Part 6 - move mIsPrerolling out of MDSM::DumpDebugInfo().

https://reviewboard.mozilla.org/r/84722/#review83452

Comment 15

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8799593 [details]
Bug 1307725. Part 4 - move MaybeStopPrerolling() into DecodingState.

https://reviewboard.mozilla.org/r/84718/#review83722

Comment 16

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8799592 [details]
Bug 1307725. Part 3 - add StateObject::HandleWaitingForData to handle NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA.

https://reviewboard.mozilla.org/r/84716/#review83724

Comment 17

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8799596 [details]
Bug 1307725. Part 7 - move mIsPrerolling into DecodingState.

https://reviewboard.mozilla.org/r/84724/#review83732

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8799597 [details]
Bug 1307725. Part 8 - move DonePrerolling{Audio,Video} into DecodingState.

https://reviewboard.mozilla.org/r/84726/#review83734

looks good to me !

Comment 19

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

Thanks!

Comment 20

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

Comment on attachment 8799590 [details]
Bug 1307725. Part 1 - add StateObject::HandleAudioCaptured() to handle audio capture.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84712/diff/1-2/

Comment 21

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

Comment on attachment 8799591 [details]
Bug 1307725. Part 2 - handle prerolling in DecodingState::HandleEndOfStream() and move it to the public section.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84714/diff/1-2/

Comment 22

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

Comment on attachment 8799592 [details]
Bug 1307725. Part 3 - add StateObject::HandleWaitingForData to handle NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84716/diff/1-2/

Comment 23

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

Comment on attachment 8799593 [details]
Bug 1307725. Part 4 - move MaybeStopPrerolling() into DecodingState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84718/diff/1-2/

Comment 24

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

Comment on attachment 8799594 [details]
Bug 1307725. Part 5 - move the check of mIsPrerolling out of MaybeStartPlayback().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84720/diff/1-2/

Comment 25

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

Comment on attachment 8799595 [details]
Bug 1307725. Part 6 - move mIsPrerolling out of MDSM::DumpDebugInfo().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84722/diff/1-2/

Comment 26

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

Comment on attachment 8799596 [details]
Bug 1307725. Part 7 - move mIsPrerolling into DecodingState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84724/diff/1-2/

Comment 27

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

Comment on attachment 8799597 [details]
Bug 1307725. Part 8 - move DonePrerolling{Audio,Video} into DecodingState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84726/diff/1-2/

Comment 28

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

Comment on attachment 8799592 [details]
Bug 1307725. Part 3 - add StateObject::HandleWaitingForData to handle NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84716/diff/2-3/

Comment 29

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

Comment on attachment 8799593 [details]
Bug 1307725. Part 4 - move MaybeStopPrerolling() into DecodingState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84718/diff/2-3/

Comment 30

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

Comment on attachment 8799594 [details]
Bug 1307725. Part 5 - move the check of mIsPrerolling out of MaybeStartPlayback().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84720/diff/2-3/

Comment 31

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

Comment on attachment 8799595 [details]
Bug 1307725. Part 6 - move mIsPrerolling out of MDSM::DumpDebugInfo().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84722/diff/2-3/

Comment 32

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

Comment on attachment 8799596 [details]
Bug 1307725. Part 7 - move mIsPrerolling into DecodingState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84724/diff/2-3/

Comment 33

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

Comment on attachment 8799597 [details]
Bug 1307725. Part 8 - move DonePrerolling{Audio,Video} into DecodingState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84726/diff/2-3/

Comment 34

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

Comment on attachment 8799590 [details]
Bug 1307725. Part 1 - add StateObject::HandleAudioCaptured() to handle audio capture.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84712/diff/2-3/

Comment 35

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

Comment on attachment 8799591 [details]
Bug 1307725. Part 2 - handle prerolling in DecodingState::HandleEndOfStream() and move it to the public section.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84714/diff/2-3/

Comment 36

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

Comment on attachment 8799592 [details]
Bug 1307725. Part 3 - add StateObject::HandleWaitingForData to handle NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84716/diff/3-4/

Comment 37

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

Comment on attachment 8799593 [details]
Bug 1307725. Part 4 - move MaybeStopPrerolling() into DecodingState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84718/diff/3-4/

Comment 38

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

Comment on attachment 8799594 [details]
Bug 1307725. Part 5 - move the check of mIsPrerolling out of MaybeStartPlayback().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84720/diff/3-4/

Comment 39

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

Comment on attachment 8799595 [details]
Bug 1307725. Part 6 - move mIsPrerolling out of MDSM::DumpDebugInfo().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84722/diff/3-4/

Comment 40

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

Comment on attachment 8799596 [details]
Bug 1307725. Part 7 - move mIsPrerolling into DecodingState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84724/diff/3-4/

Comment 41

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

Comment on attachment 8799597 [details]
Bug 1307725. Part 8 - move DonePrerolling{Audio,Video} into DecodingState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84726/diff/3-4/

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/396368d32e74
Part 1 - add StateObject::HandleAudioCaptured() to handle audio capture. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/ba15b92671b5
Part 2 - handle prerolling in DecodingState::HandleEndOfStream() and move it to the public section. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/ffd86d831e2f
Part 3 - add StateObject::HandleWaitingForData to handle NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/77220cbb860a
Part 4 - move MaybeStopPrerolling() into DecodingState. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/dc32593b8ae4
Part 5 - move the check of mIsPrerolling out of MaybeStartPlayback(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/ae9d99dd6a3d
Part 6 - move mIsPrerolling out of MDSM::DumpDebugInfo(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/b5c6affddc62
Part 7 - move mIsPrerolling into DecodingState. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/428752cd4bf4
Part 8 - move DonePrerolling{Audio,Video} into DecodingState. r=kikuo

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/396368d32e74
https://hg.mozilla.org/mozilla-central/rev/ba15b92671b5
https://hg.mozilla.org/mozilla-central/rev/ffd86d831e2f
https://hg.mozilla.org/mozilla-central/rev/77220cbb860a
https://hg.mozilla.org/mozilla-central/rev/dc32593b8ae4
https://hg.mozilla.org/mozilla-central/rev/ae9d99dd6a3d
https://hg.mozilla.org/mozilla-central/rev/b5c6affddc62
https://hg.mozilla.org/mozilla-central/rev/428752cd4bf4