Always announce video layers on SetPublisher - #1765
Open
PratimMallick wants to merge 1 commit into
Open
Conversation
Avoid empty-layer announces when the track is not LIVE yet, which caused ERROR_CODE_REQUEST_VALIDATION_FAILED and an unnecessary full rejoin. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
WalkthroughPublisher negotiation now handles SFU errors before SDP processing and rejoins on validation failures. Track-layer computation now includes non-live video tracks. Tests cover rejoin behavior and muted ended-track announcements. ChangesPublisher negotiation and error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Publisher
participant SFU
participant RejoinCallback
Publisher->>SFU: setPublisher
SFU-->>Publisher: validation error
Publisher->>RejoinCallback: rejoin
Publisher-->>Publisher: stop negotiation before SDP processing
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/PublisherTest.kt (1)
321-355: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that an error response does not apply remote SDP.
Add
coVerify(exactly = 0) { publisher.setRemoteDescription(any()) }. The test must protect the early-return contract for the emptysdpin this response.🤖 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/PublisherTest.kt` around lines 321 - 355, The SetPublisher validation-failure test must verify that an error response with empty sdp does not apply remote SDP. In `SetPublisher validation failure rejoins`, add an exact-zero verification for `publisher.setRemoteDescription(any())` while preserving the existing setPublisher and rejoin assertions.
🤖 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/src/main/kotlin/io/getstream/video/android/core/call/connection/Publisher.kt`:
- Around line 209-235: Wrap the negotiation flow in the relevant Publisher
method with try/finally immediately after setting isIceRestarting, and reset
isIceRestarting in the finally block. Ensure the SFU error return inside the
submit lambda, including the response.error handling around SetPublisherRequest,
still clears the flag before subsequent negotiations.
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/PublisherTest.kt`:
- Around line 293-355: Migrate the PublisherTest fixture to extend TestBase,
preserving its current initialization and cleanup behavior by moving or
retaining setup in the appropriate TestBase lifecycle hooks. Update the class
declaration and any affected fixture references so all existing tests, including
getAnnouncedTracks and SetPublisher validation failure rejoins, continue using
the same setup.
---
Nitpick comments:
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/PublisherTest.kt`:
- Around line 321-355: The SetPublisher validation-failure test must verify that
an error response with empty sdp does not apply remote SDP. In `SetPublisher
validation failure rejoins`, add an exact-zero verification for
`publisher.setRemoteDescription(any())` while preserving the existing
setPublisher and rejoin assertions.
🪄 Autofix
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 Plus
Run ID: 7839db21-21fd-43f0-9d05-e256a862ce25
📒 Files selected for processing (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Publisher.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/PublisherTest.kt
Comment on lines
209
to
+235
| if (response.error != null) { | ||
| logger.e { | ||
| "SetPublisherRequest Received error: ${response.error}, SetPublisherRequest: $request" | ||
| } | ||
| tracer.trace("negotiate-error-setpublisher", response.error.message ?: "unknown") | ||
| logger.e { "rejoin cause error in sfuClient.setPublisher, message:${response.error.message}" } | ||
|
|
||
| when (response.error.code) { | ||
| /** | ||
| * We are getting this error right away after joining the call first time | ||
| * Full error: 16:04:05.032 Call:PeerC...:publisher E (DefaultDispatcher-worker-17:526) SetPublisherRequest Received error: Error{code=ERROR_CODE_REQUEST_VALIDATION_FAILED, message=Invalid SetPublisher request, should_retry=false} | ||
| * This will cause emission of ParticipantLeftEvent to other person | ||
| * Historically common right after first join when video layers were omitted | ||
| * for non-LIVE tracks. Layers are now always computed from publish options, so | ||
| * remaining validation failures are treated as a real publisher/SFU mismatch. | ||
| */ | ||
| ErrorCode.ERROR_CODE_REQUEST_VALIDATION_FAILED -> rejoin() | ||
| ErrorCode.ERROR_CODE_REQUEST_VALIDATION_FAILED -> { | ||
| logger.e { | ||
| "rejoin cause error in sfuClient.setPublisher, " + | ||
| "message:${response.error.message}" | ||
| } | ||
| rejoin() | ||
| } | ||
|
|
||
| else -> {} | ||
| else -> { | ||
| logger.e { | ||
| "Unhandled SetPublisher error code=${response.error.code}, " + | ||
| "message=${response.error.message}" | ||
| } | ||
| } | ||
| } | ||
| return@submit |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reset isIceRestarting before returning on an SFU error.
When iceRestart is true, Line 235 exits the submit lambda before Line 249 resets isIceRestarting. Every later negotiation then exits at Lines 174-177 as if an ICE restart is still active.
Use try/finally after setting the flag so every error path clears it.
Proposed fix
isIceRestarting = iceRestart
- setLocalDescription(offer).onErrorSuspend {
- tracer.trace("negotiate-error-setlocaldescription", it.message ?: "unknown")
- }
- val request = SetPublisherRequest(
- sdp = offer.description,
- tracks = trackInfos,
- session_id = sessionId,
- )
- val response = sfuClient.setPublisher(request)
- if (response.error != null) {
- // existing error handling
- return@submit
- }
-
- logger.i { "Received answer: ${response.sdp}" }
- setRemoteDescription(SessionDescription(SessionDescription.Type.ANSWER, response.sdp))
+ try {
+ setLocalDescription(offer).onErrorSuspend {
+ tracer.trace("negotiate-error-setlocaldescription", it.message ?: "unknown")
+ }
+ val request = SetPublisherRequest(
+ sdp = offer.description,
+ tracks = trackInfos,
+ session_id = sessionId,
+ )
+ val response = sfuClient.setPublisher(request)
+ if (response.error != null) {
+ // existing error handling
+ return@submit
+ }
+
+ logger.i { "Received answer: ${response.sdp}" }
+ setRemoteDescription(SessionDescription(SessionDescription.Type.ANSWER, response.sdp))
+ } finally {
+ isIceRestarting = false
+ }
- isIceRestarting = false🤖 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/kotlin/io/getstream/video/android/core/call/connection/Publisher.kt`
around lines 209 - 235, Wrap the negotiation flow in the relevant Publisher
method with try/finally immediately after setting isIceRestarting, and reset
isIceRestarting in the finally block. Ensure the SFU error return inside the
submit lambda, including the response.error handling around SetPublisherRequest,
still clears the flag before subsequent negotiations.
Comment on lines
+293
to
+355
| @Test | ||
| fun `getAnnouncedTracks includes video layers even when track is not LIVE`() { | ||
| val mockVideoTrack = mockk<VideoTrack>(relaxed = true) { | ||
| every { id() } returns "video-1" | ||
| every { kind() } returns "video" | ||
| every { isDisposed } returns false | ||
| every { state() } returns MediaStreamTrack.State.ENDED | ||
| } | ||
| val mockSender = mockk<RtpSender>(relaxed = true) { | ||
| every { track() } returns mockVideoTrack | ||
| } | ||
| val mockTransceiver = mockk<RtpTransceiver>(relaxed = true) { | ||
| every { sender } returns mockSender | ||
| every { mid } returns "0" | ||
| } | ||
| every { mockTransceiverCache.items() } returns listOf( | ||
| TransceiverId(videoPublishOption, mockTransceiver), | ||
| ) | ||
| every { mockTransceiverCache.indexOf(videoPublishOption) } returns 0 | ||
| every { mockTransceiverCache.getLayers(videoPublishOption) } returns null | ||
|
|
||
| val announced = publisher.getAnnouncedTracks(null, null) | ||
|
|
||
| assertEquals(1, announced.size) | ||
| assertTrue(announced[0].muted) | ||
| assertTrue(announced[0].layers.isNotEmpty()) | ||
| } | ||
|
|
||
| @Test | ||
| fun `SetPublisher validation failure rejoins`() = runTest(coroutineContext) { | ||
| every { publisher.getAnnouncedTracks(any(), any()) } returns listOf( | ||
| TrackInfo( | ||
| track_id = "video-1", | ||
| track_type = TrackType.TRACK_TYPE_VIDEO, | ||
| mid = "0", | ||
| muted = false, | ||
| layers = listOf( | ||
| stream.video.sfu.models.VideoLayer( | ||
| rid = "f", | ||
| video_dimension = VideoDimension(1280, 720), | ||
| bitrate = 1_000_000, | ||
| fps = 30, | ||
| ), | ||
| ), | ||
| publish_option_id = videoPublishOption.id, | ||
| ), | ||
| ) | ||
| coEvery { publisher.setLocalDescription(any()) } returns Result.Success(Unit) | ||
| coEvery { publisher.setRemoteDescription(any()) } returns Result.Success(Unit) | ||
| coEvery { mockSignalServerService.setPublisher(any()) } returns SetPublisherResponse( | ||
| sdp = "", | ||
| error = Error( | ||
| code = ErrorCode.ERROR_CODE_REQUEST_VALIDATION_FAILED, | ||
| message = "Invalid SetPublisher request", | ||
| should_retry = false, | ||
| ), | ||
| ) | ||
|
|
||
| publisher.negotiate(source = "test") | ||
|
|
||
| coVerify(exactly = 1) { mockSignalServerService.setPublisher(any()) } | ||
| assertEquals(1, rejoinInvocations) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use TestBase for this unit-test fixture.
PublisherTest does not use TestBase. Migrate this fast unit-test class to TestBase and retain the existing setup in its lifecycle hooks.
As per coding guidelines, **/src/test/**/*.{kt,kts} requires TestBase for fast unit tests.
🤖 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/PublisherTest.kt`
around lines 293 - 355, Migrate the PublisherTest fixture to extend TestBase,
preserving its current initialization and cleanup behavior by moving or
retaining setup in the appropriate TestBase lifecycle hooks. Update the class
declaration and any affected fixture references so all existing tests, including
getAnnouncedTracks and SetPublisher validation failure rejoins, continue using
the same setup.
Source: Coding guidelines
Contributor
SDK Size Comparison 📏
|
|
aleksandar-apostolov
changed the title
| } | ||
| } | ||
| } | ||
| return@submit |
Contributor
There was a problem hiding this comment.
safeCall is inline, so this return@submit returns out of the whole submit lambda and skips the isIceRestarting = false reset at the end of negotiate().
On an iceRestart = true negotiate that errors here, the flag stays true and every later negotiate() no-ops on the isIceRestarting guard — renegotiation is dead until a full reconnect. The else -> branch is exposed (no rejoin()); before this PR the error path fell through and always reset. Regression.
Suggest a try/catch/finally instead of safeCall — throwable stays visible and finally guarantees the reset. Plus a regression test: iceRestart = true + non-validation error, assert the next negotiate() isn't skipped.
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
2 participants
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
Fixes AND-1377
Prevent unnecessary full rejoins when the first
SetPublisherraces ahead of the video track becomingLIVE.On first join, Android often negotiated while
track.state() != LIVE. Layer computation was gated on LIVE, so the announce went out with emptylayers. The SFU rejected that withERROR_CODE_REQUEST_VALIDATION_FAILED/Invalid SetPublisher request, andPublisherrejoined — flashing reconnect UI and emittingParticipantLeftto peers. The second session usually succeeded.Seen in dogfood and Blink (
default:019f45a0-1388-70ab-970d-ee1b4d195d3a).Dashboard: https://beta.dashboard.getstream.io/organization/1234199/1336271/video/call/viewer/default:019f45a0-1388-70ab-970d-ee1b4d195d3a/
Implementation
PublishOptionintoTrackInfo, even when the track is not LIVE (TrackInfo.mutedcan still reflect!LIVE).ERROR_CODE_REQUEST_VALIDATION_FAILED→rejoin()for remaining real mismatches (aligned with prior recovery that fixed persistent validation failures).Testing
./gradlew :stream-video-android-core:testDebugUnitTest --tests "io.getstream.video.android.core.call.connection.PublisherTest"— passedmuted=truewith nonemptyq/h/flayers; SetPublisher succeeds without validation rejoinshould_retry(this validation case isshould_retry=false). Android still rejoins on remaining validation failures after the layers fix; that recovery path is intentional.Evidence from dogfood
☑️Contributor Checklist
General
developbranchCode & documentation
stream-video-examples)☑️Reviewer Checklist
🎉 GIF
N/A — analytics/RTC publish path fix, no UI change.
Made with Cursor
Summary by CodeRabbit