Closed Bug 1127171 Opened 5 years ago Closed 5 years ago

Simplify code of stream capture for media element

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Comment on attachment 8556243 [details] [diff] [review]
1127171_make_capture_stream_atomic-v1.patch

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

Refactor stream capture code for media element. This is the 1st step to sort out the mess in clock switching (system/audio/stream clock) code in the state machine.

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7a8e351618f

::: dom/media/MediaDecoder.cpp
@@ -173,5 @@
> -{
> -  MOZ_ASSERT(NS_IsMainThread());
> -  mInitialAudioCaptured = aCaptured;
> -  if (mDecoderStateMachine) {
> -    mDecoderStateMachine->SetAudioCaptured(aCaptured);

This is moved AddOutputStream().

@@ +365,5 @@
> +    }
> +    if (!GetDecodedStream()) {
> +      int64_t t = mDecoderStateMachine ?
> +                  mDecoderStateMachine->GetCurrentTimeUs() : 0;
> +      RecreateDecodedStream(t);

By putting MediaDecoderStateMachine::SetAudioCaptured() and RecreateDecodedStream() in the same lock, we make it easier to handle clock switching for the state machine. Before this, there is a latency between |mAudioCaptured| being set in the state machine and the decoded stream being created in MediaDeocder which prevents the state machine from being able to switch to stream clock (calling GetCurrentTimeViaMediaStreamSync() in GetClock()) immediately.

@@ +556,5 @@
>  {
>    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
>    mDecoderStateMachine->SetDuration(mDuration);
>    mDecoderStateMachine->SetVolume(mInitialVolume);
> +  if (GetDecodedStream()) {

Now we can check |GetDecodedStream()| to know if we are in capture mode and tell the state machine if necessary.

::: dom/media/MediaDecoderStateMachine.cpp
@@ -1366,5 @@
>      mAudioSink->SetVolume(mVolume);
>    }
>  }
>  
> -void MediaDecoderStateMachine::SetAudioCaptured(bool aCaptured)

Remove the aCaptured paramenter to make this API less confusing since there is no way to restore from capture status.

@@ +1771,5 @@
>    mSeekTarget = SeekTarget(seekTime, aTarget.mType);
>  
>    DECODER_LOG("Changed state to SEEKING (to %lld)", mSeekTarget.mTime);
>    SetState(DECODER_STATE_SEEKING);
> +  if (mAudioCaptured) {

Now |mAudioCaptured| is always in sycn with |mDecoder->GetDecodedStream()|. We can check |mAudioCaptured| to know if we are in capture mode and tell |mDecoder| to re-create the decoded stream if necessary. We are also able to remove MediaDecoder::RecreateDecodedStreamIfNecessary().
Attachment #8556243 - Flags: review?(roc)
Blocks: 1127235
Comment on attachment 8556243 [details] [diff] [review]
1127171_make_capture_stream_atomic-v1.patch

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

beautiful!
Attachment #8556243 - Flags: review?(roc) → review+
Keywords: checkin-needed
Please run this through Try first (or provide a link if you already did). Thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7954ef8f0ba
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Blocks: 1128411
Comment on attachment 8556243 [details] [diff] [review]
1127171_make_capture_stream_atomic-v1.patch

Requesting 37 uplift as a dependency of later MSE fixes.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, harder to backport MSE fixes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This does affect general media playback, but has had a while to settle on nightly. I think risk is low. It's mostly refactoring, and more locking is usually safer.
[String/UUID change made/needed]: None.
Attachment #8556243 - Flags: approval-mozilla-aurora?
Comment on attachment 8556243 [details] [diff] [review]
1127171_make_capture_stream_atomic-v1.patch

Taking this MSE prereq. As noted, the changes have been on Nightly for a couple of weeks already so I don't expect much in the way of fallout. Aurora+
Attachment #8556243 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.