Closed Bug 1073406 Opened 5 years ago Closed 5 years ago

Pausing a MediaElement pauses the underlying MediaStream globally

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
blocking-b2g 2.2+
Tracking Status
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Depends on 1 open bug)

Details

(Keywords: verifyme)

Attachments

(2 files, 7 obsolete files)

5.83 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.09 KB, patch
roc
: review+
Details | Diff | Splinter Review
This occurs on Firefox all the way from stable (32) up to nightly (35).

Steps to reproduce:

1. Go to https://appear.in/firefox-bug-repro?supersize with two different clients (we only test on one)
2. Wait until both clients can see the remote video stream
3. In one of the clients, click on the maximizing-arrows icon in the top right corner of the remote video stream

Expected result: Layout changes to "supersize" and both the local and remote streams are playing video and audio

Actual result: Layout changes to "supersize" but neither the local or remote streams are playing video or audio
On some occurrences, the streams have been observed to "come to life" again

---

What happens in appear.in's supersize mode, and what triggers this bug, is that the HTMLMediaElement showing the stream is removed from the DOM and then re-added.

So there are two HTMLMediaElement instances holding on to the same stream. One is not in the DOM and is paused. The other is in the DOM and is not paused. Result: stream is paused (or in MediaStreamGraph terms, explicitly blocked).

---

On removal of old first media element, the underlying stream is paused: http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#2515

On adding the new media element, the underlying stream is eventually set up with the element (don't remember the exact path right now): http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#2832,
which seems to assume that the underlying stream is not blocked (though it is, in our case).

Sometimes the stream will appear again; this is because the old HTMLMediaElement gets cycle collected so EndSrcMediaStreamPlayback is called, unpausing the stream, here: http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#444
Assignee: nobody → pehrsons
If we stop playback on the media element as soon it is removed from the DOM, the issue is fixed. Not sure if there are additional side effects or tests that need patching.
Attachment #8495838 - Flags: review?(cpearce)
Comment on attachment 8495838 [details] [diff] [review]
0001-Bug-1073406-End-source-stream-playback-immediately-w.patch

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

Media stream stuff should be reviewed by Roc.
Attachment #8495838 - Flags: review?(cpearce) → review?(roc)
Comment on attachment 8495838 [details] [diff] [review]
0001-Bug-1073406-End-source-stream-playback-immediately-w.patch

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +2519,5 @@
>      Pause();
> +  if (mSrcStream) {
> +    // End playback of mSrcStream so other HTMLMediaElements can play back the same stream.
> +    EndSrcMediaStreamPlayback();
> +  }

Calling play() on the element should be able to resume it, and this patch will prevent that.
Attachment #8495838 - Flags: review?(roc) → review-
You can work around this by calling play() on the element just after you've removed it from the document.

Is there a reason you're not reinserting the same element and calling play() on it? That would be more efficient than creating a new element.

I'm not really sure how pause() should behave when called on a media element playing a MediaStream. It's probably not specified anywhere, though our current behavior is probably wrong.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (on vacation Sep 27 to Oct 12) from comment #3)
> Calling play() on the element should be able to resume it, and this patch
> will prevent that.

Even after it's been removed from the DOM, been automatically paused and have become eligible for garbage collection?
Flags: needinfo?(roc)
I think there is no way to play() it if it is eligible for GC since nobody has reference to it.
Right. Also removing it from the DOM doesn't mean that there's no reference to it anymore, it just happens to be in my case.

I'll try to work out a patch that handles pause/play per media element, independently from the mediastream.
Flags: needinfo?(roc)
Removing a media element from the document doesn't mean the browser is going to destroy it. Media elements not in documents can be manipulated just fine. They can even play audio.
Summary: Removing and re-adding a media element to the DOM leaves the underlying media stream paused → Pausing a MediaElement pauses the underlying MediaStream globally
Simpler reproduction:

1. Open https://dl.dropboxusercontent.com/u/3005667/test.html
2. Right click on one video element and pause it

Expected result: the other video element keeps playing audio and video
Actual result: both video elements are paused
This patch is a bit more elaborate. It changes the behavior of pausing a HTMLMediaElement's mSrcStream from blocking the stream, to removing the outputs from the stream.

I ran mochitests for content/media and they passed on my mac. Are there other tests I should give a go as well?
Attachment #8495838 - Attachment is obsolete: true
Attachment #8499534 - Flags: review?(roc)
Comment on attachment 8499534 [details] [diff] [review]
0001-Bug-1073406-Pause-stream-backed-HTMLMediaElements-in.patch

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

This seems OK I guess.

::: content/html/content/src/HTMLMediaElement.cpp
@@ -3325,5 @@
> -  // container.
> -  if (mReadyState >= nsIDOMHTMLMediaElement::HAVE_METADATA &&
> -      mMediaSize == nsIntSize(-1, -1)) {
> -    return nullptr;
> -  }

Why did you remove this?
Attachment #8499534 - Flags: review?(roc)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #10)
> I ran mochitests for content/media and they passed on my mac. Are there
> other tests I should give a go as well?

https://wiki.mozilla.org/ReleaseEngineering/TryServer#Try_Server
We have Try server to ensure the patch runs evenly well on all platforms.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (on vacation Sep 27 to Oct 12) from comment #11)
> Comment on attachment 8499534 [details] [diff] [review]
> 0001-Bug-1073406-Pause-stream-backed-HTMLMediaElements-in.patch
> 
> Review of attachment 8499534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems OK I guess.
> 
> ::: content/html/content/src/HTMLMediaElement.cpp
> @@ -3325,5 @@
> > -  // container.
> > -  if (mReadyState >= nsIDOMHTMLMediaElement::HAVE_METADATA &&
> > -      mMediaSize == nsIntSize(-1, -1)) {
> > -    return nullptr;
> > -  }
> 
> Why did you remove this?

With this patch we'd add mVideoFrameContainer as a video output on play(). With autoplay, play() would only occur after (mReadyState >= HAVE_CURRENT_DATA) is true (mReadyState >= HAVE_METADATA when bug 879717 lands), but at that point we can't add mVideoFrameContainer as video output because we'd be stuck with this nullptr.

I'm assuming you cleared the r? by mistake, I'll anyhow take it up again when we've landed bug 879717. Will need some rebasing.
(In reply to JW Wang [:jwwang] from comment #12)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #10)
> > I ran mochitests for content/media and they passed on my mac. Are there
> > other tests I should give a go as well?
> 
> https://wiki.mozilla.org/ReleaseEngineering/TryServer#Try_Server
> We have Try server to ensure the patch runs evenly well on all platforms.

Sure, I know. Just curious to where the relevant tests live :)
http://trychooser.pub.build.mozilla.org/
Most of them reside in mochitests(all).
ex: try: -b do -p all -u mochitests -t none
Reworked somewhat and rebased on top of bug 879717 due to some overlap.

jwwang, do you mind to take a look?
Attachment #8499534 - Attachment is obsolete: true
Attachment #8513380 - Flags: feedback?(jwwang)
try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2556f88c0566

The red ones are due to a too large log after my extra just-in-case logging patch.
Comment on attachment 8513380 [details] [diff] [review]
Part 1. Pause stream-backed HTMLMediaElements individually

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

Sorry, I am not clear about what the right stream behavior should be. But it seems easier to tell the output stream not to block the input stream in MediaDecoder::ConnectDecodedStreamToOutputStream.
Attachment #8513380 - Flags: feedback?(jwwang)
Comment on attachment 8513381 [details] [diff] [review]
Part 2. Add mochitest for individual pausing

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

I think the test case can be reduced without drawing to canvas. All you need to do is:
1. pause v1
2. ensure 'pause' event not fired on v2
Attachment #8513381 - Flags: feedback?(jwwang)
(In reply to JW Wang [:jwwang] from comment #20)
> Comment on attachment 8513381 [details] [diff] [review]
> Part 2. Add mochitest for individual pausing
> 
> Review of attachment 8513381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the test case can be reduced without drawing to canvas. All you need
> to do is:
> 1. pause v1
> 2. ensure 'pause' event not fired on v2

Current behavior is:
When v1 is paused, the underlying stream is blocked so v2 just gets NEXT_FRAME_UNAVAILABLE_BUFFERING. That doesn't fire a 'pause' event, so we need something more elaborate.
JW, here's an alternative solution inspired by your comment 19.

It's certainly a more elegant patch, but I'm unsure of the performance impact on MediaStreamGraph when adding an extra layer of streams like this. What do you think?
Attachment #8514884 - Flags: feedback?(jwwang)
Comment on attachment 8514884 [details] [diff] [review]
Part 1 alt. Add playback stream as output to mSrcStream for individual media element blocking

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

Does this really work? I see the VideoFrameContainer is connected to the original stream instead of the one you create.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +2857,5 @@
>  
>    mSrcStream = aStream;
>  
> +  nsIDOMWindow* window = OwnerDoc()->GetInnerWindow();
> +  NS_ASSERTION(window, "Should have a window");

HTMLMediaElement::CaptureStreamInternal returns when window is null. We should be consistent.

@@ +2871,5 @@
> +  nsRefPtr<nsIPrincipal> principal = GetCurrentPrincipal();
> +  mPlaybackStream->CombineWithPrincipal(principal);
> +
> +  // Let |mSrcStream| decide when the stream has finished.
> +  GetSrcMediaStream()->AsProcessedStream()->SetAutofinish(true);

Why SetAutofinish(true)? Isn't it decided by the captured media element?
Attachment #8514884 - Flags: feedback?(jwwang)
(In reply to JW Wang [:jwwang] from comment #24)
> Comment on attachment 8514884 [details] [diff] [review]
> Part 1 alt. Add playback stream as output to mSrcStream for individual media
> element blocking
> 
> Review of attachment 8514884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this really work? I see the VideoFrameContainer is connected to the
> original stream instead of the one you create.

Yes :)
Though it reveals some new intermittents.

The VideoFrameContainer is connected to the new stream in SetupSrcMediaStreamPlayback():
>  VideoFrameContainer* container = >GetVideoFrameContainer();
>
>  if (container) {
>    GetSrcMediaStream()->AddVideoOutput(container); // GetSrcMediaStream returns the new stream
>  }

> ::: content/html/content/src/HTMLMediaElement.cpp
> @@ +2857,5 @@
> >  
> >    mSrcStream = aStream;
> >  
> > +  nsIDOMWindow* window = OwnerDoc()->GetInnerWindow();
> > +  NS_ASSERTION(window, "Should have a window");
> 
> HTMLMediaElement::CaptureStreamInternal returns when window is null. We
> should be consistent.
Yep, this we'd need to fix.

> @@ +2871,5 @@
> > +  nsRefPtr<nsIPrincipal> principal = GetCurrentPrincipal();
> > +  mPlaybackStream->CombineWithPrincipal(principal);
> > +
> > +  // Let |mSrcStream| decide when the stream has finished.
> > +  GetSrcMediaStream()->AsProcessedStream()->SetAutofinish(true);
> 
> Why SetAutofinish(true)? Isn't it decided by the captured media element?

Yes, but mSrcStream handles that logic so if it never finishes mPlaybackStream will neither.
Comment on attachment 8514884 [details] [diff] [review]
Part 1 alt. Add playback stream as output to mSrcStream for individual media element blocking

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +2871,5 @@
> +  nsRefPtr<nsIPrincipal> principal = GetCurrentPrincipal();
> +  mPlaybackStream->CombineWithPrincipal(principal);
> +
> +  // Let |mSrcStream| decide when the stream has finished.
> +  GetSrcMediaStream()->AsProcessedStream()->SetAutofinish(true);

I see. It makes sense to me. However, you would probably like to hear from roc about what the expected behavior should be.
Attachment #8514884 - Flags: feedback+
Minor change for commit msg and to not have audio in the gum stream used for the test. Just for noise annoyance..
Attachment #8513381 - Attachment is obsolete: true
Attachment #8532429 - Flags: review?(roc)
roc, any preference between this approach and the first one where we pause by removing audio and video outputs?
Attachment #8514884 - Attachment is obsolete: true
Attachment #8532432 - Flags: review?(roc)
Comment on attachment 8532432 [details] [diff] [review]
Part 2. Add playback stream as output to mSrcStream for individual media element blocking

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

This is the better option. Thanks!!!
Attachment #8532432 - Flags: review?(roc) → review+
Attachment #8513380 - Attachment is obsolete: true
I had an intermittent while testing if there are any blockers remaining on this bug: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4efe3373d3b2

So here's an update to the test that gets rid of the setTimeout(). Instead we use "timeupdate" to drive the progression check of video2. Plus relying on failing by timing out in case the video frame for video2 does not get updated. (which fails as expected without part 2)
Attachment #8532429 - Attachment is obsolete: true
Attachment #8537165 - Flags: review?(roc)
Comment on attachment 8537165 [details] [diff] [review]
Part 1. Add mochitest for individual pausing

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

great!
Attachment #8537165 - Flags: review?(roc) → review+
Tests are not all done yet but they seem to be doing well. Please check in when they have finished.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2125e03c40fb
https://hg.mozilla.org/mozilla-central/rev/dbdbade50ffa
https://hg.mozilla.org/mozilla-central/rev/1b11866ce6dc
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can we please backout this patch as this is causing smoketests failure ?
Flags: needinfo?(pehrsons)
:roc/Ryan, I spoke offline with pehrsons and he does not have the commit access to perform the backout, can either of you help, so we avoid failing the smoketest 2 days in a row. Thanks!
Flags: needinfo?(ryanvm)
Flags: needinfo?(roc)
FYI I have a fix ready if it passes try and gets r+ in time. See bug 1124139.
Flags: needinfo?(pehrsons)
Fix is on inbound.
Flags: needinfo?(ryanvm)
Flags: needinfo?(roc)
Aaand the fix was merged.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
The patch is for b2g v2.2. It added a change from Bug 1123950 to remove dependency to bug 1129263.
Depends on: 1129263
No longer depends on: 1129263
Blocks: 1162639
Attachment #8605254 - Attachment is obsolete: true
blocking-b2g: --- → 2.2?
add verifyme flag for v2.2+
Keywords: verifyme
Triage: blocking. Hi Sotaro, can you request approval for approval‑mozilla‑b2g37? Thanks.
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(sotaro.ikeda.g)
(In reply to howie [:howie] from comment #44)
> Triage: blocking. Hi Sotaro, can you request approval for
> approval‑mozilla‑b2g37? Thanks.
Flags: needinfo?(pehrsons)
Comment on attachment 8537165 [details] [diff] [review]
Part 1. Add mochitest for individual pausing

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): MediaStream playback
User impact if declined: Media elements playing a MediaStream can stall with no/little chance of recovering playback. (sometimes until the page plays another media element, sometimes until the garbage collector runs)
Testing completed: On m-c with dedicated mochitest for 4 months.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: None

This approval request applies to all patches on this bug.
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(pehrsons)
Attachment #8537165 - Flags: approval-mozilla-b2g37?
Attachment #8537165 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Approving and requesting QA verifyme after patch landed on 2.2
See Also: → 1083354
You need to log in before you can comment on or make changes to this bug.