Conversation
A duplicate SubscribedQualityUpdate arriving while the backup codec
publication is still in flight (sender not yet attached) re-enters
addSimulcastTrack and crashed the app with
IllegalStateException("VP8 already added!").
Make addSimulcastTrack idempotent: log and return null for an
already-added codec, and bail out of publishAdditionalCodecForTrack so
no duplicate transceiver or AddTrackRequest is created. Mirrors the JS
SDK behavior. Dedup is per track instance, so a track republished after
a reconnect still publishes its backup codec normally.
Fixes livekit#1000
🦋 Changeset detectedLatest commit: 39f3ce4 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 |
|
Hi @davidliu @MaxHeimbrock @xianshijing-lk . Any chance of getting this fix merged? This is an active issue affecting production. Thanks! |
|
I am taking a look |
| fun duplicateSubscribedQualityUpdateDoesNotRepublishBackupCodec() = runTest { | ||
| room.videoTrackPublishDefaults = room.videoTrackPublishDefaults.copy( | ||
| videoCodec = VideoCodec.VP9.codecName, | ||
| scalabilityMode = "L3T3", | ||
| backupCodec = BackupVideoCodec(codec = VideoCodec.VP8.codecName), | ||
| ) | ||
|
|
||
| connect() | ||
| val videoTrack = createLocalTrack() | ||
| room.localParticipant.publishVideoTrack(videoTrack) | ||
|
|
||
| val trackSid = room.localParticipant.videoTrackPublications.first().first.sid | ||
| receiveSubscribedQualityUpdate(trackSid) | ||
| receiveSubscribedQualityUpdate(trackSid) | ||
|
|
||
| assertEquals(2, getPublisherPeerConnection().transceivers.size) | ||
| } |
There was a problem hiding this comment.
This test already passes on main today, so it is not testing the fix of your issue. Can you please remove it?
There was a problem hiding this comment.
Looks like there's a pretty specific hard to test window to get the original situation to pass, and the test is a reasonable guard to have overall anyways, so I'm going to keep this one and merge.
|
Otherwise it looks good and I agree with the proposed changes. |
…#1010) A duplicate SubscribedQualityUpdate arriving while the backup codec publication is still in flight (sender not yet attached) re-enters addSimulcastTrack and crashed the app with IllegalStateException("VP8 already added!"). Make addSimulcastTrack idempotent: log and return null for an already-added codec, and bail out of publishAdditionalCodecForTrack so no duplicate transceiver or AddTrackRequest is created. Mirrors the JS SDK behavior. Dedup is per track instance, so a track republished after a reconnect still publishes its backup codec normally. Fixes livekit#1000 (cherry picked from commit 37e5a2a)
Fixes #1000
Problem
When the server sends a
SubscribedQualityUpdaterequesting a backup codec that is already added (or whose publication is still in flight), the SDK crashes the app with:The exception is thrown on the SDK's internal coroutine dispatcher (
DefaultDispatcher-worker-N), so there is no way for the application to catch it — it takes down the whole process during a live call.We hit this in production (self-hosted SFU, publisher on VP9 with the default VP8 backup,
PREFER_REGRESSION). Repeated identical updates are expected server behavior by design:MediaTrack.Restart()(ICE restart / session resume) andMediaTrack.SetMuted(false)(unmuting / enabling the camera) both re-emit the current state —dynacastManager.ForceUpdate()explicitly skips the "changed" check. A client therefore has to handle a repeatedSubscribedQualityUpdateidempotently.Root cause
There is a window between the two updates in which the codec entry exists but its sender is not yet attached:
setPublishingCodecsfinds no entry for VP8 → returns it as a new codec →publishAdditionalCodecForTrackcallsaddSimulcastTrack, which inserts the entry, and then launches a coroutine to create the transceiver.simulcastTrackInfo.senderis only assigned inside that coroutine, after the signaling round-trip.setPublishingCodecsseessimulcastInfo?.sender == nulland treats the codec as new again →addSimulcastTrackfinds the key already present and throws.The check at
setPublishingCodecsconflates "never started" with "started, still in flight".Fix
Make
addSimulcastTrackidempotent: if the codec is already present, log a warning and returnnullinstead of throwing;publishAdditionalCodecForTracktreatsnullas a no-op and returns, so no duplicate transceiver is created and no duplicateAddTrackRequestis sent.This mirrors the JS SDK, where
LocalVideoTrack.addSimulcastTracklogs and returnsundefinedfor an already-added codec and the caller bails out.Note the dedup is intentionally per track instance (the
simulcastCodecsmap lives on theLocalVideoTrack): a track republished after a reconnect is a new track/sid and must still go through backup-codec publication normally. Session-level dedup would break that case.Testing
addSimulcastTrackForAlreadyAddedCodecIsNoOp— callingaddSimulcastTracktwice for the same codec no longer throws; the second call returnsnull.duplicateSubscribedQualityUpdateDoesNotRepublishBackupCodec— receiving the sameSubscribedQualityUpdatetwice keeps a single backup transceiver (2 transceivers total).LocalParticipantMockE2ETestsuite passes.