Closed Bug 1299074 Opened 4 years ago Closed 4 years ago

Remove MediaDecoderStateMachine::mDecodingFirstFrame

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(5 files)

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.
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Attachment #8786627 - Flags: review?(kaku)
Attachment #8786628 - Flags: review?(kaku)
Attachment #8786629 - Flags: review?(kaku)
Attachment #8786630 - Flags: review?(kaku)
Attachment #8786631 - Flags: review?(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
Attachment #8786627 - Flags: review?(kaku) → review+
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.
Attachment #8786628 - Flags: review?(kaku) → review+
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 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
Attachment #8786629 - Flags: review?(kaku) → review+
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 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 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 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?
Attachment #8786630 - Flags: review?(kaku) → review+
Comment on attachment 8786631 [details]
Bug 1299074. Part 5 - remove unused members.

https://reviewboard.mozilla.org/r/75564/#review73852
Attachment #8786631 - Flags: review?(kaku) → review+
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 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
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
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 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 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 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 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?
(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.
You need to log in before you can comment on or make changes to this bug.