Iñaki Baz Castillo Reporter
	Description • 10 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36

Steps to reproduce:

1) Firefox sends a audio+video offer to the peer who replies a
audio+video answer.

2) Firefox calls to  SetRemoteDescription().

3) The SetRemoteDescriptionSuccessCallback fires. Within it, I check
the number of remote tracks in the PeerConnection and get:
  - 1 remote audio track.
  - 0 remote video tracks.

4) Later the onaddstream fires. I inspect the number of tracks in the
given stream and get:
  - 1 remote audio track.
  - 1 remote video track.

5) After 1 second I inspect again the tracks in the PeerConnection and get:
  - 1 remote audio track.
  - 1 remote video track.


Actual results:

The problem is that in step 3) I get 1 audio track and 0 video tracks.


Expected results:

In step 3) I should get 1 audio track and 1 video track (as the SDP answer clearly contains).

	Iñaki Baz Castillo Reporter
	Comment 1 • 10 years ago

BTW I've found a similar (but not the same at all) issue: https://bugzilla.mozilla.org/show_bug.cgi?id=873888

In that issue the problem is that onaddstream fires when all the stream tracks are not yet available. My report is basically the same but applied to the PeerConnection.

	Iñaki Baz Castillo Reporter
	Comment 2 • 10 years ago

This issue still happens in Firefox Nightly 33.0a1 (2014-06-22). I've realized that related bug #873888 is marked as fixed. May be such a bug is just related to remote streams when "onaddstream" event fires, but current one is about remote streams in PeerConnection once the SetRemoteDescriptionSuccessCallback fires.

This is important because not all the WebRTC apps rely on the "onaddstream" event, but we should also be able to inspect remote streams in the PeerConnection once the remote description is set and its success callback fired.

	Iñaki Baz Castillo Reporter
	Comment 3 • 10 years ago

Guys, any feedback or comments please? IMHO the bug is important enough, really.

	Eric Rescorla (:ekr)
	Comment 4 • 10 years ago

Please see:
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Especially point #2.

Yes, this is a bug, no we don't have a fix yet. If you want to
contribute a patch, that would be great.

	Nils Ohlmeier [:drno]
	Comment 5 • 10 years ago

I assume that waiting for the onaddstream callback actually works as a workaround for your problem, right?

As you pointed out yourself this problem very likely depends on Bug 998546. As this comment https://bugzilla.mozilla.org/show_bug.cgi?id=998546#c1 suggests it is unfortunately significant work to make all the required changes. But 998546 was scheduled for 33 already so it is on peoples radars.

	Iñaki Baz Castillo Reporter
	Comment 6 • 10 years ago

> I assume that waiting for the onaddstream callback actually works as a workaround for your problem, right?

Yes, but that changes the whole design of the app.

	Randell Jesup [:jesup] (needinfo me)
	Comment 7 • 10 years ago

a) right now that's what's needed in Firefox, and if we fixed it tomorrow and uplifted it to Aurora and Beta, it still would be in a sizable fraction of users for some time.  (Also, it would be tough to uplift to Beta at this point, and Beta will be the bases of the next ESR release (extended support; in use especially at companies and schools for the next ~1 yr).

b) we don't have a fix-in-hand for it yet; and until now it hasn't been raised as a major issue.  How problematic is the workaround?

	Iñaki Baz Castillo Reporter
	Comment 8 • 10 years ago

> How problematic is the workaround?

It is problematic when you rely on a JS library that relies on such a valid behavior.

If it cannot be fixed soon I'll need to modify the library, sure. Anyhow for some usages, retrieving the remote tracks upon SetRemoteDescriptionSuccessCallback seems logical and easier than listening into onaddstream.

	Maire Reavy [:mreavy]
	Comment 9 • 9 years ago

jib - I think this was fixed by the placeholder track landing - can you dup it if I'm right?  Thanks

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 10 • 9 years ago

Looks like no: http://jsfiddle.net/x32cxq4f

I suppose we could un-delay onaddstream now that we have placeholders, if video.mozSrcObject et al. can cope with them?

AFAICT the spec only says "Onnaddstream happens as early as possible after the setRemoteDescription.", so the "MUST" in this bug's title is perhaps not entirely accurate.

	Iñaki Baz Castillo Reporter
	Comment 11 • 9 years ago

> AFAICT the spec only says "Onnaddstream happens as early as possible after the setRemoteDescription.", so the "MUST" in this bug's title is perhaps not entirely accurate.

Regardless Firefox fires "onaddstream" in the same cycle that setRemoteDesctipion() is called or not, the point is:

1) pc.setRemoteDescription(...)
2) pc.getRemoteStream()  // Called in the same cycle
3) pc.getRemoteStream()  // Called after 2 seconds

Do you think it is fine that 2) returns an empty list of tracks while 3) returns the proper remote tracks?

Please, make it work correctly. Step 2) must return the remote track list.

	Eric Rescorla (:ekr)
	Comment 12 • 9 years ago

(In reply to Iñaki Baz Castillo from comment #11)
> > AFAICT the spec only says "Onnaddstream happens as early as possible after the setRemoteDescription.", so the "MUST" in this bug's title is perhaps not entirely accurate.
> 
> Regardless Firefox fires "onaddstream" in the same cycle that
> setRemoteDesctipion() is called or not, the point is:
> 
> 1) pc.setRemoteDescription(...)
> 2) pc.getRemoteStream()  // Called in the same cycle
> 3) pc.getRemoteStream()  // Called after 2 seconds
> 
> Do you think it is fine that 2) returns an empty list of tracks while 3)
> returns the proper remote tracks?
> 
> Please, make it work correctly. Step 2) must return the remote track list.

This is getting close to violating Bugzilla etiquette
(https://bugzilla.mozilla.org/page.cgi?id=etiquette.html, point #2).

Yes, we agree it's a bug, but there are other bugs we believe are generally
more important. If this is important to you, please supply a patch.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 13 • 9 years ago

(In reply to Iñaki Baz Castillo from comment #11)
> 1) pc.setRemoteDescription(...)
> 2) pc.getRemoteStream()  // Called in the same cycle
> 
> Do you think it is fine that 2) returns an empty list of tracks while 3)
> returns the proper remote tracks?
> 
> Please, make it work correctly. Step 2) must return the remote track list.

It doesn't matter what you or I think. It matters what the spec says, and I find no support in the spec for the guarantee you want. This matters, because then the library you're using may be at fault for relying on a non-spec feature, and you can push back on them to fix their library.

But most importantly, if this guarantee is important to you, please raise it on the list so that the spec can be changed to provide the guarantee you want.

	Iñaki Baz Castillo Reporter
	Comment 14 • 9 years ago

This guarantee is no longer so important for me. But I'd rather have a consistent behavior across browsers.

I really expect that a call to pc.getRemoteStreams() should be deterministic. Current FF's behavior is that in which depending on when you call to it you get different results.

Right, I'll open a thread in the WG.

	Iñaki Baz Castillo Reporter
	Comment 15 • 9 years ago

Asked in public-webrtc:

https://lists.w3.org/Archives/Public/public-webrtc/2015Mar/0001.html

BTW: I want to insist that the issue here is not "when onaddstream is emitted", but when the full list of remote tracks should be available.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 16 • 9 years ago

Thanks, right now the two are entwined in the spec under http://w3c.github.io/webrtc-pc/#operation :

> 2. Add stream to connection's remote streams set.
> 3. Fire a stream event named addstream with stream at the connection object.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 17 • 8 years ago

The setLD/setRD description [1] in step 8 calls the *dispatch a receiver* procedure [2] synchronously.

[1] http://w3c.github.io/webrtc-pc/#set-description
[2] http://w3c.github.io/webrtc-pc/#processing-remote-mediastreamtracks

	Maire Reavy [:mreavy]
	Comment 18 • 8 years ago

Given that the spec has changed, the impact on use cases people care about, and the progress on other features like renegotiation, it's time to make this a P1.

	Byron Campen [:bwc] Assignee
	Comment 19 • 8 years ago

