Closed Bug 1307725 Opened 8 years ago Closed 8 years ago

Move MDSM::mIsPrerolling into DecodingState

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(8 files)

Bug 1307725. Part 1 - add StateObject::HandleAudioCaptured() to handle audio capture.
58 bytes, text/x-review-board-request
kikuo
: review+
Details
Bug 1307725. Part 2 - handle prerolling in DecodingState::HandleEndOfStream() and move it to the public section.
58 bytes, text/x-review-board-request
kikuo
: review+
Details
Bug 1307725. Part 3 - add StateObject::HandleWaitingForData to handle NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA.
58 bytes, text/x-review-board-request
kikuo
: review+
Details
Bug 1307725. Part 4 - move MaybeStopPrerolling() into DecodingState.
58 bytes, text/x-review-board-request
kikuo
: review+
Details
Bug 1307725. Part 5 - move the check of mIsPrerolling out of MaybeStartPlayback().
58 bytes, text/x-review-board-request
kikuo
: review+
Details
Bug 1307725. Part 6 - move mIsPrerolling out of MDSM::DumpDebugInfo().
58 bytes, text/x-review-board-request
kikuo
: review+
Details
Bug 1307725. Part 7 - move mIsPrerolling into DecodingState.
58 bytes, text/x-review-board-request
kikuo
: review+
Details
Bug 1307725. Part 8 - move DonePrerolling{Audio,Video} into DecodingState.
58 bytes, text/x-review-board-request
kikuo
: review+
Details 
Prerolling is the concern of DecodingState. Other states don't need to worry about it.
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Attachment #8799590 - Flags: review?(kikuo)
Attachment #8799591 - Flags: review?(kikuo)
Attachment #8799592 - Flags: review?(kikuo)
Attachment #8799593 - Flags: review?(kikuo)
Attachment #8799594 - Flags: review?(kikuo)
Attachment #8799595 - Flags: review?(kikuo)
Attachment #8799596 - Flags: review?(kikuo)
Attachment #8799597 - Flags: review?(kikuo)
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 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 on attachment 8799590 [details]
Bug 1307725. Part 1 - add StateObject::HandleAudioCaptured() to handle audio capture.

https://reviewboard.mozilla.org/r/84712/#review83392
Attachment #8799590 - Flags: review?(kikuo) → review+
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
Attachment #8799591 - Flags: review?(kikuo) → review+
Comment on attachment 8799594 [details]
Bug 1307725. Part 5 - move the check of mIsPrerolling out of MaybeStartPlayback().

https://reviewboard.mozilla.org/r/84720/#review83444
Attachment #8799594 - Flags: review?(kikuo) → review+
Comment on attachment 8799595 [details]
Bug 1307725. Part 6 - move mIsPrerolling out of MDSM::DumpDebugInfo().

https://reviewboard.mozilla.org/r/84722/#review83452
Attachment #8799595 - Flags: review?(kikuo) → review+
Comment on attachment 8799593 [details]
Bug 1307725. Part 4 - move MaybeStopPrerolling() into DecodingState.

https://reviewboard.mozilla.org/r/84718/#review83722
Attachment #8799593 - Flags: review?(kikuo) → review+
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
Attachment #8799592 - Flags: review?(kikuo) → review+
Comment on attachment 8799596 [details]
Bug 1307725. Part 7 - move mIsPrerolling into DecodingState.

https://reviewboard.mozilla.org/r/84724/#review83732
Attachment #8799596 - Flags: review?(kikuo) → review+
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 !
Attachment #8799597 - Flags: review?(kikuo) → review+
Thanks!
Pushed by jwwang@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: