-
Notifications
You must be signed in to change notification settings - Fork 18
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
base: main
Are you sure you want to change the base?
Conversation
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.
2de721f
to
cd26c2a
Compare
@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; |
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.
Was this change discussed?
It looks ok to me but this reduces the ability of creating event to candidate pairs created by the UA
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.
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
.
- If this is found to be too restrictive, then it could be discussed whether it makes sense to introduce a JS-accessible constructor for
@alvestrand made another pass over the changes, and added a small fix. Otherwise looks ready to me. |
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.
Preview | Diff