chore(video): correctness and hygiene cleanups in playback layer

Round-up of the small leftovers from the audit. None move the needle on
their own; together they remove a real cancellation bug and tighten the
playback types.

- PlaybackServiceClient.executorService: Executors.newCachedThreadPool()
  → Executors.newSingleThreadExecutor(). The work per callback is
  Future.get() on an already-completed future plus a non-blocking
  trySend; a single thread is plenty. The previous unbounded pool could
  spin up a thread per concurrent video, each lingering for the 60 s
  keep-alive afterwards.

- MediaControllerState.controller: var → val. The field was never
  reassigned anywhere (grep confirms), and a non-observable var on a
  @Stable class is a footgun — Compose can't see writes to a plain var,
  so any future write would silently miss recomposition.

- MediaControllerState.currrentMedia() → currentMedia(). Typo. Updated
  the single caller in PipVideoView.

- LoadThumbAndThenVideoView: real cancellation bug fix. The Coil fetch
  was launched into AccountViewModel.viewModelScope via a side helper
  (loadThumb), so a scroll-away didn't cancel the in-flight image
  request — wasted bandwidth and a late callback writing into stale
  state. Inline the Coil call into the LaunchedEffect's own scope so
  cancellation propagates, and key the effect on thumbUri so a recycled
  audio-track slot with a new cover doesn't stall on the prior
  Pair(true, ...) gate. Drop the now-unused AccountViewModel.loadThumb
  and its only-here imports.
