1297590 - Put DECODER_STATE_DECODING_FIRSTFRAME back to MDSM

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

Moving the job of decoding first frames out of DECODER_STATE_DECODING into its own state, it would be easier and simpler to define transitions between states.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1297590. Part 1 - add DECODER_STATE_DECODING_FIRSTFRAME to MDSM.

When separating decoding frist frames from the DECODING state, we need
to change the test `mState == DECODER_STATE_DECODING` to
`mState == DECODER_STATE_DECODING || mState == DECODER_STATE_DECODING_FIRSTFRAME`.
However, we don't make changes for those where mSentFirstFrameLoadedEvent is
proven to be true. Because there is no way for mState to be DECODING_FIRSTFRAME
when mSentFirstFrameLoadedEvent is true.

Review commit: https://reviewboard.mozilla.org/r/76632/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76632/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1297590. Part 2 - change test `mState != DECODER_STATE_DECODING` to `mState != DECODER_STATE_DECODING && mState != DECODER_STATE_DECODING_FIRSTFRAME`.

Review commit: https://reviewboard.mozilla.org/r/76634/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76634/

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1297590. Part 3 - change test `mState >= DECODER_STATE_DECODING && mSentFirstFrameLoadedEvent` to `mState >= DECODER_STATE_DECODING`.

Review commit: https://reviewboard.mozilla.org/r/76636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76636/

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1297590. Part 4 - change test `mState >= DECODER_STATE_DECODING` to `mState >= DECODER_STATE_DECODING_FIRSTFRAME`.

Review commit: https://reviewboard.mozilla.org/r/76638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76638/

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1297590. Part 5 - change some `SetState(DECODER_STATE_DECODING)` to `SetState(DECODER_STATE_DECODING_FIRSTFRAME)`.

MDSM used to transition to DECODING in the following places:
1. BUFFERING
2. OnMetadataRead()
3. WAIT_FOR_CDM
4. SeekCompleted()

We will transition to DECODING_FIRSTFRAME in case 2 and 3.

For case 1, BUFFERING makes sense only after decoding first frames. So BUFFERING
should never transition to DECODING_FIRSTFRAME.

For case 4, we always finish decoding first frames before completing seek. So
It should not transition to DECODING_FIRSTFRAME either.

Review commit: https://reviewboard.mozilla.org/r/76640/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76640/

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1297590. Part 6 - Move MaybeFinishDecodeFirstFrame() from case DECODER_STATE_DECODING to case DECODER_STATE_DECODING_FIRSTFRAME.

Review commit: https://reviewboard.mozilla.org/r/76642/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76642/

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1297590. Part 7 - remove the call to MaybeFinishDecodeFirstFrame() from OnNotDecoded().

It is impossible to finish decoding first frames while waiting for data.

Review commit: https://reviewboard.mozilla.org/r/76644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76644/

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1297590. Part 8 - add an entry action for DECODER_STATE_DECODING_FIRSTFRAME.

We move the handling of pending seek from the entry action of DECODING to that of DECODING_FIRSTFRAME.

Review commit: https://reviewboard.mozilla.org/r/76646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76646/

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1297590. Part 9 - fix MaybeFinishDecodeFirstFrame().

1. The return value is not used.
2. It should be called only when mSentFirstFrameLoadedEvent is false.
3. Transition to SEEK if there is any pending seek or DECODER_STATE_DECODING otherwise.

Review commit: https://reviewboard.mozilla.org/r/76648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76648/

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1297590. Part 10 - remove some mSentFirstFrameLoadedEvent checks for it is surely true when mState is DECODING.

Review commit: https://reviewboard.mozilla.org/r/76650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76650/

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788060 [details]
Bug 1297590. Part 1 - add DECODER_STATE_DECODING_FIRSTFRAME to MDSM.

https://reviewboard.mozilla.org/r/76632/#review74752

::: dom/media/MediaDecoderStateMachine.cpp:833
(Diff revision 1)
>  
>  bool
>  MediaDecoderStateMachine::CheckIfDecodeComplete()
>  {
>    MOZ_ASSERT(OnTaskQueue());
> +  // DecodeComplete is possible only after decoding first frames.

Why plural type for the "frames" here? I expect for a singular type; also apply to the other comments in this patch.

Comment 12

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788061 [details]
Bug 1297590. Part 2 - change test `mState != DECODER_STATE_DECODING` to `mState != DECODER_STATE_DECODING && mState != DECODER_STATE_DECODING_FIRSTFRAME`.

https://reviewboard.mozilla.org/r/76634/#review74754

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788062 [details]
Bug 1297590. Part 3 - change test `mState >= DECODER_STATE_DECODING && mSentFirstFrameLoadedEvent` to `mState >= DECODER_STATE_DECODING`.

https://reviewboard.mozilla.org/r/76636/#review74738

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788063 [details]
Bug 1297590. Part 4 - change test `mState >= DECODER_STATE_DECODING` to `mState >= DECODER_STATE_DECODING_FIRSTFRAME`.

https://reviewboard.mozilla.org/r/76638/#review74758

Comment 15

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788064 [details]
Bug 1297590. Part 5 - change some `SetState(DECODER_STATE_DECODING)` to `SetState(DECODER_STATE_DECODING_FIRSTFRAME)`.

https://reviewboard.mozilla.org/r/76640/#review74760

Comment 16

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788065 [details]
Bug 1297590. Part 6 - Move MaybeFinishDecodeFirstFrame() from case DECODER_STATE_DECODING to case DECODER_STATE_DECODING_FIRSTFRAME.

https://reviewboard.mozilla.org/r/76642/#review74762

Comment 17

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788066 [details]
Bug 1297590. Part 7 - remove the call to MaybeFinishDecodeFirstFrame() from OnNotDecoded().

https://reviewboard.mozilla.org/r/76644/#review74742

Comment 18

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788067 [details]
Bug 1297590. Part 8 - add an entry action for DECODER_STATE_DECODING_FIRSTFRAME.

https://reviewboard.mozilla.org/r/76646/#review74764

Comment 19

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788068 [details]
Bug 1297590. Part 9 - fix MaybeFinishDecodeFirstFrame().

https://reviewboard.mozilla.org/r/76648/#review74750

Comment 20

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788069 [details]
Bug 1297590. Part 10 - remove some mSentFirstFrameLoadedEvent checks for it is surely true when mState is DECODING.

https://reviewboard.mozilla.org/r/76650/#review74766

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8788060 [details]
Bug 1297590. Part 1 - add DECODER_STATE_DECODING_FIRSTFRAME to MDSM.

https://reviewboard.mozilla.org/r/76632/#review74752

> Why plural type for the "frames" here? I expect for a singular type; also apply to the other comments in this patch.

Each track has its own first frame. Since we have both audi and video tracks, we have 2 "first frames" from each track.

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce1129cc4eff
Part 1 - add DECODER_STATE_DECODING_FIRSTFRAME to MDSM. r=kaku
https://hg.mozilla.org/integration/autoland/rev/cd526ec1c208
Part 2 - change test `mState != DECODER_STATE_DECODING` to `mState != DECODER_STATE_DECODING && mState != DECODER_STATE_DECODING_FIRSTFRAME`. r=kaku
https://hg.mozilla.org/integration/autoland/rev/9b2913a5a496
Part 3 - change test `mState >= DECODER_STATE_DECODING && mSentFirstFrameLoadedEvent` to `mState >= DECODER_STATE_DECODING`. r=kaku
https://hg.mozilla.org/integration/autoland/rev/2ffa21b6fb98
Part 4 - change test `mState >= DECODER_STATE_DECODING` to `mState >= DECODER_STATE_DECODING_FIRSTFRAME`. r=kaku
https://hg.mozilla.org/integration/autoland/rev/8a478b60c1f9
Part 5 - change some `SetState(DECODER_STATE_DECODING)` to `SetState(DECODER_STATE_DECODING_FIRSTFRAME)`. r=kaku
https://hg.mozilla.org/integration/autoland/rev/6b05ec872e63
Part 6 - Move MaybeFinishDecodeFirstFrame() from case DECODER_STATE_DECODING to case DECODER_STATE_DECODING_FIRSTFRAME. r=kaku
https://hg.mozilla.org/integration/autoland/rev/1aafc799929a
Part 7 - remove the call to MaybeFinishDecodeFirstFrame() from OnNotDecoded(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/41ed9b0c1661
Part 8 - add an entry action for DECODER_STATE_DECODING_FIRSTFRAME. r=kaku
https://hg.mozilla.org/integration/autoland/rev/13b4910c4880
Part 9 - fix MaybeFinishDecodeFirstFrame(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/fcbf7d45fbe4
Part 10 - remove some mSentFirstFrameLoadedEvent checks for it is surely true when mState is DECODING. r=kaku

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ce1129cc4eff
https://hg.mozilla.org/mozilla-central/rev/cd526ec1c208
https://hg.mozilla.org/mozilla-central/rev/9b2913a5a496
https://hg.mozilla.org/mozilla-central/rev/2ffa21b6fb98
https://hg.mozilla.org/mozilla-central/rev/8a478b60c1f9
https://hg.mozilla.org/mozilla-central/rev/6b05ec872e63
https://hg.mozilla.org/mozilla-central/rev/1aafc799929a
https://hg.mozilla.org/mozilla-central/rev/41ed9b0c1661
https://hg.mozilla.org/mozilla-central/rev/13b4910c4880
https://hg.mozilla.org/mozilla-central/rev/fcbf7d45fbe4