-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simulcast: Implementations do not fail (and that seems good) #2762
Comments
@Orphis Do you agree that implementation behavior makes more sense than the spec in these cases? |
@docfaraday same question ^ |
Regarding 2 and 3, erroring out when a remote SDP tries to narrow the envelope would violate the base IETF spec, IMO. Base spec requires us to allow this. Whether this breaks webrtc-pc is another question, but my position is that webrtc-pc should not be contradicting the IETF spec that it is based on. Regarding 6, this seems wrong. If offer/answer removes a rid by narrowing the envelope (which I think needs to be allowed, see previous line), it should probably not show up in JS anymore. Regarding 7, that's just bonkers. An answer that contains rids that the offer did not is invalid per base spec, and should result in an error. |
@docfaraday what about 4 and 5? |
So, disallowing a reoffer from expanding the envelope is fine, and does not require an error; you just form an answer that says "no" by not including the new rids. An answer that attempts to expand the envelope is invalid per spec, and should result in an error (see my comment on 7). Regarding 5, that's ok by IETF standards, since that's the agent's choice. |
I don't think it breaks anything and browsers are already following the IETF spec here, so this spec should probably yield.
To clarify: 6 is about this line: "If the number of RTCRtpEncodingParameters now stored in sendEncodings is 1, then remove any rid member from the lone entry." Browsers do trim the encodings array, they just don't get rid (no pun intended) of the rid member holding the name of the last remaining encoding when the array length is down to 1. It looks like the a=simulcast line does go away a few renegotiations after this, so removing the rid member like the spec says seems consistent with the SDP browsers produce, so perhaps implementations should fix this.
I agree. I doubt an error here would break anyone. |
Ah, I see. I am not aware of any IETF requirement that forbids a simulcast with only one rid, even though one could argue unicast-with-a-rid is silly. I think it probably makes sense to make the JS parameters and the SDP agree, whatever we end up doing. |
Bringing @alvestrand's comment here from #2757 (comment) so we can discuss here:
|
I sense agreement on 2, 3, 4, and 5. On 6 and 7 leaving the spec as is SGTM. |
RESOLUTION: no objections. |
Here's a second fiddle with more data from two additional tests:
It shows that both Chrome and Safari allow trimming any layer, not just from the tail. This is reflected in the encodings array (encodings past the removed one move up). |
I closed #2779 because introducing more state requiring rollback seemed unwise when no browsers support rolling it back. It also violated the original intent which seemed to be for sRD(offer) to establish initial simulcast envelopes in time for ontrack, but trim encodings only in answers (which cannot be rolled back). This and the spec's ban on "remotely initiated RID renegotiation" prevent One way to lift our ban on "remotely initiated RID renegotiation" (to comply with JSEP O/A) without giving up these invariants would be: once envelopes have been established, defer trimming them until the answer. |
I should point out that existing browsers expose this kind of trimming in have-remote-offer right now, so this could break existing code. While rollback is obviously not working right now in this case, all cases with sRD(trimmed reoffer) would change, regardless of whether they are then followed by a rollback. |
Not all cases. Only those that rely on examining encodings ahead of sLD(answer). With no API to reject individual encodings in a browser's answer, the utility of such information in the API this early seems marginal. |
RESOLUTION: close #2762 as is. |
@jan-ivar did you submit any WPT out of your fiddle in the end? |
In PR2757, Jan-Ivar said:
"I've written some tentative WPT tests in https://jsfiddle.net/jib1/opsv1ugf/ with console.log statements instead of asserts in places, and the results are in. Chrome, Safari and Edge pretty much yield the same results. Firefox only passes the first (existing) test since we're about to update set/getParameters to spec. In Chrome:
(Note the last 3 PASSes illustrate implemented behaviors, not spec compliance)
To summarize:
1, 2, 6 and 7 violate the spec I think. We should perhaps assess how web compatible we think it would be to enforce the spec at this point."
The text was updated successfully, but these errors were encountered: