PeerConnection MUST include all remote tracks after SetRemoteDescriptionSuccessCallback

RESOLVED FIXED in Firefox 49

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: ibc, Assigned: bwc)

Tracking

(Blocks 1 bug)

Trunk
mozilla49
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

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

Comment 1

5 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.
Reporter

Updated

5 years ago
Component: Untriaged → WebRTC: Audio/Video
Product: Firefox → Core
Reporter

Updated

5 years ago
Hardware: x86 → x86_64
Reporter

Updated

5 years ago
Depends on: 998546
Reporter

Comment 2

5 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.
Reporter

Comment 3

5 years ago
Guys, any feedback or comments please? IMHO the bug is important enough, really.
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.
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.
Reporter

Comment 6

5 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.
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 8

5 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.
jib - I think this was fixed by the placeholder track landing - can you dup it if I'm right?  Thanks
Flags: needinfo?(jib)
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.
Flags: needinfo?(jib)
Reporter

Comment 11

4 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.
(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.
(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.
Reporter

Comment 14

4 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.
Reporter

Comment 15

4 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.
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.
Blocks: webrtc_spec
backlog: --- → webRTC+
Rank: 28
Priority: -- → P2
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
Rank: 28 → 20
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.
Assignee: nobody → docfaraday
Rank: 20 → 15
Priority: P2 → P1
Assignee

Comment 19

3 years ago
This should be working now, but I will modify our mochitests to verify this.
Attachment #8751498 - Flags: review?(drno) → review+
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).
Assignee

Comment 22

3 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/
Attachment #8751498 - Attachment description: MozReview Request: Bug 1019579: Check remote tracks immediately on SRD success. r?drno → MozReview Request: Bug 1019579: Check remote tracks immediately on SRD success. r=drno

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/258f53f98c75
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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
Blocks: 1208373
Flags: needinfo?(docfaraday)
Assignee

Comment 26

3 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.
Flags: needinfo?(docfaraday) → needinfo?(pehrsons)
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?
Flags: needinfo?(pehrsons) → needinfo?(docfaraday)
Assignee

Comment 28

3 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.
Flags: needinfo?(docfaraday)
Assignee

Comment 29

3 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?
Flags: needinfo?(pehrsons)
I'll poke a hole and file a bug then :(
Flags: needinfo?(pehrsons)
Depends on: 1273136
You need to log in before you can comment on or make changes to this bug.