Don't let offers to receive simulcast overwrite existing [[SendEncodings]]

[jan-ivar]

Fixes #2722. cc @docfaraday

---

Preview | Diff

[jan-ivar]

I recommend ignoring whitespace when reviewing.

[jan-ivar]

Feedback is it might be simpler to spell the failure criteria earlier in 4.1 explicitly instead of relying on it referencing whether this code that's being changed results in a change or not. That failure test would then (for the case where existing SendEncodings exist) disallow changes in SendEncodings.length and changes in ordering, or changes in rid names

[alvestrand]

Think this still needs some work, in particular about being explicit about where inappropriate changes (adding RIDs, reordering RIDs) are being applied.

I think the only modification operation we really want to allow is to delete trailing RIDs (possibly dropping down to singlecast by deleting all RIDs?)

[jan-ivar]

I've updated this PR based on #2762 (comment), with one difference: it only fails over rid name mismatch on the first encoding in re-offers, and treats mismatch past that as absence.

IOW I saw no reason to treat known out-of-order rids any different from unknowns rids. This seems to match implementations (both result in layer reduction). @alvestrand @docfaraday PTAL.

[alvestrand]

This is close to being right, I think. But I worry about the removal (rather than disabling) of non-sent layers.

I think this means that the sequence:

offer: rid: 1,2,3
answer: rid: 1,2

means that subsequent offer is

offer: rid:1,2
and that
answer: rid:1,2,3
will lead to rejection.

Does this have an interaction with pr-answer? (ex: offer: 1,2,3; pr-answer: 1,2; answer: 1,2,3)?

[alvestrand]

and stop (I'm not clear if it should say "go to end of section", "stop processing RIDs", or "go on to the next transceiver").

[jan-ivar]

I originally had "and skip the remaining sub-steps" but since we're in nested sub-steps of sub-steps I worried it wouldn't be clear which sub-steps to skip. Fortunately, the remaining steps we care to skip are all no-ops in this case (all aspects being compared for are equal already), which is why I suggest leaving it this way.

This has the added advantage of running the assert.

[jan-ivar]

This is close to being right, I think. But I worry about the removal (rather than disabling) of non-sent layers.

Having SRD toggle the active attributes would create more races like we're trying to remove in #2737 I fear.

offer: rid:1,2
and that
answer: rid:1,2,3
will lead to rejection.

This is already invalid in JSEP O/A. An answer must stay within the envelope of an offer AFAIK.

Does this have an interaction with pr-answer? (ex: offer: 1,2,3; pr-answer: 1,2; answer: 1,2,3)?

I don't think so. This PR isn't modifying any answer behavior.

[jan-ivar]

cc @Orphis

[dontcallmedom]

@jan-ivar can you rebase on main so that I add the necessary metadata on Rec changes, following #2713 ?

[jan-ivar]

This PR needs more work to incorporate additonal findings:

1. Its restriction that encodings[0].rid must match the first rid in a=simulcast seems too strict, when implementations allow rid pruning not just from the tail
2. While letting remote offers prune rids is required by JSEP, browsers' immediate exposure of this effect on encodings in"have-remote-offer" seems poorly executed as it cannot be rolled back, and effects stack over back-to-back SRDs.

Instead, waiting until local answer to prune encodings avoids both problems (no encodings to rollback and no stacking).

Such waiting would maintain invariants the spec offers today, which seems more conservative as a first PR here, while moving us closer to how browsers work.

I think that's the right place to stop, as whether we should go any further I think requires further WG discussion.

[jan-ivar]

PR updated. This simplifies quite a bit, which seems promising. PTAL.

[jan-ivar]

@dontcallmedom this one seems to be missing amendments as well, yet was able to merge. Did you change this requirement?

[dontcallmedom]

merges aren't prevented by CI failure in our current setup - the merge was done here despite such a failure :)

Could you prepare a separate PR to update amendments.json?

[jan-ivar]

To qualify the title of this PR, remote offers to receive simulcast still trounce unicast [[SendEncodings]] (containing a lone encoding without a rid member).

[dontcallmedom-bot]

This issue was mentioned in WEBRTCWG-2023-01-17 (Page 14)