-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: reverse VP8 simulcast encodings for low-res Chromium (fixes #2939) #2948
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi, thanks for your contribution! |
|
I have made the changes changing the check from !firefox to isChromiumBased() |
|
Ok se when we reversed the array, SIM_LAYERS[0] no longer meant 4.0, it meant 1.0! This broke the activation logic. I used the values like 4.0 and 1.0 to solve this error |
|
This will break the default encoding behavior on Chromium which has always been 4:2:1 |
|
Have you tested this on older versions of Chromium? What about other codecs that support simulcast, AV1 and VP9? |
|
@jallamsetty1 |
|
I'm wondering what the value of simulcast is at those resolutions? Would disabling it be acceptable there @jallamsetty1 ? |
|
The correct way to fix this would be to configure the correct number of encodings based on the captured resolution. If only one layer is needed then we would be effectively disabling simulcast. |
can i work on it? |
Yes go ahead and give it a try. |
jallamsetty1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be fixed by configuring the right number of encodings based on the captured resolution.
|
@jallamsetty1 So we need to dynamically configure the number of simulcast encodings based on resolution instead of hardcoding |
|
The code checks the captured video resolution and determines how many simulcast layers are appropriate (1, 2, or 3). |
How did you test if your solution is working as expected? If you already determining the # of simulcast layers based on the capture resolution, why do you have to reverse the encoding order? |
|
@jallamsetty1 I checked it on Chrome and tested it using WebRTC internal, although my camera doesnt support 720p or more. I tested it for the low resolution ones and found single encoding in the peer connection. |
|
@jallamsetty1 did you get to check the issues? |
| } | ||
|
|
||
| return standardSimulcastEncodings; | ||
| // Dynamically determine number of simulcast layers based on resolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this code getting executed if it after the return statement? Am I missing something here?
|
|
||
| return standardSimulcastEncodings; | ||
| // Dynamically determine number of simulcast layers based on resolution | ||
| let numLayers = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing the encodings config here based on the captured resolution, you should figure out a way to change the number of simulcast layers needed and configure
| export const SIM_LAYERS = [ |
|
@jallamsetty1 please could you rate this approach
|
Problem
VP8 simulcast on Chromium at resolutions <640px transmits heavily downscaled video:
Root cause: Chromium limits simulcast layers at low resolutions and picks the first encoding. Default order [4x, 2x, 1x] causes it to select the 4× downscaled layer.
Solution
Reverse encoding order to [1x, 2x, 4x] for VP8 camera tracks <640px on Chromium, ensuring the highest quality layer is selected.
Changes
modules/RTC/TPCUtils.ts: Added conditional reversal in_getVideoStreamEncodings()modules/RTC/TPCUtils.spec.ts: Added tests for 320px and 480px resolutionsFixes #2939