Handle RID Encondings mis-match for simulcast

RESOLVED FIXED in Firefox 55

Status

()

Core
WebRTC: Signaling
P1
normal
Rank:
15
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: drno, Assigned: drno)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
Fippo accidentally discovered that in the simulcast answerer scenario we can encounter a mis-match between the amount of encodings requested via JS and the amount of RID's signaled in the offer from the remote side. Currently this ends up an assertion when trying to start the sender complaining that the amount of SSRCs does not match the amount of RIDs.

I did not find anything in the spec how to handle this scenario.

I see two options:
#1 take min(remoteRIDs, requestedEncodings) and proceed with that
#2 throw some form of error when doing createAnswer() (?) pointing out that there is a mis-match between these two values
(Assignee)

Comment 1

5 months ago
@bwc: thoughts?
Flags: needinfo?(docfaraday)
(Assignee)

Updated

5 months ago
Rank: 25
(Assignee)

Comment 2

5 months ago
To make matters worse: any offer with simulcast and RID attributes in it, even when the JS code never uses setParameters() leads to the same crash/assert it looks like.
Rank: 25 → 15
Priority: P2 → P1

Comment 3

5 months ago
The correct thing to do is probably the stuff in setParameters that is a subset of what was in the remote offer.
Flags: needinfo?(docfaraday)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8872021 - Flags: review?(docfaraday)

Comment 5

5 months ago
mozreview-review
Comment on attachment 8872021 [details]
Bug 1367930: adjust ssrc's and encondings as simulcast answerer.

https://reviewboard.mozilla.org/r/143546/#review147944

So, are we ending up in a situation where JsepTrack has more encodings than SSRCs? If so, I would like to fix JsepTrack instead...
(Assignee)

Comment 6

5 months ago
mozreview-review-reply
Comment on attachment 8872021 [details]
Bug 1367930: adjust ssrc's and encondings as simulcast answerer.

https://reviewboard.mozilla.org/r/143546/#review147944

Yes. Both scenarios can happen: more encodings then SSRCs OR more SSRCs then encodings.
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Assignee: nobody → drno
Comment hidden (mozreview-request)

Comment 9

5 months ago
mozreview-review
Comment on attachment 8872021 [details]
Bug 1367930: adjust ssrc's and encondings as simulcast answerer.

https://reviewboard.mozilla.org/r/143546/#review148906

What happens if the remote SDP uses different RID values (eg; hi, med, lo) than were in the JS encodings?

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:283
(Diff revision 3)
> -  // For each rid in the remote, make sure we have an encoding, and configure
> +  size_t max_streams = 1;
> +
> +  if (mJsEncodeConstraints.size()) {
> +    max_streams = std::min(rids.size(), mJsEncodeConstraints.size());
> +  }
> +  // Drop SSRC's if less RID's where offered then we have encoding constraints

Editorial nits:
s/where/were
s/then/than
s/SSRC's/SSRCs
s/RID's/RIDs
Attachment #8872021 - Flags: review?(docfaraday) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
See Also: → bug 1369601
(Assignee)

Comment 13

5 months ago
mozreview-review-reply
Comment on attachment 8872021 [details]
Bug 1367930: adjust ssrc's and encondings as simulcast answerer.

https://reviewboard.mozilla.org/r/143546/#review148906

The answer appears to depend on how the mis-match looks exactly.
I filed a follow up bug 1369601 to solve that problem.

Comment 14

5 months ago
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/2ad1181dadf3
adjust ssrc's and encondings as simulcast answerer. r=bwc

Comment 15

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ad1181dadf3
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.