This should be working now, but I will modify our mochitests to verify this.

	Byron Campen [:bwc] Assignee
	Comment 20 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/52047/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52047/

	Nils Ohlmeier [:drno]
	Comment 21 • 8 years ago

Comment on attachment 8751498 [details]
MozReview Request: Bug 1019579: Check remote tracks immediately on SRD success. r=drno

https://reviewboard.mozilla.org/r/52047/#review48953

::: dom/media/tests/mochitest/pc.js:1341
(Diff revision 1)
>      info(this + " Checking remote tracks " +
>           JSON.stringify(this.expectedRemoteTrackInfoById));
>  
> -    // No tracks are expected
> -    if (this.allExpectedTracksAreObserved(this.expectedRemoteTrackInfoById,
> -                                          this.observedRemoteTrackInfoById)) {
> +    ok(this.allExpectedTracksAreObserved(this.expectedRemoteTrackInfoById,
> +                                         this.observedRemoteTrackInfoById),
> +       "Not all expected tracks have been observed"

I think the convention is to have the positive case in the message (although I admit that is confusing if things fail).

	Byron Campen [:bwc] Assignee
	Comment 22 • 8 years ago

Comment on attachment 8751498 [details]
MozReview Request: Bug 1019579: Check remote tracks immediately on SRD success. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52047/diff/1-2/

	Pulsebot
	Comment 23 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/258f53f98c75

	Carsten Book [:Tomcat]
	Comment 24 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/258f53f98c75

	Andreas Pehrson [:pehrsons]
	Comment 25 • 8 years ago

This caused bug 1208373 to be backed out.

We're now exposing MediaStreamTracks before the receiving MediaPipelines are up (they create the underlying tracks) so in case we don't finish negotiating offers and answers (getting the MediaPipelines up) we can't end those tracks. See [1] for permafails.

Exposing a MediaStreamTrack before we can guarantee that the underlying track will be created is kind of a cheat. Perhaps we can create those MediaPipelines as soon as we've created the MediaStreamTracks instead? Is it something you feel like fixing Byron? I don't want to poke more holes into MediaStreamTrack, we have enough as it is.

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=64d4466513512412251a58fe0ca70aecb7fe3b81

	Byron Campen [:bwc] Assignee
	Comment 26 • 8 years ago

Creating the MediaPipelines earlier is already on the to-do list, because waiting for offer/answer completion creates scenarios where renegotiation will cause media to be dropped for a short period.

A lot has changed since the last time I looked at how hard this will be. If it is blocking bug 1208373 without any kind of workaround, I can prioritize it.

	Andreas Pehrson [:pehrsons]
	Comment 27 • 8 years ago

Ok, cool. Workarounds are possible (poking holes for ending the track if we know there's no underlying track) but I don't want to spend time on them :-)

If you could check how much time this would take to fix at least, we can maybe work from there. Do you have a bug for getting the MediaPipelines created earlier?

	Byron Campen [:bwc] Assignee
	Comment 28 • 8 years ago

Actually, on further thought, even if we create the MediaPipelines sooner, it won't help since the new tracks don't get surfaced to JS until SetRemoteDescription anyway.

	Byron Campen [:bwc] Assignee
	Comment 29 • 8 years ago

So, I've looked into this, and creating the MediaPipelines sooner is going to be rather involved, since lots of the parts that go into them are only known at the conclusion of offer/answer (eg; what kind of bundle was negotiated, was rtcp-mux negotiated, was simulcast negotiated, etc). We do have references to the MediaStreams and MediaStreamTracks in PeerConnectionMedia that we could maybe use to ensure that onended fires, instead of trying to do that in MediaPipelineReceive::PipelineListener? If that is not possible, I suppose we could also create the MediaPipelineReceive::PipelineListener early, and hand it off to the MediaPipelineReceive once offer/answer completes, but that may cause some threading difficulties.

Thoughts?

	Andreas Pehrson [:pehrsons]
	Comment 30 • 8 years ago

I'll poke a hole and file a bug then :(