From 8a49486b37424192c4049b36cf5ba371e28bcd0e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 6 May 2026 19:36:05 +0000 Subject: [PATCH] fix(nests): G4+G5 plumb catalog sampleRate through decoder + AudioTrack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit G4: Audit-9 (catalog-driven decoder reconfig) plumbed numberOfChannels end-to-end but left sampleRate hardcoded at AudioFormat.SAMPLE_RATE_HZ in MediaCodecOpusDecoder.buildOpusIdHeader / buildFormat and in AudioTrackPlayer's MinBufferSize / 250 ms target / setSampleRate calls. For Opus this is benign in practice — Codec2 always emits 48 kHz PCM regardless of OpusHead inputSampleRate — but hardcoding the constant means a future codec or container variant whose decoder DOES respect input sample rate would mis-clock playback. And the OpusHead identification header should match what the catalog declares either way. Thread sampleRate alongside channelCount through every layer: - MediaCodecOpusDecoder constructor takes `sampleRate: Int = AudioFormat.SAMPLE_RATE_HZ`. buildFormat and buildOpusIdHeader both take it as a parameter. - AudioTrackPlayer constructor takes the same. Used in AudioTrack.getMinBufferSize, the 250 ms-equivalent target bytes calculation (`(sampleRate / 4) * BYTES_PER_SAMPLE * channelCount`), AndroidAudioFormat.Builder.setSampleRate, and the diagnostic log. - decoderFactory and playerFactory in NestViewModel become `(channelCount: Int, sampleRate: Int) -> ...`. - awaitDecoderChannelCount → awaitAudioPipelineConfig, returning a private `AudioPipelineConfig(channelCount, sampleRate)` struct so openSubscription handles both fields uniformly. `sampleRate` falls back to AudioFormat.SAMPLE_RATE_HZ on timeout / non-positive declaration with a warning log. - NestViewModelFactory + NestViewModelTest updated to the two-arg factory shape. G5: documented the SUBSCRIBE_BUFFER safety budget on CATALOG_AWAIT_TIMEOUT_MS's kdoc. With the current 500 ms timeout + SUBSCRIBE_BUFFER = 64-frame DROP_OLDEST flow + 50 fps Opus, at most 25 frames buffer during the wait — leaving ≥ 39 frames of margin (≈ 780 ms) before the oldest frame would be evicted. Even at the production framesPerGroup = 50 (1 group/sec) cadence this never trips during normal startup. No code change; just pinning the rationale so a future timeout bump is checked against the buffer size. https://claude.ai/code/session_014JfZJHSTvyYYWJbC9VbB47 --- .../room/lifecycle/NestViewModelFactory.kt | 8 +- .../commons/viewmodels/NestViewModel.kt | 136 ++++++++++++------ .../commons/viewmodels/NestViewModelTest.kt | 4 +- .../nestsclient/audio/AudioTrackPlayer.kt | 31 +++- .../audio/MediaCodecOpusDecoder.kt | 37 ++++- 5 files changed, 157 insertions(+), 59 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/nests/room/lifecycle/NestViewModelFactory.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/nests/room/lifecycle/NestViewModelFactory.kt index 69c30fe27..b1389a5c7 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/nests/room/lifecycle/NestViewModelFactory.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/nests/room/lifecycle/NestViewModelFactory.kt @@ -53,8 +53,12 @@ internal class NestViewModelFactory( // `httpClient` slot rather than as the first positional arg. httpClient = OkHttpNestsClient(httpClient = Amethyst.instance.roleBasedHttpClientBuilder::okHttpClientForVideo), transport = QuicWebTransportFactory(), - decoderFactory = { channelCount -> MediaCodecOpusDecoder(channelCount = channelCount) }, - playerFactory = { channelCount -> AudioTrackPlayer(channelCount = channelCount) }, + decoderFactory = { channelCount, sampleRate -> + MediaCodecOpusDecoder(channelCount = channelCount, sampleRate = sampleRate) + }, + playerFactory = { channelCount, sampleRate -> + AudioTrackPlayer(channelCount = channelCount, sampleRate = sampleRate) + }, signer = signer, room = room, captureFactory = { AudioRecordCapture() }, diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/NestViewModel.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/NestViewModel.kt index 942765d62..c688ed863 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/NestViewModel.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/NestViewModel.kt @@ -87,10 +87,11 @@ import kotlin.coroutines.cancellation.CancellationException * [playerFactory] so commonMain doesn't have to know which platform's * MediaCodec / AudioTrack is in play. M1 wires Android-only — desktop * passes nothing here yet. Both factories take a `channelCount` (1 for - * mono, 2 for stereo) discovered from the publisher's `catalog.json` - * audio rendition; subscriptions await a brief catalog-arrival window - * before constructing the decoder + player so a stereo web publisher - * doesn't get its frames decoded as mono with downmix artifacts. + * mono, 2 for stereo) AND a `sampleRate` (Hz) discovered from the + * publisher's `catalog.json` audio rendition; subscriptions await a + * brief catalog-arrival window before constructing the decoder + player + * so a stereo or non-48 kHz web publisher doesn't get its frames + * decoded with the wrong channel layout / clock. * * **Threading contract:** all public methods (`connect`, `disconnect`, * `updateSpeakers`, `setMuted`, `setMicMuted`, `startBroadcast`, @@ -107,8 +108,8 @@ import kotlin.coroutines.cancellation.CancellationException class NestViewModel( private val httpClient: NestsClient, private val transport: WebTransportFactory, - private val decoderFactory: (channelCount: Int) -> OpusDecoder, - private val playerFactory: (channelCount: Int) -> AudioPlayer, + private val decoderFactory: (channelCount: Int, sampleRate: Int) -> OpusDecoder, + private val playerFactory: (channelCount: Int, sampleRate: Int) -> AudioPlayer, private val signer: NostrSigner, private val room: NestsRoomConfig, // Speaker-side audio capture/encode actuals. Optional — desktop and @@ -1049,12 +1050,15 @@ class NestViewModel( /** * Wait briefly for [pubkey]'s catalog to land in [_speakerCatalogs] - * and pick the channel count for the decoder + AudioTrack. Returns - * [AudioFormat.CHANNELS] on timeout (the catalog never arrived - * within [timeoutMs]) or when the catalog declares an unsupported - * count (anything outside `1..2`). Caller is responsible for - * having started [fetchSpeakerCatalog] first; otherwise this - * always times out. + * and pick the audio config (channel count + sample rate) for the + * decoder + AudioTrack. Falls back to [AudioFormat.CHANNELS] / + * [AudioFormat.SAMPLE_RATE_HZ] on timeout (the catalog never + * arrived within [timeoutMs]) or when the catalog declares + * unsupported values (channelCount outside `1..2`, or non-positive + * sampleRate). + * + * Caller is responsible for having started [fetchSpeakerCatalog] + * first; otherwise this always times out. * * Why a timeout: the catalog handshake is best-effort. If the * publisher doesn't publish a catalog (legacy publishers) or the @@ -1065,10 +1069,10 @@ class NestViewModel( * within one round-trip of the SUBSCRIBE_OK, so this typically * returns within tens of ms. */ - private suspend fun awaitDecoderChannelCount( + private suspend fun awaitAudioPipelineConfig( pubkey: String, timeoutMs: Long, - ): Int { + ): AudioPipelineConfig { val catalog = withTimeoutOrNull(timeoutMs) { _speakerCatalogs @@ -1076,26 +1080,60 @@ class NestViewModel( .filterNotNull() .first() } - val declared = catalog?.primaryAudio()?.numberOfChannels - return when { - declared == null -> { - AudioFormat.CHANNELS - } - - declared !in 1..2 -> { - Log.w("NestRx") { - "publisher catalog for pubkey='${pubkey.take(8)}' declares numberOfChannels=$declared " + - "(only 1 / 2 supported); falling back to mono" + val rendition = catalog?.primaryAudio() + val declaredChannels = rendition?.numberOfChannels + val channels = + when { + declaredChannels == null -> { + AudioFormat.CHANNELS } - AudioFormat.CHANNELS - } - else -> { - declared + declaredChannels !in 1..2 -> { + Log.w("NestRx") { + "publisher catalog for pubkey='${pubkey.take(8)}' declares numberOfChannels=$declaredChannels " + + "(only 1 / 2 supported); falling back to mono" + } + AudioFormat.CHANNELS + } + + else -> { + declaredChannels + } } - } + val declaredRate = rendition?.sampleRate + val rate = + when { + declaredRate == null -> { + AudioFormat.SAMPLE_RATE_HZ + } + + declaredRate <= 0 -> { + Log.w("NestRx") { + "publisher catalog for pubkey='${pubkey.take(8)}' declares sampleRate=$declaredRate " + + "(must be positive); falling back to ${AudioFormat.SAMPLE_RATE_HZ} Hz" + } + AudioFormat.SAMPLE_RATE_HZ + } + + else -> { + declaredRate + } + } + return AudioPipelineConfig(channelCount = channels, sampleRate = rate) } + /** + * Output of [awaitAudioPipelineConfig] — the validated audio + * pipeline configuration for one subscription. Internal because the + * factories' two-arg shape (`(channelCount, sampleRate)`) is what + * the public surface deals in; this struct just bundles them inside + * `openSubscription`. + */ + private data class AudioPipelineConfig( + val channelCount: Int, + val sampleRate: Int, + ) + /** * Open the speaker's `catalog.json` track in the background, parse * the first frame, and stash it in [speakerCatalogs]. Best-effort — @@ -1171,25 +1209,28 @@ class NestViewModel( // [CATALOG_AWAIT_TIMEOUT_MS] — covers legacy publishers and // the failure-mode where the relay never forwards the catalog // group; audio still plays, just in the default config. - val channelCount = awaitDecoderChannelCount(pubkey, CATALOG_AWAIT_TIMEOUT_MS) + val audioCfg = awaitAudioPipelineConfig(pubkey, CATALOG_AWAIT_TIMEOUT_MS) // Allocate native resources (MediaCodec decoder + AudioTrack // player on Android). Both are heavy and leaky if dropped on // the floor — wrap them in a nested try so any cancellation // or throw between here and slot.attach releases them // (audit round-2 VM #7). // Per-subscription decoder factory closure: captures the - // catalog-derived [channelCount] so a publisher-boundary + // catalog-derived [audioCfg] so a publisher-boundary // decoder rebuild (see [NestPlayer]'s `decoderFactory` - // kdoc) reuses the SAME channel layout — without it, a - // rebuild after a cliff-recycle would default to mono and - // a stereo publisher would silently downmix on the new - // decoder. The factory is also called for the initial - // decoder so the construction path is uniform. - val perSubscriptionDecoderFactory: () -> OpusDecoder = { decoderFactory(channelCount) } + // kdoc) reuses the SAME channel layout AND sample rate — + // without it, a rebuild after a cliff-recycle would + // default to mono / 48 kHz and a stereo or non-48 kHz + // publisher would silently downmix / clock-mismatch on + // the new decoder. The factory is also called for the + // initial decoder so the construction path is uniform. + val perSubscriptionDecoderFactory: () -> OpusDecoder = { + decoderFactory(audioCfg.channelCount, audioCfg.sampleRate) + } val decoder = perSubscriptionDecoderFactory() val player = try { - playerFactory(channelCount) + playerFactory(audioCfg.channelCount, audioCfg.sampleRate) } catch (t: Throwable) { runCatching { decoder.release() } throw t @@ -1808,16 +1849,27 @@ const val SPEAKING_TIMEOUT_MS: Long = 250L /** * How long [NestViewModel.openSubscription] waits for the publisher's * `catalog.json` to land before constructing the decoder + AudioTrack. - * The catalog declares the audio rendition's `numberOfChannels`, which - * the decoder needs at construction time so a stereo web publisher - * doesn't get its frames decoded as mono with downmix artifacts. + * The catalog declares the audio rendition's `numberOfChannels` and + * `sampleRate`, which the decoder needs at construction time so a + * stereo or non-48 kHz web publisher doesn't get its frames decoded + * with the wrong layout / clock. * * Sized to be generous enough that a typical publisher's catalog * (which arrives within a single round-trip of the SUBSCRIBE_OK with * the speaker-side emit-on-subscribe hook in place) lands well before * the timeout, and short enough that a publisher that never emits a * catalog (legacy publishers, relay-side bug) doesn't visibly stall - * the listener — audio playback proceeds in the default mono config. + * the listener — audio playback proceeds in the default config. + * + * **Frame-buffer budget**: while we wait, audio frames stream from + * the relay into the wrapper's + * [com.vitorpamplona.nestsclient.ReconnectingNestsListener] + * `SUBSCRIBE_BUFFER = 64` SharedFlow with `DROP_OLDEST` overflow. + * At 50 fps Opus that's 1.28 s of headroom; the 500 ms wait + * consumes at most 25 frames of buffer, leaving ≥ 39 frames of + * margin (≈780 ms) before the oldest frame would be dropped. Even + * at the production `framesPerGroup = 50` (= 1 group / sec) cadence + * this never trips the eviction path during normal startup. */ const val CATALOG_AWAIT_TIMEOUT_MS: Long = 500L diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/NestViewModelTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/NestViewModelTest.kt index d9c5d7d76..5c7415ab5 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/NestViewModelTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/viewmodels/NestViewModelTest.kt @@ -455,8 +455,8 @@ class NestViewModelTest { NestViewModel( httpClient = NoopNestsClient, transport = NoopWebTransportFactory, - decoderFactory = { _ -> NoopOpusDecoder }, - playerFactory = { _ -> NoopAudioPlayer() }, + decoderFactory = { _, _ -> NoopOpusDecoder }, + playerFactory = { _, _ -> NoopAudioPlayer() }, signer = NoopSigner, room = ROOM_CONFIG, connector = diff --git a/nestsClient/src/androidMain/kotlin/com/vitorpamplona/nestsclient/audio/AudioTrackPlayer.kt b/nestsClient/src/androidMain/kotlin/com/vitorpamplona/nestsclient/audio/AudioTrackPlayer.kt index 0d6901e04..27871b739 100644 --- a/nestsClient/src/androidMain/kotlin/com/vitorpamplona/nestsclient/audio/AudioTrackPlayer.kt +++ b/nestsClient/src/androidMain/kotlin/com/vitorpamplona/nestsclient/audio/AudioTrackPlayer.kt @@ -86,11 +86,26 @@ class AudioTrackPlayer( * call sites that don't pass a channel count keep the prior behaviour. */ private val channelCount: Int = AudioFormat.CHANNELS, + /** + * PCM sample rate in Hz. Drives the AudioTrack output rate and the + * 250 ms target-buffer calculation. For Opus, Android's Codec2 + * decoder always emits 48 kHz PCM regardless of the OpusHead + * `inputSampleRate`, so for the production Opus path this is + * always [AudioFormat.SAMPLE_RATE_HZ] — but threading the + * parameter through keeps the AudioTrack's declared rate matched + * to whatever the catalog says, and lets a future codec or + * container variant whose decoder DOES respect input sample rate + * (a non-Opus rendition) get the correct PCM clock. + */ + private val sampleRate: Int = AudioFormat.SAMPLE_RATE_HZ, ) : AudioPlayer { init { require(channelCount in 1..2) { "AudioTrackPlayer supports mono (1) or stereo (2) only, got $channelCount" } + require(sampleRate > 0) { + "AudioTrackPlayer sampleRate must be positive, got $sampleRate" + } } private var track: AudioTrack? = null @@ -123,14 +138,14 @@ class AudioTrackPlayer( val minBuffer = AudioTrack.getMinBufferSize( - AudioFormat.SAMPLE_RATE_HZ, + sampleRate, channelMask, AndroidAudioFormat.ENCODING_PCM_16BIT, ) if (minBuffer <= 0) { throw AudioException( AudioException.Kind.DeviceUnavailable, - "AudioTrack.getMinBufferSize returned $minBuffer for ${AudioFormat.SAMPLE_RATE_HZ} Hz", + "AudioTrack.getMinBufferSize returned $minBuffer for $sampleRate Hz", ) } // Target ~250 ms of audio: enough headroom so the decode loop can @@ -138,10 +153,12 @@ class AudioTrackPlayer( // underruns. Take the larger of `minBuffer * 16` and an explicit // 250 ms-equivalent so devices that report a small minBuffer still // get the same wall-clock slack. Stereo doubles the byte count - // per sample (interleaved L,R 16-bit shorts) — scaling - // [channelCount] in keeps the wall-clock target constant. + // per sample (interleaved L,R 16-bit shorts); a higher sample + // rate scales the per-second byte count linearly — both factor + // into the target so the wall-clock 250 ms target is preserved + // regardless of channel layout / sample rate. val targetBytes250Ms = - (AudioFormat.SAMPLE_RATE_HZ / 4) * AudioFormat.BYTES_PER_SAMPLE * channelCount + (sampleRate / 4) * AudioFormat.BYTES_PER_SAMPLE * channelCount val bufferBytes = maxOf(minBuffer * 16, targetBytes250Ms) val newTrack = @@ -158,7 +175,7 @@ class AudioTrackPlayer( AndroidAudioFormat .Builder() .setEncoding(AndroidAudioFormat.ENCODING_PCM_16BIT) - .setSampleRate(AudioFormat.SAMPLE_RATE_HZ) + .setSampleRate(sampleRate) .setChannelMask(channelMask) .build(), ).setBufferSizeInBytes(bufferBytes) @@ -199,7 +216,7 @@ class AudioTrackPlayer( track = newTrack com.vitorpamplona.quartz.utils.Log.d("NestPlay") { "AudioTrack allocated: state=${newTrack.state} playState=${newTrack.playState} " + - "bufferSizeBytes=$bufferBytes minBuffer=$minBuffer sampleRate=${AudioFormat.SAMPLE_RATE_HZ}" + "bufferSizeBytes=$bufferBytes minBuffer=$minBuffer sampleRate=$sampleRate" } } diff --git a/nestsClient/src/androidMain/kotlin/com/vitorpamplona/nestsclient/audio/MediaCodecOpusDecoder.kt b/nestsClient/src/androidMain/kotlin/com/vitorpamplona/nestsclient/audio/MediaCodecOpusDecoder.kt index b2d93e7b7..ec9077f31 100644 --- a/nestsClient/src/androidMain/kotlin/com/vitorpamplona/nestsclient/audio/MediaCodecOpusDecoder.kt +++ b/nestsClient/src/androidMain/kotlin/com/vitorpamplona/nestsclient/audio/MediaCodecOpusDecoder.kt @@ -50,11 +50,30 @@ import java.nio.ByteOrder */ class MediaCodecOpusDecoder( private val channelCount: Int = AudioFormat.CHANNELS, + /** + * Source sample rate in Hz. Drives the OpusHead `inputSampleRate` + * field and the MediaFormat `audio/opus` sample-rate hint. + * + * **Note on Opus's actual decode rate**: Opus always decodes at + * 48 kHz internally regardless of [sampleRate] (the codec's + * design — RFC 6716). Android's Codec2 `audio/opus` decoder + * always emits 48 kHz PCM. So this parameter is informational at + * the OpusHead level and doesn't affect the PCM rate the + * downstream [AudioPlayer] sees. We thread it through anyway so + * the decoder declaration matches what the catalog says, and so a + * future codec or container variant that DOES respect input + * sample rate (e.g. a non-Opus rendition) gets the correct + * configuration. + */ + private val sampleRate: Int = AudioFormat.SAMPLE_RATE_HZ, ) : OpusDecoder { init { require(channelCount in 1..2) { "MediaCodecOpusDecoder supports mono (1) or stereo (2) only, got $channelCount" } + require(sampleRate > 0) { + "MediaCodecOpusDecoder sampleRate must be positive, got $sampleRate" + } } private val codec: MediaCodec = @@ -62,7 +81,7 @@ class MediaCodecOpusDecoder( MediaCodec .createDecoderByType(MediaFormat.MIMETYPE_AUDIO_OPUS) .apply { - configure(buildFormat(channelCount), null, null, 0) + configure(buildFormat(channelCount, sampleRate), null, null, 0) start() }.also { com.vitorpamplona.quartz.utils.Log.d("NestPlay") { @@ -221,14 +240,17 @@ class MediaCodecOpusDecoder( private const val FRAME_DURATION_US = 20_000L // 20 ms - private fun buildFormat(channelCount: Int): MediaFormat { + private fun buildFormat( + channelCount: Int, + sampleRate: Int, + ): MediaFormat { val format = MediaFormat.createAudioFormat( MediaFormat.MIMETYPE_AUDIO_OPUS, - AudioFormat.SAMPLE_RATE_HZ, + sampleRate, channelCount, ) - format.setByteBuffer("csd-0", ByteBuffer.wrap(buildOpusIdHeader(channelCount))) + format.setByteBuffer("csd-0", ByteBuffer.wrap(buildOpusIdHeader(channelCount, sampleRate))) // Pre-skip + seek pre-roll: both zero, encoded as little-endian // 64-bit nanoseconds per Android's MediaCodec contract. format.setByteBuffer("csd-1", ByteBuffer.wrap(zeroLongLe())) @@ -236,7 +258,10 @@ class MediaCodecOpusDecoder( return format } - private fun buildOpusIdHeader(channelCount: Int): ByteArray { + private fun buildOpusIdHeader( + channelCount: Int, + sampleRate: Int, + ): ByteArray { // RFC 7845 §5.1 — 19 bytes, mapping family 0 (covers mono and // stereo with implicit L,R interleaving; no per-channel // mapping table required). @@ -245,7 +270,7 @@ class MediaCodecOpusDecoder( buf.put(1.toByte()) // version buf.put(channelCount.toByte()) // channel count (1 or 2) buf.putShort(0) // pre-skip - buf.putInt(AudioFormat.SAMPLE_RATE_HZ) // input sample rate + buf.putInt(sampleRate) // input sample rate buf.putShort(0) // output gain (Q7.8 dB) buf.put(0.toByte()) // mapping family 0 return buf.array()