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

Add RTCIceTransport method to remove candidate pairs #175

Merged
merged 14 commits into from
Jan 12, 2024

Conversation

sam-vi
Copy link
Contributor

@sam-vi sam-vi commented Sep 5, 2023

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 method removeCandidatePairs immediately removes the supplied candidate pairs, which are then sent in non-cancelable icecandidatepairremove events.


Preview | Diff

@aboba aboba added the TPAC 2023 For discussion at TPAC label Sep 7, 2023
@dontcallmedom-bot
Copy link

This issue was mentioned in WEBRTCWG-2023-09-12 (Page 66)

@aboba
Copy link
Contributor

aboba commented Oct 26, 2023

@pthatcher PTAL

@henbos
Copy link
Collaborator

henbos commented Oct 27, 2023

FYI reviewers, the PR is now rebased and much smaller than it looked before

Copy link
Collaborator

@henbos henbos left a 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

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
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.
Copy link
Collaborator

@henbos henbos left a comment

Choose a reason for hiding this comment

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

LGTM

index.html Outdated Show resolved Hide resolved
@pthatcher
Copy link

Seems fine to me except for the sentence I mentioned.

Copy link
Member

@jan-ivar jan-ivar left a 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?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@dontcallmedom-bot
Copy link

@henbos
Copy link
Collaborator

henbos commented Jan 9, 2024

Is this ready to merge at the next meeting?

Copy link
Member

@jan-ivar jan-ivar left a 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.

index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
Instead, there should be a requirement on the user agent to validate the
application's input.

This requirement is added by w3c#194.
@sam-vi
Copy link
Contributor Author

sam-vi commented Jan 12, 2024

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.

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.

@henbos
Copy link
Collaborator

henbos commented Jan 12, 2024

OK let's merge this, @sam-vi vill you care a follow-up issue regarding internal slots and validation?

@henbos henbos merged commit a1c3d1c into w3c:main Jan 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE improvements: remove candidate pairs
7 participants