Skip to content
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

Merged
merged 6 commits into from
Jul 20, 2023

Conversation

fippo
Copy link
Contributor

@fippo fippo commented Jul 10, 2023

Both are defined as empty in order to allow webrtc-extensions to extend them which is done in
w3c/webrtc-extensions#167


Preview | Diff

@fippo fippo changed the title Add empty RTCEncodeOptions dict and empty encodeOptions argument to s… Add empty RTCEncodeOptions dict and empty encodeOptions argument to setParameters Jul 10, 2023
@fippo fippo force-pushed the setparameters-extensions branch from ad44b95 to 65ac21e Compare July 10, 2023 07:42
webrtc.html Outdated
<div id="rtcencodeoptions"><!-- kept for candidate amendments management purposes --></div>
<section id="rtcencodeoptions">
<h3>
<dfn>RTCEncodeOptions</dfn> Dictionary
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an export?

Copy link
Member

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
Comment on lines 8865 to 8866
Promise&lt;undefined&gt; setParameters(RTCRtpSendParameters parameters,
optional sequence&lt;RTCEncodeOptions&gt; encodeOptions = []);
Copy link
Member

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]});

Copy link
Contributor Author

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?

Copy link
Member

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?

fippo and others added 3 commits July 13, 2023 17:40
…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
@fippo fippo force-pushed the setparameters-extensions branch from 56b6646 to 154ab53 Compare July 13, 2023 15:44
@fippo fippo changed the title Add empty RTCEncodeOptions dict and empty encodeOptions argument to setParameters Add empty RTCSetParameterOptions dict as argument to setParameters Jul 13, 2023
@aboba aboba merged commit d8489ee into w3c:main Jul 20, 2023
@fippo fippo deleted the setparameters-extensions branch July 20, 2023 14:13
@fippo
Copy link
Contributor Author

fippo commented Jul 20, 2023

hrm... any idea why it shows up like this even though it is defined in the same section-h2-h3 as the previous dict?
image

fippo added a commit to fippo/webrtc-pc that referenced this pull request Jul 31, 2023
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 18, 2023
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 18, 2023
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 15, 2024
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
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 19, 2024
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}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 19, 2024
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}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 19, 2024
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}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 22, 2024
…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
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Jan 23, 2024
…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
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 23, 2024
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}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants