1115505 - Need to keep decoding to ensure the stream is initialized in the decode-to-stream case

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Found this bug when debugging bug 1114434 which changes the algorithm of calculating MediaQueue::Duration().

With the change in bug 1114434, MediaDecoderStateMachine::GetDecodedAudioDuration would return a larger value and cause HaveEnoughDecodedAudio() to return true to stop decoding. If decoding stops before the stream is initialized (stream->mStreamInitialized set to true in SendStreamData()), the stream will never be able to start playing since it is not initialized.

We should keep decoding until the stream is initialized in SendStreamData().

Comment 1

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

Attachment: 1115505_keep_decoding_until_stream_initialized.patch

keep decoding to ensure the stream is initialized in the decode-to-stream case.

Comment 2

[Benjamin Chen [:bechen]]

Comment on attachment 8541442 [details] [diff] [review]
1115505_keep_decoding_until_stream_initialized.patch

Review of attachment 8541442 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +556,5 @@
>    DecodedStreamData* stream = mDecoder->GetDecodedStream();
> +
> +  if (stream && !stream->mStreamInitialized) {
> +    return false;
> +  }

Is it possible that previous condition |if (static_cast<uint32_t>(VideoQueue().GetSize()) < mAmpleVideoFrames * mPlaybackRate) | is not true (want to return true), but your patch return false here?

Because on B2G platform, if the VideoQueue().GetSize() is larger than a constant, the video HW decoder won't decode any  frames until we consume some frames.

Comment 3

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

Comment on attachment 8541442 [details] [diff] [review]
1115505_keep_decoding_until_stream_initialized.patch

Review of attachment 8541442 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +556,5 @@
>    DecodedStreamData* stream = mDecoder->GetDecodedStream();
> +
> +  if (stream && !stream->mStreamInitialized) {
> +    return false;
> +  }

Why is that a concern? We will start to consume frames as long as playback starts.

Comment 4

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

(In reply to JW Wang [:jwwang] from comment #1)
> Created attachment 8541442 [details] [diff] [review]
> 1115505_keep_decoding_until_stream_initialized.patch
> 
> keep decoding to ensure the stream is initialized in the decode-to-stream
> case.

In the case of decode-to-stream, it is possible the 1st audio sample is large enough to stop the state machine from continuing decoding (HaveEnoughDecodedAudio returns true). Then the stream won't be initialized, tracks won't be added and playback can't be started.

Hi roc,
Can you review this patch? Thanks.

Comment 5

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

try: https://tbpl.mozilla.org/?tree=Try&rev=3d36fe757850

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74895ad1425a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74895ad1425a

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8541442 [details] [diff] [review]
1115505_keep_decoding_until_stream_initialized.patch

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: This affects non-MSE playback, but is small and straightforward.
[String/UUID change made/needed]: None.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d3d7478ffa4