Closed Bug 1301675 Opened 3 years ago Closed 3 years ago

Allow stopping all MediaStreamTracks

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(15 files)

58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
58 bytes, text/x-review-board-request
jib
: review+
Details
Since implementing MediaStreamTrack.stop, the spec has changed from allowing only stopping of local tracks to all kinds of tracks.

We need to remove MediaStreamTrackSource::mRemote and add support for stopping all different kinds of MediaStreamTrackSources per the following


Canvas captureStream: can stop the capturing completely, also disconnect the track and stream so the canvas can be GCed even with the track or stream alive

Media captureStream: will keep producing new tracks so we'll probably do nothing

WebAudio MediaStreamDestination: can stop the capturing completely, also disconnect the track and stream so the node can be GCed even with the track or stream alive

RTCPeerConnection: A stopped track shouldn't affect the peer connection
Rank: 23
Priority: -- → P2
Assignee: nobody → pehrson
Status: NEW → ASSIGNED
Comment on attachment 8794267 [details]
Bug 1301675 - Implement AudioDestinationTrackSource.

https://reviewboard.mozilla.org/r/80784/#review79656
Attachment #8794267 - Flags: review?(padenot) → review+
Comment on attachment 8794268 [details]
Bug 1301675 - Test that a track from MediaStreamAudioDestinationNode can be stopped.

https://reviewboard.mozilla.org/r/80786/#review79658
Attachment #8794268 - Flags: review?(padenot) → review+
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #0)
> RTCPeerConnection: A stopped track shouldn't affect the peer connection

Actually, track.stop() has been overloaded to be how apps reject an offered track, both during offer-answer negotiation [1], and after [2].

[1] http://w3c.github.io/webrtc-pc/#processing-remote-mediastreamtracks
[2] http://w3c.github.io/webrtc-pc/#setting-negotiation-needed
Comment on attachment 8794259 [details]
Bug 1301675 - Remove MediaStreamTrackSource::mIsRemote.

https://reviewboard.mozilla.org/r/80768/#review81248
Attachment #8794259 - Flags: review?(jib) → review+
Comment on attachment 8794260 [details]
Bug 1301675 - Test stopping tracks from canvas captureStream.

https://reviewboard.mozilla.org/r/80770/#review81252

::: dom/canvas/test/test_capture.html:111
(Diff revision 1)
> +        ok(true, "Element " + elem.id + " ended.");
> +        resolve();
> +        elem.removeEventListener("ended", endedListener);
> +      }));
> +  });
> +  return Promise.all(promises);

promises could be inlined...
Attachment #8794260 - Flags: review?(jib) → review+
Comment on attachment 8794261 [details]
Bug 1301675 - Test stopping tracks from canvas (webgl) captureStream.

https://reviewboard.mozilla.org/r/80772/#review81254

::: dom/canvas/test/webgl-mochitest/test_capture.html:121
(Diff revision 1)
> +    return new Promise(resolve =>
> +      elem.addEventListener("ended", function endedListener(event) {
> +        ok(true, "Element " + elem.id + " ended.");
> +        resolve();
> +        elem.removeEventListener("ended", endedListener);
> +      }));

It'd be nice to move haveEvent() someplace more central, but where?
Attachment #8794261 - Flags: review?(jib) → review+
Comment on attachment 8794262 [details]
Bug 1301675 - Refactor canvas captureStream to be more clear on removing frame listeners.

https://reviewboard.mozilla.org/r/80774/#review81256
Attachment #8794262 - Flags: review?(jib) → review+
Comment on attachment 8794263 [details]
Bug 1301675 - Implement CanvasCaptureTrackSource that allows stopping canvas capture.

https://reviewboard.mozilla.org/r/80776/#review81262

::: dom/html/HTMLCanvasElement.cpp:688
(Diff revision 1)
> +NS_IMPL_ADDREF_INHERITED(CanvasCaptureTrackSource,
> +                         MediaStreamTrackSource)
> +NS_IMPL_RELEASE_INHERITED(CanvasCaptureTrackSource,
> +                          MediaStreamTrackSource)

Any reason not to use NS_IMPL_ISUPPORTS_INHERITED ?
Attachment #8794263 - Flags: review?(jib) → review+
Comment on attachment 8794264 [details]
Bug 1301675 - Clean up MediaStream Constructor test.

https://reviewboard.mozilla.org/r/80778/#review81264
Attachment #8794264 - Flags: review?(jib) → review+
Comment on attachment 8794265 [details]
Bug 1301675 - Implement StreamCaptureTrackSource::Stop.

https://reviewboard.mozilla.org/r/80780/#review81266

::: dom/html/HTMLMediaElement.cpp:1422
(Diff revision 1)
> +    if (!ms.mCapturingMediaStream) {
> +      continue;
> +    }
> +
> +    if (ms.mStream != aOwningStream) {
> +      continue;
> +    }

combine?

