Regressions in MediaStream behavior from TrackUnion changes in bug 822956

VERIFIED FIXED in mozilla20

Status

()

Core
WebRTC: Audio/Video
P1
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jesup, Assigned: roc)

Tracking

17 Branch
mozilla20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [getUserMedia][blocking-gum+])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
Created attachment 698286 [details] [diff] [review]
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
Attachment #698286 - Flags: review?(tterribe)
Attachment #698286 - Flags: review?(roc)
Attachment #698286 - Flags: review?(tterribe) → review+
(Reporter)

Comment 3

5 years ago
Created attachment 698298 [details] [diff] [review]
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.
(Reporter)

Comment 4

5 years ago
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.
Attachment #698298 - Flags: review?(tterribe)
Attachment #698298 - Flags: review?(roc)
(Reporter)

Updated

5 years ago
Severity: blocker → critical
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.
Attachment #698298 - Flags: review?(tterribe) → review+
Whiteboard: [getUserMedia][blocking-gum+]
(Reporter)

Comment 6

5 years ago
Created attachment 698405 [details] [diff] [review]
Proxy stop() command to ProcessedMediaStreams and their sources

with update for layers ProcessedStreams
(Reporter)

Updated

5 years ago
Attachment #698298 - Attachment is obsolete: true
Attachment #698298 - Flags: review?(roc)
(Reporter)

Updated

5 years ago
Blocks: 826576
(Reporter)

Comment 7

5 years ago
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)
Attachment #698405 - Flags: review+
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?
(Reporter)

Comment 9

5 years ago
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.
(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?
(Reporter)

Comment 11

5 years ago
Created attachment 698494 [details] [diff] [review]
Implement Stop for UserMediaStreams; add NotifyRemoved for MediaStream listeners
(Reporter)

Updated

5 years ago
Attachment #698494 - Flags: review?(roc)
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?
Attachment #698494 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/f141d1e00ceb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Attachment #698286 - Attachment is obsolete: true
Attachment #698286 - Flags: review?(roc)

Updated

5 years ago
Keywords: verifyme

Updated

5 years ago
Flags: in-testsuite?
I think I covered automating this as part of bug 822109's patch.
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.