Closed Bug 1115505 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

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().
keep decoding to ensure the stream is initialized in the decode-to-stream case.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Blocks: 1114434
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 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.
(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.
Attachment #8541442 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/74895ad1425a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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.
Attachment #8541442 - Flags: approval-mozilla-beta?
Attachment #8541442 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.