842455 - Call addStream & removeStream with valid MediaStream, then call addStream again with that same stream - NS_ERROR_FAILURE fires, should work with no issues

Status: RESOLVED WONTFIX

(Core :: WebRTC, defect)

Description

[Jason Smith [:jsmith]]

Steps:

1. Load the attached test case
2. Accept permissions for your camera

Expected:

We should be able to call addStream and removeStream in two cycles on the same MediaStream with no errors.

Actual:

After calling addStream & removeStream once with a media stream, calling addStream again with that media stream results in a NS_ERROR_FAILURE firing. We should have been able to successfully add the stream in this case - we are just adding a stream we recently removed from the peer connection.

Comment 1

[Jason Smith [:jsmith]]

Attachment: Testcase

Comment 2

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

Though odd, it's probably an edge case we should handle in some way.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #2)
> Though odd, it's probably an edge case we should handle in some way.

Uhh...I wouldn't call this an edge case. This is a simple add and remove workflow for a stream. I don't think I agree this wouldn't block.

Comment 4

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

Well, I didn't think addStream(a); removeStream(a); addStream(a); was an expected real-world usage.  Why would an application do that?  I agree it's valid, but as I said odd.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #4)
> Well, I didn't think addStream(a); removeStream(a); addStream(a); was an
> expected real-world usage.  Why would an application do that?  I agree it's
> valid, but as I said odd.

They start a call with someone remote, cancel it, and make the call again while having the camera active locally.

Comment 6

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

Ok, that's a different case than is tested here - that involves a number of stable states, and different PeerConnections, and should work.  Even if the same PeerConnection (impossible if you call stop() I believe), you'd have stable states you don't have in this test.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #6)
> Ok, that's a different case than is tested here - that involves a number of
> stable states, and different PeerConnections, and should work.  Even if the
> same PeerConnection (impossible if you call stop() I believe), you'd have
> stable states you don't have in this test.

Fair enough. If I find a simpler real world scenario that demonstrates this better, I'll renom.

Comment 8

[Jason Smith [:jsmith]]

Back to nom. Something weird is going on here upon further investigation - it doesn't just happen in the case of reusing the same stream - it appears that removeStream may be leaving "crumbs" behind that is causing the NS_ERROR_FAILURE to occur if you call addStream again with *any* stream - new or different. I just tried a case where I did something along the lines of the following:

1. Create the PC object
2. Call addStream with stream 1
3. Call removeStream with stream 1
4. Call addStream with stream 2

We get NS_ERROR_FAILURE here. Even weirder, if you call removeStream multiple times after #3, we don't get an error. Doing some other combos shows that there is most definitely something weird going on with addStream. It's almost as if removeStream isn't actually removing the stream...

Comment 9

[Jason Smith [:jsmith]]

Explained in meeting and now agree this doesn't block.

Will file a different bug though for considering to temporarily unexpose the removeStream.

Comment 11

[Simon Eisenmann]

How can this not block - please explain. I tried to implement renegotiation support for Firefox in a WebRTC application and remove stream is required whenever something else should be sent through the existing peer connection. 

Eg: Audio/video stream is sent, and now i want to disable the video and continue with audio only. We remove the audio/video (using removeStream, which should trigger renegotiation), and then add back a new stream with audio only (which again will trigger renegotiation).

This is kind of disappointing as i was under the impression that Firefox 38 would be ready to do renegotiation. 

Please let me know what you think.

Comment 12

[Eric Rescorla (:ekr)]

Uh, the last time this bug was updated was in 2013.

Have you tried it with Firefox 38?

Comment 13

[Simon Eisenmann]

Yes. FF 38, 39 and 40 all raise NotSupportedError: "removeStream not yet implemented", when calling PeerConnection.removeStream(oldStream)

May be i need to do something different, but the whole process works fine in Chrome.

Comment 14

[Eric Rescorla (:ekr)]

It looks to me like what happened is that we implemented the new API(removeTrack).
Based on a quick glance at the test cases, this seems like it should work though
I haven't tested it.

http://w3c.github.io/mediacapture-main/#dom-mediastream-removetrack

You'll notice that the API no longer contains removeStream at all, so you should
update your code to use removeTrack and re-test.

Comment 15

[Simon Eisenmann]