::: dom/html/HTMLMediaElement.cpp:1430
(Diff revision 1)
> +    for (int32_t i = ms.mTrackPorts.Length() - 1; i >= 0; --i) {
> +      MediaInputPort* port = ms.mTrackPorts[i].second();
> +      if (port->GetDestinationTrackId() != aDestinationTrackID) {
> +        continue;
> +      }
> +
> +      port->Destroy();
> +      ms.mTrackPorts.RemoveElementAt(i);
> +      return;

Any reason this needs to be iterated in reverse?

::: dom/html/HTMLMediaElement.cpp:2266
(Diff revision 1)
> -  explicit StreamCaptureTrackSource(MediaStreamTrackSource* aCapturedTrackSource)
> +  StreamCaptureTrackSource(HTMLMediaElement* aElement,
> +                           MediaStreamTrackSource* aCapturedTrackSource,
> +                           DOMMediaStream* aOwningStream,
> +                           TrackID aDestinationTrackID)

Why implicit?
Attachment #8794265 - Flags: review?(jib) → review+
Comment on attachment 8794266 [details]
Bug 1301675 - Clarify why we don't need to do anything on DecoderCaptureTrackSource::Stop().

https://reviewboard.mozilla.org/r/80782/#review81270
Attachment #8794266 - Flags: review?(jib) → review+
Comment on attachment 8794269 [details]
Bug 1301675 - Received tracks from RTCPeerConnection are stoppable.

https://reviewboard.mozilla.org/r/80788/#review81272

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h:177
(Diff revision 1)
>    ApplyConstraints(nsPIDOMWindowInner* aWindow,
>                     const dom::MediaTrackConstraints& aConstraints) override;
>  
>    void Stop() override
>    {
> -    // XXX Fix in later patch.
> +    // No need to do anything when stopping a remote track.

See comment 18. Update code comment if nothing else.
Attachment #8794269 - Flags: review?(jib) → review-
Comment on attachment 8794270 [details]
Bug 1301675 - Rename BasicUnstoppableTrackSource to BasicTrackSource.

https://reviewboard.mozilla.org/r/80790/#review81276
Attachment #8794270 - Flags: review?(jib) → review+
Comment on attachment 8794271 [details]
Bug 1301675 - Fix track cloning test.

https://reviewboard.mozilla.org/r/80792/#review81278
Attachment #8794271 - Flags: review?(jib) → review+
Comment on attachment 8794272 [details]
Bug 1301675 - Uncomment assertion for when removed live track was not found.

https://reviewboard.mozilla.org/r/80794/#review81280

::: dom/html/HTMLMediaElement.cpp:4307
(Diff revision 1)
> -    // XXX (bug 1208328) Uncomment this when DOMMediaStream doesn't call
> -    // NotifyTrackRemoved multiple times for the same track, i.e., when it
> +    NS_ASSERTION(aTrack->AsVideoStreamTrack() && !IsVideo(),
> +                 "MediaStreamTrack ended but did not exist in track lists");

Please explain this assertion.
Attachment #8794272 - Flags: review?(jib) → review+
Comment on attachment 8794273 [details]
Bug 1301675 - Assert that a MediaStreamTrackSource is not stopped more than once.

https://reviewboard.mozilla.org/r/80796/#review81284
Attachment #8794273 - Flags: review?(jib) → review+
Comment on attachment 8794263 [details]
Bug 1301675 - Implement CanvasCaptureTrackSource that allows stopping canvas capture.

https://reviewboard.mozilla.org/r/80776/#review81262

> Any reason not to use NS_IMPL_ISUPPORTS_INHERITED ?

I don't know these macros enough. But it seems to me like NS_IMPL_ISUPPORTS_INHERITED and friends don't give us the NS_INTERFACE macro that we need. We need NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED. And they can't be combined.
Comment on attachment 8794265 [details]
Bug 1301675 - Implement StreamCaptureTrackSource::Stop.

https://reviewboard.mozilla.org/r/80780/#review81266

> combine?

I find it cleaner to separate the guards. Especially when you want to add log statements to see which of them are guarding the call off - even if it's just a temporary local debug statement.

> Any reason this needs to be iterated in reverse?

`ms.mTrackPorts.RemoveElementAt(i)` wouldn't work if done in a regular loop.

> Why implicit?

Explicit only comes into effect when there's exactly one argument to the constructor, no?
Comment on attachment 8794265 [details]
Bug 1301675 - Implement StreamCaptureTrackSource::Stop.

https://reviewboard.mozilla.org/r/80780/#review81266

> Explicit only comes into effect when there's exactly one argument to the constructor, no?

Yeah, D'oh!
(In reply to Jan-Ivar Bruaroey [:jib] from comment #28)
> Comment on attachment 8794269 [details]
> Bug 1301675 - Received tracks from RTCPeerConnection are stoppable.
> 
> https://reviewboard.mozilla.org/r/80788/#review81272
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h:177
> (Diff revision 1)
> >    ApplyConstraints(nsPIDOMWindowInner* aWindow,
> >                     const dom::MediaTrackConstraints& aConstraints) override;
> >  
> >    void Stop() override
> >    {
> > -    // XXX Fix in later patch.
> > +    // No need to do anything when stopping a remote track.
> 
> See comment 18. Update code comment if nothing else.

I can't tell from the links in comment 18 and 19 what is supposed to happen when a received track is stopped. Can you point this out in the spec, or should we wait until it becomes more clear? It's a recent decision I take it.
Flags: needinfo?(jib)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #36)
> I can't tell from the links in comment 18 and 19 what is supposed to happen
> when a received track is stopped. Can you point this out in the spec, or
> should we wait until it becomes more clear? It's a recent decision I take it.

It's not that recent, but not particularly clear either. From running this fiddle [1] in Firefox you can see that ontrack events fire during setRemoteDescription. From reading earlier versions of the spec [2] it seems the intent is for JS to call track.stop() from within an ontrack callback to "reject" that track, which I think has an effect on the SDP negotiation at that point, or at least allocated bandwidth.

Comment 19 suggests more generally that track.stop() on a remote track even after negotiation causes it at least to stop playback. It says "Since receiver.track.stop() does not implicitly stop receiver, Receiver Reports continue to be sent.", so that may be it. I see no mention of this setting the negotiationneeded flag...

Except the second link in comment 18 suggests that stopping a track sets negotiationneeded, but it doesn't say whether this is sender side, receiver side or both.

CCing Byron to see if he knows.

Looking at Chrome for clues: Chrome doesn't have ontrack yet, so when I run the fiddle [1] in Chrome with adapter.js, you'll note they fire after setRemoteDescription (this is possibly caused by adapter's ontrack polyfill), so nothing happens to the SDP AFAICT.

Note that the fiddle calls track.stop() on the video, and in Chrome, this causes the remote video to not appear. Stopping audio instead didn't work though, so we may have some time here.

[1] https://jsfiddle.net/c2vwpuqg/
[1] https://github.com/w3c/webrtc-pc/commit/092504a8276dee977c4710666b3203f0d853736b
Flags: needinfo?(jib) → needinfo?(docfaraday)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #37)
> Comment 19 suggests more generally that track.stop() on a remote track even
> after negotiation causes it at least to stop playback. It says "Since
> receiver.track.stop() does not implicitly stop receiver, Receiver Reports
> continue to be sent.", so that may be it. I see no mention of this setting
> the negotiationneeded flag...

This is what I thought would happen, and was agreed upon at some point. That the tracks stops but we don't have to tell anyone.

Most of the discussion seems to be here: https://github.com/w3c/webrtc-pc/issues/597

I don't see anyone mentioning track.stop() having a different effect if called during negotiation.
I can't resolve the ambiguity here, I'm afraid.
Flags: needinfo?(docfaraday)
Blocks: 1258143
I raised an issue with the spec at https://github.com/w3c/webrtc-pc/issues/898.
See Also: → 1314270
Comment on attachment 8794269 [details]
Bug 1301675 - Received tracks from RTCPeerConnection are stoppable.

https://reviewboard.mozilla.org/r/80788/#review89272
Attachment #8794269 - Flags: review?(jib) → review+
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7414623c1788
Remove MediaStreamTrackSource::mIsRemote. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/6000b9799462
Test stopping tracks from canvas captureStream. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/d06f0960937f
Test stopping tracks from canvas (webgl) captureStream. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/39cdaa7a776e
Refactor canvas captureStream to be more clear on removing frame listeners. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d21ef28d04
Implement CanvasCaptureTrackSource that allows stopping canvas capture. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f28fe5a7449
Clean up MediaStream Constructor test. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/b01b14b70c58
Implement StreamCaptureTrackSource::Stop. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31e0a89ecce
Clarify why we don't need to do anything on DecoderCaptureTrackSource::Stop(). r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/945dfdabefaa
Implement AudioDestinationTrackSource. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/f64665a7edc7
Test that a track from MediaStreamAudioDestinationNode can be stopped. r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/b55b83b03e87
Received tracks from RTCPeerConnection are stoppable. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e5dc6155ced
Rename BasicUnstoppableTrackSource to BasicTrackSource. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7dfea97b90
Fix track cloning test. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ffc03664bf
Uncomment assertion for when removed live track was not found. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e63884e4c09
Assert that a MediaStreamTrackSource is not stopped more than once. r=jib
Depends on: 1321235
No longer depends on: 1321235
I’ve updated the documentation for MediaStreamTrack.stop(), hopefully accurately. I would appreciate a quick look at it to be sure it’s accurate.

The following pages have changed:

https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/stop

With a mention on Firefox 52 for developers.
Thanks! I took a look and made some fixes, though I'm not sure if they affected your changes or older text.
Depends on: 1329075
You need to log in before you can comment on or make changes to this bug.