1128411 - Refactor MediaDecoderStateMachine::SendStreamData()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

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

Following bug 1127171, this is the 2nd step to refactor the stream-capture code in MediaDecoderStateMachine.

Comment 2

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

try: https://tbpl.mozilla.org/?tree=Try&rev=77c8f23cfee2

Comment 3

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

Attachment: 1128411_part1_fix_some_bugs-v1.patch

Part 1 - fix some bugs in MediaDecoderStateMachine::SendStreamData().

Comment 4

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

Comment on attachment 8557897 [details] [diff] [review]
1128411_part1_fix_some_bugs-v1.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ -337,2 @@
>      AudioSegment silence;
> -    silence.InsertNullDataAtStart(frameOffset.value() - audioWrittenOffset.value());

Stream time is required instead of frame count.

@@ -347,5 @@
> -    // We're starting in the middle of a packet. Split the packet.
> -    offset = audioWrittenOffset.value() - frameOffset.value();
> -  } else {
> -    // Write the entire packet.
> -    offset = 0;

This doesn't handle overlap in adjacent frames.

@@ +415,5 @@
>          AudioSegment* audio = new AudioSegment();
>          mediaStream->AddAudioTrack(kAudioTrack, mInfo.mAudio.mRate, 0, audio);
>          stream->mStream->DispatchWhenNotEnoughBuffered(kAudioTrack,
>              GetStateMachineThread(), GetWakeDecoderRunnable());
> +        stream->mNextAudioTime = mStartTime + stream->mInitialTime;

mNext{Audio,Video}Time needs to take mStartTime into account.

Comment 5

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

Attachment: 1128411_part2_call_SendStreamData_in_AdvanceFrame_only-v1.patch

Part 2 - call SendStreamData() in AdvanceFrame() only to simplify the code of SendStreamData().

Comment 6

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

Comment on attachment 8557899 [details] [diff] [review]
1128411_part2_call_SendStreamData_in_AdvanceFrame_only-v1.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +379,5 @@
>  }
>  
>  void MediaDecoderStateMachine::SendStreamData()
>  {
> +  MOZ_ASSERT(OnStateMachineThread(), "Should be on state machine thread");

This function is now called in AdvanceFrame() which is called in the state machine thread.

@@ +384,2 @@
>    AssertCurrentThreadInMonitor();
> +  MOZ_ASSERT(!mAudioSink, "Should've been stopped in CallRunStateMachine()");

StopAudioThread() won't return until audio thread is shut down. We can assert mAudioSink is null here.

@@ -387,3 @@
>  
>    DecodedStreamData* stream = mDecoder->GetDecodedStream();
> -  if (!stream) {

We can get rid of these checks for this function is called in AdvanceFrame() only when mAudioCaptured is true.

@@ -572,5 @@
> -  // audio/video samples are received, we need to keep decoding to ensure
> -  // the stream is initialized.
> -  if (stream && !stream->mStreamInitialized) {
> -    return false;
> -  }

We can get rid of this workaround since AdvanceFrame() will call SendStreamData() to initialize the stream even if the 1st sample is large enough to stop decoding.

@@ -596,5 @@
>    DecodedStreamData* stream = mDecoder->GetDecodedStream();
>  
> -  if (stream && !stream->mStreamInitialized) {
> -    return false;
> -  }

Ditto.

Comment 7

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

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=77c8f23cfee2

The oranges in B2G ICS Emulator Debug is not about capture stream.

Comment 8

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

Hi Ryan,

It looks like the inbound tree was closed and caused some error in my push. Only part 2 get into the tree without part 1. Can you back out the patch? Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d28c88e9705d

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://wiki.mozilla.org/Sheriffing/How:To:Backouts

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb703d85677

Comment 11

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

I see. Thanks.

Comment 12

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

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/123ecd63f082
https://hg.mozilla.org/integration/mozilla-inbound/rev/975698b131d8

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/123ecd63f082
https://hg.mozilla.org/mozilla-central/rev/975698b131d8

Comment 14

[Ralph Giles (:rillian)]

JW Wang, any idea why this causes orange with a media recorder test on aurora? I'd like to uplift this as a blocking dependency. Applied to aurora this causes a crash on try debug builds with dom/media/test/test_bug1113600.html. I can't reproduce the crash locally, but do get a leak report.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=030a49f0899e

Comment 15

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

Since this bug depends on bug 1127171, maybe you also need to uplift bug 1127171. Any particular reason to uplift this bug which is not about MSE to Aurora?

Comment 16

[Ralph Giles (:rillian)]

Thanks. I'll give bug 1127171 a try.

> Any particular reason to uplift this bug which is not about MSE to Aurora?

This touches the same code as later MSE patches, so if it looks like a backport won't cause problems I like to nominate patches like this as well. Significantly reduces the work to backport further changes, and reduces code variance between the branches which helps with testing.

Do you anticipate any issues from uplifting this?

Comment 17

[Ralph Giles (:rillian)]

Comment on attachment 8557899 [details] [diff] [review]
1128411_part2_call_SendStreamData_in_AdvanceFrame_only-v1.patch

Requesting 37 uplift of both patches as a dependency for later MSE fixes.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, harder to backport MSE changes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This affects general media playback, but has had a week to settle on nightly. Risk looks low.
[String/UUID change made/needed]: None.

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8557899 [details] [diff] [review]
1128411_part2_call_SendStreamData_in_AdvanceFrame_only-v1.patch

Taking this MSE related refactoring, which has already been on m-c for a couple of weeks. Aurora+

Comment 19

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d89186268fb4
https://hg.mozilla.org/releases/mozilla-aurora/rev/636a4a911276

Comment 20

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

(In reply to Ralph Giles (:rillian) from comment #16)
> This touches the same code as later MSE patches, so if it looks like a
> backport won't cause problems I like to nominate patches like this as well.
> Significantly reduces the work to backport further changes, and reduces code
> variance between the branches which helps with testing.
I see the reason. Thanks for the explanation.

> Do you anticipate any issues from uplifting this?
Since it works fine on Central, I think Aurora should also be fine.