Improve definition of required fields in RTCRtpParameters #1610
Conversation
There was a problem hiding this comment.
I don't think any of the changes in this PR really buy us anything. The issue this is trying to solve is:
when writing applications and tests we cannot make assumption of any of the field returned from getParameters() is defined.
But required doesn't help there. required only tells you what things the JS->IDL binding validates on input, not what fields are required of the implementation on output. Ultimately that must be described in prose. Remember this? whatwg/webidl#382
| @@ -4524,7 +4524,7 @@ <h2 id="sec.cert-mgmt">Certificate Management</h2> | |||
| given as an argument is already being sent to avoid sending | |||
| the same <code>MediaStreamTrack</code> twice, the other ways | |||
| do not, allowing the same <code>MediaStreamTrack</code> (possibly | |||
| using different <code><a>RTCRtpParameters</a></code> with different | |||
| using different <code><a>RTCRtpSenderParameters</a></code> with different | |||
There was a problem hiding this comment.
I don't know that introducing the RTCRtpSenderParameters dictionary is really worth it. It's one more dictionary to keep track of and one more link to follow, vs. two fields that are described as "sender-only."
| @@ -5972,8 +5981,8 @@ <h2 id="sec.cert-mgmt">Certificate Management</h2> | |||
| </div> | |||
| <div> | |||
| <pre class="idl">dictionary RTCRtcpParameters { | |||
| DOMString cname; | |||
| boolean reducedSize; | |||
| required DOMString cname; | |||
There was a problem hiding this comment.
We don't need these to be marked required, or anything else in RtpParameters. The input validation is handled by comparing against [[LastReturnedParameters]].
| @@ -5271,8 +5271,8 @@ <h2 id="sec.cert-mgmt">Certificate Management</h2> | |||
| readonly attribute RTCDtlsTransport? transport; | |||
| readonly attribute RTCDtlsTransport? rtcpTransport; // Feature at risk | |||
| static RTCRtpCapabilities getCapabilities (DOMString kind); | |||
| Promise<void> setParameters (optional RTCRtpParameters parameters); | |||
| RTCRtpParameters getParameters (); | |||
| Promise<void> setParameters (RTCRtpSenderParameters parameters); | |||
There was a problem hiding this comment.
This needs to be optional. It's a weird WebIDL thing. #419
|
As noted in the above WebIDL link, |
Fixes #1493.
This PR attempts to clarify which fields are required in the
RTCRtpParametersdictionary returned fromgetParameters(). I have also factor out thetransactionIdanddegradationPreferencefields to a new dictionaryRTCRtpSenderParameters, since these two fields are required only forRTCRtpSender.It would be great if someone with more experience on RTP parameters can verify whether some of the fields marked required make sense. Note that we'd also have to consider the case when no SDP has been set yet, e.g.
pc.addTransceiver().sender.getParameters()before any offer is generated.