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

Handling of unknown scalability modes is underspecified #57

Closed
alvestrand opened this issue Jan 6, 2022 · 4 comments
Closed

Handling of unknown scalability modes is underspecified #57

alvestrand opened this issue Jan 6, 2022 · 4 comments

Comments

@alvestrand
Copy link
Contributor

Consider the following snippet:

    const pc = new RTCPeerConnection();
    t.add_cleanup(() => pc.close());
    const { sender } = pc.addTransceiver('video', {
      sendEncodings: [{scalabilityMode: 'TotalNonsense'}],
    });
    const param = sender.getParameters();
    const encoding = getFirstEncoding(param);

What should the value of encoding.scalabilityMode be?

This matters a lot for the case where "supported scalability modes" is not part of the API.
(At the moment the Chrome implementation returns 'TotalNonsense')

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 6, 2022
See w3c/webrtc-svc#57, 58, 59 for background.

Bug: none
Change-Id: I4bad7ee6659d01e28cc1d13e9bb595d05e512565
@aboba
Copy link
Contributor

aboba commented Jan 6, 2022

'TotalNonsense' isn't a valid scalabilityMode for any codec. This is an argument for addTransceiver throwing an exception. On the other hand, the text for addTransceiver in WebRTC 1.0 API doesn't specify any validation steps for sendEncodings. So one could argue that {sendEncodings: 'totalNonsense'} would also not throw an exception.

@Orphis
Copy link
Contributor

Orphis commented Jan 19, 2022

In the SVC spec https://w3c.github.io/webrtc-svc/#error-handling:

However, an error will result only if the requested scalabilityMode value is invalid for any supported codec.. I guess this should throw an RTCError with description hardware-encoder-error or just an OperationError. We should probably be more clear about what to do there.

While it doesn't apply for addTransceiver, we may want to make sure that setParameters only checks the negotiated codec list or the subset indicated by setCodecPreferences prior negotiation.

@aboba aboba self-assigned this Jan 24, 2022
@alvestrand
Copy link
Contributor Author

This section should probably be written in imperative language. Something like:

Add to the operation of setParameters step 6:

If any scalabilityMode of any encoding is not supported by any codec in parameters.codecs, reject p with a newly created
InvalidModificationError.

Add to the operation of addTransceivers step 8:

If any scalabilityMode is not supported by any codec in RTCRtpSender.getCapabilities(kind).codecs, reject p with a newly created OperationError.

@aboba
Copy link
Contributor

aboba commented Feb 24, 2022

Closed after merger of PR 64.

@aboba aboba closed this as completed Feb 24, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 16, 2022
See w3c/webrtc-svc#57, 58, 59 for background.

Bug: none
Change-Id: I4bad7ee6659d01e28cc1d13e9bb595d05e512565
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 17, 2022
See w3c/webrtc-svc#57, 58, 59 for background.

Bug: none
Change-Id: I4bad7ee6659d01e28cc1d13e9bb595d05e512565
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3369962
Reviewed-by: Florent Castelli <orphis@chromium.org>
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982200}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 17, 2022
See w3c/webrtc-svc#57, 58, 59 for background.

Bug: none
Change-Id: I4bad7ee6659d01e28cc1d13e9bb595d05e512565
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3369962
Reviewed-by: Florent Castelli <orphis@chromium.org>
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982200}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 17, 2022
See w3c/webrtc-svc#57, 58, 59 for background.

Bug: none
Change-Id: I4bad7ee6659d01e28cc1d13e9bb595d05e512565
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3369962
Reviewed-by: Florent Castelli <orphis@chromium.org>
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982200}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 26, 2022
…parameters, a=testonly

Automatic update from web-platform-tests
Add webrtc-svc tests for error-inducing parameters

See w3c/webrtc-svc#57, 58, 59 for background.

Bug: none
Change-Id: I4bad7ee6659d01e28cc1d13e9bb595d05e512565
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3369962
Reviewed-by: Florent Castelli <orphis@chromium.org>
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982200}

--

wpt-commits: ce1ae2d8cbd6052da722c7210fa3b7cfa78276ed
wpt-pr: 32269
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 27, 2022
…parameters, a=testonly

Automatic update from web-platform-tests
Add webrtc-svc tests for error-inducing parameters

See w3c/webrtc-svc#57, 58, 59 for background.

Bug: none
Change-Id: I4bad7ee6659d01e28cc1d13e9bb595d05e512565
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3369962
Reviewed-by: Florent Castelli <orphis@chromium.org>
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982200}

--

wpt-commits: ce1ae2d8cbd6052da722c7210fa3b7cfa78276ed
wpt-pr: 32269
aosmond pushed a commit to aosmond/gecko that referenced this issue Mar 28, 2022
…parameters, a=testonly

Automatic update from web-platform-tests
Add webrtc-svc tests for error-inducing parameters

See w3c/webrtc-svc#57, 58, 59 for background.

Bug: none
Change-Id: I4bad7ee6659d01e28cc1d13e9bb595d05e512565
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3369962
Reviewed-by: Florent Castelli <orphis@chromium.org>
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982200}

--

wpt-commits: ce1ae2d8cbd6052da722c7210fa3b7cfa78276ed
wpt-pr: 32269
aosmond pushed a commit to aosmond/gecko that referenced this issue Mar 28, 2022
…parameters, a=testonly

Automatic update from web-platform-tests
Add webrtc-svc tests for error-inducing parameters

See w3c/webrtc-svc#57, 58, 59 for background.

Bug: none
Change-Id: I4bad7ee6659d01e28cc1d13e9bb595d05e512565
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3369962
Reviewed-by: Florent Castelli <orphis@chromium.org>
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982200}

--

wpt-commits: ce1ae2d8cbd6052da722c7210fa3b7cfa78276ed
wpt-pr: 32269
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
See w3c/webrtc-svc#57, 58, 59 for background.

Bug: none
Change-Id: I4bad7ee6659d01e28cc1d13e9bb595d05e512565
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3369962
Reviewed-by: Florent Castelli <orphis@chromium.org>
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982200}
NOKEYCHECK=True
GitOrigin-RevId: 8cdb88830c8c409c52e6fde66334b33029400116
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants