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 RTCRtpEncodingParameters API from BCD #19584

Merged
merged 8 commits into from Nov 13, 2023

Conversation

queengooborg
Copy link
Collaborator

This PR removes the RTCRtpEncodingParameters API from BCD. This feature is a type (ex. a dictionary, enum, mixin, constant or WebIDL typedef) that we have explicitly stated not to document separately from the feature(s) that depend on it, as they are virtually invisible to the end developer.

This PR removes the `RTCRtpEncodingParameters` API from BCD. This feature is a type (ex. a dictionary, enum, mixin, constant or WebIDL typedef) that we have explicitly stated not to document separately from the feature(s) that depend on it, as they are virtually invisible to the end developer.
@queengooborg queengooborg added data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API needs content update 📝 This PR needs a corresponding update to mdn/content to update the documentation labels Apr 29, 2023
@queengooborg queengooborg added the project: dictionary removal https://github.com/mdn/browser-compat-data/issues/6810 label Jun 26, 2023
@github-actions github-actions bot added the merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. label Aug 16, 2023
@github-actions
Copy link

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 This PR needs to merge latest "main" branch to resolve a merge conflict or other issue. label Aug 16, 2023
api/RTCRtpSender.json Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Collaborator

hamishwillee commented Nov 7, 2023

@queengooborg This adds features for RTCRtpSender.setParameters() encodings parameter object, including: active, dtx, fec, maxBitrate, maxFramerate, priority, rid, rtx, scaleResolutionDownBy, ssrc. I presume these are tested for versions.

However the RTCRtpEncodingParameters are also used in:

  • RTCRtpSender.getParameters() - we should test, but I would expect to see the same values under "return_object_property_encodings" as you have above, because before setting params you should first get them.
  • RTCPeerConnection.addTransceiver(trackOrKind, init) - the init has sendEncodings which is the same encodings again - so we need a mirror there (again tested).

Can you update/test those cases too?

See also #19584 (comment)

Docs are in progress in mdn/content#30080

@hamishwillee
Copy link
Collaborator

FF - the properties like fec, rtx, srcc are in web IDL (https://bugzilla.mozilla.org/show_bug.cgi?id=1230184#c30) but not actually exposed - that bug is to remove them. From https://bugzilla.mozilla.org/show_bug.cgi?id=1863534#c3 the encodings that FF actually supports is active, maxBitrate, maxFramerate, priority, rid, scaledResolutionDownBy.

Because I know the intent is to remove them, we have know way of knowing when they may have existed or not in reality, and they are non-spec, I am completely removing those entries.

api/RTCRtpSender.json Outdated Show resolved Hide resolved
api/RTCRtpSender.json Outdated Show resolved Hide resolved
api/RTCRtpSender.json Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Collaborator

FYI @queengooborg The corresponding docs merged, so these questions need to be addressed please.

Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
@Elchi3 Elchi3 removed the needs content update 📝 This PR needs a corresponding update to mdn/content to update the documentation label Nov 13, 2023
api/RTCRtpSender.json Outdated Show resolved Hide resolved
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Safari 11 should probably be used (was 11 in the dictionary too)

api/RTCRtpSender.json Outdated Show resolved Hide resolved
api/RTCRtpSender.json Outdated Show resolved Hide resolved
api/RTCRtpSender.json Outdated Show resolved Hide resolved
api/RTCRtpSender.json Outdated Show resolved Hide resolved
api/RTCRtpSender.json Outdated Show resolved Hide resolved
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thanks @queengooborg and @hamishwillee! 👍 I've cleaned this up so that it can be merged. We can always send follow-up PRs to correct the data further.

@Elchi3 Elchi3 merged commit 80f84dd into mdn:main Nov 13, 2023
4 checks passed
@queengooborg queengooborg deleted the api/RTCRtpEncodingParameters/removal branch November 13, 2023 20:31
@hamishwillee
Copy link
Collaborator

Thanks very much!

Elchi3 added a commit to Elchi3/browser-compat-data that referenced this pull request Nov 14, 2023
* Remove RTCRtpEncodingParameters API from BCD

This PR removes the `RTCRtpEncodingParameters` API from BCD. This feature is a type (ex. a dictionary, enum, mixin, constant or WebIDL typedef) that we have explicitly stated not to document separately from the feature(s) that depend on it, as they are virtually invisible to the end developer.

* Fix consistency

* Remove never supported parameters

Co-authored-by: Hamish Willee <hamishwillee@gmail.com>

* Safari 12.1 -> Safari 11

* Safari 11 for setParameters (like getParameters)

* Safari 11 for encoding tree as well

---------

Co-authored-by: Florian Scholz <fs@florianscholz.com>
Co-authored-by: Hamish Willee <hamishwillee@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API project: dictionary removal https://github.com/mdn/browser-compat-data/issues/6810
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants