Don't let offers to receive simulcast overwrite existing [[SendEncodings]] #2758
Conversation
|
I recommend ignoring whitespace when reviewing. |
|
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 |
There was a problem hiding this comment.
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?)
|
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. |
There was a problem hiding this comment.
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)?
webrtc.html
Outdated
| [=map/contains=] no | ||
| {{RTCRtpCodingParameters/rid}} member, set | ||
| <var>transceiver</var>.{{RTCRtpTransceiver/[[Sender]]}}.{{RTCRtpSender/[[SendEncodings]]}} | ||
| to <var>proposedSendEncodings</var>. |
There was a problem hiding this comment.
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").
There was a problem hiding this comment.
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.
Having SRD toggle the
This is already invalid in JSEP O/A. An answer must stay within the envelope of an offer AFAIK.
I don't think so. This PR isn't modifying any answer behavior. |
|
cc @Orphis |
jan-ivar
force-pushed
the
norescaledown
branch
from
60bf88b to
df59f75
Compare
|
This PR needs more work to incorporate additonal findings:
Instead, waiting until local answer to prune 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. |
|
PR updated. This simplifies quite a bit, which seems promising. PTAL. |
…r non-tail removal is ok
|
@dontcallmedom this one seems to be missing amendments as well, yet was able to merge. Did you change this requirement? |
|
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? |
|
To qualify the title of this PR, remote offers to receive simulcast still trounce unicast [[SendEncodings]] (containing a lone encoding without a |
|
This issue was mentioned in WEBRTCWG-2023-01-17 (Page 14) |
6 participants
Fixes #2722. cc @docfaraday
Preview | Diff