Skip to content

Use RTCIceCandidatePair interface in RTCIceTransport #205

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sam-vi
Copy link
Contributor

@sam-vi sam-vi commented Apr 18, 2024

Addresses: w3c/webrtc-pc#2930.

This change is built on top of w3c/webrtc-pc#2961, which converts RTCIceCandidatePair from a dictionary to an interface.

  • Change RTCIceCandidatePairEvent to contain a single RTCIceCandidatePair attribute.
  • Modify RTCIceTransport algo so that getSelectedCandidatePair() returns an item from [[CandidatePairs]].
  • Validate RTCIceCandidatePair inputs using object identity instead of candidate pair match algo.

Preview | Diff

@sam-vi
Copy link
Contributor Author

sam-vi commented Aug 6, 2024

Filed #217 to address the validation errors.

Probably best to merge #218 or fix #217 another way before proceeding with this PR.

Addresses: w3c/webrtc-pc#2930.

As an interface, RTCIceCandidatePair can be used as an attribute of
other interfaces.

So simplify RTCIceCandidatePairEvent by having a single candidatePair
attribute instead of individual local and remote candidates.
An application only acquires objects of RTCIceCandidatePair through
RTCIceTransport events and getSelectedCandidatePair().

So simplify input validation where RTCIceCandidatePair is used as an
argument by directly comparing object identity instead of using the
candidate pair match algorithm.
This ensures that each candidate pair has a unique identity, simplifying
the validation of RTCIceCandidatePair arguments to RTCIceTransport
methods.
@dontcallmedom dontcallmedom force-pushed the 2930-candidatepair-interface branch from 2de721f to cd26c2a Compare October 15, 2024 11:24
@alvestrand
Copy link
Contributor

@sam-vi this has been hanging around for a while - can you check that it's still up to date?

@@ -1055,105 +1038,87 @@ <h4>Attributes</h4>
<div>
<pre class="idl">
dictionary RTCIceCandidatePairEventInit : EventInit {
required RTCIceCandidate local;
required RTCIceCandidate remote;
required RTCIceCandidatePair candidatePair;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this change discussed?
It looks ok to me but this reduces the ability of creating event to candidate pairs created by the UA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in line with the goals of w3c/webrtc-pc#2930, namely restricting JS's ability to create invalid pairs and simplifying validation steps in specs. It reduces the burden of validation in the RTCIceTransport extension API specs, and provides web developers with a loose guarantee that events will signal candidate pairs that can be provided as inputs to other RTCIceTransport methods (barring state errors).

I hadn't considered this particular change to be especially significant, but we could discuss in the next interim if needed.

My thoughts are:

  • It is a good idea to limit the general purpose RTCIceCandidatePairEventInit to valid candidate pairs created by the UA.
  • If in the future there is a need for an event to signal candidate pairs not created by the UA, it should be a more specific event (similar to RTCPeerConnectionIceErrorEvent) that is used in that limited scope.
    • If this is found to be too restrictive, then it could be discussed whether it makes sense to introduce a JS-accessible constructor for RTCIceCandidatePair.

@sam-vi
Copy link
Contributor Author

sam-vi commented May 19, 2025

@alvestrand made another pass over the changes, and added a small fix. Otherwise looks ready to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants