Closed Bug 1310140 Opened 3 years ago Closed 3 years ago

Reconcile suspend-video-decoder with the states of MDSM

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

(Blocks 2 open bugs)

Details

Attachments

(13 files)

58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
58 bytes, text/x-review-board-request
kamidphish
: review+
kaku
: review+
Details
We should handle it in different ways in different states:

1. DECODING_METADATA: no need to do that because no decoding is going on.
2. WAIT_FOR_CDM: ditto.
3. DORMANT: no need to do that because we release the decoders when entering dormant state.
4. DECODING_FIRSTFRAME: to be discussed.
5. DECODING: should do.
6. SEEKING: shouldn't do that for we are seeking.
7. BUFFERING: should do.
8. COMPLETED: no need to do that because no decoding is going on.
9. SHUTDOWN: ditto.
Blocks: 1276556
Blocks: 1295921
Blocks: 1295892
Assignee: nobody → jwwang
Priority: -- → P3
Attachment #8801996 - Flags: review?(kaku)
Attachment #8801996 - Flags: review?(dglastonbury)
Attachment #8801997 - Flags: review?(kaku)
Attachment #8801997 - Flags: review?(dglastonbury)
Attachment #8801998 - Flags: review?(kaku)
Attachment #8801998 - Flags: review?(dglastonbury)
Attachment #8801999 - Flags: review?(kaku)
Attachment #8801999 - Flags: review?(dglastonbury)
Attachment #8802000 - Flags: review?(kaku)
Attachment #8802000 - Flags: review?(dglastonbury)
Attachment #8802001 - Flags: review?(kaku)
Attachment #8802001 - Flags: review?(dglastonbury)
Attachment #8802002 - Flags: review?(kaku)
Attachment #8802002 - Flags: review?(dglastonbury)
Attachment #8802003 - Flags: review?(kaku)
Attachment #8802003 - Flags: review?(dglastonbury)
Attachment #8802004 - Flags: review?(kaku)
Attachment #8802004 - Flags: review?(dglastonbury)
Attachment #8802005 - Flags: review?(kaku)
Attachment #8802005 - Flags: review?(dglastonbury)
Attachment #8802006 - Flags: review?(kaku)
Attachment #8802006 - Flags: review?(dglastonbury)
Attachment #8802007 - Flags: review?(kaku)
Attachment #8802007 - Flags: review?(dglastonbury)
Attachment #8802008 - Flags: review?(kaku)
Attachment #8802008 - Flags: review?(dglastonbury)
Comment on attachment 8801996 [details]
Bug 1310140. Part 1 - add StateObject::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86560/#review85718

::: dom/media/MediaDecoderStateMachine.cpp:218
(Diff revision 1)
>  
>    virtual bool HandleAudioCaptured() { return false; }
>  
>    virtual RefPtr<ShutdownPromise> HandleShutdown();
>  
> +  virtual void HandleVideoSuspendTimeout()

Why does this event handler return void? Shouldn't it follow the others to return something indicating that if the state handles this event or not?
Comment on attachment 8802006 [details]
Bug 1310140. Part 11 - make StateObject::HandleVideoSuspendTimeout() pure virtual for all sub-classes override it.

https://reviewboard.mozilla.org/r/86580/#review85732

The HandleVideoSuspendTimeout() implementations of Decoding and Seeking are just that same, why not just have a common implementation here?
Comment on attachment 8801996 [details]
Bug 1310140. Part 1 - add StateObject::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86560/#review85718

> Why does this event handler return void? Shouldn't it follow the others to return something indicating that if the state handles this event or not?

That is used for a transition where some events are handled by state objects and others by MDSM itself. When HandleXXX() returns true, MDSM assumes the event is handled by the underlying state object and will not process it furthermore. Since now we have all events handled by the state objects, we don't need the return value anymore. And we will also remove the return value from other handlers in the near future.
Comment on attachment 8802006 [details]
Bug 1310140. Part 11 - make StateObject::HandleVideoSuspendTimeout() pure virtual for all sub-classes override it.

https://reviewboard.mozilla.org/r/86580/#review85732

The clean-up work will be done in next bugs to extract and remove duplicated code.
Comment on attachment 8801996 [details]
Bug 1310140. Part 1 - add StateObject::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86560/#review85748
Attachment #8801996 - Flags: review?(kaku) → review+
Comment on attachment 8801997 [details]
Bug 1310140. Part 2 - add DecodeMetadataState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86562/#review85720
Attachment #8801997 - Flags: review?(kaku) → review+
Comment on attachment 8801998 [details]
Bug 1310140. Part 3 - add WaitForCDMState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86564/#review85750
Attachment #8801998 - Flags: review?(kaku) → review+
Comment on attachment 8801999 [details]
Bug 1310140. Part 4 - add DormantState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86566/#review85752
Attachment #8801999 - Flags: review?(kaku) → review+
Comment on attachment 8802000 [details]
Bug 1310140. Part 5 - add DecodingFirstFrameState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86568/#review85754
Attachment #8802000 - Flags: review?(kaku) → review+
Comment on attachment 8802001 [details]
Bug 1310140. Part 6 - have DecodingState handle suspend-video-decoding.

https://reviewboard.mozilla.org/r/86570/#review85724

::: dom/media/MediaDecoderStateMachine.cpp:538
(Diff revision 1)
> +  void HandleVideoSuspendTimeout() override
> +  {
> +    mMaster->mVideoDecodeSuspended = true;
> +    mMaster->mOnPlaybackEvent.Notify(MediaEventType::EnterVideoSuspend);
> +    Reader()->SetVideoBlankDecode(true);
> +  }

r+
https://reviewboard.mozilla.org/r/86580/#review85732
Attachment #8802001 - Flags: review?(kaku) → review+
Comment on attachment 8802002 [details]
Bug 1310140. Part 7 - add BufferingState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86572/#review85726

::: dom/media/MediaDecoderStateMachine.cpp:822
(Diff revision 1)
> +  void HandleVideoSuspendTimeout() override
> +  {
> +    mMaster->mVideoDecodeSuspended = true;
> +    mMaster->mOnPlaybackEvent.Notify(MediaEventType::EnterVideoSuspend);
> +    Reader()->SetVideoBlankDecode(true);
> +  }

r+ 
https://reviewboard.mozilla.org/r/86580/#review85732
Attachment #8802002 - Flags: review?(kaku) → review+
Comment on attachment 8802003 [details]
Bug 1310140. Part 8 - add CompletedState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86574/#review85758
Attachment #8802003 - Flags: review?(kaku) → review+
Comment on attachment 8802004 [details]
Bug 1310140. Part 9 - add ShutdownState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86576/#review85760
Attachment #8802004 - Flags: review?(kaku) → review+
Comment on attachment 8802005 [details]
Bug 1310140. Part 10 - have SeekingState handle suspend-video-decoding.

https://reviewboard.mozilla.org/r/86578/#review85728

