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

Remove extension to getCapabilities() in favour of MediaCapabilities #76

Merged
merged 6 commits into from
Jan 25, 2023

Conversation

Orphis
Copy link
Contributor

@Orphis Orphis commented Jan 18, 2023

Rerefences to scalabilityModes in getCapabilties('video').codecs have been removed and replaced with references to the MediaCapabilities API when appropriate and an example for the MediaCapabilites API usage have been added.

The SFM capabilities description has not been updated since it is not normative or reflective of the Javascript APIs we support anyway and falls into the domain of native APIs.

Fixes: #22, #49


💥 Error: 522 💥

PR Preview failed to build. (Last tried on Jan 20, 2023, 9:14 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

Rerefences to scalabilityModes in getCapabilties('video').codecs have
been removed and replaced with references to the MediaCapabilities
API when appropriate and an example for the MediaCapabilites API
usage have been added.

The SFM capabilities description has not been updated since it is not
normative or reflective of the Javascript APIs we support anyway and
falls into the domain of native APIs.
@Orphis Orphis requested review from aboba and youennf January 18, 2023 18:54
@Orphis Orphis changed the title Remove extension to getCapabilties() in favor of MediaCapabilities Remove extension to getCapabilities() in favour of MediaCapabilities Jan 18, 2023
@Orphis
Copy link
Contributor Author

Orphis commented Jan 19, 2023

@dontcallmedom Do you know what's going on with the CI on this PR?

@dontcallmedom
Copy link
Member

seems to have been a temporary hiccup, relaunching the CI fixed it

@aboba
Copy link
Contributor

aboba commented Jan 19, 2023

The PR does not modify Sections 4.2.1 addTransceiver() or 4.2.2 setParameters(), which refer togetCapabilities() to determine whether a scalabilityMode is supported. Prior to this PR, those Sections could be tested by comparing the results of setParameters() and addTransceiver() against getCapabilities(). For example, if getCapabilities() indicated that a scalabilityMode was not supported for a codec, then an attempt by setParameters() to set that scalabilityMode should fail.

I am not sure how a test can be written based on the PR. As noted in w3c/media-capabilities#192, most of the time supported does not depend on width, height, bitrate, and framerate, but this is not the case for every browser implementation or every parameter combination.

Does the browser implementation maintain an internal slot with a list of supported scalabilityMode values for each codec? If so, is there a way for an application to determine the value of this internal slot? For example, should the application expect setParameters() to succeed for a scalabilityMode if there is some combination of width, height, bitrate and framerate for which supported will be returned in Media Capabilities?

Or can setParameters() fail based on the particular values of width, height, bitrate and framerate that are in use at a given moment? If so, how can the application determine what the offending parameters are?

index.html Outdated Show resolved Hide resolved
@aboba aboba requested a review from henbos January 19, 2023 17:01
Copy link

@henbos henbos left a comment

Choose a reason for hiding this comment

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

+1 on updating the spec match what is shipping so I'm happy to approve.

That being said...

  • From a casual reading of SVC getCapabilities() is redundant with Media Capabilities query #49, I can't tell if removing scalabilityModes from getCapabilities() is a good or bad thing.
  • Having one method that provides both the codecs that are available and how they can be configured, without trial-and-error querying MediaCapabilities, seem to have several advantages. I proposed an async version here which might help solve other issues as well.

But I don't have a strong opinion. This seems to already have been discussed at lengths and if MC already gets the job done then perhaps we're just talking about ergonomics. I did have some concerns about feature detection earlier, but those seem to have been overblown.

Perhaps "Should there exist an async getCapabilities()" deserves a separate issue?

@youennf
Copy link

youennf commented Jan 20, 2023

  • Having one method that provides both the codecs that are available and how they can be configured, without trial-and-error querying MediaCapabilities, seem to have several advantages.

MediaCapabilities is designed this way for privacy reasons.
An async getCapabilities would probably go against that goal.

@henbos
Copy link

henbos commented Jan 20, 2023

MediaCapabilities is designed this way for privacy reasons.
An async getCapabilities would probably go against that goal.

Makes sense. What happens if the app queries MC too much?

@youennf
Copy link

youennf commented Jan 20, 2023

Makes sense. What happens if the app queries MC too much?

It is up to the UA to put some protection when detecting such pattern (throttling at least...).

@aboba
Copy link
Contributor

aboba commented Jan 20, 2023

@henbos At the moment, it is possible to call MC on all scalabilityMode values for a given codec, without complaint: https://webrtc.internaut.com/mc/

@henbos
Copy link

henbos commented Jan 20, 2023

Ack, so while theoretical at this point, it sounds like an app that does not want to risk throttling delaying call setup time should only query a small number of times

@aboba
Copy link
Contributor

aboba commented Jan 20, 2023

@dontcallmedom The hiccup seems to be back, CI is failing again.

Copy link
Contributor

@aboba aboba left a comment

Choose a reason for hiding this comment

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

Added lines 241-246 (text looks to have been deleted by mistake). Also fixed references to Section 9 (should be Section 10), and provided a link to the definition of "simulcast envelope" in WebRTC-PC.

@aboba
Copy link
Contributor

aboba commented Jan 23, 2023

Previous CI issues appear fixed. CI now completes successfully.

@aboba aboba merged commit 656088d into w3c:main Jan 25, 2023
@aboba aboba added the Needs Test Needs a Test label Jan 25, 2023
@Orphis Orphis deleted the remove_getcapabilities branch January 26, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Test Needs a Test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getCapabilities seems to leak hardware capabilities w/o a permission
6 participants