From 9985b65f641127158e3646588a9f8c58a9dbcb3d Mon Sep 17 00:00:00 2001 From: Beth Thibodeau Date: Thu, 22 Apr 2021 21:50:52 -0400 Subject: [PATCH] Handle media session destroyed It is possible for a media session to be destroyed without updating the playback state or removing the notification, in which case the controller will no longer be valid. So we can treat this like resumption, and re-register the controller and re-activate the controls if we later get an event where the media is playing. Fixes: 185899440 Test: MediaTimeoutListenerTest Change-Id: If8b8922b436261ccf703ee9e1e3c0294171a6c2e --- .../systemui/media/MediaTimeoutListener.kt | 72 +++++++++++++------ .../media/MediaTimeoutListenerTest.kt | 37 ++++++++++ 2 files changed, 87 insertions(+), 22 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt b/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt index ce72991d01c05..8bfe94bbd0ded 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt @@ -51,36 +51,53 @@ class MediaTimeoutListener @Inject constructor( lateinit var timeoutCallback: (String, Boolean) -> Unit override fun onMediaDataLoaded(key: String, oldKey: String?, data: MediaData) { - if (mediaListeners.containsKey(key)) { - return + var reusedListener: PlaybackStateListener? = null + + // First check if we already have a listener + mediaListeners.get(key)?.let { + if (!it.destroyed) { + return + } + + // If listener was destroyed previously, we'll need to re-register it + if (DEBUG) { + Log.d(TAG, "Reusing destroyed listener $key") + } + reusedListener = it } + // Having an old key means that we're migrating from/to resumption. We should update // the old listener to make sure that events will be dispatched to the new location. val migrating = oldKey != null && key != oldKey if (migrating) { - val reusedListener = mediaListeners.remove(oldKey) + reusedListener = mediaListeners.remove(oldKey) if (reusedListener != null) { - val wasPlaying = reusedListener.playing ?: false if (DEBUG) Log.d(TAG, "migrating key $oldKey to $key, for resumption") - reusedListener.mediaData = data - reusedListener.key = key - mediaListeners[key] = reusedListener - if (wasPlaying != reusedListener.playing) { - // If a player becomes active because of a migration, we'll need to broadcast - // its state. Doing it now would lead to reentrant callbacks, so let's wait - // until we're done. - mainExecutor.execute { - if (mediaListeners[key]?.playing == true) { - if (DEBUG) Log.d(TAG, "deliver delayed playback state for $key") - timeoutCallback.invoke(key, false /* timedOut */) - } - } - } - return } else { Log.w(TAG, "Old key $oldKey for player $key doesn't exist. Continuing...") } } + + reusedListener?.let { + val wasPlaying = it.playing ?: false + if (DEBUG) Log.d(TAG, "updating listener for $key, was playing? $wasPlaying") + it.mediaData = data + it.key = key + mediaListeners[key] = it + if (wasPlaying != it.playing) { + // If a player becomes active because of a migration, we'll need to broadcast + // its state. Doing it now would lead to reentrant callbacks, so let's wait + // until we're done. + mainExecutor.execute { + if (mediaListeners[key]?.playing == true) { + if (DEBUG) Log.d(TAG, "deliver delayed playback state for $key") + timeoutCallback.invoke(key, false /* timedOut */) + } + } + } + return + } + mediaListeners[key] = PlaybackStateListener(key, data) } @@ -99,9 +116,11 @@ class MediaTimeoutListener @Inject constructor( var timedOut = false var playing: Boolean? = null + var destroyed = false var mediaData: MediaData = data set(value) { + destroyed = false mediaController?.unregisterCallback(this) field = value mediaController = if (field.token != null) { @@ -126,15 +145,25 @@ class MediaTimeoutListener @Inject constructor( fun destroy() { mediaController?.unregisterCallback(this) cancellation?.run() + destroyed = true } override fun onPlaybackStateChanged(state: PlaybackState?) { processState(state, dispatchEvents = true) } + override fun onSessionDestroyed() { + // If the session is destroyed, the controller is no longer valid, and we will need to + // recreate it if this key is updated later + if (DEBUG) { + Log.d(TAG, "Session destroyed for $key") + } + destroy() + } + private fun processState(state: PlaybackState?, dispatchEvents: Boolean) { if (DEBUG) { - Log.v(TAG, "processState: $state") + Log.v(TAG, "processState $key: $state") } val isPlaying = state != null && isPlayingState(state.state) @@ -173,8 +202,7 @@ class MediaTimeoutListener @Inject constructor( private fun expireMediaTimeout(mediaKey: String, reason: String) { cancellation?.apply { if (DEBUG) { - Log.v(TAG, - "media timeout cancelled for $mediaKey, reason: $reason") + Log.v(TAG, "media timeout cancelled for $mediaKey, reason: $reason") } run() } diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt index f3979592c894b..0a573cd6020c5 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt @@ -227,4 +227,41 @@ class MediaTimeoutListenerTest : SysuiTestCase() { mediaTimeoutListener.onMediaDataLoaded(KEY, null, mediaData) assertThat(mediaTimeoutListener.isTimedOut(KEY)).isFalse() } + + @Test + fun testOnSessionDestroyed_clearsTimeout() { + // GIVEN media that is paused + val mediaPaused = mediaData.copy(isPlaying = false) + mediaTimeoutListener.onMediaDataLoaded(KEY, null, mediaPaused) + verify(mediaController).registerCallback(capture(mediaCallbackCaptor)) + assertThat(executor.numPending()).isEqualTo(1) + + // WHEN the session is destroyed + mediaCallbackCaptor.value.onSessionDestroyed() + + // THEN the controller is unregistered and timeout run + verify(mediaController).unregisterCallback(anyObject()) + assertThat(executor.numPending()).isEqualTo(0) + } + + @Test + fun testSessionDestroyed_thenRestarts_resetsTimeout() { + // Assuming we have previously destroyed the session + testOnSessionDestroyed_clearsTimeout() + + // WHEN we get an update with media playing + val playingState = mock(android.media.session.PlaybackState::class.java) + `when`(playingState.state).thenReturn(PlaybackState.STATE_PLAYING) + `when`(mediaController.playbackState).thenReturn(playingState) + val mediaPlaying = mediaData.copy(isPlaying = true) + mediaTimeoutListener.onMediaDataLoaded(KEY, null, mediaPlaying) + + // THEN the timeout runnable will update the state + assertThat(executor.numPending()).isEqualTo(1) + with(executor) { + advanceClockToNext() + runAllReady() + } + verify(timeoutCallback).invoke(eq(KEY), eq(false)) + } }