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

The prose around "simulcast envelope" falsely implies that simulcast encodings can never be removed #2723

Closed
docfaraday opened this issue Apr 22, 2022 · 8 comments · Fixed by #2814

Comments

@docfaraday
Copy link
Contributor

Spec says "Once the envelope is determined, layers cannot be removed.", but the language for sRD(answer) says that if rids are rejected by an answer, they are removed.

There are a couple of ways to fix this:

  1. We remove this assurance from the section on "simulcast envelope", or
  2. We only allow the first answer to remove rids from [[SendEncodings]].

Disallowing an answer to remove rids on a previously negotiated sender is probably not appropriate, since this would violate the simulcast spec, which requires the offerer to handle this case regardless of whether this is the initial negotiation or not. I think option 1 is the correct course of action here.

@aboba
Copy link
Contributor

aboba commented Apr 22, 2022

Re-reading Section 5.4.1, I agree that there is an issue. While the section seems to imply that the simulcast envelope is established by addTransceiver() alone, this is not correct, if only because the answer can reject layers/RIDs. My inclination is that the envelope is only really determined once the initial negotiation has completed. However, this does raise the question of whether setParameters() can modify parameters originally set by addTransceiver() prior to completion of the initial negotiation.

@docfaraday
Copy link
Contributor Author

docfaraday commented Apr 22, 2022

Given that RFC8853 requires a reofferer to honor the removal of rids from the answer, and given that RFC8853 also requires the answerer to honor the removal of rids from a reoffer, I think it would be a mistake to have a blanket prohibition on the renegotiation of rids, whatever we end up doing. I think we need to at least support the removal of rids in renegotiation, if only to remain compliant with RFC8853.

Prohibiting the addition of rids seems acceptable to me; there's nothing in RFC8853 that requires an answerer to accept new rids in a reoffer, and of course it is already a RFC8853 violation for an answerer to add a rid that is not in an offer. That said, if there's a strong enough desire to allow the addition of rids in renegotiation, it would be acceptable also, but we'd need to think carefully about how that would fit with the automatic selection of scaleResolutionDownBy. My inclination is to avoid this mess unless there's some compelling use-case.

What we do with rid reordering is a little more subtle. RFC8853 states that the rid order establishes a transmission preference, but does not say anything about what ordering implies about fidelity; in fact, there are examples where the first rid is the highest fidelity encoding. webrtc-pc is what specifies that the fidelity goes from low to high (and even then, this only applies when creating a set of encodings from an answer a remote description). RFC8853 does not explicitly forbid reordering rids in an answer as far as I can tell, although this seems like an oversight to me; if an offerer requests simulcast with a transmission preference, I think it is a bad idea for the answerer to switch up that ordering. RFC8853 definitely does not prohibit a reoffer from reordering rids, so throwing an error if we receive such a reoffer seems like a violation of RFC8853 to me. I think that if we are to be compliant with RFC8853, we have to allow rid reordering, if only to allow modification of the transmission preference. I do not think it is a great idea to try to modify the fidelity through reordering, though.

@docfaraday
Copy link
Contributor Author

I should also note that if we do allow rid reordering in a reoffer, we should not mess with [[SendEncodings]] in any way as a result. We should probably just create our answer using the same order, and let it have no other effect.

@youennf
Copy link
Contributor

youennf commented May 17, 2022

we should not mess with [[SendEncodings]]

I agree with not changing [[SendEncodings]], but then we have descriptions that are not synchronised with [[SendEncodings]].
It seems rejecting in that case is something we could do and would avoid corner cases as well as potentially simplify implementations.
I would think we already sometimes reject descriptions that are valid SDPs so I am not sure what we gain in not rejecting?

@docfaraday
Copy link
Contributor Author

Generally, my preference is to follow the SDP negotiation rules in the IETF specifications wherever possible. While throwing errors when we encounter something valid-but-weird is one way to simplify the web side of things, from an IETF perspective this is a bug in our implementation of RFC 8853.

@aboba
Copy link
Contributor

aboba commented Jul 19, 2022

Feedback from June 19, 2022 WEBRTC WG Virtual Interim: "Ready for PR"

@jan-ivar
Copy link
Member

Spec says "Once the envelope is determined, layers cannot be removed.",

This was added in #2081 and, as I read it, is only talking about envelopes established by remote offers to receive simulcast, and the limitations on browser client answers to those. In that case the statement seems true, because SLD disallows anything but unadulterated createAnswer, and AFAIK no API allows JS to reduce layers in answers.

I've tried to clarify this in #2760.

@jan-ivar
Copy link
Member

I hope we agree any JS that munges the answer before returning it to the SFU is on its own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants