Closed Bug 1311901 Opened 4 years ago Closed 4 years ago

Add comments to the internal 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

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Attachment #8803213 - Flags: review?(kikuo)
Attachment #8803213 - Flags: review?(kaku)
Comment on attachment 8803213 [details]
Bug 1311901 - document the internal states of MDSM.

https://reviewboard.mozilla.org/r/87500/#review86504

::: dom/media/MediaDecoderStateMachine.cpp:951
(Diff revision 1)
>    const uint32_t mBufferingWait = 15;
>  };
>  
> +/**
> + * Purpose: play all the decoded data and fire the 'ended' event.
> + *   DORMANT if any dormant request.

Maybe follow the comment pattern of other states, add the following two lines:

```
 *
 * Transition to:
```
Attachment #8803213 - Flags: review?(kaku) → review+
Comment on attachment 8803213 [details]
Bug 1311901 - document the internal states of MDSM.

https://reviewboard.mozilla.org/r/87500/#review86516

::: dom/media/MediaDecoderStateMachine.cpp:361
(Diff revision 1)
> + * Transition to other states when CDM is ready:
> + *   DORMANT if any pending dormant request.
> + *   DECODING_FIRSTFRAME otherwise.

Just my personal suggestion, I would like to see all kinds of possible state-transition for each states, i.e. SHUTDONW here and elsewhere also.  Though it's defined in the base one.  It would be easier to understand what each state may transit-to without having a look around first.
Attachment #8803213 - Flags: review?(kikuo) → review+
Comment on attachment 8803213 [details]
Bug 1311901 - document the internal states of MDSM.

https://reviewboard.mozilla.org/r/87500/#review86504

> Maybe follow the comment pattern of other states, add the following two lines:
> 
> ```
>  *
>  * Transition to:
> ```

Good catch!
Comment on attachment 8803213 [details]
Bug 1311901 - document the internal states of MDSM.

https://reviewboard.mozilla.org/r/87500/#review86516

> Just my personal suggestion, I would like to see all kinds of possible state-transition for each states, i.e. SHUTDONW here and elsewhere also.  Though it's defined in the base one.  It would be easier to understand what each state may transit-to without having a look around first.

The transition list should be complete for each state, except SHUTDOWN is omitted for any other state could transition to SHUTDOWN. Is this what you are looking for?
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e892f4718b0
document the internal states of MDSM. r=kaku,kikuo
https://hg.mozilla.org/mozilla-central/rev/6e892f4718b0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.