-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add RTCIceTransport method to remove candidate pairs #175
Conversation
fa3129a
to
9d5bfc3
Compare
This issue was mentioned in WEBRTCWG-2023-09-12 (Page 66) |
@pthatcher PTAL |
FYI reviewers, the PR is now rebased and much smaller than it looked before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I overall like this, just have a question about the "immediate-ness" of this operation
The promise is resolved once the ICE agent has finished removing the supplied candidate pairs and the corresponding remove events have been fired. Also added some extra validation to setSelectedCandidatePair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Seems fine to me except for the sentence I mentioned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case that requires this new method?
This issue was discussed in WebRTC December 5 2023 meeting – 05 December 2023 (PR #175: Add RTCIceTransport method to remove pairs) |
Is this ready to merge at the next meeting? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I don't mind this API, so I'm approving it for the sake of progress.
However, I worry there are some API races here that we still need to sort out. We appear to have two async methods, select and remove basically, which appear as if they can race. We should clarify what's supposed to happen then.
For instance, what happens if someone calls both methods at once with the same argument?
My recommendation of adding an internal slot in a follow-up might help clarify this.
Instead, there should be a requirement on the user agent to validate the application's input. This requirement is added by w3c#194.
Thank you! I think the internal slot and validations should address a lot of the race conditions. I would certainly want to fix any remaining issues once the proposed changes are submitted. I am happy to continue discussing and iterating. |
OK let's merge this, @sam-vi vill you care a follow-up issue regarding internal slots and validation? |
Fixes #170.
This PR extends the
RTCIceTransport
interface by adding a new method to allow an application to remove candidate pairs that are no longer needed for the peer connection. The new methodremoveCandidatePairs
immediately removes the supplied candidate pairs, which are then sent in non-cancelableicecandidatepairremove
events.Preview | Diff