1427162 - getLocalStreams changed behaviour with transceivers

Status: RESOLVED WONTFIX

(Core :: WebRTC: Signaling, enhancement, P3)

Description

[Philipp Hancke [:fippo]]

Looking at adapter.js test failures for FF59 I noticed that the removeTrack ones changed behaviour. Some, such as making assertions about the number of senders after removeTrack are wrong but this one seems worrying:
    https://jsfiddle.net/dvy57ptc/1/
In FF57 this returns an empty array. In FF59 this returns the stream even though all tracks associated with the stream have been removed. It does that even when initially using addStream.

Comment 1

[Nils Ohlmeier [:drno]]

I guess it's an edge case in that I hope nobody write code which counts on the fact that the stream is empty after removing all streams (leaving aside special test cases for shims). But on the other hand returning an empty stream also appears pretty useless to me.

Byron, what do you think about this?

Comment 2

[Philipp Hancke [:fippo]]

this made me wonder if we have a spec issue, depending on how this is implemented internally. I know chrome does this (lacking spec) iterating over the [[AssociatedMediaStreams]]

http://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-addtrack
adds to the [[AssociatedMediaStreams]]

but http://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-removetrack
does not set them to an empty list.

Arguably when getting the list of streams senders with null tracks should not be considered.

filed a spec issue https://github.com/w3c/webrtc-pc/issues/1756

Comment 3

[Byron Campen [:bwc]]

(In reply to Nils Ohlmeier [:drno] from comment #1)
> I guess it's an edge case in that I hope nobody write code which counts on
> the fact that the stream is empty after removing all streams (leaving aside
> special test cases for shims). But on the other hand returning an empty
> stream also appears pretty useless to me.
> 
> Byron, what do you think about this?

So this API hasn't been in the spec for some time, but we can implement the old behavior I guess. Couldn't hurt.

Comment 4

[Byron Campen [:bwc]]

jib, what's your take here?

Comment 5

[Philipp Hancke [:fippo]]

pasting from IRC:
< jib> bwc, fippo: yes that should not be fixed
< jib> See https://github.com/w3c/webrtc-pc/issues/1756#issuecomment-362373359
< jib> removeTrack does not remove streams

I agree with the spec argument. However, getLocalStreams is non-spec anyway and I think this is an easy fix.

I suspect the definition of getLocalStreams needs to be something along these lines:
1) let senders be the result of getSenders() -- senders = pc.getSenders()
2) let streams be the set of the senders associated media streams -- streams = senders.map(s => s.associatedMediaStreams)
3) let activeTracks be the set of tracks transmitted -- activeTracks = senders.filter(s => s.track).map(s => s.track)
3) let streamsWithActiveTracks be the set of streams which have at least one track in activeTracks

The current implementation does (1) and (2). I've updated https://jsfiddle.net/dvy57ptc/2/ to show steps (3) and (4).

btw, I was wrong about the stream being empty. It contains both tracks. Which makes sense since that stream still has two tracks even if they are no longer sent via the peerconnection.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

To echo my github comment, streams are associated with transceivers, not tracks, and this association is not affected by removeTrack.

Doesn't seem different from making assertions about the number of senders after removeTrack.

Barring some revelation at the February interim I'd say this works as expected.

It might be good to add telemetry to find out how many sites use getLocalStreams() so we can decide if we can deprecate it.

Comment 7

[Philipp Hancke [:fippo]]

nobody else complained so I assume nobody ran into this. Lets close this :-)