From 471dbd62db3b00a39567e1cbb1ae2acd42607a69 Mon Sep 17 00:00:00 2001 From: Damontecres Date: Wed, 3 Jun 2026 12:41:13 -0400 Subject: [PATCH] Fixes a crash during playback (#1514) ## Description Fixes a crash that could occur during the beginning of playback Also adds better handling for clicking next extremely fast ### Technical details The playlist objects would be created on the IO thread and it had a `mutableIntStateOf` to track the current index. This meant that there is a race conditions for the IO thread to commit the snapshot for the main thread to see them. If the main thread is faster, it will crash. To fix that, this PR moves the mutable state in `Playlist` into the `PlaybackState` flow in the view model. This enures both the background threads and re-compositions have the right state. ### Related issues Fixes #1509 ### Testing Emulator ## Screenshots N/A ## AI or LLM usage None --- .../wholphin/data/model/Playlist.kt | 34 +------ .../wholphin/services/PlaylistCreator.kt | 8 +- .../wholphin/ui/playback/PlaybackPage.kt | 10 +- .../wholphin/ui/playback/PlaybackState.kt | 17 +++- .../wholphin/ui/playback/PlaybackViewModel.kt | 93 ++++++++++++------- .../ui/playback/overlay/ChapterRowOverlay.kt | 5 +- .../ui/playback/overlay/PlaybackOverlay.kt | 14 +-- .../ui/playback/overlay/QueueRowOverlay.kt | 7 +- 8 files changed, 99 insertions(+), 89 deletions(-) diff --git a/app/src/main/java/com/github/damontecres/wholphin/data/model/Playlist.kt b/app/src/main/java/com/github/damontecres/wholphin/data/model/Playlist.kt index 3e3cbe18..68771f6a 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/data/model/Playlist.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/data/model/Playlist.kt @@ -1,8 +1,5 @@ package com.github.damontecres.wholphin.data.model -import androidx.compose.runtime.getValue -import androidx.compose.runtime.mutableIntStateOf -import androidx.compose.runtime.setValue import org.jellyfin.sdk.model.api.MediaType import java.util.UUID @@ -11,36 +8,9 @@ import java.util.UUID * * This is not the same thing as a Jellyfin server playlist */ -class Playlist( - items: List, - startIndex: Int = 0, +data class Playlist( + val items: List, ) { - val items = items.subList(startIndex, items.size) - - var index by mutableIntStateOf(0) - - fun hasPrevious(): Boolean = index > 0 - - fun hasNext(): Boolean = (index + 1) < items.size - - fun getPreviousAndReverse(): PlaylistItem = items[--index] - - fun getAndAdvance(): PlaylistItem = items[++index] - - fun peek(): PlaylistItem? = items.getOrNull(index + 1) - - fun upcomingItems(): List = items.subList(index + 1, items.size) - - fun advanceTo(id: UUID): PlaylistItem? { - while (hasNext()) { - val potential = getAndAdvance() - if (potential.id == id) { - return potential - } - } - return null - } - companion object { const val MAX_SIZE = 100 } diff --git a/app/src/main/java/com/github/damontecres/wholphin/services/PlaylistCreator.kt b/app/src/main/java/com/github/damontecres/wholphin/services/PlaylistCreator.kt index bf5a1269..055c9186 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/services/PlaylistCreator.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/services/PlaylistCreator.kt @@ -78,7 +78,7 @@ class PlaylistCreator .convertAndAddParts(false) val startIndex = episodeId?.let { episodes.indexOfFirstOrNull { it.id == episodeId } } ?: 0 - return Playlist(episodes, startIndex) + return Playlist(episodes.subList(startIndex, episodes.size)) } /** @@ -104,7 +104,7 @@ class PlaylistCreator ), ) val items = GetItemsRequestHandler.execute(api, request).content.items - return Playlist(items.convertAndAddParts(), 0) + return Playlist(items.convertAndAddParts()) } private suspend fun createFromCollection( @@ -147,7 +147,7 @@ class PlaylistCreator .execute(api, request) .content.items .convertAndAddParts() - return Playlist(items, 0) + return Playlist(items) } /** @@ -263,7 +263,7 @@ class PlaylistCreator }.let(::addAll) } } - PlaylistCreationResult.Success(Playlist(list, 0)) + PlaylistCreationResult.Success(Playlist(list)) } // Not support yet diff --git a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackPage.kt b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackPage.kt index d25375a2..6fe79290 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackPage.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackPage.kt @@ -287,7 +287,7 @@ fun PlaybackPageContent( PlaybackAction.Previous -> { val pos = player.currentPosition - if (pos < player.maxSeekToPreviousPosition && state.playlist.hasPrevious()) { + if (pos < player.maxSeekToPreviousPosition && state.hasPrevious) { viewModel.playPrevious() } else { player.seekToPrevious() @@ -401,7 +401,7 @@ fun PlaybackPageContent( controllerViewState = controllerViewState, showPlay = playPauseState.showPlay, previousEnabled = true, - nextEnabled = state.playlist.hasNext(), + nextEnabled = state.hasNext, seekEnabled = true, seekForward = preferences.appPreferences.playbackPreferences.skipForwardMs.milliseconds, seekBack = preferences.appPreferences.playbackPreferences.skipBackMs.milliseconds, @@ -414,10 +414,8 @@ fun PlaybackPageContent( chapters = state.currentMediaInfo.chapters, trickplayInfo = state.currentMediaInfo.trickPlayInfo, trickplayUrlFor = viewModel::getTrickplayUrl, - playlist = state.playlist, - onClickPlaylist = { - viewModel.playItemInPlaylist(it) - }, + queue = remember(state.playlistIndex, state.playlist) { state.upcomingItems }, + onClickPlaylist = viewModel::playItemInPlaylist, currentSegment = state.currentSegment?.segment, showClock = preferences.appPreferences.interfacePreferences.showClock, analyticsState = state.analyticsState, diff --git a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackState.kt b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackState.kt index a520df2e..b42b7151 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackState.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackState.kt @@ -6,6 +6,7 @@ import androidx.media3.common.text.Cue import com.github.damontecres.wholphin.data.model.BaseItem import com.github.damontecres.wholphin.data.model.ItemPlayback import com.github.damontecres.wholphin.data.model.Playlist +import com.github.damontecres.wholphin.data.model.PlaylistItem import com.github.damontecres.wholphin.preferences.PlayerBackend import com.github.damontecres.wholphin.ui.formatBitrate import com.github.damontecres.wholphin.util.LoadingState @@ -21,8 +22,20 @@ data class PlaybackState( val analyticsState: AnalyticsState = AnalyticsState(), val subtitleCues: List = emptyList(), val nextUp: BaseItem? = null, - val playlist: Playlist = Playlist(listOf()), -) + val playlistIndex: Int = 0, + val playlist: Playlist = Playlist(emptyList()), +) { + val hasNext: Boolean get() = (playlistIndex + 1) < playlist.items.size + val hasPrevious: Boolean get() = playlistIndex > 0 + val upcomingItems: List + get() = + playlist.items.subList( + (playlistIndex + 1).coerceAtMost(playlist.items.size), + playlist.items.size, + ) + + fun nextItem(): PlaylistItem? = playlist.items.getOrNull(playlistIndex + 1) +} data class PlayerInstance( val player: Player, diff --git a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackViewModel.kt b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackViewModel.kt index 631aacc4..696e2eb9 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackViewModel.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackViewModel.kt @@ -92,6 +92,7 @@ import kotlinx.coroutines.flow.update import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import org.jellyfin.sdk.api.client.ApiClient import org.jellyfin.sdk.api.client.extensions.mediaInfoApi @@ -164,7 +165,10 @@ class PlaybackViewModel internal lateinit var player: Player private var mediaSession: MediaSession? = null - internal val mutex = Mutex() + + // Mutex & Job for changing playlist index + private val playlistMutex = Mutex() + private var playlistJob: Job? = null val controllerViewState = ControllerViewState( @@ -345,7 +349,6 @@ class PlaybackViewModel viewModelScope.launch(ExceptionHandler()) { controllerViewState.observe() } - playlistItem.item.type == BaseItemKind.TRAILER val intros = // If not resuming playback & cinema mode is enabled, get potential intros if (positionMs == 0L && preferences.appPreferences.playbackPreferences.cinemaMode) { @@ -1017,7 +1020,7 @@ class PlaybackViewModel if (playbackState == Player.STATE_ENDED) { Timber.v("Playback state is STATE_ENDED") viewModelScope.launchDefault { - when (val nextItem = state.value.playlist.peek()) { + when (val nextItem = state.value.nextItem()) { is PlaylistItem.Intro -> { Timber.v("Next item is intro, so playing immediately") playNextUp() @@ -1093,14 +1096,14 @@ class PlaybackViewModel currentSegment.type, ) } - val playlist = state.value.playlist + val state = state.value if (currentSegment.type == MediaSegmentType.OUTRO && prefs.showNextUpWhen == ShowNextUpWhen.DURING_CREDITS && - playlist.hasNext() && + state.hasNext && outroShownSegments.add(currentSegment.id) ) { - val nextItem = playlist.peek() + val nextItem = state.nextItem() if (nextItem is PlaylistItem.Media) { Timber.v("Setting next up during outro to ${nextItem?.id}") _state.update { it.copy(nextUp = nextItem.item) } @@ -1218,30 +1221,50 @@ class PlaybackViewModel } fun playNextUp() { - state.value.playlist.let { - if (it.hasNext()) { - viewModelScope.launchDefault { + viewModelScope.launchDefault { + playlistMutex.withLock { + val state = state.value + if (state.hasNext) { cancelUpNextEpisode() - val item = it.getAndAdvance() - val played = play(item, 0) - if (!played) { - playNextUp() - } + val nextIndex = state.playlistIndex + 1 + val item = state.playlist.items[nextIndex] + _state.update { it.copy(playlistIndex = nextIndex) } + playlistJob?.cancel() + playlistJob = + viewModelScope.launchDefault { + val played = play(item, 0) + if (!played) { + playNextUp() + } + } + } else { + Timber.w("Attempting to play next, but there are no more items") + return@launchDefault } } } } fun playPrevious() { - state.value.playlist.let { - if (it.hasPrevious()) { - viewModelScope.launchDefault { + viewModelScope.launchDefault { + playlistMutex.withLock { + val state = state.value + if (state.hasPrevious) { cancelUpNextEpisode() - val item = it.getPreviousAndReverse() - val played = play(item, 0) - if (!played) { - playPrevious() - } + val previousIndex = state.playlistIndex - 1 + val item = state.playlist.items[previousIndex] + _state.update { it.copy(playlistIndex = previousIndex) } + playlistJob?.cancel() + playlistJob = + viewModelScope.launchDefault { + val played = play(item, 0) + if (!played) { + playPrevious() + } + } + } else { + Timber.w("Attempting to play next, but there are no more items") + return@launchDefault } } } @@ -1252,16 +1275,24 @@ class PlaybackViewModel } fun playItemInPlaylist(item: BaseItem) { - state.value.playlist.let { playlist -> - viewModelScope.launchIO { - val toPlay = playlist.advanceTo(item.id) - if (toPlay != null) { - val played = play(toPlay, 0) - if (!played) { - playNextUp() - } + viewModelScope.launchDefault { + playlistMutex.withLock { + val state = state.value + val index = state.playlist.items.indexOfFirst { it.id == item.id } + if (index in state.playlist.items.indices) { + val toPlay = state.playlist.items[index] + _state.update { it.copy(playlistIndex = index) } + playlistJob?.cancel() + playlistJob = + viewModelScope.launchDefault { + val played = play(toPlay, 0) + if (!played) { + playNextUp() + } + } } else { - // TODO + Timber.w("Item not found in playlist %s", item.id) + return@launchDefault } } } diff --git a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/ChapterRowOverlay.kt b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/ChapterRowOverlay.kt index 30254061..45e335ab 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/ChapterRowOverlay.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/ChapterRowOverlay.kt @@ -31,7 +31,6 @@ import androidx.tv.material3.MaterialTheme import androidx.tv.material3.Text import com.github.damontecres.wholphin.R import com.github.damontecres.wholphin.data.model.Chapter -import com.github.damontecres.wholphin.data.model.Playlist import com.github.damontecres.wholphin.ui.cards.ChapterCard import com.github.damontecres.wholphin.ui.components.HiddenFocusBox import com.github.damontecres.wholphin.ui.ifElse @@ -44,7 +43,7 @@ fun ChapterRowOverlay( player: Player, controllerViewState: ControllerViewState, chapters: List, - playlist: Playlist, + hasNext: Boolean, aspectRatio: Float, onChangeState: (OverlayViewState) -> Unit, modifier: Modifier = Modifier, @@ -147,7 +146,7 @@ fun ChapterRowOverlay( ) } } - if (playlist.hasNext()) { + if (hasNext) { Text( text = stringResource(R.string.queue), style = MaterialTheme.typography.titleLarge, diff --git a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/PlaybackOverlay.kt b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/PlaybackOverlay.kt index 96adc4df..2c2aefe8 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/PlaybackOverlay.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/PlaybackOverlay.kt @@ -50,7 +50,7 @@ import androidx.tv.material3.Text import coil3.compose.AsyncImage import com.github.damontecres.wholphin.data.model.BaseItem import com.github.damontecres.wholphin.data.model.Chapter -import com.github.damontecres.wholphin.data.model.Playlist +import com.github.damontecres.wholphin.data.model.PlaylistItem import com.github.damontecres.wholphin.data.model.aspectRatioFloat import com.github.damontecres.wholphin.ui.AppColors import com.github.damontecres.wholphin.ui.AspectRatios @@ -91,10 +91,10 @@ fun PlaybackOverlay( currentPlayback: CurrentPlayback?, currentSegment: MediaSegmentDto?, analyticsState: AnalyticsState, + queue: List, modifier: Modifier = Modifier, trickplayInfo: TrickplayInfo? = null, trickplayUrlFor: (Int) -> String? = { null }, - playlist: Playlist = Playlist(listOf(), 0), onClickPlaylist: (BaseItem) -> Unit = {}, seekBarInteractionSource: MutableInteractionSource = remember { MutableInteractionSource() }, ) { @@ -180,10 +180,10 @@ fun PlaybackOverlay( } } val nextState = - remember(chapters, playlist) { + remember(chapters, nextEnabled) { if (chapters.isNotEmpty()) { OverlayViewState.CHAPTERS - } else if (playlist.hasNext()) { + } else if (nextEnabled) { OverlayViewState.QUEUE } else { null @@ -225,7 +225,7 @@ fun PlaybackOverlay( player = player, controllerViewState = controllerViewState, chapters = chapters, - playlist = playlist, + hasNext = nextEnabled, aspectRatio = item?.data?.aspectRatioFloat ?: AspectRatios.WIDE, onChangeState = onChangeState, modifier = @@ -237,9 +237,9 @@ fun PlaybackOverlay( } OverlayViewState.QUEUE -> { - if (playlist.hasNext()) { + if (nextEnabled) { QueueRowOverlay( - playlist = playlist, + queue = queue, controllerViewState = controllerViewState, nextState = remember(chapters) { diff --git a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/QueueRowOverlay.kt b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/QueueRowOverlay.kt index a44d1fc7..92b7afd1 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/QueueRowOverlay.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/overlay/QueueRowOverlay.kt @@ -22,7 +22,7 @@ import androidx.tv.material3.MaterialTheme import androidx.tv.material3.Text import com.github.damontecres.wholphin.R import com.github.damontecres.wholphin.data.model.BaseItem -import com.github.damontecres.wholphin.data.model.Playlist +import com.github.damontecres.wholphin.data.model.PlaylistItem import com.github.damontecres.wholphin.ui.cards.SeasonCard import com.github.damontecres.wholphin.ui.components.HiddenFocusBox import com.github.damontecres.wholphin.ui.ifElse @@ -31,14 +31,13 @@ import com.github.damontecres.wholphin.ui.tryRequestFocus @Composable fun QueueRowOverlay( - playlist: Playlist, + queue: List, controllerViewState: ControllerViewState, nextState: OverlayViewState, onChangeState: (OverlayViewState) -> Unit, onClickPlaylist: (BaseItem) -> Unit, modifier: Modifier = Modifier, ) { - val items = remember { playlist.upcomingItems() } val focusRequester = remember { FocusRequester() } val hiddenFocusRequester = remember { FocusRequester() } LaunchedEffect(Unit) { focusRequester.tryRequestFocus() } @@ -70,7 +69,7 @@ fun QueueRowOverlay( } }, ) { - itemsIndexed(items) { index, item -> + itemsIndexed(queue) { index, item -> val interactionSource = remember { MutableInteractionSource() } val isFocused =