Skip to content
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

Closed
aboba opened this issue Aug 4, 2022 · 15 comments
Closed

Simulcast: Implementations do not fail (and that seems good) #2762

aboba opened this issue Aug 4, 2022 · 15 comments
Assignees
Labels
TPAC 2022 For discussion at TPAC

Comments

@aboba
Copy link
Contributor

aboba commented Aug 4, 2022

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)

FAIL createAnswer() attaches to an existing transceiver with a remote simulcast offer: Expected exactly one transceiver - Expected 1, got 2 failed
PASS SRD(simulcastOffer) creates a simulcast transceiver with rids
FAIL SRD(simulcastOffer) creates a simulcast transceiver with scaleResolutionDownBys: Expected scaleResolutionDownBys - Expected 4,2,1, got ,, failed
PASS SRD(simulcastOffer) creates a simulcast transceiver with actives
foo,bar,baz
foo,bar
foo
foo
foo
PASS Remote reoffer can narrow but not (re-)expand simulcast envelope. Never fails.
0,1,2
0,1
0
0
0
PASS Remote reanswer can narrow but not (re-)expand simulcast envelope. Never fails.
0,1
0
0
PASS Remote reanswer altering rids is treated as removal. Never fails.
5/7
PASS 

To summarize:

  1. SRD never fails in response, ignoring changes if needed instead
  2. remote offers can narrow the simulcast envelope
  3. remote answers can narrow the simulcast envelope
  4. neither remote offers nor answers can expand the simulcast envelope, but don't fail
  5. Once narrowed, the envelope stays narrowed and cannot be re-expanded
  6. Once present, rid ids are never removed from an encoding
  7. renaming rids in remote answers is treated as layer removal, but doesn't fail

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."

@aboba
Copy link
Contributor Author

aboba commented Aug 4, 2022

@Orphis Do you agree that implementation behavior makes more sense than the spec in these cases?

@jan-ivar
Copy link
Member

jan-ivar commented Aug 4, 2022

@docfaraday same question ^

@docfaraday
Copy link
Contributor

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.

@jan-ivar
Copy link
Member

jan-ivar commented Aug 4, 2022

@docfaraday what about 4 and 5?

@docfaraday
Copy link
Contributor

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.

@jan-ivar
Copy link
Member

jan-ivar commented Aug 4, 2022

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.

I don't think it breaks anything and browsers are already following the IETF spec here, so this spec should probably yield.

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.

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.

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.

I agree. I doubt an error here would break anyone.

@docfaraday
Copy link
Contributor

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.

I don't think it breaks anything and browsers are already following the IETF spec here, so this spec should probably yield.

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.

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.

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.

@jan-ivar
Copy link
Member

jan-ivar commented Aug 9, 2022

Bringing @alvestrand's comment here from #2757 (comment) so we can discuss here:

I am worried about 7.

In particular, if the offer is 1,2,3 and the answer is 1,foo,2, exactly what is being removed?

If we want to be lenient, I would treat any answer that has changes as erroneous layers and ignore them. Thus:

1,2,3 + 1,2 -> 1,2 (narrowing allowed)
1,2,3 + 1,3,2 -> 1
1,2,3 + 1,foo,2 -> 1
1,2,3 + foo, 1, 2 -> one layer with no RID

And CreateOffer() after 1,2,3 + 1,2 should offer 1,2 (no widening allowed from either side).

@jan-ivar
Copy link
Member

jan-ivar commented Aug 9, 2022

I sense agreement on 2, 3, 4, and 5. On 6 and 7 leaving the spec as is SGTM.

@aboba aboba added the TPAC 2022 For discussion at TPAC label Aug 25, 2022
@jan-ivar
Copy link
Member

RESOLUTION: no objections.

@jan-ivar
Copy link
Member

Here's a second fiddle with more data from two additional tests:

Encoding rids 0,1,2
Encoding srdb 4,2,1
Encoding rids 0
Encoding srdb 4
PASS Remote unicast reanswer reduces to 1 layer. Never fails.
Encoding rids 0,1,2
Encoding srdb 4,2,1
Encoding rids 0,2
Encoding srdb 4,1
Encoding rids 2
Encoding srdb 1
PASS Remote reanswer trimming middle layer then first layer. Never fails.

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

@jan-ivar
Copy link
Member

jan-ivar commented Oct 7, 2022

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 sender.getParameters().encodings.length from decreasing in have-remote-offer (or increasing from their rollback).

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.

@docfaraday
Copy link
Contributor

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.

@jan-ivar
Copy link
Member

jan-ivar commented Oct 14, 2022

all cases with sRD(trimmed reoffer) would change

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.

@jan-ivar
Copy link
Member

RESOLUTION: close #2762 as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TPAC 2022 For discussion at TPAC
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants