diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/nests/NestForegroundService.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/nests/NestForegroundService.kt index dc5f06873..aefe23703 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/nests/NestForegroundService.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/nests/NestForegroundService.kt @@ -138,9 +138,20 @@ class NestForegroundService : Service() { * default network just changed under us" (publish). Volatile because * the callback fires on a binder thread; the publish path is * lock-free via [NestNetworkChangeBus]. + * + * `seenInitialNetwork` is the registration-time suppression flag — + * set true on the first onAvailable and never cleared. The earlier + * shape used `previous != null` as the suppression check, which + * also suppressed the WiFi-loss-then-cellular-available case + * (`onLost` clears [currentDefaultNetwork] back to null, then the + * follow-up `onAvailable` looks identical to the registration + * callback). That broke the most important scenario this whole + * code path exists for — a clean Wi-Fi ↔ cellular handover. */ @Volatile private var currentDefaultNetwork: Network? = null + @Volatile private var seenInitialNetwork: Boolean = false + /** * Listens for the device's default-network changing (Wi-Fi ↔ * cellular handover, plane mode toggle, hotspot swap) and signals @@ -159,7 +170,14 @@ class NestForegroundService : Service() { override fun onAvailable(network: Network) { val previous = currentDefaultNetwork currentDefaultNetwork = network - if (previous != null && previous != network) { + if (!seenInitialNetwork) { + // Registration-time callback — fires once with the + // currently-active default network. Suppress so the + // VM doesn't recycle on every service start. + seenInitialNetwork = true + return + } + if (previous != network) { Log.i("NestNet") { "default network changed ($previous → $network), recycling QUIC sessions" } @@ -193,6 +211,7 @@ class NestForegroundService : Service() { mgr.unregisterNetworkCallback(networkCallback) } currentDefaultNetwork = null + seenInitialNetwork = false } /** @@ -232,19 +251,20 @@ class NestForegroundService : Service() { .setOnAudioFocusChangeListener { focusChange -> NestAudioFocusBus.publish(translateFocusChange(focusChange)) }.build() - // Best-effort: a refused request just means the OS will - // duck/pause us based on its own policy. Don't fail the - // service start over a focus denial. - val granted = runCatching { mgr.requestAudioFocus(request) }.getOrNull() - // If the OS refused outright (rare — typically only when an - // active call is already in progress at the moment we start), - // publish TransientLoss immediately so the VM mutes from t=0 - // rather than playing for ~50 ms before the listener fires. - // [AudioManager.AUDIOFOCUS_REQUEST_FAILED] = 0; granted = 1. - if (granted == AudioManager.AUDIOFOCUS_REQUEST_FAILED) { - NestAudioFocusBus.publish(NestAudioFocusState.TransientLoss) - } else { + // Strict GRANTED check: anything else — FAILED (active call + // blocking us), DELAYED (we set [setAcceptsDelayedFocusGain] + // false so this shouldn't happen but treat it as not-granted + // defensively), or a swallowed exception from runCatching — + // means we don't actually own playback yet, so the VM must + // start muted. The earlier shape used `!= FAILED` which fell + // through to Granted on `null` (exception path) and on + // DELAYED (= 2), playing audio over a call that the OS hadn't + // released to us. + val result = runCatching { mgr.requestAudioFocus(request) }.getOrNull() + if (result == AudioManager.AUDIOFOCUS_REQUEST_GRANTED) { NestAudioFocusBus.publish(NestAudioFocusState.Granted) + } else { + NestAudioFocusBus.publish(NestAudioFocusState.TransientLoss) } audioFocusRequest = request } 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 0ec19cad5..b06bd9f36 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 @@ -34,6 +34,7 @@ import com.vitorpamplona.nestsclient.NestsRoomConfig import com.vitorpamplona.nestsclient.NestsSpeaker import com.vitorpamplona.nestsclient.NestsSpeakerState import com.vitorpamplona.nestsclient.audio.AudioCapture +import com.vitorpamplona.nestsclient.audio.AudioException import com.vitorpamplona.nestsclient.audio.AudioPlayer import com.vitorpamplona.nestsclient.audio.NestPlayer import com.vitorpamplona.nestsclient.audio.OpusDecoder @@ -1015,7 +1016,32 @@ class NestViewModel( val instrumented = handle.objects.onEach { onSpeakerActivity(pubkey) } roomPlayer.play( instrumented, - onError = { /* swallow per-packet decoder errors */ }, + onError = { err -> + // Per-packet decoder errors are noise — Opus PLC + // papers over a single bad frame. Swallow those. + // PlaybackFailed (or DeviceUnavailable from a + // deferred [AudioPlayer.beginPlayback], or the + // audio-priority dispatcher dying mid-stream) is + // terminal: the device is wedged for THIS + // subscription. Roll the slot back so a future + // reconcile can retry rather than leave a + // perma-spinning speaker tile. + when (err.kind) { + AudioException.Kind.DecoderError, + AudioException.Kind.EncoderError, + -> { + Unit + } + + AudioException.Kind.PlaybackFailed, + AudioException.Kind.DeviceUnavailable, + -> { + if (activeSubscriptions[pubkey] === slot) { + activeSubscriptions.remove(pubkey)?.let { closeSubscription(it) } + } + } + } + }, onLevel = { onAudioLevel(pubkey, it) }, ) slot.attach(handle, roomPlayer, player) diff --git a/nestsClient/src/commonMain/kotlin/com/vitorpamplona/nestsclient/audio/NestPlayer.kt b/nestsClient/src/commonMain/kotlin/com/vitorpamplona/nestsclient/audio/NestPlayer.kt index 33946f706..fe641b994 100644 --- a/nestsClient/src/commonMain/kotlin/com/vitorpamplona/nestsclient/audio/NestPlayer.kt +++ b/nestsClient/src/commonMain/kotlin/com/vitorpamplona/nestsclient/audio/NestPlayer.kt @@ -126,11 +126,22 @@ class NestPlayer( suspend fun beginAndFlushIfNeeded() { if (playbackBegun) return if (preroll.isEmpty()) return - player.beginPlayback() - playbackBegun = true + // Flush BEFORE [beginPlayback] so the device starts + // playing against an already-populated buffer rather + // than emitting silence for the microseconds it + // takes the flush loop to fill. AudioTrack + // MODE_STREAM explicitly supports write() before + // play() per the Android docs — that's the + // textbook pre-roll pattern. Order matters + // because [beginPlayback] is the moment the + // hardware starts pulling samples; getting + // [enqueue] in first means the very first sample + // pulled is from our pre-rolled audio, not silence. while (preroll.isNotEmpty()) { player.enqueue(preroll.removeFirst()) } + player.beginPlayback() + playbackBegun = true } try { objects.collect { obj -> diff --git a/nestsClient/src/commonTest/kotlin/com/vitorpamplona/nestsclient/audio/NestPlayerTest.kt b/nestsClient/src/commonTest/kotlin/com/vitorpamplona/nestsclient/audio/NestPlayerTest.kt index b84a05471..9d63e22ca 100644 --- a/nestsClient/src/commonTest/kotlin/com/vitorpamplona/nestsclient/audio/NestPlayerTest.kt +++ b/nestsClient/src/commonTest/kotlin/com/vitorpamplona/nestsclient/audio/NestPlayerTest.kt @@ -216,6 +216,130 @@ class NestPlayerTest { sut.stop() } + /** + * Pre-roll: with `prerollFrames=3`, [AudioPlayer.beginPlayback] + * must NOT fire until the third decoded frame has arrived, and + * once it fires the AudioPlayer's queue must already contain + * the buffered pre-roll (i.e. flushed atomically with playback + * start, not lazily on the next enqueue). + */ + @Test + fun preroll_defers_beginPlayback_until_threshold_is_met() = + runTest { + val channel = Channel(capacity = 8) + val decoder = FakeOpusDecoder { byteToShorts(it) } + val player = FakeAudioPlayer() + + val sut = + NestPlayer( + decoder = decoder, + player = player, + scope = this, + prerollFrames = 3, + ) + sut.play(channel.receiveAsFlow()) + testScheduler.runCurrent() + + // Frames 1 and 2: pre-roll buffer fills, beginPlayback NOT + // called yet, AudioPlayer hasn't seen a single enqueue. + channel.send(moqObject(byteArrayOf(0x01))) + channel.send(moqObject(byteArrayOf(0x02))) + testScheduler.advanceUntilIdle() + assertEquals(0, player.beginPlaybackCount, "beginPlayback before threshold") + assertEquals(0, player.queued.size, "no enqueue before threshold") + + // Frame 3 trips the threshold: beginPlayback fires exactly + // once, and the pre-rolled frames must ALREADY be sitting + // in the AudioPlayer queue at that moment (otherwise the + // device starts playback against an empty buffer and + // pre-roll's whole point is defeated). [NestPlayer.play] + // implements this by flushing-then-beginPlayback — + // AudioTrack MODE_STREAM explicitly supports write() + // before play(). + channel.send(moqObject(byteArrayOf(0x03))) + testScheduler.advanceUntilIdle() + assertEquals(1, player.beginPlaybackCount) + // The fake records the queue size at the moment + // beginPlayback fires. With flush-then-begin ordering, + // all 3 pre-rolled frames are already in the queue. + assertEquals(3, player.queuedAtBeginPlayback, "pre-roll flushed before beginPlayback") + assertEquals(3, player.queued.size, "buffer populated when device starts") + + // Subsequent frames bypass the buffer and go directly + // through enqueue. + channel.send(moqObject(byteArrayOf(0x04))) + testScheduler.advanceUntilIdle() + assertEquals(1, player.beginPlaybackCount, "beginPlayback only fires once") + assertEquals(4, player.queued.size) + + sut.stop() + } + + /** + * Pre-roll: a flow that ends BEFORE the pre-roll threshold fires + * must still flush its partial buffer to the AudioPlayer. + * Otherwise a fast-cycling publisher could leave already-decoded + * frames stranded forever. + */ + @Test + fun preroll_flushes_partial_buffer_when_flow_ends_early() = + runTest { + val decoder = FakeOpusDecoder { byteToShorts(it) } + val player = FakeAudioPlayer() + + val sut = + NestPlayer( + decoder = decoder, + player = player, + scope = this, + prerollFrames = 5, + ) + // Only 2 frames — pre-roll never reaches its 5-frame floor, + // but the upstream Flow ends so the loop's flush hook must + // begin playback and drain whatever's queued. + sut.play( + flowOf( + moqObject(byteArrayOf(0x01)), + moqObject(byteArrayOf(0x02)), + ), + ) + testScheduler.advanceUntilIdle() + + assertEquals(1, player.beginPlaybackCount) + assertEquals(2, player.queued.size) + assertContentEquals(byteToShorts(byteArrayOf(0x01)), player.queued[0]) + assertContentEquals(byteToShorts(byteArrayOf(0x02)), player.queued[1]) + + sut.stop() + } + + /** + * Pre-roll edge case: an empty flow shouldn't start playback at + * all. The AudioTrack stays in its allocated-but-not-playing + * state until [stop] tears it down. + */ + @Test + fun preroll_does_not_begin_playback_when_flow_emits_no_pcm() = + runTest { + val decoder = FakeOpusDecoder { ShortArray(0) } + val player = FakeAudioPlayer() + + val sut = + NestPlayer( + decoder = decoder, + player = player, + scope = this, + prerollFrames = 3, + ) + sut.play(flowOf(moqObject(byteArrayOf(0x01)), moqObject(byteArrayOf(0x02)))) + testScheduler.advanceUntilIdle() + + assertEquals(0, player.beginPlaybackCount, "no PCM → no playback") + assertEquals(0, player.queued.size) + + sut.stop() + } + // -- helpers ----------------------------------------------------------- private fun moqObject(payload: ByteArray): MoqObject = @@ -246,6 +370,26 @@ class NestPlayerTest { private class FakeAudioPlayer : AudioPlayer { var started = false private set + + /** + * Counts the [AudioPlayer.beginPlayback] invocations. Used by + * the pre-roll regression tests to assert that playback only + * begins AFTER `prerollFrames` decoded frames have arrived (or + * after the upstream flow ends with a partial buffer). The + * default no-op `beginPlayback` in the interface lets fakes + * skip overriding when they don't care; we override here so + * the tests can verify the pre-roll wiring. + * + * Also tracks the size of `queued` at the moment beginPlayback + * fired — pre-roll's contract is that the buffer is flushed + * IN A TIGHT LOOP after beginPlayback, so we can read the + * snapshot to verify the flush ordering. + */ + var beginPlaybackCount = 0 + private set + var queuedAtBeginPlayback: Int = -1 + private set + var stopCount = 0 private set val stopped: Boolean get() = stopCount > 0 @@ -257,6 +401,11 @@ class NestPlayerTest { started = true } + override fun beginPlayback() { + beginPlaybackCount++ + queuedAtBeginPlayback = queued.size + } + override suspend fun enqueue(pcm: ShortArray) { queued.add(pcm) }