refactor(video): apply code review fixes

amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/VideoViewInner.kt

@@ -27,6 +27,7 @@ import androidx.compose.runtime.remember
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.layout.ContentScale
 import com.vitorpamplona.amethyst.service.playback.composable.controls.ApplyInitialVideoQuality
+import com.vitorpamplona.amethyst.service.playback.composable.controls.VideoQualityPolicy
 import com.vitorpamplona.amethyst.service.playback.composable.mainVideo.VideoPlayerActiveMutex
 import com.vitorpamplona.amethyst.service.playback.composable.mediaitem.GetMediaItem
 import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel
@@ -75,7 +76,7 @@ fun VideoViewInner(
         ) { controller ->
             ApplyInitialVideoQuality(
                 player = controller.controller,
-                isFullscreen = isFullscreen,
+                policy = if (isFullscreen) VideoQualityPolicy.AUTO else VideoQualityPolicy.LOWEST,
             )
             VideoPlayerActiveMutex(controller) { videoModifier, isClosestToTheCenterOfTheScreen ->
                 ControlWhenPlayerIsActive(controller, automaticallyStartPlayback, isClosestToTheCenterOfTheScreen)

amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/InitialVideoQualitySelector.kt

@@ -20,77 +20,98 @@
  */
 package com.vitorpamplona.amethyst.service.playback.composable.controls
 
+import androidx.annotation.MainThread
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.DisposableEffect
+import androidx.compose.runtime.MutableState
+import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.media3.common.Player
 import androidx.media3.common.Tracks
 
+/**
+ * Which HLS rendition to pick automatically the first time tracks become available for a media
+ * item. Expressed as an explicit policy instead of a `Boolean` so call sites must commit to one
+ * value at construction and any future dynamic-state caller is forced to key the effect on it.
+ */
+enum class VideoQualityPolicy {
+    /** Lock to the lowest-resolution rendition (feed and PiP: save bandwidth). */
+    LOWEST,
+
+    /** Clear any video override so the player uses adaptive bitrate (fullscreen). */
+    AUTO,
+}
+
 /**
  * Applies a default video quality when tracks become available on the given player.
  *
- * - In feed context (`isFullscreen = false`): locks to the lowest-resolution rendition to save
- *   bandwidth. The user can still manually pick any quality (or "Auto") via the quality button.
- * - In fullscreen context (`isFullscreen = true`): clears any video override so the player uses
- *   adaptive bitrate selection (Auto).
- *
  * The initial selection is applied once per media item. If the user later changes the quality
  * manually, or swaps to a different media item, the new choice wins — we don't reapply for the
  * same media id. Selections intentionally don't persist across composable lifecycles, so opening
- * a feed video in fullscreen starts with Auto and returning to the feed starts with lowest again.
+ * a feed video in fullscreen starts with [VideoQualityPolicy.AUTO] and returning to the feed
+ * starts with [VideoQualityPolicy.LOWEST] again.
  */
 @Composable
 fun ApplyInitialVideoQuality(
     player: Player,
-    isFullscreen: Boolean,
+    policy: VideoQualityPolicy,
 ) {
     // Tracks the media id we've already initialized so we don't fight user overrides after the
-    // first application. Scoped to this composable instance so fullscreen <-> feed transitions
+    // first application.
-    // reset the choice as required (they're separate VideoViewInner instances with separate
+    val appliedForMediaId = remember(player) { mutableStateOf<String?>(null) }
-    // players, so isFullscreen never flips on a given instance).
+
-    val appliedForMediaId = remember(player) { arrayOf<String?>(null) }
+    DisposableEffect(player, policy) {
+        // Re-arm the guard whenever the player or the policy changes so a new policy gets a
+        // chance to apply even if the same media id has already been handled under the old one.
+        appliedForMediaId.value = null
 
-    DisposableEffect(player) {
         val listener =
             object : Player.Listener {
                 override fun onTracksChanged(tracks: Tracks) {
-                    applyInitialQuality(player, tracks, isFullscreen, appliedForMediaId)
+                    applyInitialQuality(player, tracks, policy, appliedForMediaId)
                 }
             }
 
         // Tracks might already be available by the time we attach the listener.
-        applyInitialQuality(player, player.currentTracks, isFullscreen, appliedForMediaId)
+        applyInitialQuality(player, player.currentTracks, policy, appliedForMediaId)
         player.addListener(listener)
         onDispose { player.removeListener(listener) }
     }
 }
 
+// Invoked from Player.Listener callbacks and DisposableEffect bodies, both of which run on
+// the player's application looper (main thread for ExoPlayer). The body mutates Compose state
+// and trackSelectionParameters; both are main-thread-only.
+@MainThread
 private fun applyInitialQuality(
     player: Player,
     tracks: Tracks,
-    isFullscreen: Boolean,
+    policy: VideoQualityPolicy,
-    appliedForMediaId: Array<String?>,
+    appliedForMediaId: MutableState<String?>,
 ) {
     val mediaId = player.currentMediaItem?.mediaId ?: return
-    if (appliedForMediaId[0] == mediaId) return
+    if (appliedForMediaId.value == mediaId) return
 
     val videoGroup = getVideoTrackGroup(tracks) ?: return
-    // No point forcing a choice when there's only one rendition.
+    // No point forcing a choice when there's only one rendition, and no future update will
+    // change that for this media id, so mark it as settled.
     if (videoGroup.length <= 1) {
-        appliedForMediaId[0] = mediaId
+        appliedForMediaId.value = mediaId
         return
     }
 
-    if (isFullscreen) {
+    when (policy) {
-        // Ensure adaptive selection is active by removing any pre-existing video override.
+        VideoQualityPolicy.AUTO -> {
-        if (hasVideoOverride(player)) {
+            if (hasVideoOverride(player)) clearVideoOverride(player)
-            clearVideoOverride(player)
+            appliedForMediaId.value = mediaId
         }
-    } else {
+
-        val lowestIndex = findLowestResolutionTrackIndex(videoGroup)
+        VideoQualityPolicy.LOWEST -> {
-        if (lowestIndex != null) {
+            // If no supported track has a positive short side yet, leave the guard unset so we
+            // retry on the next onTracksChanged when real video dimensions arrive.
+            val lowestIndex = findLowestResolutionTrackIndex(videoGroup) ?: return
             selectVideoTrack(player, videoGroup, lowestIndex)
+            appliedForMediaId.value = mediaId
         }
     }
-    appliedForMediaId[0] = mediaId
 }

amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/VideoQualityButton.kt

@@ -71,26 +71,15 @@ fun VideoQualityButton(
     modifier: Modifier = Modifier,
 ) {
     var tracks by remember(player) { mutableStateOf(player.currentTracks) }
-    // Track the rendering video size separately from tracks. In adaptive playback,
-    // Tracks.Group.isTrackSelected(i) returns true for every rung in the adaptive
-    // set, so we can't derive the currently-playing resolution from the Tracks
-    // object. Player.videoSize + onVideoSizeChanged gives the real rendered size
-    // and updates whenever ABR steps up or down.
-    var videoSize by remember(player) { mutableStateOf(player.videoSize) }
     var openDialog by remember { mutableStateOf(false) }
 
     DisposableEffect(player) {
         tracks = player.currentTracks
-        videoSize = player.videoSize
         val listener =
             object : Player.Listener {
                 override fun onTracksChanged(newTracks: Tracks) {
                     tracks = newTracks
                 }
-
-                override fun onVideoSizeChanged(newSize: VideoSize) {
-                    videoSize = newSize
-                }
             }
         player.addListener(listener)
         onDispose { player.removeListener(listener) }
@@ -99,8 +88,6 @@ fun VideoQualityButton(
     val videoGroup = getVideoTrackGroup(tracks) ?: return
     if (videoGroup.length <= 1) return
 
-    val currentShortSide = minOf(videoSize.width, videoSize.height).takeIf { it > 0 }
-
     AnimatedVisibility(
         visible = controllerVisible.value,
         modifier = modifier,
@@ -131,6 +118,21 @@ fun VideoQualityButton(
     }
 
     if (openDialog) {
+        // Scope videoSize tracking to the open popup: HLS ABR fires onVideoSizeChanged on every
+        // rung switch, and we don't want to pay recomposition cost on cards whose menu isn't open.
+        var videoSize by remember(player) { mutableStateOf(player.videoSize) }
+        DisposableEffect(player) {
+            val listener =
+                object : Player.Listener {
+                    override fun onVideoSizeChanged(newSize: VideoSize) {
+                        videoSize = newSize
+                    }
+                }
+            player.addListener(listener)
+            onDispose { player.removeListener(listener) }
+        }
+        val currentShortSide = minOf(videoSize.width, videoSize.height).takeIf { it > 0 }
+
         Popup(
             alignment = Alignment.BottomCenter,
             onDismissRequest = { openDialog = false },
@@ -141,7 +143,7 @@ fun VideoQualityButton(
                 currentShortSide = currentShortSide,
                 isAuto = !hasVideoOverride(player),
                 onSelectAuto = {
-                    clearVideoOverride(player)
+                    if (hasVideoOverride(player)) clearVideoOverride(player)
                     openDialog = false
                 },
                 onSelectTrack = { trackIndex ->
@@ -207,6 +209,7 @@ private data class QualityChoice(
 private fun buildQualityChoices(group: Tracks.Group): ImmutableList<QualityChoice> {
     val choices = mutableListOf<QualityChoice>()
     for (i in 0 until group.length) {
+        if (!group.isTrackSupported(i)) continue
         val format = group.getTrackFormat(i)
         val shortSide = minOf(format.width, format.height)
         if (shortSide > 0) {

amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/composable/controls/VideoQualityControls.kt

@@ -27,16 +27,18 @@ import androidx.media3.common.TrackSelectionOverride
 import androidx.media3.common.Tracks
 import androidx.media3.common.util.UnstableApi
 
-fun getVideoTrackGroup(tracks: Tracks): Tracks.Group? = tracks.groups.firstOrNull { it.type == C.TRACK_TYPE_VIDEO && it.length > 0 }
+internal fun getVideoTrackGroup(tracks: Tracks): Tracks.Group? = tracks.groups.firstOrNull { it.type == C.TRACK_TYPE_VIDEO && it.length > 0 }
 
 // Finds the track with the smallest short side (min(width, height)) in the given video group.
 // Returns null if no track has a positive short side. Used to force lowest-resolution playback
-// in feeds to save bandwidth.
+// in feeds to save bandwidth. Skips tracks the device can't decode — ExoPlayer would silently
+// reject an override pointing at an unsupported track and fall back to adaptive selection.
 @OptIn(UnstableApi::class)
-fun findLowestResolutionTrackIndex(group: Tracks.Group): Int? {
+internal fun findLowestResolutionTrackIndex(group: Tracks.Group): Int? {
     var bestIndex: Int? = null
     var bestShortSide = Int.MAX_VALUE
     for (i in 0 until group.length) {
+        if (!group.isTrackSupported(i)) continue
         val format = group.getTrackFormat(i)
         val shortSide = minOf(format.width, format.height)
         if (shortSide > 0 && shortSide < bestShortSide) {
@@ -48,9 +50,9 @@ fun findLowestResolutionTrackIndex(group: Tracks.Group): Int? {
 }
 
 @OptIn(UnstableApi::class)
-fun hasVideoOverride(player: Player): Boolean = player.trackSelectionParameters.overrides.any { (key, _) -> key.type == C.TRACK_TYPE_VIDEO }
+internal fun hasVideoOverride(player: Player): Boolean = player.trackSelectionParameters.overrides.any { (key, _) -> key.type == C.TRACK_TYPE_VIDEO }
 
-fun clearVideoOverride(player: Player) {
+internal fun clearVideoOverride(player: Player) {
     player.trackSelectionParameters =
         player.trackSelectionParameters
             .buildUpon()
@@ -58,7 +60,8 @@ fun clearVideoOverride(player: Player) {
             .build()
 }
 
-fun selectVideoTrack(
+@OptIn(UnstableApi::class)
+internal fun selectVideoTrack(
     player: Player,
     group: Tracks.Group,
     trackIndex: Int,

amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/pip/PipVideoActivity.kt

@@ -35,6 +35,7 @@ import androidx.media3.common.util.UnstableApi
 import com.vitorpamplona.amethyst.service.playback.composable.DEFAULT_MUTED_SETTING
 import com.vitorpamplona.amethyst.service.playback.composable.GetVideoController
 import com.vitorpamplona.amethyst.service.playback.composable.controls.ApplyInitialVideoQuality
+import com.vitorpamplona.amethyst.service.playback.composable.controls.VideoQualityPolicy
 import com.vitorpamplona.amethyst.service.playback.composable.mediaitem.GetMediaItem
 import com.vitorpamplona.amethyst.service.playback.composable.mediaitem.MediaItemData
 
@@ -55,7 +56,7 @@ class PipVideoActivity : ComponentActivity() {
                         // rendition. User can still manually change quality via controls.
                         ApplyInitialVideoQuality(
                             player = controllerState.controller,
-                            isFullscreen = false,
+                            policy = VideoQualityPolicy.LOWEST,
                         )
                         RegisterBackgroundMedia(controllerState)
                         RegisterControllerReceiver(controllerState)