-
Notifications
You must be signed in to change notification settings - Fork 114
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
Add empty RTCSetParameterOptions dict as argument to setParameters #2885
Conversation
ad44b95
to
65ac21e
Compare
webrtc.html
Outdated
<div id="rtcencodeoptions"><!-- kept for candidate amendments management purposes --></div> | ||
<section id="rtcencodeoptions"> | ||
<h3> | ||
<dfn>RTCEncodeOptions</dfn> Dictionary |
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.
Add an export?
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.
(not needed - as an IDL term, this gets exported automatically)
webrtc.html
Outdated
Promise<undefined> setParameters(RTCRtpSendParameters parameters, | ||
optional sequence<RTCEncodeOptions> encodeOptions = []); |
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 extends the setParameters method with a feature/encoding-specific argument:
await setParameters(params, [{keyFrame: true}, {keyFrame: false}]);
This violates § 6.4. Accept optional and/or primitive arguments through dictionaries, as well as my understanding that we agreed to add a feature-agnostic options object:
Promise<undefined> setParameters(RTCRtpSendParameters parameters,
optional setParametersOptions = {});
Webrtc-extensions would then define the rest, with a final shape e.g. maybe like this:
await setParameters(params, {encodingOptions: [{keyFrame: true}, {keyFrame: false}]});
or e.g. maybe this?
await setParameters(params, {encodingKeyframes: [true, false]});
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.
Updated (with slight changes).
Can you please explain what future extensions you see and how they are not specific to the encodings and would not fit into an array?
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.
I don't have a specific extension in mind, but to exempt ourselves from following § 6.4. Accept optional and/or primitive arguments through dictionaries, I think would require an argument that we would never add any future options that aren't encoding-related. Since we have per-header and per-codec parameters, why paint ourselves in that corner?
…etParameters Both are defined as empty in order to allow webrtc-extensions to extend them which is done in w3c/webrtc-extensions#167
arguably no need to mark up the change in the algo since it's essentially a no-op, so no substantive impact
56b6646
to
154ab53
Compare
adding a new RTCSetParameterOptions object which has a sequence of RTCEncodingOptions similar to WebCodecs VideoEncoderEncodeOptions https://w3c.github.io/webcodecs/#dictdef-videoencoderencodeoptions and its keyFrame flag. On the native side, this adds the request_key_frame flag to the RtpEncodingParameters. WebRTC CL: https://webrtc-review.googlesource.com/c/src/+/286741 Spec PRs: w3c/webrtc-pc#2885 w3c/webrtc-extensions#167 BUG=chromium:1354101 Change-Id: I5bfe266eac5990b1921212babdee1af35edc4242
adding a new RTCSetParameterOptions object which has a sequence of RTCEncodingOptions similar to WebCodecs VideoEncoderEncodeOptions https://w3c.github.io/webcodecs/#dictdef-videoencoderencodeoptions and its keyFrame flag. On the native side, this adds the request_key_frame flag to the RtpEncodingParameters. WebRTC CL: https://webrtc-review.googlesource.com/c/src/+/286741 Spec PRs: w3c/webrtc-pc#2885 w3c/webrtc-extensions#167 Chromestatus feature: https://chromestatus.com/feature/5161082937409536 BUG=chromium:1354101 Change-Id: I5bfe266eac5990b1921212babdee1af35edc4242
adding a new RTCSetParameterOptions object which has a sequence of RTCEncodingOptions similar to WebCodecs VideoEncoderEncodeOptions https://w3c.github.io/webcodecs/#dictdef-videoencoderencodeoptions and its keyFrame flag. On the native side, this adds the request_key_frame flag to the RtpEncodingParameters. WebRTC CL: https://webrtc-review.googlesource.com/c/src/+/286741 Spec PRs: w3c/webrtc-pc#2885 w3c/webrtc-extensions#167 Chromestatus feature: https://chromestatus.com/feature/5161082937409536 BUG=chromium:1354101 Change-Id: I5bfe266eac5990b1921212babdee1af35edc4242
adding a new RTCSetParameterOptions object which has a sequence of RTCEncodingOptions similar to WebCodecs VideoEncoderEncodeOptions https://w3c.github.io/webcodecs/#dictdef-videoencoderencodeoptions and its keyFrame flag. On the native side, this adds the request_key_frame flag to the RtpEncodingParameters. WebRTC CL: https://webrtc-review.googlesource.com/c/src/+/286741 Spec PRs: w3c/webrtc-pc#2885 w3c/webrtc-extensions#167 Chromestatus feature: https://chromestatus.com/feature/5161082937409536 BUG=chromium:1354101 Change-Id: I5bfe266eac5990b1921212babdee1af35edc4242 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4643591 Reviewed-by: Harald Alvestrand <hta@chromium.org> Commit-Queue: Philipp Hancke <phancke@microsoft.com> Cr-Commit-Position: refs/heads/main@{#1249577}
adding a new RTCSetParameterOptions object which has a sequence of RTCEncodingOptions similar to WebCodecs VideoEncoderEncodeOptions https://w3c.github.io/webcodecs/#dictdef-videoencoderencodeoptions and its keyFrame flag. On the native side, this adds the request_key_frame flag to the RtpEncodingParameters. WebRTC CL: https://webrtc-review.googlesource.com/c/src/+/286741 Spec PRs: w3c/webrtc-pc#2885 w3c/webrtc-extensions#167 Chromestatus feature: https://chromestatus.com/feature/5161082937409536 BUG=chromium:1354101 Change-Id: I5bfe266eac5990b1921212babdee1af35edc4242 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4643591 Reviewed-by: Harald Alvestrand <hta@chromium.org> Commit-Queue: Philipp Hancke <phancke@microsoft.com> Cr-Commit-Position: refs/heads/main@{#1249577}
adding a new RTCSetParameterOptions object which has a sequence of RTCEncodingOptions similar to WebCodecs VideoEncoderEncodeOptions https://w3c.github.io/webcodecs/#dictdef-videoencoderencodeoptions and its keyFrame flag. On the native side, this adds the request_key_frame flag to the RtpEncodingParameters. WebRTC CL: https://webrtc-review.googlesource.com/c/src/+/286741 Spec PRs: w3c/webrtc-pc#2885 w3c/webrtc-extensions#167 Chromestatus feature: https://chromestatus.com/feature/5161082937409536 BUG=chromium:1354101 Change-Id: I5bfe266eac5990b1921212babdee1af35edc4242 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4643591 Reviewed-by: Harald Alvestrand <hta@chromium.org> Commit-Queue: Philipp Hancke <phancke@microsoft.com> Cr-Commit-Position: refs/heads/main@{#1249577}
…r.setParameters, a=testonly Automatic update from web-platform-tests webrtc: add setParameterOptions to sender.setParameters adding a new RTCSetParameterOptions object which has a sequence of RTCEncodingOptions similar to WebCodecs VideoEncoderEncodeOptions https://w3c.github.io/webcodecs/#dictdef-videoencoderencodeoptions and its keyFrame flag. On the native side, this adds the request_key_frame flag to the RtpEncodingParameters. WebRTC CL: https://webrtc-review.googlesource.com/c/src/+/286741 Spec PRs: w3c/webrtc-pc#2885 w3c/webrtc-extensions#167 Chromestatus feature: https://chromestatus.com/feature/5161082937409536 BUG=chromium:1354101 Change-Id: I5bfe266eac5990b1921212babdee1af35edc4242 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4643591 Reviewed-by: Harald Alvestrand <hta@chromium.org> Commit-Queue: Philipp Hancke <phancke@microsoft.com> Cr-Commit-Position: refs/heads/main@{#1249577} -- wpt-commits: 3be83c4de8cafc13d4c00f3e9ddb07132a8a0705 wpt-pr: 43705
…r.setParameters, a=testonly Automatic update from web-platform-tests webrtc: add setParameterOptions to sender.setParameters adding a new RTCSetParameterOptions object which has a sequence of RTCEncodingOptions similar to WebCodecs VideoEncoderEncodeOptions https://w3c.github.io/webcodecs/#dictdef-videoencoderencodeoptions and its keyFrame flag. On the native side, this adds the request_key_frame flag to the RtpEncodingParameters. WebRTC CL: https://webrtc-review.googlesource.com/c/src/+/286741 Spec PRs: w3c/webrtc-pc#2885 w3c/webrtc-extensions#167 Chromestatus feature: https://chromestatus.com/feature/5161082937409536 BUG=chromium:1354101 Change-Id: I5bfe266eac5990b1921212babdee1af35edc4242 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4643591 Reviewed-by: Harald Alvestrand <hta@chromium.org> Commit-Queue: Philipp Hancke <phancke@microsoft.com> Cr-Commit-Position: refs/heads/main@{#1249577} -- wpt-commits: 3be83c4de8cafc13d4c00f3e9ddb07132a8a0705 wpt-pr: 43705
adding a new RTCSetParameterOptions object which has a sequence of RTCEncodingOptions similar to WebCodecs VideoEncoderEncodeOptions https://w3c.github.io/webcodecs/#dictdef-videoencoderencodeoptions and its keyFrame flag. On the native side, this adds the request_key_frame flag to the RtpEncodingParameters. WebRTC CL: https://webrtc-review.googlesource.com/c/src/+/286741 Spec PRs: w3c/webrtc-pc#2885 w3c/webrtc-extensions#167 Chromestatus feature: https://chromestatus.com/feature/5161082937409536 BUG=chromium:1354101 Change-Id: I5bfe266eac5990b1921212babdee1af35edc4242 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4643591 Reviewed-by: Harald Alvestrand <hta@chromium.org> Commit-Queue: Philipp Hancke <phancke@microsoft.com> Cr-Commit-Position: refs/heads/main@{#1249577}
Both are defined as empty in order to allow webrtc-extensions to extend them which is done in
w3c/webrtc-extensions#167
Preview | Diff