From a95377cf7bc87da7f0c96418283de32f67de193d Mon Sep 17 00:00:00 2001 From: darken Date: Tue, 28 Jul 2026 16:00:03 +0200 Subject: [PATCH] fix(media): Move audio playback callback off the main thread MediaControl registered its AudioPlaybackCallback with a null Handler, binding delivery to the main looper. Both the callback body and the constructor's seed read call AudioManager.isMusicActive, a binder transaction into AudioService, producing two ANR clusters: one in onPlaybackConfigChanged and one in , the latter on the cold-start critical path since MediaControl is constructed during App.onCreate. Registration and seeding now run on a dedicated, injected Handler backed by a "CAPod-MediaControl" HandlerThread, and the callback is delivered on that same looper. Registration happens before seeding so a transition during registration is queued behind the seed instead of being lost. The handler is constructor-injected via a new @AudioCallbackHandler qualifier so unit tests can drive it without Robolectric. --- .../eu/darken/capod/common/MediaControl.kt | 26 +++++- .../capod/common/dagger/AndroidModule.kt | 14 ++++ .../darken/capod/common/MediaControlTest.kt | 82 ++++++++++++++++++- 3 files changed, 117 insertions(+), 5 deletions(-) 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 9c0af6f4..1bf27ae8 100644 --- a/app/src/main/java/eu/darken/capod/common/MediaControl.kt +++ b/app/src/main/java/eu/darken/capod/common/MediaControl.kt @@ -3,7 +3,9 @@ package eu.darken.capod.common import android.media.AudioManager import android.media.AudioPlaybackConfiguration import android.os.Build +import android.os.Handler import android.view.KeyEvent +import eu.darken.capod.common.dagger.AudioCallbackHandler 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 @@ -16,6 +18,7 @@ import javax.inject.Singleton class MediaControl @Inject constructor( private val audioManager: AudioManager, private val timeSource: TimeSource, + @AudioCallbackHandler private val audioCallbackHandler: Handler, ) { /** * Set when [sendPause] dispatches a pause we expect to take effect, cleared when [sendPlay] @@ -43,10 +46,25 @@ class MediaControl @Inject constructor( } init { - // Seed the active flag from current state so we won't miss the next inactive→active - // transition if music is already playing when MediaControl is constructed. - lastKnownMusicActive = audioManager.isMusicActive - audioManager.registerAudioPlaybackCallback(playbackCallback, null) + // 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 + // produced ANRs. + // + // Register BEFORE seeding: both run on this handler's Looper, so any playback callback that + // fires during registration is queued behind this runnable and cannot execute until the seed + // is done. Seeding first would instead leave a gap in which a transition is neither delivered + // (not yet registered) nor reflected in the seed. + // + // Residual window: between construction and this runnable draining, no callback is live. That + // is one runnable-drain on an empty queue. If an auto-pause armed capPaused and the user + // manually resumed inside it, the inactive→active edge is missed and capPaused stays stale, + // costing one redundant (idempotent) MEDIA_PLAY on a later pod-in. Accepted deliberately; + // gating reactions on a readiness latch would risk auto-play/pause failing silently instead. + audioCallbackHandler.post { + audioManager.registerAudioPlaybackCallback(playbackCallback, audioCallbackHandler) + lastKnownMusicActive = audioManager.isMusicActive + log(TAG, INFO) { "Playback callback registered on ${Thread.currentThread().name}" } + } } val isPlaying: Boolean diff --git a/app/src/main/java/eu/darken/capod/common/dagger/AndroidModule.kt b/app/src/main/java/eu/darken/capod/common/dagger/AndroidModule.kt index 1cf8de1c..8c164581 100644 --- a/app/src/main/java/eu/darken/capod/common/dagger/AndroidModule.kt +++ b/app/src/main/java/eu/darken/capod/common/dagger/AndroidModule.kt @@ -5,10 +5,13 @@ import android.app.NotificationManager import android.bluetooth.BluetoothManager import android.content.Context import android.media.AudioManager +import android.os.Handler +import android.os.HandlerThread import dagger.Module import dagger.Provides import dagger.hilt.InstallIn import dagger.hilt.components.SingletonComponent +import javax.inject.Qualifier import javax.inject.Singleton @InstallIn(SingletonComponent::class) @@ -34,4 +37,15 @@ class AndroidModule { fun audioManager(context: Context): AudioManager = context.getSystemService(Context.AUDIO_SERVICE) as AudioManager + @Provides + @Singleton + @AudioCallbackHandler + fun audioCallbackHandler(): Handler = + Handler(HandlerThread("CAPod-MediaControl").apply { start() }.looper) + } + +@Qualifier +@MustBeDocumented +@Retention(AnnotationRetention.RUNTIME) +annotation class AudioCallbackHandler 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 3dfe4d41..7aaa2bda 100644 --- a/app/src/test/java/eu/darken/capod/common/MediaControlTest.kt +++ b/app/src/test/java/eu/darken/capod/common/MediaControlTest.kt @@ -1,6 +1,7 @@ package eu.darken.capod.common import android.media.AudioManager +import android.os.Handler import io.mockk.CapturingSlot import io.mockk.Runs import io.mockk.clearMocks @@ -9,6 +10,7 @@ import io.mockk.just import io.mockk.mockk import io.mockk.slot import io.mockk.verify +import io.mockk.verifyOrder import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertTrue @@ -22,7 +24,9 @@ class MediaControlTest : BaseTest() { private lateinit var audioManager: AudioManager private lateinit var mediaControl: MediaControl private lateinit var timeSource: TestTimeSource + private lateinit var handler: Handler private lateinit var playbackCallbackSlot: CapturingSlot + private lateinit var initRunnableSlot: CapturingSlot @BeforeEach fun setup() { @@ -35,7 +39,15 @@ class MediaControlTest : BaseTest() { every { audioManager.isMusicActive } returns false playbackCallbackSlot = slot() every { audioManager.registerAudioPlaybackCallback(capture(playbackCallbackSlot), any()) } just Runs - mediaControl = MediaControl(audioManager, timeSource) + // The handler is captured, not inlined: registration and seeding now happen on a posted + // runnable, and an unconditionally-inline post would hide the window that exists between + // construction and that runnable draining. + handler = mockk() + initRunnableSlot = slot() + every { handler.post(capture(initRunnableSlot)) } returns true + mediaControl = MediaControl(audioManager, timeSource, handler) + // Drain the init runnable so `playbackCallbackSlot` is populated for `fireCallback()`. + initRunnableSlot.captured.run() } private fun fireCallback() { @@ -228,4 +240,72 @@ class MediaControlTest : BaseTest() { // we don't want to claim our pause "stuck" when audio is playing. assertFalse(mediaControl.wasRecentlyPausedByCap) } + + @Test + fun `playback callback is registered on the injected background handler, not the main looper`() { + // 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. + verify { audioManager.registerAudioPlaybackCallback(any(), handler) } + } + + @Test + fun `construction does no binder work on the calling thread`() { + // Regression for the ANR cluster in MediaControl's constructor: it runs during + // App.onCreate via Hilt, so nothing here may touch AudioService inline. + val freshAudioManager = mockk(relaxed = true) + val freshHandler = mockk() + every { freshHandler.post(any()) } returns true + + MediaControl(freshAudioManager, timeSource, freshHandler) + + verify(exactly = 0) { freshAudioManager.isMusicActive } + verify(exactly = 0) { freshAudioManager.registerAudioPlaybackCallback(any(), any()) } + verify(exactly = 1) { freshHandler.post(any()) } + } + + @Test + fun `init registers the callback before seeding the active flag`() { + // Seeding first would leave a gap in which a transition is neither delivered (not yet + // registered) nor reflected in the seed. + val freshAudioManager = mockk(relaxed = true) + every { freshAudioManager.isMusicActive } returns false + every { freshAudioManager.registerAudioPlaybackCallback(any(), any()) } just Runs + val freshHandler = mockk() + val runnableSlot = slot() + every { freshHandler.post(capture(runnableSlot)) } returns true + + MediaControl(freshAudioManager, timeSource, freshHandler) + runnableSlot.captured.run() + + verifyOrder { + freshAudioManager.registerAudioPlaybackCallback(any(), freshHandler) + freshAudioManager.isMusicActive + } + } + + @Test + fun `media control still works before the init runnable has drained`() = runTest { + // The window between construction and the posted runnable running: no callback is live + // yet, but the key-dispatch paths must behave normally. + val freshAudioManager = mockk(relaxed = true) + every { freshAudioManager.isMusicActive } returns true + every { freshAudioManager.dispatchMediaKeyEvent(any()) } just Runs + every { freshAudioManager.registerAudioPlaybackCallback(any(), any()) } just Runs + val freshHandler = mockk() + every { freshHandler.post(any()) } returns true + + val undrained = MediaControl(freshAudioManager, timeSource, freshHandler) + + assertFalse(undrained.wasRecentlyPausedByCap) + + assertTrue(undrained.sendPause(rememberForResume = true)) + assertTrue(undrained.wasRecentlyPausedByCap) + verify(exactly = 2) { freshAudioManager.dispatchMediaKeyEvent(any()) } + + clearMocks(freshAudioManager, answers = false, recordedCalls = true) + + undrained.sendPlay() + assertFalse(undrained.wasRecentlyPausedByCap) + verify(exactly = 2) { freshAudioManager.dispatchMediaKeyEvent(any()) } + } }