827007 - Regressions in MediaStream behavior from TrackUnion changes in bug 822956

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

New behavior perhaps related since bug 822956 landed:

MediaStreamGraph continues to run on the webrtc-landing pages after "Stop" is clicked, camera stays on (use "Video" button).  This means that video.stop() didn't work on the nsDOMLocalMediaStream. This appears never to end (and shouldn't if stop doesn't work, as we hold a reference to the stream).  

Reloading a page with an active MediaStream now takes ~10 seconds to actually stop capture and turn off the camera light.  (Bug 819867 was concerned with this, but when stop is called it isn't a problem).  When this happens, the graph finally stops and dies.

I don't *remember* seeing these problems with earlier versions of the TrackUnion changes (that used nsDOMLocalMediaStreams directly, instead of SourceMediaStreams).


See also Bug 826576 - crashes with null gGraph on shutdown (and in another test that starts/stops mediastreams quickly).  Also "Stream destroyed twice" assertions fire when reloading pages with MediaStreams.


Raising to blocker as we may need to temporarily back out part or all of the TrackUnion changes before uplift.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

I tried backing out the SourceMediaStream stuff, and the stop/etc issues still happen with the original TrackUnion change.  Probably because the stop doesn't propagate from the TrackUnion to the stream it's attached to.

I note (perhaps because of leaking the Port) the graph stops but doesn't appear to fully shut down, which may be why we weren't noticing problems with gGraph being NULL (bug 826576).  Haven't tested for the "already-destroyed" thing of the null graph yet.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Back out both trackunion patches

straight backout, so I'll take r+ from either.
Working to try to fix at least stop issues with the patches

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Proxy stop() command to ProcessedMediaStreams and their sources

Tested, and this appears to work - camera shuts off immediately, the MediaStreamGraph continues to run until the next GC (not unexpected) and then stops. (Tested using webrtc-landing GUM test page.)  Note there still may very well be issues with gGraph destruction (bug 826576), as this really doesn't address that issue, but that's rather hard to hit.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 698298 [details] [diff] [review]
Proxy stop() command to ProcessedMediaStreams and their sources

Actual patch is pretty clean.  Roc's r+ would be preferred, but I think this is a viable alternative to backing out even without review from roc.

Comment 5

[Timothy B. Terriberry (:derf)]

Comment on attachment 698298 [details] [diff] [review]
Proxy stop() command to ProcessedMediaStreams and their sources

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

LGTM

::: content/media/MediaStreamGraph.cpp
@@ +2145,5 @@
> +                     mSource, mSource ? mSource->AsSourceStream() : NULL));
> +  if (!mSource)
> +    return;
> +  SourceMediaStream *source = mSource->AsSourceStream();
> +  if (!source)

We should generalize this to proxy to other ProcessedMediaStreams, as well, so they can be chained.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Proxy stop() command to ProcessedMediaStreams and their sources

with update for layers ProcessedStreams

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 698405 [details] [diff] [review]
Proxy stop() command to ProcessedMediaStreams and their sources

carry forward r=derf (includes change asked for in review)

Comment 8

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

Propagating this up the graph through all ProcessedMediaStreams doesn't make sense to me.

Don't we just want nsDOMUserMediaStream to override stop() and stop its mSourceStream?

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

nsDOMLocalMediaStream is now wrapped around a TrackUnion, so I believe to get to the SourceStream it has to ripple it up through the input ports, or am I wrong?

Also, will we be able to build a composite MediaStream (from tracks, processed streams like WebAudio, etc) and call .stop() on it, and have it ripple up to the source, or do I have to know where the source is coming from and stop it there in JS?  What if a processing node hides which source it's using (like a switcher node)?  Etc.  Not sure these are practical concerns, just a few off the top of my head.

Comment 10

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

(In reply to Randell Jesup [:jesup] from comment #9)
> nsDOMLocalMediaStream is now wrapped around a TrackUnion, so I believe to
> get to the SourceStream it has to ripple it up through the input ports, or
> am I wrong?

I said "nsDOMUserMediaStream", which we just gave a direct reference to the SourceMediaStream!

> Also, will we be able to build a composite MediaStream (from tracks,
> processed streams like WebAudio, etc) and call .stop() on it, and have it
> ripple up to the source, or do I have to know where the source is coming
> from and stop it there in JS?  What if a processing node hides which source
> it's using (like a switcher node)?  Etc.  Not sure these are practical
> concerns, just a few off the top of my head.

Such a composite MediaStream isn't a LocalMediaStream and wouldn't have a stop() method, would it?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Implement Stop for UserMediaStreams; add NotifyRemoved for MediaStream listeners

Comment 12

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

Comment on attachment 698494 [details] [diff] [review]
Implement Stop for UserMediaStreams; add NotifyRemoved for MediaStream listeners

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

r+ except for the RemoveAllListeners() API. If we need that for something, please create a separate patch.

::: content/media/MediaStreamGraph.cpp
@@ +2002,5 @@
> +MediaStream::RemoveAllListeners()
> +{
> +  class Message : public ControlMessage {
> +  public:
> +    Message(MediaStream* aStream) :

I'm not sure why you added this API. Is there a use for it?

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f141d1e00ceb

Comment 15

[Jason Smith [:jsmith]]

I think I covered automating this as part of bug 822109's patch.