Change default degradation preference by video source - #991
Conversation
🦋 Changeset detectedLatest commit: 85a76ee The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
@adrian-niculescu @davidliu , I broke down default degradation preference changes from #973 to keep the discussion more focused.
Please review this PR.
|
Diffuse output: AARJAR |
adrian-niculescu
left a comment
adrian-niculescu
left a comment
There was a problem hiding this comment.
The backup codec sender never gets the preference.
publishAdditionalCodecForTrack creates a second transceiver over the same rtcTrack and never touches sender.parameters, so that sender keeps resolving implicitly from the native source's is_screencast. Consequences:
- Camera and screen share stay media-equivalent across the two encoders as long as
options.sourceagrees withtrack.options.isScreencast. A screencast-backed track published withsource = CAMERAnow diverges: primary MAINTAIN_FRAMERATE, backup MAINTAIN_RESOLUTION. - An application-supplied
degradationPreferencereaches the primary encoder only. - The new BALANCED fallback reaches the primary encoder only.
The explicit-override half predates this PR and JS has the same gap, but this PR is what makes a resolved preference a value worth carrying, so applying it to the backup sender belongs here.
| return when (source) { | ||
| Track.Source.CAMERA -> RtpParameters.DegradationPreference.MAINTAIN_FRAMERATE | ||
| Track.Source.SCREEN_SHARE -> RtpParameters.DegradationPreference.MAINTAIN_RESOLUTION | ||
| else -> RtpParameters.DegradationPreference.BALANCED |
There was a problem hiding this comment.
A custom feed published with VideoTrackPublishOptions(source = Track.Source.UNKNOWN) lands here, and this overwrites what libwebrtc would have derived from the native source: a screencast-backed track goes MAINTAIN_RESOLUTION to BALANCED, a camera-like one MAINTAIN_FRAMERATE to BALANCED. track.options.isScreencast already carries that information. BALANCED is also the mode libwebrtc keeps behind the WebRTC-Video-BalancedDegradation field trial, with the in-tree note that it "needs to be tuned first".
Proposal:
private fun getDefaultDegradationPreference(source: Track.Source): RtpParameters.DegradationPreference? {
return when (source) {
Track.Source.CAMERA -> RtpParameters.DegradationPreference.MAINTAIN_FRAMERATE
Track.Source.SCREEN_SHARE -> RtpParameters.DegradationPreference.MAINTAIN_RESOLUTION
else -> null
}
}The KDoc bullet above changes with it. Not a blocker: JS made the same BALANCED choice deliberately, so if this is cross-SDK alignment it stands, but the argument applies there too.
There was a problem hiding this comment.
I think BALANCED is fine for the unknown source here.
And balanced mode is set when the source is neither camera nor screen share, which requires an app to explicitly declare something else, source defaults to null and resolves to Camera/ScreenShare from isScreencast. And it only takes effect when the app hasn't set degradationPreference itself, so anyone who wants a specific behavior (including libwebrtc's implicit derivation) can name it directly.
I ran this locally in both good and constrained network conditions and personally found BALANCED to work pretty well, even without further tuning on the WebRTC side. And for a feed the app has declined to label as motion or detail, a balanced tradeoff seems like the more honest default than committing hard to either axis.
Once this PR is landed, I am going to make follow-up on other SDKs to follow what this PR is doing.
Good catch. Confirmed: degradation preference is a sender level property (a top level field on RtpParameters, not per encoding), and publishAdditionalCodecForTrack adds a second transceiver over the same rtc track, so the backup codec has its own sender with its own parameters, which we were never setting. Worth noting both senders sink from the same VideoSource, and VideoBroadcaster::UpdateWants takes the MIN of max_pixel_count and max_framerate_fps across sinks, so a diverging backup drags its restriction onto the primary too. Applied the resolved preference to the backup sender, resolving the source the same way the primary publish does rather than reading it back off the publication, so the two can't disagree. Added tests asserting both senders match for the default camera, default screen share, explicit-override, and screencast-backed-published-as-camera cases. I will port the same fix to other sdks once landing this PR. |
|
Hi @adrian-niculescu , thanks for the comments, can you take another look ? |
adrian-niculescu
left a comment
adrian-niculescu
left a comment
There was a problem hiding this comment.
Read the changes and your comments. LGTM, thanks!
|
Thanks @adrian-niculescu and @davidliu |
3 participants
Summary
degradationPreferenceis not explicitly set.MAINTAIN_FRAMERATE.MAINTAIN_RESOLUTION.BALANCED.degradationPreferenceoverrides.Testing
git diff --checkAGENT_ERROR,PUBLISH_DATA_TRACK_RESPONSE,clientProtocol, etc.).