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.

Sign up for GitHub

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

Jump to bottom

Add an internal slot for active candidate pairs. #194

Closed
wants to merge 9 commits into from
Closed

Add an internal slot for active candidate pairs. #194

wants to merge 9 commits into from

Conversation

sam-vi
Copy link
Contributor

@sam-vi sam-vi commented Jan 10, 2024 •
edited by pr-preview bot

Fixes: #188.

The [[CandidatePairs]] slot in RTCIceTransport tracks candidate pairs formed by the ICE agent and surfaced to the application, and not removed via icecandidatepairremove or removeCandidatePair().

The slot is used to validate the inputs to selectCandidatePair() and removeCandidatePair().

EDIT: To help Sameer make progress while OOO, I've re-uploaded this as #197 with comments addressed.

Preview | Diff

@sam-vi
Add an internal slot for active candidate pairs.
9a97fe2 
Fixes: w3c#188.

The [[CandidatePairs]] slot in RTCIceTransport tracks candidate pairs
formed by the ICE agent and surfaced to the application, and not removed
via icecandidatepairremove or removeCandidatePair().

The slot is used to validate the inputs to selectCandidatePair() and
removeCandidatePair().
@sam-vi sam-vi mentioned this pull request Jan 10, 2024
Merged
sam-vi added a commit to sam-vi/webrtc-extensions that referenced this pull request Jan 10, 2024
@sam-vi
Validate removeCandidatePair input and update internal slot.
262bc2a 
This depends on several other PRs to be merged first:
- w3c#170: Addition of removeCandidatePair method
- w3c#192: Definition of candidate match algo
  - alternately, w3c/webrtc-pc#2906 to enforce RTCIceCandidate object
    equality
- w3c#193: Definition of candidate pair match algo
- w3c#194: Addition of CandidatePairs internal slot
sam-vi added a commit to sam-vi/webrtc-extensions that referenced this pull request Jan 10, 2024
@sam-vi
Validate removeCandidatePair input and update internal slot.
21deec1 
This depends on several other PRs to be merged first:
- w3c#170: Addition of removeCandidatePair method
- w3c#192: Definition of candidate match algo
  - alternately, w3c/webrtc-pc#2906 to enforce RTCIceCandidate object
    equality
- w3c#193: Definition of candidate pair match algo
- w3c#194: Addition of CandidatePairs internal slot
@sam-vi sam-vi mentioned this pull request Jan 10, 2024
Closed
sam-vi added a commit to sam-vi/webrtc-extensions that referenced this pull request Jan 12, 2024
@sam-vi
Validate removeCandidatePair input and update internal slot.
2b5fd80 
This depends on several other PRs to be merged first:
- w3c#170: Addition of removeCandidatePair method
- w3c#192: Definition of candidate match algo
  - alternately, w3c/webrtc-pc#2906 to enforce RTCIceCandidate object
    equality
- w3c#193: Definition of candidate pair match algo
- w3c#194: Addition of CandidatePairs internal slot
sam-vi added a commit to sam-vi/webrtc-extensions that referenced this pull request Jan 12, 2024
@sam-vi
Remove text appearing to put a requirement on the app.
d0d0ae1 
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 sam-vi mentioned this pull request Jan 12, 2024
Merged
sam-vi added a commit to sam-vi/webrtc-extensions that referenced this pull request Jan 12, 2024
@sam-vi
Validate removeCandidatePair input and update internal slot.
ecd06e1 
This depends on several other PRs to be merged first:
- w3c#170: Addition of removeCandidatePair method
- w3c#192: Definition of candidate match algo
  - alternately, w3c/webrtc-pc#2906 to enforce RTCIceCandidate object
    equality
- w3c#193: Definition of candidate pair match algo
- w3c#194: Addition of CandidatePairs internal slot
@sam-vi sam-vi marked this pull request as ready for review January 12, 2024 13:43
henbos
henbos approved these changes Jan 15, 2024
View reviewed changes
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

jan-ivar
jan-ivar reviewed Jan 18, 2024
View reviewed changes
index.html Show resolved Hide resolved
index.html Outdated
Comment on lines 750 to 757
Let |transport:RTCIceTransport| be the {{RTCIceTransport}} object associated with |candidatePair|.
</p>
</li>
<li>
<p>
[= list/Append =] |candidatePair| to {{RTCIceTransport/[[CandidatePairs]]}}.
</p>
</li>
Copy link
Member

@jan-ivar jan-ivar Jan 18, 2024 •
edited

Choose a reason for hiding this comment

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

Github won't let me comment on the line directly above this section, which is:

<p>
  Let |candidatePair:RTCIceCandidatePair| be the candidate pair that was [= formed =].
</p>

While readers can hover over the candidatePair variable to learn its type, I think we need to spell out in prose what the type is now, since to a non-interactive reader candidatePair reads like an internal concept so far in this algorithm. We need to expound on its type before adding it to [[CandidatePairs]] since objects there need to be usable with the new match algorithm.

Also this appears to be the right place to instantiate candidate interfaces. Something like (updated):

<p>
  Let |candidatePair:RTCIceCandidatePair| be a new {{RTCIceCandidatePair}} dictionary
  with its {{RTCIceCandidatePair/local}} and {{RTCIceCandidatePair/remote}} members
  initialized to new {{RTCIceCandidate}}s representing the local and remote part of the
  [= formed =] pair respectively.
</p>

Copy link
Collaborator

Choose a reason for hiding this comment

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

To help Sameer make progress while OOO, I re-uploaded this PR as #197 with this comment addressed

jan-ivar
jan-ivar requested changes Jan 18, 2024
View reviewed changes
Copy link
Member

@jan-ivar jan-ivar left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for the cleanup. I'd like to specify some more invariants around this internal slot.

index.html 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
jan-ivar
jan-ivar reviewed Jan 18, 2024
View reviewed changes
index.html
</li>
<li>
<p>
[= list/Remove =] all items that [= candidate pair match | match =] |candidatePair| from [=this=].{{RTCIceTransport/[[CandidatePairs]]}}.
</p>
</li>
<li>
Copy link
Member

@jan-ivar jan-ivar Jan 18, 2024 •
edited

Choose a reason for hiding this comment

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

Also, do we need to clear [[CandidatePairs]] at some point during ICE restart?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good question, I filed #196 to decide if we need to add more steps or if this is already covered by the existing removal steps, including whether or not the onicecandidatepairremoved should fire?

henbos and others added 2 commits January 25, 2024 13:22
@henbos @jan-ivar
Update index.html
e8a0f76 
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
@henbos @jan-ivar
Update index.html
31a97db 
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
henbos
henbos reviewed Jan 25, 2024
View reviewed changes
index.html Outdated Show resolved Hide resolved
henbos
henbos reviewed Jan 25, 2024
View reviewed changes
index.html Outdated Show resolved Hide resolved
henbos and others added 2 commits January 25, 2024 13:24
@henbos
Update index.html
e271605
@henbos @jan-ivar
Update index.html
0c3c86c 
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
This was referenced Jan 25, 2024
Open
Merged
@alvestrand
Copy link
Collaborator

Merged as #197

@alvestrand alvestrand closed this Jan 25, 2024
@sam-vi sam-vi deleted the samvi-188-pairs-slot branch March 11, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jan-ivar jan-ivar jan-ivar requested changes

@henbos henbos henbos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Maintain an internal slot of candidate pairs that are valid inputs to RTCIceTransport operations
4 participants
@sam-vi @alvestrand @jan-ivar @henbos