::: dom/media/MediaDecoderStateMachine.cpp:638
(Diff revision 1)
> +    // in the middle of seeking and won't have a valid video frame to show
> +    // when seek is done.
> +    if (mMaster->mVideoDecodeSuspended) {
> +      mMaster->mVideoDecodeSuspended = false;
> +      mMaster->mOnPlaybackEvent.Notify(MediaEventType::ExitVideoSuspend);
> +      mMaster->mReader->SetVideoBlankDecode(false);

Looks like the convention is using Reader(), right?
Attachment #8802005 - Flags: review?(kaku) → review+
Comment on attachment 8802006 [details]
Bug 1310140. Part 11 - make StateObject::HandleVideoSuspendTimeout() pure virtual for all sub-classes override it.

https://reviewboard.mozilla.org/r/86580/#review85762
Attachment #8802006 - Flags: review?(kaku) → review+
Comment on attachment 8802007 [details]
Bug 1310140. Part 12 - move the HasVideo() check into HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86582/#review85764
Attachment #8802007 - Flags: review?(kaku) → review+
Comment on attachment 8802008 [details]
Bug 1310140. Part 13 - handle resuming video decoding in various state objects.

https://reviewboard.mozilla.org/r/86584/#review85730
Attachment #8802008 - Flags: review?(kaku) → review+
Comment on attachment 8801996 [details]
Bug 1310140. Part 1 - add StateObject::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86560/#review85766
Attachment #8801996 - Flags: review?(dglastonbury) → review+
Comment on attachment 8801997 [details]
Bug 1310140. Part 2 - add DecodeMetadataState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86562/#review85768
Attachment #8801997 - Flags: review?(dglastonbury) → review+
Comment on attachment 8801998 [details]
Bug 1310140. Part 3 - add WaitForCDMState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86564/#review85772
Attachment #8801998 - Flags: review?(dglastonbury) → review+
Comment on attachment 8801999 [details]
Bug 1310140. Part 4 - add DormantState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86566/#review85774
Attachment #8801999 - Flags: review?(dglastonbury) → review+
Comment on attachment 8802000 [details]
Bug 1310140. Part 5 - add DecodingFirstFrameState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86568/#review85776
Attachment #8802000 - Flags: review?(dglastonbury) → review+
Comment on attachment 8802001 [details]
Bug 1310140. Part 6 - have DecodingState handle suspend-video-decoding.

https://reviewboard.mozilla.org/r/86570/#review85778
Attachment #8802001 - Flags: review?(dglastonbury) → review+
Comment on attachment 8802002 [details]
Bug 1310140. Part 7 - add BufferingState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86572/#review85780
Attachment #8802002 - Flags: review?(dglastonbury) → review+
Comment on attachment 8802003 [details]
Bug 1310140. Part 8 - add CompletedState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86574/#review85782
Attachment #8802003 - Flags: review?(dglastonbury) → review+
Comment on attachment 8802004 [details]
Bug 1310140. Part 9 - add ShutdownState::HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86576/#review85784
Attachment #8802004 - Flags: review?(dglastonbury) → review+
Comment on attachment 8802005 [details]
Bug 1310140. Part 10 - have SeekingState handle suspend-video-decoding.

https://reviewboard.mozilla.org/r/86578/#review85788
Attachment #8802005 - Flags: review?(dglastonbury) → review+
Comment on attachment 8802006 [details]
Bug 1310140. Part 11 - make StateObject::HandleVideoSuspendTimeout() pure virtual for all sub-classes override it.

https://reviewboard.mozilla.org/r/86580/#review85790
Attachment #8802006 - Flags: review?(dglastonbury) → review+
Comment on attachment 8802007 [details]
Bug 1310140. Part 12 - move the HasVideo() check into HandleVideoSuspendTimeout().

https://reviewboard.mozilla.org/r/86582/#review85792
Attachment #8802007 - Flags: review?(dglastonbury) → review+
Comment on attachment 8802008 [details]
Bug 1310140. Part 13 - handle resuming video decoding in various state objects.

https://reviewboard.mozilla.org/r/86584/#review85794

::: dom/media/MediaDecoderStateMachine.cpp:220
(Diff revision 1)
>  
>    virtual RefPtr<ShutdownPromise> HandleShutdown();
>  
>    virtual void HandleVideoSuspendTimeout() = 0;
>  
> +  virtual void HandleResumeVideoDecoding();

Typo in commit description: *doens't*
Attachment #8802008 - Flags: review?(dglastonbury) → review+
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13bdbcb9078d
Part 1 - add StateObject::HandleVideoSuspendTimeout(). r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/abbb1e22b2c1
Part 2 - add DecodeMetadataState::HandleVideoSuspendTimeout(). r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/c065a6766910
Part 3 - add WaitForCDMState::HandleVideoSuspendTimeout(). r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/8f14d38a0830
Part 4 - add DormantState::HandleVideoSuspendTimeout(). r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/008b75b607a6
Part 5 - add DecodingFirstFrameState::HandleVideoSuspendTimeout(). r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/bc9daff4c5e0
Part 6 - have DecodingState handle suspend-video-decoding. r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/d4a4999dcf15
Part 7 - add BufferingState::HandleVideoSuspendTimeout(). r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/95cf675aed1f
Part 8 - add CompletedState::HandleVideoSuspendTimeout(). r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/d2870b4ff3d0
Part 9 - add ShutdownState::HandleVideoSuspendTimeout(). r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/df85afebba7b
Part 10 - have SeekingState handle suspend-video-decoding. r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/6b99ecc25e0a
Part 11 - make StateObject::HandleVideoSuspendTimeout() pure virtual for all sub-classes override it. r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/385c13b85702
Part 12 - move the HasVideo() check into HandleVideoSuspendTimeout(). r=kaku,kamidphish
https://hg.mozilla.org/integration/autoland/rev/5e74cf7dec1b
Part 13 - handle resuming video decoding in various state objects. r=kaku,kamidphish
You need to log in before you can comment on or make changes to this bug.