This commit is contained in:
Claude
2026-04-26 13:55:53 +00:00
parent ba1a1bfc12
commit d7bd78cc32
5 changed files with 52 additions and 77 deletions
@@ -29,7 +29,14 @@ import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import androidx.compose.ui.layout.ContentScale
import androidx.compose.ui.platform.LocalContext
import coil3.asDrawable
import coil3.imageLoader
import coil3.request.ImageRequest
import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel
import com.vitorpamplona.quartz.utils.Log
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext
import kotlin.coroutines.cancellation.CancellationException
@Composable
fun LoadThumbAndThenVideoView(
@@ -45,56 +52,47 @@ fun LoadThumbAndThenVideoView(
accountViewModel: AccountViewModel,
onDialog: (() -> Unit)? = null,
) {
var loadingFinished by remember { mutableStateOf<Pair<Boolean, Drawable?>>(Pair(false, null)) }
var loadingFinished by remember(thumbUri) { mutableStateOf<Pair<Boolean, Drawable?>>(Pair(false, null)) }
val context = LocalContext.current
LaunchedEffect(Unit) {
accountViewModel.loadThumb(
context,
thumbUri,
onReady = {
loadingFinished =
if (it != null) {
Pair(true, it)
} else {
Pair(true, null)
// Run the Coil fetch in this LaunchedEffect's scope (was previously launched into the
// AccountViewModel's viewModelScope, which meant a scroll-away wouldn't cancel the in-flight
// image request — wasted bandwidth, plus the late callback wrote into a state that no
// longer mattered). Keying on thumbUri also makes the effect re-fire when a recycled slot
// gets a new audio track with a new cover instead of stalling on the stale Pair(true, ...).
LaunchedEffect(thumbUri) {
loadingFinished =
try {
val request = ImageRequest.Builder(context).data(thumbUri).build()
val drawable =
withContext(Dispatchers.IO) {
context.imageLoader
.execute(request)
.image
?.asDrawable(context.resources)
}
},
onError = { loadingFinished = Pair(true, null) },
)
Pair(true, drawable)
} catch (e: Exception) {
if (e is CancellationException) throw e
Log.e("VideoView", "Fail to load cover $thumbUri", e)
Pair(true, null)
}
}
if (loadingFinished.first) {
if (loadingFinished.second != null) {
VideoView(
videoUri = videoUri,
mimeType = mimeType,
title = title,
thumb = VideoThumb(loadingFinished.second),
roundedCorner = roundedCorner,
contentScale = contentScale,
artworkUri = thumbUri,
authorName = authorName,
nostrUriCallback = nostrUriCallback,
isLiveStream = isLiveStream,
accountViewModel = accountViewModel,
onDialog = onDialog,
)
} else {
VideoView(
videoUri = videoUri,
mimeType = mimeType,
title = title,
thumb = null,
roundedCorner = roundedCorner,
contentScale = contentScale,
artworkUri = thumbUri,
authorName = authorName,
nostrUriCallback = nostrUriCallback,
isLiveStream = isLiveStream,
accountViewModel = accountViewModel,
onDialog = onDialog,
)
}
VideoView(
videoUri = videoUri,
mimeType = mimeType,
title = title,
thumb = loadingFinished.second?.let { VideoThumb(it) },
roundedCorner = roundedCorner,
contentScale = contentScale,
artworkUri = thumbUri,
authorName = authorName,
nostrUriCallback = nostrUriCallback,
isLiveStream = isLiveStream,
accountViewModel = accountViewModel,
onDialog = onDialog,
)
}
}
@@ -31,14 +31,13 @@ import kotlin.uuid.Uuid
class MediaControllerState(
// each composable has an ID.
val id: String = Uuid.random().toString(),
// This is filled after the controller returns from this class
var controller: Player,
val controller: Player,
// visibility onscreen
val visibility: VisibilityData = VisibilityData(),
) {
fun isPlaying() = controller.isPlaying
fun currrentMedia() = controller.currentMediaItem?.mediaId
fun currentMedia() = controller.currentMediaItem?.mediaId
fun toggleMute() {
controller.volume = if (controller.volume == 0f) 1f else 0f
@@ -58,7 +58,7 @@ fun RenderPipVideo(
val modifier =
remember {
val ratio =
controller.currrentMedia()?.let {
controller.currentMedia()?.let {
MediaAspectRatioCache.get(it)
}
@@ -36,7 +36,12 @@ import kotlin.uuid.ExperimentalUuidApi
import kotlin.uuid.Uuid
object PlaybackServiceClient {
val executorService: ExecutorService = Executors.newCachedThreadPool()
// Runs the MediaController.buildAsync() completion callbacks. The work per callback is
// trivial — Future.get() on an already-completed future plus a non-blocking trySend into
// the callbackFlow channel — so a single thread is plenty. The previous newCachedThreadPool
// could spin up an unbounded number of threads when many videos appeared at once, each
// sticking around for the executor's keep-alive (60s) afterwards.
val executorService: ExecutorService = Executors.newSingleThreadExecutor()
fun shutdown() {
executorService.shutdown()
@@ -22,7 +22,6 @@ package com.vitorpamplona.amethyst.ui.screen.loggedIn
import android.annotation.SuppressLint
import android.content.Context
import android.graphics.drawable.Drawable
import android.os.Handler
import android.os.Looper
import android.util.LruCache
@@ -34,9 +33,6 @@ import androidx.compose.runtime.rememberCoroutineScope
import androidx.lifecycle.ViewModel
import androidx.lifecycle.ViewModelProvider
import androidx.lifecycle.viewModelScope
import coil3.asDrawable
import coil3.imageLoader
import coil3.request.ImageRequest
import com.vitorpamplona.amethyst.AccountInfo
import com.vitorpamplona.amethyst.Amethyst
import com.vitorpamplona.amethyst.LocalPreferences
@@ -1578,29 +1574,6 @@ class AccountViewModel(
super.onCleared()
}
fun loadThumb(
context: Context,
thumbUri: String,
onReady: (Drawable?) -> Unit,
onError: (String?) -> Unit,
) {
viewModelScope.launch(Dispatchers.IO) {
try {
val request = ImageRequest.Builder(context).data(thumbUri).build()
val myCover =
context.imageLoader
.execute(request)
.image
?.asDrawable(context.resources)
onReady(myCover)
} catch (e: Exception) {
if (e is CancellationException) throw e
Log.e("VideoView", "Fail to load cover $thumbUri", e)
onError(e.message)
}
}
}
fun loadMentions(
mentions: ImmutableList<String>,
onReady: (ImmutableList<User>) -> Unit,