w3c / webrtc-pc

WebRTC 1.0 API
https://w3c.github.io/webrtc-pc/
Other
438 stars 115 forks source link

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

Closed aboba closed 2 years ago

aboba commented 2 years ago

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 commented 2 years ago

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

jan-ivar commented 2 years ago

@docfaraday same question ^

docfaraday commented 2 years ago

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 commented 2 years ago

@docfaraday what about 4 and 5?

docfaraday commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Bringing @alvestrand's comment here from https://github.com/w3c/webrtc-pc/pull/2757#issuecomment-1209270441 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 commented 2 years ago

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

jan-ivar commented 2 years ago

RESOLUTION: no objections.

jan-ivar commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

RESOLUTION: close #2762 as is.

dontcallmedom commented 1 month ago

@jan-ivar did you submit any WPT out of your fiddle in the end?