Separate Send/ReceiveCodecs and NegotiatedCodecs #2972
Conversation
This makes it unambiguous what to return in RTCRtpSender/Receiver's getParameters() function (null before negotiation, filled in after) Fixes: 2956
Set NegotiatedCodecs only on answers for both senders and receivers.
| <li>Locate the matching codec description in <var>transceiver</var>.{{RTCRtpTransceiver/[[Sender]]}}.{{RTCRtpSender/[[SendCodecs]]}}.</li> | ||
| <li>If the matching codec description is not found, abort these steps.</li> | ||
| <li>Set the "enabled" flag in the matching codec description to "true".</li> | ||
| <li>Add the codec description to the sender's {{RTCRtpSender/[[NegotiatedCodecs]]}} internal slot.</li> |
There was a problem hiding this comment.
This puts tuples in [[NegotiatedCodecs]], which while expedient, seems a bit confusing since they'll always have "enabled" true. Is this on purpose?
Also since these are internal objects kept in lists in internal slots, these entries are added by reference, so if we ever introduce code that sets an enabled flag to false, it would affect both [[SendCodecs]] and [[NegotiatedCodecs]] potentially causing the codec entry to disappear from getParameters(). Is this desirable?
Even if desirable, this behavior seems a bit hard to grasp from this prose.
The other thing I noticed is that payloadType isn't filled in.
While [[SendCodecs]] is "a list of tuples, each containing an RTCRtpCodecParameters dictionary and an "enabled" boolean" it's actually initialized to "an implementation-defined list of RTCRtpCodecCapability dictionaries"
Spot the different dictionary types above. RTCRtpCodecParameters != RTCRtpCodecCapability
The former contains payloadType, the latter doesn't. So it needs to be filled in here I think.
It might be good to use language shown in https://infra.spec.whatwg.org/#example-tuple to be more precise. This includes declaring the tuple where it's created with member names like codec and enabled, and "Let entry be ..." etc.
This would let us be more precise and be clear whether we want to put the tuples in [[NegotiatedCodecs]] or simply the entry.codec.
There was a problem hiding this comment.
The intent of "negotiatedCodecs" is to reflect the currently negotiated codecs. The "enabled" flag has no purpose there, so it's not carried along.
Good point about using RTCRtpCodecParameters and filling in the PT field - this accentuates the need for it being a copy not a reference.
There was a problem hiding this comment.
Actually RTCRtpCodecCapability is a dictionary with no extra members over RTCRtpCodec. We shouldn't be using it. It is RTCRtpCodecParameters that contains the PT.
| @@ -8800,6 +8806,9 @@ <h3> | |||
| [=RTCRtpSender/list of implemented send codecs=], with the "enabled" flag | |||
| set in an implementation defined manner. | |||
| </p> | |||
| <p> | |||
| Let <var>sender</var> have a <dfn data-dfn-for="RTCRtpSender">[[\NegotiatedCodecs]]</dfn> internal slot, consisting of {{RTCRtpCodecParameters}}, and initialized to an empty list. | |||
There was a problem hiding this comment.
Ah, this says [[NegotiatedCodecs]] contains RTCRtpCodecParameters, not tuples. Good, but means the code above needs to be fixed to not add tuples.
There was a problem hiding this comment.
Or rather ... construct an RTCRtpCodecParameters from the RTCRtpCodec and add the PT from the description.
In support of landing this spec PR: w3c/webrtc-pc#2972 Bug: None Change-Id: I698276ccf739a872d791fc0923c2725ec303fbd3
In support of landing this spec PR: w3c/webrtc-pc#2972 Bug: None Change-Id: I698276ccf739a872d791fc0923c2725ec303fbd3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5642762 Commit-Queue: Harald Alvestrand <hta@chromium.org> Reviewed-by: Florent Castelli <orphis@chromium.org> Cr-Commit-Position: refs/heads/main@{#1317347}
In support of landing this spec PR: w3c/webrtc-pc#2972 Bug: None Change-Id: I698276ccf739a872d791fc0923c2725ec303fbd3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5642762 Commit-Queue: Harald Alvestrand <hta@chromium.org> Reviewed-by: Florent Castelli <orphis@chromium.org> Cr-Commit-Position: refs/heads/main@{#1317347}
In support of landing this spec PR: w3c/webrtc-pc#2972 Bug: None Change-Id: I698276ccf739a872d791fc0923c2725ec303fbd3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5642762 Commit-Queue: Harald Alvestrand <hta@chromium.org> Reviewed-by: Florent Castelli <orphis@chromium.org> Cr-Commit-Position: refs/heads/main@{#1317347}
…the right time., a=testonly Automatic update from web-platform-tests Add tests for if codec info surfaces at the right time. In support of landing this spec PR: w3c/webrtc-pc#2972 Bug: None Change-Id: I698276ccf739a872d791fc0923c2725ec303fbd3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5642762 Commit-Queue: Harald Alvestrand <hta@chromium.org> Reviewed-by: Florent Castelli <orphis@chromium.org> Cr-Commit-Position: refs/heads/main@{#1317347} -- wpt-commits: 4d7ec06c827c841bfe9090a92521de6a2fb265cf wpt-pr: 46840
…the right time., a=testonly Automatic update from web-platform-tests Add tests for if codec info surfaces at the right time. In support of landing this spec PR: w3c/webrtc-pc#2972 Bug: None Change-Id: I698276ccf739a872d791fc0923c2725ec303fbd3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5642762 Commit-Queue: Harald Alvestrand <hta@chromium.org> Reviewed-by: Florent Castelli <orphis@chromium.org> Cr-Commit-Position: refs/heads/main@{#1317347} -- wpt-commits: 4d7ec06c827c841bfe9090a92521de6a2fb265cf wpt-pr: 46840
…the right time., a=testonly Automatic update from web-platform-tests Add tests for if codec info surfaces at the right time. In support of landing this spec PR: w3c/webrtc-pc#2972 Bug: None Change-Id: I698276ccf739a872d791fc0923c2725ec303fbd3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5642762 Commit-Queue: Harald Alvestrand <hta@chromium.org> Reviewed-by: Florent Castelli <orphis@chromium.org> Cr-Commit-Position: refs/heads/main@{#1317347} -- wpt-commits: 4d7ec06c827c841bfe9090a92521de6a2fb265cf wpt-pr: 46840
…the right time., a=testonly Automatic update from web-platform-tests Add tests for if codec info surfaces at the right time. In support of landing this spec PR: w3c/webrtc-pc#2972 Bug: None Change-Id: I698276ccf739a872d791fc0923c2725ec303fbd3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5642762 Commit-Queue: Harald Alvestrand <htachromium.org> Reviewed-by: Florent Castelli <orphischromium.org> Cr-Commit-Position: refs/heads/main{#1317347} -- wpt-commits: 4d7ec06c827c841bfe9090a92521de6a2fb265cf wpt-pr: 46840 UltraBlame original commit: b98fa37e2e0425dbfe0f272d58e1c9e0486ec712
…the right time., a=testonly Automatic update from web-platform-tests Add tests for if codec info surfaces at the right time. In support of landing this spec PR: w3c/webrtc-pc#2972 Bug: None Change-Id: I698276ccf739a872d791fc0923c2725ec303fbd3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5642762 Commit-Queue: Harald Alvestrand <htachromium.org> Reviewed-by: Florent Castelli <orphischromium.org> Cr-Commit-Position: refs/heads/main{#1317347} -- wpt-commits: 4d7ec06c827c841bfe9090a92521de6a2fb265cf wpt-pr: 46840 UltraBlame original commit: b98fa37e2e0425dbfe0f272d58e1c9e0486ec712
This makes it unambiguous what to return in RTCRtpSender/Receiver's getParameters() function (null before negotiation, filled in after, which corresponds to implementations).
Fixes #2956
Preview | Diff