1073406 - Pausing a MediaElement pauses the underlying MediaStream globally

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Andreas Pehrson [:pehrsons]]

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

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: 0001-Bug-1073406-End-source-stream-playback-immediately-w.patch

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.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[Andreas Pehrson [:pehrsons]]

(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?

Comment 6

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

I think there is no way to play() it if it is eligible for GC since nobody has reference to it.

Comment 7

[Andreas Pehrson [:pehrsons]]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Andreas Pehrson [:pehrsons]]

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

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: 0001-Bug-1073406-Pause-stream-backed-HTMLMediaElements-in.patch

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?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 12

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

(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.

Comment 13

[Andreas Pehrson [:pehrsons]]

(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.

Comment 14

[Andreas Pehrson [:pehrsons]]

(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 :)

Comment 15

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

http://trychooser.pub.build.mozilla.org/
Most of them reside in mochitests(all).
ex: try: -b do -p all -u mochitests -t none

Comment 16

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1. Pause stream-backed HTMLMediaElements individually

Reworked somewhat and rebased on top of bug 879717 due to some overlap.

jwwang, do you mind to take a look?

Comment 17

[Andreas Pehrson [:pehrsons]]

Attachment: Part 2. Add mochitest for individual pausing

Comment 18

[Andreas Pehrson [:pehrsons]]

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 19

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

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.

Comment 20

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

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

Comment 21

[Andreas Pehrson [:pehrsons]]

(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.

Comment 22

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1 alt. Add playback stream as output to mSrcStream for individual media element blocking

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?

Comment 23

[Andreas Pehrson [:pehrsons]]

Here's a try of this patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cd2b6e1532bd

Looks like a lot more hits on bug 1078017.

Comment 24

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

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?

Comment 25

[Andreas Pehrson [:pehrsons]]

(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 26

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

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.

Comment 27

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1. Add mochitest for individual pausing

Minor change for commit msg and to not have audio in the gum stream used for the test. Just for noise annoyance..

Comment 28

[Andreas Pehrson [:pehrsons]]

Attachment: Part 2. Add playback stream as output to mSrcStream for individual media element blocking

roc, any preference between this approach and the first one where we pause by removing audio and video outputs?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!!!

Comment 30

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1. Add mochitest for individual pausing

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)

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8537165 [details] [diff] [review]
Part 1. Add mochitest for individual pausing

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

great!

Comment 32

[Andreas Pehrson [:pehrsons]]

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

Comment 33

[Andreas Pehrson [:pehrsons]]

Fixing bug 879717 first.

Comment 34

[Andreas Pehrson [:pehrsons]]

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

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbdbade50ffa
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b11866ce6dc

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dbdbade50ffa
https://hg.mozilla.org/mozilla-central/rev/1b11866ce6dc

Comment 37

[bhavana bajaj [:bajaj]]

Can we please backout this patch as this is causing smoketests failure ?

Comment 38

[bhavana bajaj [:bajaj]]

: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!

Comment 39

[Andreas Pehrson [:pehrsons]]

FYI I have a fix ready if it passes try and gets r+ in time. See bug 1124139.

Comment 40

[Andreas Pehrson [:pehrsons]]

Fix is on inbound.

Comment 41

[Andreas Pehrson [:pehrsons]]

Aaand the fix was merged.

Comment 42

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2g v2.2 - Part 2. Add playback stream as output to mSrcStream for individual media element blocking

The patch is for b2g v2.2. It added a change from Bug 1123950 to remove dependency to bug 1129263.

Comment 43

[Bobby Chien]

add verifyme flag for v2.2+

Comment 44

[howie [:howie]]

Triage: blocking. Hi Sotaro, can you request approval for approval‑mozilla‑b2g37? Thanks.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

(In reply to howie [:howie] from comment #44)
> Triage: blocking. Hi Sotaro, can you request approval for
> approval‑mozilla‑b2g37? Thanks.

Comment 46

[Andreas Pehrson [: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.

Comment 47

[Josh Cheng [:josh]]

Approving and requesting QA verifyme after patch landed on 2.2

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/02e68a230eec
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ebe9798dea2c