(In reply to Eric Rescorla (:ekr) from comment #14)
> It looks to me like what happened is that we implemented the new
> API(removeTrack).
> Based on a quick glance at the test cases, this seems like it should work
> though
> I haven't tested it.
> 
> http://w3c.github.io/mediacapture-main/#dom-mediastream-removetrack
> 
> You'll notice that the API no longer contains removeStream at all, so you
> should
> update your code to use removeTrack and re-test.

Ah that might just the solution. Thanks! Then i second the idea of unexposing removeStream from the API as suggested in comment #9. Thanks!

Comment 16

[Simon Eisenmann]

(In reply to Simon Eisenmann from comment #15)
> (In reply to Eric Rescorla (:ekr) from comment #14)
> > It looks to me like what happened is that we implemented the new
> > API(removeTrack).
> > Based on a quick glance at the test cases, this seems like it should work
> > though
> > I haven't tested it.
> > 
> > http://w3c.github.io/mediacapture-main/#dom-mediastream-removetrack
> > 
> > You'll notice that the API no longer contains removeStream at all, so you
> > should
> > update your code to use removeTrack and re-test.
> 
> Ah that might just the solution. Thanks! Then i second the idea of
> unexposing removeStream from the API as suggested in comment #9. Thanks!

Btw are you suggesting that removeTrack is a replacement for removeStream? I think they are different. For my use case i might just be fine with removeTrack, but in general removeStream should be implemented too eventually.

The docs suggest that removeStream is implemented in Firefox (https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/removeStream) which is obviously not the case.

Comment 17

[Eric Rescorla (:ekr)]

(In reply to Simon Eisenmann from comment #16)
> (In reply to Simon Eisenmann from comment #15)
> > (In reply to Eric Rescorla (:ekr) from comment #14)
> > > It looks to me like what happened is that we implemented the new
> > > API(removeTrack).
> > > Based on a quick glance at the test cases, this seems like it should work
> > > though
> > > I haven't tested it.
> > > 
> > > http://w3c.github.io/mediacapture-main/#dom-mediastream-removetrack
> > > 
> > > You'll notice that the API no longer contains removeStream at all, so you
> > > should
> > > update your code to use removeTrack and re-test.
> > 
> > Ah that might just the solution. Thanks! Then i second the idea of
> > unexposing removeStream from the API as suggested in comment #9. Thanks!
> 
> Btw are you suggesting that removeTrack is a replacement for removeStream? I
> think they are different. For my use case i might just be fine with
> removeTrack, but in general removeStream should be implemented too
> eventually.

The API is moving away from having any stream add/remove methods.


> The docs suggest that removeStream is implemented in Firefox
> (https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/
> removeStream) which is obviously not the case.

Comment 18

[Byron Campen [:bwc]]

Yes, we've only implemented removeTrack, although we have not removed removeStream yet. That's easy enough to do.

Comment 19

[Dmitry]

(In reply to Eric Rescorla (:ekr) from comment #17)
> (In reply to Simon Eisenmann from comment #16)
> > (In reply to Simon Eisenmann from comment #15)
> > > (In reply to Eric Rescorla (:ekr) from comment #14)
> > > > It looks to me like what happened is that we implemented the new
> > > > API(removeTrack).
> > > > Based on a quick glance at the test cases, this seems like it should work
> > > > though
> > > > I haven't tested it.
> > > > 
> > > > http://w3c.github.io/mediacapture-main/#dom-mediastream-removetrack
> > > > 
> > > > You'll notice that the API no longer contains removeStream at all, so you
> > > > should
> > > > update your code to use removeTrack and re-test.
> > > 
> > > Ah that might just the solution. Thanks! Then i second the idea of
> > > unexposing removeStream from the API as suggested in comment #9. Thanks!
> > 
> > Btw are you suggesting that removeTrack is a replacement for removeStream? I
> > think they are different. For my use case i might just be fine with
> > removeTrack, but in general removeStream should be implemented too
> > eventually.
> 
> The API is moving away from having any stream add/remove methods.
> 

Where did you get that? http://www.w3.org/TR/webrtc/#widl-RTCPeerConnection-removeStream-void-MediaStream-stream

Comment 20

[Eric Rescorla (:ekr)]

Look at the editor's draft I linked to above.

Comment 21

[Dmitry]

(In reply to Eric Rescorla (:ekr) from comment #20)
> Look at the editor's draft I linked to above.

You mean this one? http://w3c.github.io/mediacapture-main/#dom-mediastream-removetrack

Comment 22

[Eric Rescorla (:ekr)]

(In reply to Dmitry from comment #21)
> (In reply to Eric Rescorla (:ekr) from comment #20)
> > Look at the editor's draft I linked to above.
> 
> You mean this one?
> http://w3c.github.io/mediacapture-main/#dom-mediastream-removetrack

Yes

Comment 23

[Dmitry]

(In reply to Eric Rescorla (:ekr) from comment #22)
> (In reply to Dmitry from comment #21)
> > (In reply to Eric Rescorla (:ekr) from comment #20)
> > > Look at the editor's draft I linked to above.
> > 
> > You mean this one?
> > http://w3c.github.io/mediacapture-main/#dom-mediastream-removetrack
> 
> Yes

You are right. So you do not plan to implement pc.removeStream() anymore?

Comment 24

[Eric Rescorla (:ekr)]

We will eventually remove it.

Comment 25

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

removeStream has been removed...