feat(audio): negotiate stereo opus on the subscriber answer - #1007
feat(audio): negotiate stereo opus on the subscriber answer#1007kirill-jjj wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: d787073 The changes in this PR will be included in the next version bump. 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ment stereo option Addresses Devin review feedback on livekit#1007: - if the munged answer is rejected by libwebrtc, retry with the original answer and send that instead of leaving the subscriber without a local description (mirrors PeerConnectionTransport.setMungedSdp) - add KDoc to LocalParticipant.AudioTrackPublishOptions.stereo
davidliu
left a comment
davidliu
left a comment
There was a problem hiding this comment.
This really first needs a PR on https://github.com/webrtc-sdk/webrtc to expose the useStereoInput/useStereoOutput, because a lot of it creates potential state mismatches between what the JavaAudioDeviceModule is actually configured to do and what the user thinks these options will do.
Separately, I think this might be better served as two separate PRs, one for the subscriber side and one on the publisher side.
| } | ||
| // Note: stereo is sourced from options, not introspected from | ||
| // JavaAudioDeviceModule; callers that enable stereo input via a module | ||
| // customizer should set LocalAudioTrackOptions.stereo to match. |
There was a problem hiding this comment.
this would need to be surfaced to users, and ideally should be sourced from JavaAudioDeviceModule entirely, to avoid a mismatch in state.
| /** | ||
| * Publish the track as stereo, sent as `stereo` in the AddTrackRequest. | ||
| */ | ||
| val stereo: Boolean = false, |
There was a problem hiding this comment.
This should be in BaseAudioTrackPublishOptions, and the stereo option should be added to the end of the list, so that it won't break existing constructors who use positional arguments.
| return@launch | ||
| } | ||
| } | ||
| client.sendAnswer(answer, offerId) |
There was a problem hiding this comment.
rather than nesting the fallback here, should return from the run block with an indicator that it should try the fallback in a separate same-level block.
| override val dtx: Boolean = true, | ||
| override val red: Boolean = true, | ||
| /** | ||
| * Publish the track as stereo, sent as `stereo` in the AddTrackRequest. |
There was a problem hiding this comment.
AddTrackRequest is an internal class the consumers don't need to know about.
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| 'livekit-android': patch | |||
There was a problem hiding this comment.
There are API changes in here, so it should be marked as a minor upgrade.
Adds stereo=1 to the Opus fmtp line of the subscriber answer for media sections where the server offer advertised sprop-stereo=1, so stereo tracks published by other participants are not downmixed to mono. Falls back to the un-munged answer if setLocalDescription fails. Subscriber-only scope; publisher-side stereo options to follow separately after useStereoInput/useStereoOutput exposure in webrtc-sdk/webrtc. Generated with Codebuff 🤖 Co-Authored-By: Codebuff <noreply@codebuff.com>
onServerOffer exceeded detekt's cyclomatic complexity threshold (16 > 15) after adding the fallback path. Generated with Codebuff 🤖 Co-Authored-By: Codebuff <noreply@codebuff.com>
Generic Either is not smart-cast from an if-check; when() narrows the Right branch for .value access. Generated with Codebuff 🤖 Co-Authored-By: Codebuff <noreply@codebuff.com>
0212c81 to
4e07201
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
If the answer already carried stereo=0, appending stereo=1 left two conflicting values and the decoder could stay mono. Drop any existing stereo=N and append stereo=1. Generated with Codebuff 🤖 Co-Authored-By: Codebuff <noreply@codebuff.com>
3 participants
Problem
When a remote participant publishes a stereo audio track, Android subscribers receive it as mono. libwebrtc's native Opus decoder downmixes incoming stereo packets to mono unless the local answer negotiates
stereo=1in the fmtp line — even when the server offer correctly announces the track withsprop-stereo=1.Observed on real hardware (Android 10): the track is published with
stereo=1, the server relayssprop-stereo=1in the subscriber offer — yet the decoded audio plays back mono untilstereo=1is added to the answer.Related issues
setUseStereoInput(true), but that only covers the publisher side. The subscriber side remained broken.remoteStereoMidsfrom the offer and rewrites the matching fmtp lines when creating the answer (PCTransport.ts).Changes (receive path only)
StereoSdpMunging.kt:ensureStereoOpus()extracts mids carryingsprop-stereo=1from the server offer and addsstereo=1to the matching fmtp lines of the answer. Fully fail-safe on parse failures.RTCEngine.onServerOffermunges the answer beforesetLocalDescription; if setting the munged answer fails, it falls back to the un-munged one (mirrorsPeerConnectionTransport.setMungedSdp). The fallback lives in a dedicated helper to keeponServerOffercomplexity in check.patch— no public API changes).Scope note (per review)
The publisher-side options (
LocalAudioTrackOptions.stereo,TF_STEREO,AddTrackRequest.stereo) were removed from this PR and will follow separately, afteruseStereoInput/useStereoOutputare exposed by webrtc-sdk/webrtc — so an SDK-level flag can never mismatch whatJavaAudioDeviceModuleis actually configured to do.Testing
livekit-android-test, using SDP fixtures captured from a real LiveKit sessionAssisted by AI; verified end-to-end on real hardware.