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

Fixes ordering issue with sendEncodings #976

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

digitalix
Copy link
Contributor

@digitalix digitalix commented Jun 5, 2022

This pr fixes the issue with incorrect order of RTCRtpEncodings. Currently the order will always be reversed.

For example, a list of [h, m, l] will be turned into [l, m, h] which wouldn't be much of a problem if it wasn't breaking SVC. This fix simply makes sure the order will be maintained.

In addition, here is example how SVC should be enabled https://github.com/w3c/webrtc-svc/blob/main/explainer.md#example-of-setting-a-scalabilitymode and my fix makes it compatible.

Copy link
Member

@ycherniavskyi ycherniavskyi left a comment

Choose a reason for hiding this comment

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

It will be much easier to review and merge if you provide at last some general description (possible with links to spec/doc) of your pull request.

common/darwin/Classes/FlutterRTCPeerConnection.m Outdated Show resolved Hide resolved
Previously with a list of [h, m, l] we were
converting them to [l, m, h] which would prevent
SVC from working. Unless the encodings were provided
in the reversed order.
Copy link
Member

@ycherniavskyi ycherniavskyi left a comment

Choose a reason for hiding this comment

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

I just get dipper in history of this lines and found that your commit revert the 40bde2a.
That is why for sure we need to add @cloudwebrtc in this discussion.

@digitalix
Copy link
Contributor Author

@cloudwebrtc any update on this?

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.

None yet

2 participants