Closed Bug 1300956 Opened 7 years ago Closed 7 years ago

Add state objects to MDSM

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(10 files)

Bug 1300956. Part 1 - Add declarations of the state objects of MDSM. Also implement DecodeMetadataState.
58 bytes, text/x-review-board-request
kaku
: review+
Details
Bug 1300956. Part 2 - Implement WaitForCDMState.
58 bytes, text/x-review-board-request
kaku
: review+
Details
Bug 1300956. Part 3 - Implement DormantState.
58 bytes, text/x-review-board-request
kaku
: review+
Details
Bug 1300956. Part 4 - Implement DecodingFirstFrameState.
58 bytes, text/x-review-board-request
kaku
: review+
Details
Bug 1300956. Part 5 - Implement DecodingState.
58 bytes, text/x-review-board-request
kaku
: review+
Details
Bug 1300956. Part 6 - Implement SeekingState.
58 bytes, text/x-review-board-request
kaku
: review+
Details
Bug 1300956. Part 7 - Implement BufferingState.
58 bytes, text/x-review-board-request
kaku
: review+
Details
Bug 1300956. Part 8 - Implement CompletedState.
58 bytes, text/x-review-board-request
kaku
: review+
Details
Bug 1300956. Part 9 - Implement ShutdownState.
58 bytes, text/x-review-board-request
kaku
: review+
Details
Bug 1300956. Part 10 - Remove null-checks for mStateObj.
58 bytes, text/x-review-board-request
kaku
: review+
Details 
A step forward to refactor MDSM using the state pattern.
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Attachment #8788735 - Flags: review?(kaku)
Attachment #8788736 - Flags: review?(kaku)
Attachment #8788737 - Flags: review?(kaku)
Attachment #8788738 - Flags: review?(kaku)
Attachment #8788739 - Flags: review?(kaku)
Attachment #8788740 - Flags: review?(kaku)
Attachment #8788741 - Flags: review?(kaku)
Attachment #8788742 - Flags: review?(kaku)
Attachment #8788743 - Flags: review?(kaku)
Attachment #8788744 - Flags: review?(kaku)
Could State Object be reused instead of creating new state object by SetState?
Why do you want reuse?
I think state machine might switch the state rapidly.
Naively, for saving repeatedly memory allocation / deallocation overhead.
To reuse an object means you need a function like Reset() to restore the object states to its pristine values which complicates the design and is a approach not taken by JAVA where you allocate a new object instead. We should only take this approach if the perf numbers say so.
Comment on attachment 8788735 [details]
Bug 1300956. Part 1 - Add declarations of the state objects of MDSM. Also implement DecodeMetadataState.

https://reviewboard.mozilla.org/r/77128/#review75724

The curly bracket also apply to all the following patches.

::: dom/media/MediaDecoderStateMachine.h:259
(Diff revision 1)
> +  class StateObject;
> +  class DecodeMetadataState;
> +  class WaitForCDMState;
> +  class DormantState;
> +  class DecodingFirstFrameState;
> +  class Decodingstate;

Capitalize "State". Also apply to patch 5.

::: dom/media/MediaDecoderStateMachine.cpp:207
(Diff revision 1)
>  {
>    return TimeDuration::FromMilliseconds(
>      MediaPrefs::MDSMSuspendBackgroundVideoDelay());
>  }
>  
> +class MediaDecoderStateMachine::StateObject {

Put this curly bracket to next line.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

::: dom/media/MediaDecoderStateMachine.cpp:213
(Diff revision 1)
> +public:
> +  virtual ~StateObject() {}
> +  virtual void Enter() {}; // Entry action.
> +  virtual void Exit() {};  // Exit action.
> +  virtual void Step() {}   // Perform a 'cycle' of this state object.
> +  virtual State GetState() const = 0;

Maybe also provide a method to access the state in string which helps logging, or move the the ToStateStr() function into this class as a static method. But don't bother, this could be handled later.

::: dom/media/MediaDecoderStateMachine.cpp:225
(Diff revision 1)
> +  // It is guaranteed to be valid by MDSM.
> +  Master* mMaster;
> +};
> +
> +class MediaDecoderStateMachine::DecodeMetadataState
> +  : public MediaDecoderStateMachine::StateObject {

curly bracket.

::: dom/media/MediaDecoderStateMachine.cpp:229
(Diff revision 1)
> +class MediaDecoderStateMachine::DecodeMetadataState
> +  : public MediaDecoderStateMachine::StateObject {
> +public:
> +  explicit DecodeMetadataState(Master* aPtr) : StateObject(aPtr) {}
> +
> +  void Enter() override {

curly bracket.

::: dom/media/MediaDecoderStateMachine.cpp:233
(Diff revision 1)
> +
> +  void Enter() override {
> +    mMaster->ReadMetadata();
> +  }
> +
> +  State GetState() const override {

curly bracket.

::: dom/media/MediaDecoderStateMachine.cpp:1102
(Diff revision 1)
>  
>    ExitState();
>    mState = aState;
> +
> +  switch (mState) {
> +    case DECODER_STATE_DECODING_METADATA:

The coding guideline also asks for putting curly brackets around the switch-case.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

However, I am not sure if we're still sticking to it or not since it is seldom followed in our code case.
Attachment #8788735 - Flags: review?(kaku) → review+
Comment on attachment 8788736 [details]
Bug 1300956. Part 2 - Implement WaitForCDMState.

https://reviewboard.mozilla.org/r/77130/#review75732
Attachment #8788736 - Flags: review?(kaku) → review+
Comment on attachment 8788737 [details]
Bug 1300956. Part 3 - Implement DormantState.

https://reviewboard.mozilla.org/r/77132/#review75734
Attachment #8788737 - Flags: review?(kaku) → review+
Comment on attachment 8788738 [details]
Bug 1300956. Part 4 - Implement DecodingFirstFrameState.

https://reviewboard.mozilla.org/r/77134/#review75736
Attachment #8788738 - Flags: review?(kaku) → review+
Comment on attachment 8788739 [details]
Bug 1300956. Part 5 - Implement DecodingState.

https://reviewboard.mozilla.org/r/77136/#review75726

::: dom/media/MediaDecoderStateMachine.cpp:281
(Diff revision 1)
>    State GetState() const override {
>      return DECODER_STATE_DECODING_FIRSTFRAME;
>    }
>  };
>  
> +class MediaDecoderStateMachine::Decodingstate

Capitalize "DecodingState".
Attachment #8788739 - Flags: review?(kaku) → review+
Comment on attachment 8788740 [details]
Bug 1300956. Part 6 - Implement SeekingState.

https://reviewboard.mozilla.org/r/77138/#review75738
Attachment #8788740 - Flags: review?(kaku) → review+
Comment on attachment 8788741 [details]
Bug 1300956. Part 7 - Implement BufferingState.

https://reviewboard.mozilla.org/r/77140/#review75740
Attachment #8788741 - Flags: review?(kaku) → review+
Comment on attachment 8788742 [details]
Bug 1300956. Part 8 - Implement CompletedState.

https://reviewboard.mozilla.org/r/77142/#review75742
Attachment #8788742 - Flags: review?(kaku) → review+
Comment on attachment 8788743 [details]
Bug 1300956. Part 9 - Implement ShutdownState.

https://reviewboard.mozilla.org/r/77144/#review75744
Attachment #8788743 - Flags: review?(kaku) → review+
Comment on attachment 8788744 [details]
Bug 1300956. Part 10 - Remove null-checks for mStateObj.

https://reviewboard.mozilla.org/r/77146/#review75730

::: dom/media/MediaDecoderStateMachine.cpp:1036
(Diff revision 1)
> -  OwnerThread()->Dispatch(
> -    NewRunnableMethod(this, &MediaDecoderStateMachine::EnterState));
> +  RefPtr<MediaDecoderStateMachine> self = this;
> +  OwnerThread()->Dispatch(NS_NewRunnableFunction([self] () {
> +    self->mStateObj->Enter();
> +  }));

How about just calling ScheduleStateMachine() here? Maybe with an assertion on the state must be the ReadMetaData.
Attachment #8788744 - Flags: review?(kaku) → review+
Comment on attachment 8788735 [details]
Bug 1300956. Part 1 - Add declarations of the state objects of MDSM. Also implement DecodeMetadataState.

https://reviewboard.mozilla.org/r/77128/#review75724

> The coding guideline also asks for putting curly brackets around the switch-case.
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
> 
> However, I am not sure if we're still sticking to it or not since it is seldom followed in our code case.

The braces are needed when you need to declare a variable in a switch.
Comment on attachment 8788744 [details]
Bug 1300956. Part 10 - Remove null-checks for mStateObj.

https://reviewboard.mozilla.org/r/77146/#review75788

::: dom/media/MediaDecoderStateMachine.cpp:1036
(Diff revision 1)
> -  OwnerThread()->Dispatch(
> -    NewRunnableMethod(this, &MediaDecoderStateMachine::EnterState));
> +  RefPtr<MediaDecoderStateMachine> self = this;
> +  OwnerThread()->Dispatch(NS_NewRunnableFunction([self] () {
> +    self->mStateObj->Enter();
> +  }));

No, we can't. ScheduleStateMachine() will call mStateObj->Step() instead of mStateObj->Enter().
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b867d9ef5e64
Part 1 - Add declarations of the state objects of MDSM. Also implement DecodeMetadataState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/782c54cd404f
Part 2 - Implement WaitForCDMState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/174da142669c
Part 3 - Implement DormantState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/30b53fa5af13
Part 4 - Implement DecodingFirstFrameState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/b9ea5ce506a7
Part 5 - Implement DecodingState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/d12930f864f3
Part 6 - Implement SeekingState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/a37e83f746ee
Part 7 - Implement BufferingState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/ce221b3846e6
Part 8 - Implement CompletedState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/1d0bab504e28
Part 9 - Implement ShutdownState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/5c0e25ea512c
Part 10 - Remove null-checks for mStateObj. r=kaku
You need to log in before you can comment on or make changes to this bug.