feat(core): pick up SFU DegradationPreference and add WebRTC mapper#1699
Open
PratimMallick wants to merge 2 commits into
Open
feat(core): pick up SFU DegradationPreference and add WebRTC mapper#1699PratimMallick wants to merge 2 commits into
PratimMallick wants to merge 2 commits into
Conversation
Regenerate SFU protos to pick up the new DegradationPreference enum and its degradation_preference fields on PublishOption and VideoSender (plus TrackInfo.self_sub_audio_video), and refresh the public API dump. Add toRtcDegradationPreference() converting the SFU enum to org.webrtc.RtpParameters.DegradationPreference, returning null for UNSPECIFIED so callers can keep the current value. Includes unit tests covering every enum variant. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
WalkthroughThis PR extends the Stream Video Android SDK with degradation preference support. A new ChangesDegradation Preference Support
🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
stream-video-android-core/src/main/proto/video/sfu/models/models.proto (1)
229-229: 💤 Low valueConsider adding a documentation comment for the new field.
Other fields in
TrackInfoinclude brief comments (e.g.,// for audio tracks). Adding a comment here would clarify the purpose ofself_sub_audio_videofor SDK consumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-video-android-core/src/main/proto/video/sfu/models/models.proto` at line 229, Add a brief documentation comment for the TrackInfo field self_sub_audio_video that explains its purpose and usage (e.g., whether the local participant is subscribed to their own audio and video streams), following the style of other comments in TrackInfo (like "// for audio tracks") so SDK consumers understand what true/false means and any relevant caveats.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@stream-video-android-core/api/stream-video-android-core.api`:
- Around line 17141-17144: The API changed by adding a DegradationPreference
parameter (and a new getter on TrackInfo), breaking binary compatibility for
callers of the all-args constructors and copy(...) methods for VideoSender,
PublishOption and TrackInfo; restore compatibility by adding deprecated overload
shims that match the previous JVM signatures (constructors and copy methods
without DegradationPreference) which delegate to the new constructors/copy
methods supplying a sensible default DegradationPreference, and for TrackInfo
add the legacy getter getSelf_sub_audio_video() that delegates to the new
property; ensure these shims are public and annotated/deprecated so existing
compiled consumers keep working while signaling the intended breaking change.
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/utils/DegradationPreferenceMapperTest.kt`:
- Line 25: Update the DegradationPreferenceMapperTest class to extend the
project test harness by making it inherit from TestBase (replace the current
plain class declaration with one that subclasses TestBase) and add any necessary
import for TestBase; ensure the test class name DegradationPreferenceMapperTest
uses TestBase so it runs as a fast unit test under the repo conventions.
---
Nitpick comments:
In `@stream-video-android-core/src/main/proto/video/sfu/models/models.proto`:
- Line 229: Add a brief documentation comment for the TrackInfo field
self_sub_audio_video that explains its purpose and usage (e.g., whether the
local participant is subscribed to their own audio and video streams), following
the style of other comments in TrackInfo (like "// for audio tracks") so SDK
consumers understand what true/false means and any relevant caveats.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8a6dff3c-b795-4ea3-a731-6cc3005c0fba
⛔ Files ignored due to path filters (4)
stream-video-android-core/src/main/proto/video/sfu/event/events.pb.gois excluded by!**/*.pb.gostream-video-android-core/src/main/proto/video/sfu/event/events_vtproto.pb.gois excluded by!**/*.pb.gostream-video-android-core/src/main/proto/video/sfu/models/models.pb.gois excluded by!**/*.pb.gostream-video-android-core/src/main/proto/video/sfu/models/models_vtproto.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
stream-video-android-core/api/stream-video-android-core.apistream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/utils/DegradationPreferenceMapper.ktstream-video-android-core/src/main/proto/video/sfu/event/events.protostream-video-android-core/src/main/proto/video/sfu/models/models.protostream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/utils/DegradationPreferenceMapperTest.kt
Comment on lines
+17141
to
+17144
| public fun <init> (Lstream/video/sfu/models/Codec;Ljava/util/List;Lstream/video/sfu/models/TrackType;ILstream/video/sfu/models/DegradationPreference;Lokio/ByteString;)V | ||
| public synthetic fun <init> (Lstream/video/sfu/models/Codec;Ljava/util/List;Lstream/video/sfu/models/TrackType;ILstream/video/sfu/models/DegradationPreference;Lokio/ByteString;ILkotlin/jvm/internal/DefaultConstructorMarker;)V | ||
| public final fun copy (Lstream/video/sfu/models/Codec;Ljava/util/List;Lstream/video/sfu/models/TrackType;ILstream/video/sfu/models/DegradationPreference;Lokio/ByteString;)Lstream/video/sfu/event/VideoSender; | ||
| public static synthetic fun copy$default (Lstream/video/sfu/event/VideoSender;Lstream/video/sfu/models/Codec;Ljava/util/List;Lstream/video/sfu/models/TrackType;ILstream/video/sfu/models/DegradationPreference;Lokio/ByteString;ILjava/lang/Object;)Lstream/video/sfu/event/VideoSender; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check whether repo docs define ABI/API compatibility expectations.
rg -n --iglob '*.md' --iglob '*.kts' --iglob '*.gradle.kts' \
'binary compat|binary compatibility|api compatibility|semver|breaking change'
# 2) Inspect this API dump around affected classes/signatures to confirm removed/changed ctor descriptors.
rg -n 'class stream/video/sfu/event/VideoSender|class stream/video/sfu/models/PublishOption|class stream/video/sfu/models/TrackInfo|fun <init> \(|fun copy \(' \
stream-video-android-core/api/stream-video-android-core.api -A6 -B3Repository: GetStream/stream-video-android
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="stream-video-android-core/api/stream-video-android-core.api"
echo "== AGENTS.md ABI compatibility guideline =="
sed -n '30,60p' AGENTS.md || true
echo
echo "== stream-video-android-core.api around 17141-17144 =="
sed -n '17130,17160p' "$FILE" | nl -ba -w2 -s':'
echo
echo "== stream-video-android-core.api around 17830-17833 =="
sed -n '17820,17850p' "$FILE" | nl -ba -w2 -s':'
echo
echo "== stream-video-android-core.api around 18035-18038 =="
sed -n '18025,18060p' "$FILE" | nl -ba -w2 -s':'Repository: GetStream/stream-video-android
Length of output: 2554
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="stream-video-android-core/api/stream-video-android-core.api"
echo "== stream-video-android-core.api exact 17141-17144 =="
sed -n '17141,17144p' "$FILE"
echo
echo "== stream-video-android-core.api around 17141 (context) =="
sed -n '17135,17150p' "$FILE"
echo
echo "== stream-video-android-core.api exact 17830-17833 =="
sed -n '17830,17833p' "$FILE"
echo
echo "== stream-video-android-core.api around 17830 (context) =="
sed -n '17825,17840p' "$FILE"
echo
echo "== stream-video-android-core.api exact 18035-18038 =="
sed -n '18035,18038p' "$FILE"
echo
echo "== stream-video-android-core.api around 18035 (context) =="
sed -n '18030,18050p' "$FILE"Repository: GetStream/stream-video-android
Length of output: 8093
Potential binary-compatibility break from generated Wire model constructor/copy signature changes
stream-video-android-core/api/stream-video-android-core.api shows updated public all-args constructor + copy(...) signatures that remove the previous JVM call sites for:
stream/video/sfu/event/VideoSender(addedDegradationPreference), lines 17141-17144stream/video/sfu/models/PublishOption(addedDegradationPreference), lines 17830-17833stream/video/sfu/models/TrackInfo(constructor/copy signature change + newgetSelf_sub_audio_video()), lines 18035-18038
Since AGENTS.md requires Maven-shipped modules to retain binary compatibility, this likely breaks already-compiled consumers that directly call those constructors/copy methods—either add binary-compatible overload shims (deprecated) or document/bump as an intentional breaking change with migration notes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stream-video-android-core/api/stream-video-android-core.api` around lines
17141 - 17144, The API changed by adding a DegradationPreference parameter (and
a new getter on TrackInfo), breaking binary compatibility for callers of the
all-args constructors and copy(...) methods for VideoSender, PublishOption and
TrackInfo; restore compatibility by adding deprecated overload shims that match
the previous JVM signatures (constructors and copy methods without
DegradationPreference) which delegate to the new constructors/copy methods
supplying a sensible default DegradationPreference, and for TrackInfo add the
legacy getter getSelf_sub_audio_video() that delegates to the new property;
ensure these shims are public and annotated/deprecated so existing compiled
consumers keep working while signaling the intended breaking change.
| import org.webrtc.RtpParameters | ||
| import stream.video.sfu.models.DegradationPreference | ||
|
|
||
| class DegradationPreferenceMapperTest { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Adopt TestBase for this unit test class.
Line 25 should use the project test base for unit tests to stay consistent with the repo test harness conventions.
As per coding guidelines, "Use TestBase for fast unit tests and IntegrationTestBase for end-to-end call flows".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/utils/DegradationPreferenceMapperTest.kt`
at line 25, Update the DegradationPreferenceMapperTest class to extend the
project test harness by making it inherit from TestBase (replace the current
plain class declaration with one that subclasses TestBase) and add any necessary
import for TestBase; ensure the test class name DegradationPreferenceMapperTest
uses TestBase so it runs as a fast unit test under the repo conventions.
Contributor
SDK Size Comparison 📏
|
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Goal
Close Closes AND-1192
Honors the DegradationPreference that the SFU now sends on
PublishOption(initial publish) andVideoSender(runtime quality changes), so the WebRTC layer optimizes for the right tradeoff (framerate vs. resolution) under constrained bandwidth instead of usingbalancedalwaysImplementation
Protos: regenerated SFU protos to pick up the new DegradationPreference enum and degradation_preference fields on PublishOption and VideoSender (plus TrackInfo.self_sub_audio_video). Public API dump refreshed accordingly.Mapper(utils/DegradationPreferenceMapper.kt): SfuDegradationPreference.toRtcDegradationPreference() maps the SFU enum to org.webrtc.RtpParameters.DegradationPreference, returning null for UNSPECIFIED so callers can keep the current value.Publisher:applyDegradationPreference(transceiver, publishOption)runs right after connection.addTransceiver(...) and only writes sender.parameters when the SFU set a preference and it actually differs from the current value.changePublishQuality(videoSender), the SFU-provided preference is folded into the existing changed flag, so a quality update that only flips the preference still triggers a single setParameters(...) call, while UNSPECIFIED never forces an unnecessary write.Testing
☑️Contributor Checklist
General
developbranchCode & documentation
stream-video-examples)☑️Reviewer Checklist
🎉 GIF
Please provide a suitable gif that describes your work on this pull request
Summary by CodeRabbit