Skip to content

Fix "does not allow remotely initiated RID renegotiation" step #2757

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

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Jul 27, 2022

Fixes #2743.


Preview | Diff

@jan-ivar jan-ivar requested review from alvestrand and aboba July 27, 2022 21:03
@jan-ivar jan-ivar self-assigned this Jul 27, 2022
@jan-ivar jan-ivar added the Needs Test Needs a WPT test label Jul 28, 2022
@jan-ivar
Copy link
Member Author

Editor's agree the original language was probably overly strict compare to initial intent of it (as judged by the summary in the next sentence after the modified language). But the restrictive language may still match current implementations. So we should write some tests to confirm whether this is true or not before merging this.

Copy link
Contributor

@alvestrand alvestrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This effectively says "allow answers to change RIDs".
This may be too open-ended, but I think that will be fixed by #2758 once it's finished.

Approving.

@jan-ivar
Copy link
Member Author

jan-ivar commented Aug 3, 2022

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.

@jan-ivar
Copy link
Member Author

jan-ivar commented Aug 3, 2022

4 also violates the spec since it doesn't fail, I think.

@aboba
Copy link
Contributor

aboba commented Aug 4, 2022

These results seem reasonable, in general. So as a goal, it would seem like a good idea to align the spec to the implementations.

@jan-ivar
Copy link
Member Author

jan-ivar commented Aug 4, 2022

If we agree on this, then I think this means the paragraph this PR is modifying instead goes away entirely, because of 1.

@jan-ivar
Copy link
Member Author

jan-ivar commented Aug 4, 2022

@Orphis does this way forward we suggest make sense to you?

@alvestrand
Copy link
Contributor

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 Author

jan-ivar commented Aug 9, 2022

The conversation moved to #2762 (comment) where the current suggestion seems to be to fail over 7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Test Needs a WPT test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SLD/SRD(answer) trips over itself when narrowing simulcast envelope
3 participants