Closed Bug 1329110 Opened 3 years ago Closed 3 years ago

Split StateObject::Handle{Audio,Video}NotDecoded into small functions

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(7 files)

Small functions are more likely to be reused.

We will split HandleAudioNotDecoded into:
1. HandleWaitingForAudio when the error code is NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA.
2. HandleAudioCanceled when the error code is NS_ERROR_DOM_MEDIA_CANCELED.
3. HandleEndOfAudio when the error code is NS_ERROR_DOM_MEDIA_END_OF_STREAM.
Assignee: nobody → jwwang
Blocks: 1324999
Priority: -- → P3
Attachment #8825259 - Flags: review?(kaku)
Attachment #8825260 - Flags: review?(kaku)
Attachment #8825261 - Flags: review?(kaku)
Attachment #8825262 - Flags: review?(kaku)
Attachment #8825263 - Flags: review?(kaku)
Attachment #8825264 - Flags: review?(kaku)
Attachment #8825265 - Flags: review?(kaku)
Comment on attachment 8825259 [details]
Bug 1329110. Part 1 - add HandleWaitingForAudio().

https://reviewboard.mozilla.org/r/103430/#review104072

::: dom/media/MediaDecoderStateMachine.cpp:2181
(Diff revision 1)
>  AccurateSeekingState::HandleAudioNotDecoded(const MediaResult& aError)
>  {
>    if (mSeekJob.mTarget->IsVideoOnly()) {
>      return;
>    }
>    MOZ_ASSERT(!mDoneAudioSeeking);

Also copy this assertion into AccurateSeekingState::HandleWaitingForAudio() ?
Attachment #8825259 - Flags: review?(kaku) → review+
Comment on attachment 8825260 [details]
Bug 1329110. Part 2 - add HandleAudioCanceled().

https://reviewboard.mozilla.org/r/103432/#review104074

::: dom/media/MediaDecoderStateMachine.cpp:2206
(Diff revision 1)
>  AccurateSeekingState::HandleAudioNotDecoded(const MediaResult& aError)
>  {
>    if (mSeekJob.mTarget->IsVideoOnly()) {
>      return;
>    }
>    MOZ_ASSERT(!mDoneAudioSeeking);

Same here.
Attachment #8825260 - Flags: review?(kaku) → review+
Comment on attachment 8825261 [details]
Bug 1329110. Part 3 - add HandleEndOfAudio().

https://reviewboard.mozilla.org/r/103434/#review104076

::: dom/media/MediaDecoderStateMachine.cpp:2233
(Diff revision 1)
>  AccurateSeekingState::HandleAudioNotDecoded(const MediaResult& aError)
>  {
>    if (mSeekJob.mTarget->IsVideoOnly()) {
>      return;
>    }
>    MOZ_ASSERT(!mDoneAudioSeeking);

Here again.
Attachment #8825261 - Flags: review?(kaku) → review+
Comment on attachment 8825262 [details]
Bug 1329110. Part 4 - remove StateObject::HandleAudioNotDecoded().

https://reviewboard.mozilla.org/r/103436/#review104078

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 1)
>    State GetState() const override
>    {
>      return DECODER_STATE_SHUTDOWN;
>    }
>  
> -  void HandleAudioNotDecoded(const MediaResult& aError) override {}

Shouldn't we implement HandleWaitingForAudio(), HandleAudioCanceled() and HandleEndOfAudio() for the ShutdownState? We can put an assertion there or at least do nothing.

Same fo the video case.
Attachment #8825262 - Flags: review?(kaku)
Comment on attachment 8825263 [details]
Bug 1329110. Part 5 - split Split StateObject::HandleVideoNotDecoded into small functions.

https://reviewboard.mozilla.org/r/103438/#review104080

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 1)
>    State GetState() const override
>    {
>      return DECODER_STATE_SHUTDOWN;
>    }
>  
> -  void HandleVideoNotDecoded(const MediaResult& aError) override {}

Same issue here.
Attachment #8825263 - Flags: review?(kaku)
Comment on attachment 8825264 [details]
Bug 1329110. Part 6 - remove StateObject::HandleWaitingForData().

https://reviewboard.mozilla.org/r/103440/#review104082
Attachment #8825264 - Flags: review?(kaku) → review+
Comment on attachment 8825265 [details]
Bug 1329110. Part 7 - remove StateObject::HandleEndOfStream().

https://reviewboard.mozilla.org/r/103442/#review104084
Attachment #8825265 - Flags: review?(kaku) → review+
Comment on attachment 8825262 [details]
Bug 1329110. Part 4 - remove StateObject::HandleAudioNotDecoded().

https://reviewboard.mozilla.org/r/103436/#review104078

> Shouldn't we implement HandleWaitingForAudio(), HandleAudioCanceled() and HandleEndOfAudio() for the ShutdownState? We can put an assertion there or at least do nothing.
> 
> Same fo the video case.

It will be done in bug 1329891 where the default implementation is to crash and states that expect the event (e.g. NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA, NS_ERROR_DOM_MEDIA_CANCELED, or NS_ERROR_DOM_MEDIA_END_OF_STREAM) should override the virtual function to do something meaningful.
Comment on attachment 8825263 [details]
Bug 1329110. Part 5 - split Split StateObject::HandleVideoNotDecoded into small functions.

https://reviewboard.mozilla.org/r/103438/#review104088

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 1)
>    State GetState() const override
>    {
>      return DECODER_STATE_SHUTDOWN;
>    }
>  
> -  void HandleVideoNotDecoded(const MediaResult& aError) override {}

It will be done in bug 1329891 to improve the assertion/invariant.
Comment on attachment 8825262 [details]
Bug 1329110. Part 4 - remove StateObject::HandleAudioNotDecoded().

https://reviewboard.mozilla.org/r/103436/#review104090
Attachment #8825262 - Flags: review+
Comment on attachment 8825263 [details]
Bug 1329110. Part 5 - split Split StateObject::HandleVideoNotDecoded into small functions.

https://reviewboard.mozilla.org/r/103438/#review104092
Attachment #8825263 - Flags: review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a38fae269e27
Part 1 - add HandleWaitingForAudio(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/a79dbf426f4b
Part 2 - add HandleAudioCanceled(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/b98174a7a769
Part 3 - add HandleEndOfAudio(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/94b7ee3c58d4
Part 4 - remove StateObject::HandleAudioNotDecoded(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/fc50385b69b0
Part 5 - split Split StateObject::HandleVideoNotDecoded into small functions. r=kaku
https://hg.mozilla.org/integration/autoland/rev/390ac9c6b655
Part 6 - remove StateObject::HandleWaitingForData(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/bb6cb6b7cd9c
Part 7 - remove StateObject::HandleEndOfStream(). r=kaku
You need to log in before you can comment on or make changes to this bug.