diff --git a/app/src/main/java/eu/darken/capod/common/MediaControl.kt b/app/src/main/java/eu/darken/capod/common/MediaControl.kt index 1bf27ae8..f7de26b0 100644 --- a/app/src/main/java/eu/darken/capod/common/MediaControl.kt +++ b/app/src/main/java/eu/darken/capod/common/MediaControl.kt @@ -10,7 +10,11 @@ import eu.darken.capod.common.debug.logging.Logging.Priority.INFO import eu.darken.capod.common.debug.logging.Logging.Priority.WARN import eu.darken.capod.common.debug.logging.log import eu.darken.capod.common.debug.logging.logTag +import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.delay +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import kotlinx.coroutines.withContext import javax.inject.Inject import javax.inject.Singleton @@ -32,9 +36,28 @@ class MediaControl @Inject constructor( @Volatile private var capPaused: Boolean = false @Volatile private var lastKnownMusicActive: Boolean = false + /** + * The compound isMusicActive-check → key dispatch → [capPaused] transition must not interleave + * across concurrent callers: the `delay(100)` suspension inside [sendKey] is the window in which + * a second sender could observe stale state and lose the other's flag update (issue #647). + * Callers genuinely race — stem presses run on the app scope, ear/sleep/conversation reactions + * on the monitor scope. + */ + private val dispatchLock = Mutex() + private val playbackCallback = object : AudioManager.AudioPlaybackCallback() { override fun onPlaybackConfigChanged(configs: List) { - val nowActive = audioManager.isMusicActive + // The edge is derived from this delivery's own snapshot, not a live isMusicActive read: + // per-invocation snapshots preserve the sequence when deliveries queue up behind the + // handler (a live read makes every queued delivery see the newest state, so an + // inactive→active edge in the middle of the queue is never observed). It also keeps a + // binder call out of the callback body. + // + // Residual: API 36+ may replace a pending config message with the newest one, delivering + // a single callback for two transitions. A state that is never delivered is unrecoverable + // at the receiver; when that happens capPaused stays stale and costs one redundant + // (idempotent) MEDIA_PLAY on a later pod-in — the same cost as before, just rarer. + val nowActive = configs.any { it.isMusicStream() } if (!lastKnownMusicActive && nowActive) { // Music started by some source (could be us via sendPlay or someone else). // Either way, our pause memory is stale — drop it so a future pod-in doesn't @@ -45,6 +68,18 @@ class MediaControl @Inject constructor( } } + /** + * Parity with what [AudioManager.isMusicActive] counts (active STREAM_MUSIC players), using the + * platform's own attribute→stream mapping (flags and OEM strategies included) instead of a + * hand-rolled usage set. Ambiguous or exceptional configs count as not-music: that errs toward a + * missed clear (one redundant, idempotent MEDIA_PLAY) rather than a false clear (lost auto-resume). + */ + private fun AudioPlaybackConfiguration.isMusicStream(): Boolean = try { + audioAttributes.volumeControlStream == AudioManager.STREAM_MUSIC + } catch (e: IllegalArgumentException) { + false + } + init { // Both calls below are binder transactions into AudioService. On the main thread they sat on // the cold-start critical path (MediaControl is constructed during App.onCreate via Hilt) and @@ -73,14 +108,20 @@ class MediaControl @Inject constructor( val wasRecentlyPausedByCap: Boolean get() = capPaused - suspend fun sendPlay() { + suspend fun sendPlay() = dispatchLock.withLock { sendPlayLocked() } + + private suspend fun sendPlayLocked() { log(TAG, INFO) { "sendPlay()" } if (audioManager.isMusicActive && !capPaused) { log(TAG, INFO) { "Music is already playing, not sending play" } return } - sendKey(KeyEvent.KEYCODE_MEDIA_PLAY) + // Cleared before the first suspension: sendKey's delay is a cancellation point (the key pair + // itself completes under NonCancellable, but the caller can still be cancelled at the lock + // boundaries), so clearing first means a cancelled resume can never strand a stale armed + // flag. Under the mutex the ordering no longer matters for racing senders. capPaused = false + sendKey(KeyEvent.KEYCODE_MEDIA_PLAY) } /** @@ -102,7 +143,11 @@ class MediaControl @Inject constructor( * on the return value rather than checking [isPlaying] themselves to avoid a * check-then-act race with the audio system. */ - suspend fun sendPause(rememberForResume: Boolean = false): Boolean { + suspend fun sendPause(rememberForResume: Boolean = false): Boolean = dispatchLock.withLock { + sendPauseLocked(rememberForResume) + } + + private suspend fun sendPauseLocked(rememberForResume: Boolean): Boolean { log(TAG, INFO) { "sendPause(rememberForResume=$rememberForResume)" } if (!audioManager.isMusicActive) { log(TAG, INFO) { "Music is not playing, not sending pause" } @@ -115,6 +160,11 @@ class MediaControl @Inject constructor( // This single explicit assignment also covers the contract that an explicit user // pause cancels a pending auto-resume. capPaused = rememberForResume + // The live active-check we just passed is itself an observation of music activity. Recording + // it stops a music-start snapshot that was still queued on the handler when this pause armed + // capPaused from draining afterwards and reading as a fresh inactive→active edge that would + // wrongly clear the new arm. + lastKnownMusicActive = true sendKey(KeyEvent.KEYCODE_MEDIA_PAUSE) return true } @@ -123,26 +173,31 @@ class MediaControl @Inject constructor( * Dispatches MEDIA_STOP and clears any pending auto-resume — Stop is an explicit user * "stay stopped" action, so a later pod-in must not auto-resume from a prior auto-pause. */ - suspend fun sendStop() { + suspend fun sendStop() = dispatchLock.withLock { log(TAG, INFO) { "sendStop()" } capPaused = false sendKey(KeyEvent.KEYCODE_MEDIA_STOP) } suspend fun sendPlayPause() { - log(TAG) { "sendPlayPause()" } - if (capPaused) { - sendPlay() - return - } - if (audioManager.isMusicActive) { - sendPause() - } else { - sendPlay() + dispatchLock.withLock { + log(TAG) { "sendPlayPause()" } + if (capPaused) { + sendPlayLocked() + return@withLock + } + if (audioManager.isMusicActive) { + sendPauseLocked(rememberForResume = false) + } else { + sendPlayLocked() + } } } - internal suspend fun sendKey(keyCode: Int) { + internal suspend fun sendKey(keyCode: Int) = withContext(NonCancellable) { + // DOWN and UP must always be dispatched as a pair: once DOWN is out, cancellation may not + // strand it unpaired or the media session keeps seeing a held key. Bounded — this defers + // cancellation by at most the 100ms delay below. log(TAG) { "Sending up+down KeyEvent: $keyCode" } val eventTime = timeSource.uptimeMillis() audioManager.dispatchMediaKeyEvent(KeyEvent(eventTime, eventTime, KeyEvent.ACTION_DOWN, keyCode, 0)) diff --git a/app/src/test/java/eu/darken/capod/common/MediaControlTest.kt b/app/src/test/java/eu/darken/capod/common/MediaControlTest.kt index 5d5b5f72..96fb57ee 100644 --- a/app/src/test/java/eu/darken/capod/common/MediaControlTest.kt +++ b/app/src/test/java/eu/darken/capod/common/MediaControlTest.kt @@ -1,7 +1,10 @@ package eu.darken.capod.common +import android.media.AudioAttributes import android.media.AudioManager +import android.media.AudioPlaybackConfiguration import android.os.Handler +import android.view.KeyEvent import io.mockk.CapturingSlot import io.mockk.Runs import io.mockk.clearMocks @@ -11,7 +14,11 @@ import io.mockk.mockk import io.mockk.slot import io.mockk.verify import io.mockk.verifyOrder +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest +import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.BeforeEach @@ -50,8 +57,34 @@ class MediaControlTest : BaseTest() { initRunnableSlot.captured.run() } - private fun fireCallback() { - playbackCallbackSlot.captured.onPlaybackConfigChanged(emptyList()) + private fun playbackConfig(stream: Int): AudioPlaybackConfiguration { + val attributes = mockk() + every { attributes.volumeControlStream } returns stream + return mockk().also { + every { it.audioAttributes } returns attributes + } + } + + /** + * The callback derives the edge from the delivered snapshot, so the snapshot has to carry the + * state — stubbing `isMusicActive` no longer influences it. + */ + private fun fireCallback(musicActive: Boolean) { + val configs: List = + if (musicActive) listOf(playbackConfig(AudioManager.STREAM_MUSIC)) else emptyList() + playbackCallbackSlot.captured.onPlaybackConfigChanged(configs) + } + + private fun fireCallbackWithStream(stream: Int) { + playbackCallbackSlot.captured.onPlaybackConfigChanged(listOf(playbackConfig(stream))) + } + + private fun fireCallbackWithUnmappableConfig() { + val attributes = mockk() + every { attributes.volumeControlStream } throws IllegalArgumentException("Unknown usage") + val config = mockk() + every { config.audioAttributes } returns attributes + playbackCallbackSlot.captured.onPlaybackConfigChanged(listOf(config)) } @Test @@ -194,20 +227,20 @@ class MediaControlTest : BaseTest() { @Test fun `wasRecentlyPausedByCap clears when music transitions inactive to active from any source`() = runTest { every { audioManager.isMusicActive } returns true - fireCallback() // seed lastKnownMusicActive=true + fireCallback(true) // seed lastKnownMusicActive=true mediaControl.sendPause(rememberForResume = true) assertTrue(mediaControl.wasRecentlyPausedByCap) // Music goes inactive (CAP's pause took effect). every { audioManager.isMusicActive } returns false - fireCallback() + fireCallback(false) assertTrue(mediaControl.wasRecentlyPausedByCap) // Music starts again (e.g. user manually resumed via phone). Sticky flag must clear so // a later pod-in doesn't fire a stray play key on top of already-playing music. every { audioManager.isMusicActive } returns true - fireCallback() + fireCallback(true) assertFalse(mediaControl.wasRecentlyPausedByCap) } @@ -218,7 +251,7 @@ class MediaControlTest : BaseTest() { // callback would clear it, then sendPause's post-dispatch line would put it back to // true while music is genuinely playing again — wrongly arming a future pod-in resume. every { audioManager.isMusicActive } returns true - fireCallback() // seed lastKnownMusicActive=true + fireCallback(true) // seed lastKnownMusicActive=true // sendKey is implemented with an internal delay(100). Drive a callback during that // window by sending a single coalesced inactive→active sequence right after kicking @@ -227,9 +260,9 @@ class MediaControlTest : BaseTest() { every { audioManager.dispatchMediaKeyEvent(any()) } answers { // First DOWN dispatch: pretend music briefly went inactive then active mid-pause. every { audioManager.isMusicActive } returns false - fireCallback() + fireCallback(false) every { audioManager.isMusicActive } returns true - fireCallback() + fireCallback(true) } mediaControl.sendPause(rememberForResume = true) @@ -241,15 +274,146 @@ class MediaControlTest : BaseTest() { assertFalse(mediaControl.wasRecentlyPausedByCap) } + @Test + fun `callback edge detection uses the event snapshot so coalesced queued deliveries still clear the flag`() = + runTest { + // Deliveries queue up on the callback handler. A live isMusicActive read makes every + // queued delivery observe the newest state, so an inactive→active edge that happened + // while they were queued is never seen and capPaused stays stale. + every { audioManager.isMusicActive } returns true + fireCallback(true) + + mediaControl.sendPause(rememberForResume = true) + assertTrue(mediaControl.wasRecentlyPausedByCap) + + // Live state stays "active" the whole time — only the snapshots carry the sequence. + fireCallback(false) + fireCallback(true) + + assertFalse(mediaControl.wasRecentlyPausedByCap) + } + + @Test + fun `replacement delivery that drops the intermediate inactive snapshot leaves the flag armed`() = runTest { + // Pins the documented residual: API 36+ may replace a pending config message with the + // newest one, so two transitions arrive as a single callback. A state that is never + // delivered is unrecoverable at the receiver — the accepted cost is a stale capPaused + // producing one redundant (idempotent) MEDIA_PLAY on a later pod-in. + every { audioManager.isMusicActive } returns true + fireCallback(true) + + mediaControl.sendPause(rememberForResume = true) + assertTrue(mediaControl.wasRecentlyPausedByCap) + + fireCallback(true) + + assertTrue(mediaControl.wasRecentlyPausedByCap) + } + + @Test + fun `non-music playback configs do not count as a music-start edge`() = runTest { + // Init drained with isMusicActive=false, so lastKnownMusicActive starts false. + every { audioManager.isMusicActive } returns true + + mediaControl.sendPause(rememberForResume = true) + assertTrue(mediaControl.wasRecentlyPausedByCap) + + // The pause records music as active; bring the observation back to inactive so the + // following deliveries are candidates for an inactive→active edge. + fireCallback(false) + assertTrue(mediaControl.wasRecentlyPausedByCap) + + fireCallbackWithStream(AudioManager.STREAM_NOTIFICATION) + assertTrue(mediaControl.wasRecentlyPausedByCap) + + // A config whose attributes have no legacy stream mapping counts as not-music too. + fireCallbackWithUnmappableConfig() + assertTrue(mediaControl.wasRecentlyPausedByCap) + } + + @Test + fun `queued music-start snapshot draining after an owned pause does not clear the fresh arm`() = runTest { + // Init drained with isMusicActive=false, so lastKnownMusicActive starts false. + every { audioManager.isMusicActive } returns true + + mediaControl.sendPause(rememberForResume = true) + assertTrue(mediaControl.wasRecentlyPausedByCap) + + // A music-start snapshot that was already queued when the pause armed the flag drains now. + // The pause's own live active-check already observed that music, so this must not read as a + // fresh inactive→active edge. + fireCallback(true) + assertTrue(mediaControl.wasRecentlyPausedByCap) + + // A genuine resume afterwards still clears it. + fireCallback(false) + fireCallback(true) + assertFalse(mediaControl.wasRecentlyPausedByCap) + } + + @Test + fun `concurrent sendPause during sendPlay dispatch keeps the later pause armed`() = runTest { + // Repro for the lost update: sendPlay's flag write used to land after sendKey's delay(100), + // so a sendPause arming capPaused inside that window was overwritten back to false. + every { audioManager.isMusicActive } returns true + mediaControl.sendPause(rememberForResume = true) + assertTrue(mediaControl.wasRecentlyPausedByCap) + clearMocks(audioManager, answers = false, recordedCalls = true) + + val dispatched = mutableListOf() + // KeyEvent getters throw in plain JVM unit tests (mockable android.jar), so the dispatched + // events are counted, and the pairs are told apart by the flag state each one was sent + // under: the play pair dispatches with capPaused=false, the pause pair with capPaused=true. + // Interleaved dispatch would therefore not produce false,false,true,true. + val armedAtDispatch = mutableListOf() + every { audioManager.dispatchMediaKeyEvent(capture(dispatched)) } answers { + armedAtDispatch += mediaControl.wasRecentlyPausedByCap + } + + val playJob = launch { mediaControl.sendPlay() } + runCurrent() + val pauseJob = launch { mediaControl.sendPause(rememberForResume = true) } + runCurrent() + advanceUntilIdle() + playJob.join() + pauseJob.join() + + assertTrue(mediaControl.wasRecentlyPausedByCap) + assertEquals(4, dispatched.size) + assertEquals(listOf(false, false, true, true), armedAtDispatch) + } + + @Test + fun `cancellation during sendPlay dispatch neither strands the flag nor an unpaired key event`() = runTest { + every { audioManager.isMusicActive } returns true + mediaControl.sendPause(rememberForResume = true) + assertTrue(mediaControl.wasRecentlyPausedByCap) + clearMocks(audioManager, answers = false, recordedCalls = true) + + val dispatched = mutableListOf() + every { audioManager.dispatchMediaKeyEvent(capture(dispatched)) } just Runs + + val job = launch { mediaControl.sendPlay() } + runCurrent() // DOWN is out, we are inside sendKey's delay + job.cancel() + advanceUntilIdle() + + // Cleared before the first suspension, so a cancelled resume cannot strand a stale arm. + assertFalse(mediaControl.wasRecentlyPausedByCap) + // NonCancellable completes the pair rather than leaving a held key. + assertEquals(2, dispatched.size) + } + @Test fun `playback callback is registered with the injected handler rather than null`() { - // Regression for the ANR cluster in onPlaybackConfigChanged: passing `null` here binds - // callback delivery to the main looper, and the callback body does a binder call. + // Regression for the ANR cluster around the playback callback: registration itself is + // binder work into AudioService and must not run on the main looper, which is what passing + // `null` here would select. The handler choice also decides the Looper that serializes + // callback delivery, and the snapshot-based edge detection depends on that ordering. // Scope of this assertion: it only proves the injected handler is forwarded, not which // Looper that handler is bound to — a JVM unit test cannot inspect a Looper here (this // module does not use Robolectric). The Looper identity is covered instead by - // `AndroidModule.audioCallbackHandler()`, which is the single place that constructs it, - // and by the runtime QA check asserting the registration logs thread `CAPod-MediaControl`. + // `AndroidModule.audioCallbackHandler()`, which is the single place that constructs it. verify { audioManager.registerAudioPlaybackCallback(any(), handler) } }