From c0dd97e770b180ad2a2381feb2667f88e0257286 Mon Sep 17 00:00:00 2001 From: tonihei Date: Wed, 9 Apr 2025 06:13:02 -0700 Subject: [PATCH] Clear surface from previous player when assigning a new player The surface must only be used by one player at a time. To ensure that, we can keep a reference to the previously used player and clear its surface reference before assigning to a new one. Note that we do not need to clear the surface in onDispose of a DisposableEffect because the lifecycle management of the surface is moved to the Player and the Player takes care of unregistering its surface reference as soon as the surface is destroyed (which happens when the AndroidView element is no longer is the Composable tree). PiperOrigin-RevId: 745558414 (cherry picked from commit f9617e1f8dade2b957fefd7eaa69da6d5c158eb8) --- RELEASENOTES.md | 1 + .../media3/ui/compose/PlayerSurface.kt | 67 ++++++++++++------- .../media3/ui/compose/PlayerSurfaceTest.kt | 30 +++++++++ 3 files changed, 75 insertions(+), 23 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 0ee60e5cba..6b5e6bf6d6 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -65,6 +65,7 @@ * UI: * Enable `PlayerSurface` to work with `ExoPlayer.setVideoEffects` and `CompositionPlayer`. + * Fix bug where `PlayerSurface` can't be recomposed with a new `Player`. * Downloads: * Add partial download support for progressive streams. Apps can prepare a progressive stream with `DownloadHelper`, and request a diff --git a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/PlayerSurface.kt b/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/PlayerSurface.kt index a89b8c97b5..77d83d31a2 100644 --- a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/PlayerSurface.kt +++ b/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/PlayerSurface.kt @@ -16,12 +16,17 @@ package androidx.media3.ui.compose +import android.content.Context import android.view.SurfaceView import android.view.TextureView +import android.view.View import androidx.annotation.IntDef import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue -import androidx.compose.runtime.rememberUpdatedState +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier import androidx.compose.ui.viewinterop.AndroidView import androidx.media3.common.Player @@ -47,37 +52,53 @@ fun PlayerSurface( modifier: Modifier = Modifier, surfaceType: @SurfaceType Int = SURFACE_TYPE_SURFACE_VIEW, ) { - // Player might change between compositions, - // we need long-lived surface-related lambdas to always use the latest value - val currentPlayer by rememberUpdatedState(player) - when (surfaceType) { SURFACE_TYPE_SURFACE_VIEW -> - AndroidView( - factory = { - SurfaceView(it).apply { - if (currentPlayer.isCommandAvailable(Player.COMMAND_SET_VIDEO_SURFACE)) - currentPlayer.setVideoSurfaceView(this) - } - }, - onReset = {}, - modifier = modifier, + PlayerSurfaceInternal( + player, + modifier, + createView = { SurfaceView(it) }, + setViewOnPlayer = { player, view -> player.setVideoSurfaceView(view) }, + clearViewFromPlayer = { player, view -> player.clearVideoSurfaceView(view) }, ) SURFACE_TYPE_TEXTURE_VIEW -> - AndroidView( - factory = { - TextureView(it).apply { - if (currentPlayer.isCommandAvailable(Player.COMMAND_SET_VIDEO_SURFACE)) - currentPlayer.setVideoTextureView(this) - } - }, - onReset = {}, - modifier = modifier, + PlayerSurfaceInternal( + player, + modifier, + createView = { TextureView(it) }, + setViewOnPlayer = { player, view -> player.setVideoTextureView(view) }, + clearViewFromPlayer = { player, view -> player.clearVideoTextureView(view) }, ) else -> throw IllegalArgumentException("Unrecognized surface type: $surfaceType") } } +@Composable +private fun PlayerSurfaceInternal( + player: Player, + modifier: Modifier, + createView: (Context) -> T, + setViewOnPlayer: (Player, T) -> Unit, + clearViewFromPlayer: (Player, T) -> Unit, +) { + var view by remember { mutableStateOf(null) } + var registeredPlayer by remember { mutableStateOf(null) } + AndroidView(factory = { createView(it).apply { view = this } }, onReset = {}, modifier = modifier) + view?.let { view -> + LaunchedEffect(view, player) { + registeredPlayer?.let { previousPlayer -> + if (previousPlayer.isCommandAvailable(Player.COMMAND_SET_VIDEO_SURFACE)) + clearViewFromPlayer(previousPlayer, view) + registeredPlayer = null + } + if (player.isCommandAvailable(Player.COMMAND_SET_VIDEO_SURFACE)) { + setViewOnPlayer(player, view) + registeredPlayer = player + } + } + } +} + /** * The type of surface used for media playbacks. One of [SURFACE_TYPE_SURFACE_VIEW] or * [SURFACE_TYPE_TEXTURE_VIEW]. diff --git a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/PlayerSurfaceTest.kt b/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/PlayerSurfaceTest.kt index 81ed7a4c83..efe319f59b 100644 --- a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/PlayerSurfaceTest.kt +++ b/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/PlayerSurfaceTest.kt @@ -21,6 +21,7 @@ import androidx.compose.runtime.MutableIntState import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.remember import androidx.compose.ui.test.junit4.createComposeRule +import androidx.media3.common.ForwardingPlayer import androidx.media3.common.Player import androidx.media3.ui.compose.utils.TestPlayer import androidx.test.ext.junit.runners.AndroidJUnit4 @@ -28,6 +29,9 @@ import com.google.common.truth.Truth.assertThat import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import org.mockito.ArgumentMatchers.any +import org.mockito.Mockito.inOrder +import org.mockito.Mockito.spy /** Unit test for [PlayerSurface]. */ @RunWith(AndroidJUnit4::class) @@ -87,4 +91,30 @@ class PlayerSurfaceTest { assertThat(player.videoOutput).isInstanceOf(SurfaceView::class.java) } + + @Test + fun playerSurface_withNewPlayer_unsetsSurfaceOnOldPlayerFirst() { + val player0 = TestPlayer() + val player1 = TestPlayer() + val spyPlayer0 = spy(ForwardingPlayer(player0)) + val spyPlayer1 = spy(ForwardingPlayer(player1)) + + lateinit var playerIndex: MutableIntState + composeTestRule.setContent { + playerIndex = remember { mutableIntStateOf(0) } + PlayerSurface( + player = if (playerIndex.intValue == 0) spyPlayer0 else spyPlayer1, + surfaceType = SURFACE_TYPE_SURFACE_VIEW, + ) + } + composeTestRule.waitForIdle() + playerIndex.intValue = 1 + composeTestRule.waitForIdle() + + assertThat(player0.videoOutput).isNull() + assertThat(player1.videoOutput).isNotNull() + val inOrder = inOrder(spyPlayer0, spyPlayer1) + inOrder.verify(spyPlayer0).clearVideoSurfaceView(any()) + inOrder.verify(spyPlayer1).setVideoSurfaceView(any()) + } }