Add per-frame encoding time in video encoding CPU connection health policy #2986
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shi-su
force-pushed
the
per-frame-encode
branch
2 times, most recently
from
603adb9 to
b2224c2
Compare
shi-su
force-pushed
the
per-frame-encode
branch
from
b2224c2 to
9c35809
Compare
| @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ### Changed | |||
|
|
|||
| - Add per-frame encoding time in video encoding CPU connection health policy | |||
There was a problem hiding this comment.
I messed this up in a previous PR (David's commit added Unreleased section). Can you put your change in the section below? I will move David's change in my PR.
| } | ||
|
|
||
| health(): number { | ||
| const videoEncodingTimeIsHigh = | ||
| this.currentData.videoEncodingTimeInMs >= this.highEncodeCpuMsThreshold && |
There was a problem hiding this comment.
It seems like we don't need both checks. But I assume you have a good reason already? :)
If so, maybe it can be a comment
There was a problem hiding this comment.
Having both checks there is to avoid degrading codec in two conditions:
- When framerate is low but per-frame encoding time is high (e.g., high resolution screen share with low motion on screen, encoder sometimes just encode like one frame every second. There's no need to degrade codec if each of these frames takes 50 ms to encode);
- When frame rate is high (e.g., high framerate and SVC at the same time).
I don't think these two conditions can be excluded with the same check. So kept both here.
There was a problem hiding this comment.
Added a comment to explain the logic
hensmi-amazon
approved these changes
Nov 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #:
Description of changes:
This change is to add per-frame encoding time as an additional criterion in video encoding CPU connection health policy. Coded will only degrade when both total encode time and per-frame encode time is above threshold. The purpose is to avoid codec degradation with high framerate SVC. Note that video encoder counts each spatial layer of a frame as a frame encoded (e.g., 30 fps video with L3T3 scalability mode has a encoded framerate of 90 fps), the default value of per-frame encode time threshold has this pattern considered.
Testing:
Smoke tested and verified video codec degradation only when both total encode time and per-frame encode time is above threshold.
Can these tested using a demo application? Please provide reproducible step-by-step instructions.
No
Checklist:
Have you successfully run
npm run build:releaselocally?y
Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?
n
Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved?
n
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.