From c6b275e7fea5b3743eb0f9c1e032c3c43d7084bb Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Apr 2026 12:37:56 +0000 Subject: [PATCH] perf(video): tighten remember keys and stabilize controller-overlay tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 audit cleanups. Each item is small but each runs on the hot path that recomposes during every active video, so they add up while scrolling. P1 — DimensionTag identity invalidating remember: - DimensionTag (in quartz) is a regular class with no equals override, so reference equality means a freshly parsed tag for the same event is != to the previous one. The remember(videoUri, dimensions) blocks added in the earlier perf commits were re-running their lambda on every recompose. Switch to primitive (width, height) keys in VideoView and GifVideoView so the cache lookups + MediaAspectRatioCache writes only fire when the dimensions actually change. P1 — Static gradient brushes: - TopGradientOverlay / BottomGradientOverlay were calling Brush.verticalGradient(colors = colors) inside the modifier chain, which allocated a fresh Brush on every recomposition while the controllers were visible (i.e. on every active video most of the time). Pre-build both brushes as file-level vals so they're allocated exactly once per process. P2 — ImmutableList for action collections: - RenderTopButtons / AnimatedOverflowMenuButton / OverflowMenuButton were passing List across composable boundaries. Plain List is unstable in Compose, forcing the overflow tree to recompose any time an unrelated parent state (volume, tracks, controllerVisible) ticked. Use ImmutableList end-to-end via toImmutableList() at the producer side. P2 — videoPlayerButtonItemsFlow remember: - accountViewModel.videoPlayerButtonItemsFlow() was being called fresh every recomposition, with the result handed straight to collectAsStateWithLifecycle. Hoist the call into remember(accountViewModel) so the flow reference is stable. P2 — MuteButton dispatcher cleanup: - The 2-second hold timer was using LaunchedEffect { launch(Dispatchers.IO) { delay(2000); holdOn.value = false } }. The wrapped launch was just redundant dispatcher hopping — delay() doesn't hold a thread and the Compose write is fine on Main. Inline it. --- .../service/playback/composable/VideoView.kt | 15 +++++--- .../composable/controls/GradientOverlay.kt | 37 ++++++++++++------- .../composable/controls/MuteButton.kt | 11 +++--- .../composable/controls/OverflowMenu.kt | 8 ++-- .../composable/controls/RenderTopButtons.kt | 11 +++++- .../amethyst/ui/components/GifVideoView.kt | 14 ++++++- 6 files changed, 65 insertions(+), 31 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/VideoView.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/VideoView.kt index 0160a12468..d7263930d6 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/VideoView.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/VideoView.kt @@ -115,13 +115,16 @@ fun VideoView( // Resolve the aspect ratio once per composition. Prime the URL-keyed cache from the imeta // dim tag so the next time this video appears (PiP, dialog, list re-enter) the cache hits - // without waiting for ExoPlayer's onVideoSizeChanged. + // without waiting for ExoPlayer's onVideoSizeChanged. Keys are primitive width/height so + // a freshly parsed DimensionTag instance for the same event doesn't re-run this lambda — + // DimensionTag uses reference equality, not structural. + val dimW = dimensions?.width + val dimH = dimensions?.height val ratio = - remember(videoUri, dimensions) { - val fromDim = dimensions?.takeIf { it.hasSize() } - if (fromDim != null) { - MediaAspectRatioCache.add(videoUri, fromDim.width, fromDim.height) - fromDim.aspectRatio() + remember(videoUri, dimW, dimH) { + if (dimW != null && dimH != null && dimW > 0 && dimH > 0) { + MediaAspectRatioCache.add(videoUri, dimW, dimH) + dimW.toFloat() / dimH.toFloat() } else { MediaAspectRatioCache.get(videoUri) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/GradientOverlay.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/GradientOverlay.kt index 938aa8ef9b..be5d946f8e 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/GradientOverlay.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/GradientOverlay.kt @@ -38,24 +38,33 @@ import androidx.compose.ui.unit.dp private val FadeIn = fadeIn() private val FadeOut = fadeOut() -private val TopGradientColors = - listOf( - Color.Black.copy(alpha = 0.6f), - Color.Black.copy(alpha = 0.3f), - Color.Transparent, +// Both gradient brushes are static; pre-build them once at class init so we don't allocate a +// new Brush on every recomposition while the controllers are visible (which is most of the +// time during playback / interaction). +private val TopGradientBrush = + Brush.verticalGradient( + colors = + listOf( + Color.Black.copy(alpha = 0.6f), + Color.Black.copy(alpha = 0.3f), + Color.Transparent, + ), ) -private val BottomGradientColors = - listOf( - Color.Transparent, - Color.Black.copy(alpha = 0.4f), - Color.Black.copy(alpha = 0.7f), +private val BottomGradientBrush = + Brush.verticalGradient( + colors = + listOf( + Color.Transparent, + Color.Black.copy(alpha = 0.4f), + Color.Black.copy(alpha = 0.7f), + ), ) @Composable private fun GradientOverlay( controllerVisible: State, - colors: List, + brush: Brush, height: Dp, modifier: Modifier = Modifier, ) { @@ -70,7 +79,7 @@ private fun GradientOverlay( Modifier .fillMaxWidth() .height(height) - .background(brush = Brush.verticalGradient(colors = colors)), + .background(brush = brush), ) } } @@ -80,11 +89,11 @@ fun TopGradientOverlay( controllerVisible: State, modifier: Modifier = Modifier, height: Dp = 80.dp, -) = GradientOverlay(controllerVisible, TopGradientColors, height, modifier) +) = GradientOverlay(controllerVisible, TopGradientBrush, height, modifier) @Composable fun BottomGradientOverlay( controllerVisible: State, modifier: Modifier = Modifier, height: Dp = 120.dp, -) = GradientOverlay(controllerVisible, BottomGradientColors, height, modifier) +) = GradientOverlay(controllerVisible, BottomGradientBrush, height, modifier) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/MuteButton.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/MuteButton.kt index 5c44ba3c89..787c7e30a3 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/MuteButton.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/MuteButton.kt @@ -47,9 +47,7 @@ import com.vitorpamplona.amethyst.ui.theme.Size30Modifier import com.vitorpamplona.amethyst.ui.theme.Size50Modifier import com.vitorpamplona.amethyst.ui.theme.ThemeComparisonColumn import com.vitorpamplona.amethyst.ui.theme.VolumeBottomIconSize -import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay -import kotlinx.coroutines.launch @Preview @Composable @@ -79,11 +77,12 @@ fun MuteButton( ) } + // LaunchedEffect already runs on Main, and delay() suspends without holding a thread, so + // the previous launch(Dispatchers.IO) was just unnecessary dispatcher hopping for a state + // mutation that's also fine on Main. LaunchedEffect(key1 = controllerVisible) { - launch(Dispatchers.IO) { - delay(2000) - holdOn.value = false - } + delay(2000) + holdOn.value = false } val mutedInstance = remember(startingMuteState) { mutableStateOf(startingMuteState) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/OverflowMenu.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/OverflowMenu.kt index b8a115a5c0..1bf719a92e 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/OverflowMenu.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/OverflowMenu.kt @@ -50,6 +50,8 @@ import com.vitorpamplona.amethyst.ui.theme.Size20Modifier import com.vitorpamplona.amethyst.ui.theme.Size50Modifier import com.vitorpamplona.amethyst.ui.theme.ThemeComparisonColumn import com.vitorpamplona.amethyst.ui.theme.VolumeBottomIconSize +import kotlinx.collections.immutable.ImmutableList +import kotlinx.collections.immutable.persistentListOf private val FadeIn = fadeIn() private val FadeOut = fadeOut() @@ -60,7 +62,7 @@ fun OverflowMenuButtonPreview() { ThemeComparisonColumn { Box(Modifier.background(BitcoinOrange)) { OverflowMenuButton( - actions = listOf(VideoPlayerAction.Share, VideoPlayerAction.Download, VideoPlayerAction.PictureInPicture), + actions = persistentListOf(VideoPlayerAction.Share, VideoPlayerAction.Download, VideoPlayerAction.PictureInPicture), startingMuteState = false, onFullscreenClick = {}, onMuteClick = {}, @@ -76,7 +78,7 @@ fun OverflowMenuButtonPreview() { @Composable fun AnimatedOverflowMenuButton( controllerVisible: State, - actions: List, + actions: ImmutableList, startingMuteState: Boolean, onFullscreenClick: (() -> Unit)?, onMuteClick: () -> Unit, @@ -107,7 +109,7 @@ fun AnimatedOverflowMenuButton( @Composable fun OverflowMenuButton( - actions: List, + actions: ImmutableList, startingMuteState: Boolean, onFullscreenClick: (() -> Unit)?, onMuteClick: () -> Unit, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderTopButtons.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderTopButtons.kt index d7c1645f0d..5d182bd842 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderTopButtons.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/RenderTopButtons.kt @@ -70,6 +70,7 @@ import com.vitorpamplona.amethyst.ui.theme.PinBottomIconSize import com.vitorpamplona.amethyst.ui.theme.Size20Modifier import com.vitorpamplona.amethyst.ui.theme.Size50Modifier import com.vitorpamplona.amethyst.ui.theme.ThemeComparisonColumn +import kotlinx.collections.immutable.toImmutableList @Preview @Composable @@ -194,7 +195,10 @@ fun RenderTopButtons( modifier: Modifier, accountViewModel: AccountViewModel, ) { - val buttonItems by accountViewModel.videoPlayerButtonItemsFlow().collectAsStateWithLifecycle() + // Hold the StateFlow itself across recompositions so collectAsStateWithLifecycle isn't + // keyed on the result of a property getter call that happens every recompose. + val buttonItemsFlow = remember(accountViewModel) { accountViewModel.videoPlayerButtonItemsFlow() } + val buttonItems by buttonItemsFlow.collectAsStateWithLifecycle() val shareDialogVisible = remember { mutableStateOf(false) } val saveAction = rememberSaveMediaAction { context -> @@ -212,17 +216,22 @@ fun RenderTopButtons( } val canFullscreen = onZoomClick != null + // ImmutableList so Compose can treat the action lists as stable parameters when they're + // passed through to AnimatedOverflowMenuButton — a plain List is unstable and forces the + // overflow tree to recompose whenever any unrelated parent state ticks. val topBarActions = remember(buttonItems, canFullscreen, hasMultipleQualities, isLive, pipSupported) { buttonItems .filter { it.location == VideoButtonLocation.TopBar && isAvailable(it.action) } .map { it.action } + .toImmutableList() } val overflowActions = remember(buttonItems, canFullscreen, hasMultipleQualities, isLive, pipSupported) { buttonItems .filter { it.location == VideoButtonLocation.OverflowMenu && isAvailable(it.action) } .map { it.action } + .toImmutableList() } Row(modifier) { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt index 1b33389f0e..e9bb15e63a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/GifVideoView.kt @@ -35,6 +35,7 @@ import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.SideEffect import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue +import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip @@ -68,7 +69,18 @@ fun GifVideoView( accountViewModel: AccountViewModel, thumbhash: String? = null, ) { - val ratio = dimensions?.aspectRatio() ?: MediaAspectRatioCache.get(videoUri) + // Keys are primitive width/height so a freshly parsed DimensionTag instance for the same + // event doesn't re-run this lambda — DimensionTag uses reference equality, not structural. + val dimW = dimensions?.width + val dimH = dimensions?.height + val ratio = + remember(videoUri, dimW, dimH) { + if (dimW != null && dimH != null && dimH > 0) { + dimW.toFloat() / dimH.toFloat() + } else { + MediaAspectRatioCache.get(videoUri) + } + } val autoPlay = accountViewModel.settings.autoPlayVideos() val borderModifier = if (roundedCorner) MaterialTheme.colorScheme.imageModifier else Modifier val context = LocalContext.current