1299074 - Remove MediaDecoderStateMachine::mDecodingFirstFrame

Status: RESOLVED FIXED

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

Description

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

mSentFirstFrameLoadedEvent is sufficient to tell us if we need to decode first frames. |mSentFirstFrameLoadedEvent==true| means we have decoded first frames to have the start time and can begin seeking if necessary.

Comment 1

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

Attachment: Bug 1299074. Part 1 - fix the comment of mSentFirstFrameLoadedEvent because it is not necessarily related to dormant state.

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

Comment 2

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

Attachment: Bug 1299074. Part 2 - simplify the handling of pending seek in StartDecoding().

mSentFirstFrameLoadedEvent is sufficient to tell us whether we can handle the pending seek now or later.

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

Comment 3

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

Attachment: Bug 1299074. Part 3 - check |mSentFirstFrameLoadedEvent| to know whether we can finish decoding first frames.

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

Comment 4

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

Attachment: Bug 1299074. Part 4 - replace checks for IsDecodingFirstFrame() with !mSentFirstFrameLoadedEvent.

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

Comment 5

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

Attachment: Bug 1299074. Part 5 - remove unused members.

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

Comment 6

[Tzuhao Kuo [:kaku]]

Comment on attachment 8786627 [details]
Bug 1299074. Part 1 - fix the comment of mSentFirstFrameLoadedEvent because it is not necessarily related to dormant state.

https://reviewboard.mozilla.org/r/75556/#review73834

Comment 7

[Tzuhao Kuo [:kaku]]

Comment on attachment 8786628 [details]
Bug 1299074. Part 2 - simplify the handling of pending seek in StartDecoding().

https://reviewboard.mozilla.org/r/75558/#review73828

::: dom/media/MediaDecoderStateMachine.cpp:1263
(Diff revision 1)
>  MediaDecoderStateMachine::StartDecoding()
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    MOZ_ASSERT(mState == DECODER_STATE_DECODING);
>  
> -  if (mDecodingFirstFrame && mSentFirstFrameLoadedEvent) {
> +  // Handle the pending seek now if we've decoded first frames. Otherwise it

This was interesting since that mDecodingFirstFrame and mSentFirstFrameLoadedEvent can never be TURE at the same time.

Comment 8

[Tzuhao Kuo [:kaku]]

Comment on attachment 8786629 [details]
Bug 1299074. Part 3 - check |mSentFirstFrameLoadedEvent| to know whether we can finish decoding first frames.

https://reviewboard.mozilla.org/r/75560/#review73830

::: dom/media/MediaDecoderStateMachine.cpp:2171
(Diff revision 1)
>    // data requests.
>  
> -  if (mDecodingFirstFrame) {
> -    // We were resuming from dormant, or initiated a seek early.
> -    // We can fire loadeddata now.
> +  // Notify FirstFrameLoaded now if we haven't since we've decoded some data
> +  // for readyState to transition to HAVE_CURRENT_DATA and fire 'loadeddata'.
> +  if (!mSentFirstFrameLoadedEvent) {
> +    // Only MSE can start seeking before finishing decoding first frames.

But why do we allow it? Is there any case that MSE needs to seek before the 1st frame been decoded?

Comment 9

[Tzuhao Kuo [:kaku]]

Comment on attachment 8786629 [details]
Bug 1299074. Part 3 - check |mSentFirstFrameLoadedEvent| to know whether we can finish decoding first frames.

https://reviewboard.mozilla.org/r/75560/#review73840

Comment 10

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

Comment on attachment 8786628 [details]
Bug 1299074. Part 2 - simplify the handling of pending seek in StartDecoding().

https://reviewboard.mozilla.org/r/75558/#review73842

::: dom/media/MediaDecoderStateMachine.cpp:1263
(Diff revision 1)
>  MediaDecoderStateMachine::StartDecoding()
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    MOZ_ASSERT(mState == DECODER_STATE_DECODING);
>  
> -  if (mDecodingFirstFrame && mSentFirstFrameLoadedEvent) {
> +  // Handle the pending seek now if we've decoded first frames. Otherwise it

No, it is possible for both of them to be true. Consider:
1. Enter dormant state after decoding first frames. mSentFirstFrameLoadedEvent is true.
2. Exit dormant state and decode metadata again. mSentFirstFrameLoadedEvent is set to true in the entry action of DECODING_METADATA.

Comment 11

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

Comment on attachment 8786629 [details]
Bug 1299074. Part 3 - check |mSentFirstFrameLoadedEvent| to know whether we can finish decoding first frames.

https://reviewboard.mozilla.org/r/75560/#review73830

> But why do we allow it? Is there any case that MSE needs to seek before the 1st frame been decoded?

Yes, that is exactly bug 1185972.

Comment 12

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

Comment on attachment 8786628 [details]
Bug 1299074. Part 2 - simplify the handling of pending seek in StartDecoding().

https://reviewboard.mozilla.org/r/75558/#review73842

> No, it is possible for both of them to be true. Consider:
> 1. Enter dormant state after decoding first frames. mSentFirstFrameLoadedEvent is true.
> 2. Exit dormant state and decode metadata again. mSentFirstFrameLoadedEvent is set to true in the entry action of DECODING_METADATA.

I mean mDecodingFirstFrame is set to true in step 2.

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8786630 [details]
Bug 1299074. Part 4 - replace checks for IsDecodingFirstFrame() with !mSentFirstFrameLoadedEvent.

https://reviewboard.mozilla.org/r/75562/#review73850

::: dom/media/MediaDecoderStateMachine.cpp:742
(Diff revision 1)
>    FinishDecodeFirstFrame();
>    if (!mQueuedSeek.Exists()) {
>      return false;
>    }

*Not related to this bug.*
It's not logical here to return FALSE after invoking FinishDecodeFirstFrame(), right?

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8786631 [details]
Bug 1299074. Part 5 - remove unused members.

https://reviewboard.mozilla.org/r/75564/#review73852

Comment 15

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

Comment on attachment 8786628 [details]
Bug 1299074. Part 2 - simplify the handling of pending seek in StartDecoding().

https://reviewboard.mozilla.org/r/75558/#review73854

::: dom/media/MediaDecoderStateMachine.cpp:1263
(Diff revision 1)
>  MediaDecoderStateMachine::StartDecoding()
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    MOZ_ASSERT(mState == DECODER_STATE_DECODING);
>  
> -  if (mDecodingFirstFrame && mSentFirstFrameLoadedEvent) {
> +  // Handle the pending seek now if we've decoded first frames. Otherwise it

As you see, it is confusing to have both mDecodingFirstFrame and mSentFirstFrameLoadedEvent to be true at the same time. That is why I want to remove mDecodingFirstFrame in this bug.

Comment 16

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

Comment on attachment 8786630 [details]
Bug 1299074. Part 4 - replace checks for IsDecodingFirstFrame() with !mSentFirstFrameLoadedEvent.

https://reviewboard.mozilla.org/r/75562/#review73856

::: dom/media/MediaDecoderStateMachine.cpp:742
(Diff revision 1)
>    FinishDecodeFirstFrame();
>    if (!mQueuedSeek.Exists()) {
>      return false;
>    }

The return value of MaybeFinishDecodeFirstFrame() is kinda confusing. It returns true when the state transitions to SEEKING so the caller will not continue its flow for the stae is changed. See [1] for an example.

If it returns true after invoking FinishDecodeFirstFrame(), the caller at [1] will never stop prerolling and playback will never start.

[1] https://hg.mozilla.org/mozilla-central/file/tip/dom/media/MediaDecoderStateMachine.cpp#l589

Comment 17

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

Thanks for the reviews!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf568dfa5fd1
Part 1 - fix the comment of mSentFirstFrameLoadedEvent because it is not necessarily related to dormant state. r=kaku
https://hg.mozilla.org/integration/autoland/rev/0e2ee827b4be
Part 2 - simplify the handling of pending seek in StartDecoding(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/13ca0eb5671e
Part 3 - check |mSentFirstFrameLoadedEvent| to know whether we can finish decoding first frames. r=kaku
https://hg.mozilla.org/integration/autoland/rev/c454a29b2ede
Part 4 - replace checks for IsDecodingFirstFrame() with !mSentFirstFrameLoadedEvent. r=kaku
https://hg.mozilla.org/integration/autoland/rev/0dfbee74cdbb
Part 5 - remove unused members. r=kaku

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf568dfa5fd1
https://hg.mozilla.org/mozilla-central/rev/0e2ee827b4be
https://hg.mozilla.org/mozilla-central/rev/13ca0eb5671e
https://hg.mozilla.org/mozilla-central/rev/c454a29b2ede
https://hg.mozilla.org/mozilla-central/rev/0dfbee74cdbb

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8786629 [details]
Bug 1299074. Part 3 - check |mSentFirstFrameLoadedEvent| to know whether we can finish decoding first frames.

https://reviewboard.mozilla.org/r/75560/#review73830

> Yes, that is exactly bug 1185972.

It is part of the spec. the ability to seek is irrelevent to what frames has been decoded or not. you must be able to seek if readyState is >= HAVE_METADATA.
in fact we should be doing the same for all media, not just MSE, if we have managed to get all necessary metadata without having to decode the first frame (that is whenever we dont have to decode the first frame to calculate the start time)

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8786628 [details]
Bug 1299074. Part 2 - simplify the handling of pending seek in StartDecoding().

https://reviewboard.mozilla.org/r/75558/#review73828

> This was interesting since that mDecodingFirstFrame and mSentFirstFrameLoadedEvent can never be TURE at the same time.

That is not correct.. when resuming from dormant, mSentFirstFrameLoadedEvent would have been true and we used to go back to decoding the first frame state.

Comment 22

[Tzuhao Kuo [:kaku]]

Comment on attachment 8786628 [details]
Bug 1299074. Part 2 - simplify the handling of pending seek in StartDecoding().

https://reviewboard.mozilla.org/r/75558/#review73828

> That is not correct.. when resuming from dormant, mSentFirstFrameLoadedEvent would have been true and we used to go back to decoding the first frame state.

Thanks for correction!

Comment 23

[Tzuhao Kuo [:kaku]]

Comment on attachment 8786629 [details]
Bug 1299074. Part 3 - check |mSentFirstFrameLoadedEvent| to know whether we can finish decoding first frames.

https://reviewboard.mozilla.org/r/75560/#review73830

> It is part of the spec. the ability to seek is irrelevent to what frames has been decoded or not. you must be able to seek if readyState is >= HAVE_METADATA.
> in fact we should be doing the same for all media, not just MSE, if we have managed to get all necessary metadata without having to decode the first frame (that is whenever we dont have to decode the first frame to calculate the start time)

Oh! so, there is a chance to do it?

Comment 24

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

(In reply to Jean-Yves Avenard [:jya] from comment #20)
> in fact we should be doing the same for all media, not just MSE, if we have
> managed to get all necessary metadata without having to decode the first
> frame (that is whenever we dont have to decode the first frame to calculate
> the start time)

The separation of the demuxer from the decoder allows us to demux first frames to get the start times without actually decoding the first frames. If we don't mind breaking legacy readers, I can work out some patches to do that. This will also greatly simplify the code of MDSM. The benefit of maintaining the legacy readers doesn't seem worth it.