From a356226e48790ea96c9fab271f9b2e03f7815273 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Fri, 25 Sep 2026 13:27:50 +0000 Subject: [PATCH] address the review: keep the stored seed on a failed decode, one cache, static bar colour - resolve() no longer discards a stored seed when the bitmap cannot be decoded (it used to return null, dropping the theme for pre-v12 rows); it falls back to the stored pair - fold the busy flag into the seed cache value so eviction is atomic (a busy entry could be evicted from the separate flag LRU while its seed stayed, silently disabling the scrim) - the artist top bar samples heroBarColor at scroll 0 to match the static HeroGradient, and drops the wrong scroll estimate and per-pixel recomposition - rememberOverlaidNameBusy keys its state so the cached flag is read on key change, not only the first composition; bottomBandBusy bails out on an empty bitmap --- .../ui/library/ArtistDetailScreen.kt | 9 +--- .../subtracks/ui/theme/ArtworkSeedCache.kt | 52 +++++++++++-------- .../ui/theme/ArtworkExtractionTest.kt | 20 +++++++ docs/architecture.md | 4 +- docs/implementation-plan.md | 14 +++++ 5 files changed, 68 insertions(+), 31 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt index 8ca33e40..41d42124 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt @@ -151,14 +151,7 @@ fun ArtistDetailScreen( BoxWithConstraints(modifier.fillMaxSize().background(Color.Black)) { val screenHeightPx = with(density) { maxHeight.toPx() } val barHeightPx = with(density) { barHeight.toPx() } - val scrollPx by remember { - derivedStateOf { - val index = listState.firstVisibleItemIndex - val before = if (index <= 0) 0f else with(density) { ART_HEIGHT.toPx() } * index - before + listState.firstVisibleItemScrollOffset - } - } - val barColor = heroBarColor(artwork, scrollPx, barHeightPx, screenHeightPx) + val barColor = heroBarColor(artwork, 0f, barHeightPx, screenHeightPx) HeroGradient( colors = artwork, scrollPx = { 0f }, diff --git a/app/src/main/kotlin/com/subtracks/ui/theme/ArtworkSeedCache.kt b/app/src/main/kotlin/com/subtracks/ui/theme/ArtworkSeedCache.kt index d7d8b930..8d998660 100644 --- a/app/src/main/kotlin/com/subtracks/ui/theme/ArtworkSeedCache.kt +++ b/app/src/main/kotlin/com/subtracks/ui/theme/ArtworkSeedCache.kt @@ -6,7 +6,10 @@ import androidx.annotation.VisibleForTesting import androidx.collection.LruCache import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect -import androidx.compose.runtime.produceState +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.luminance import androidx.palette.graphics.Palette @@ -39,28 +42,33 @@ private const val BUSY_BRIGHT_FRACTION = 0.05f private const val BUSY_EDGE_LUMINANCE = 0.2f private const val BUSY_EDGE_FRACTION = 0.1f +private data class Seed( + val primary: Int, + val secondary: Int?, + val busy: Boolean?, +) + object ArtworkSeedCache { private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO) private val jobs = ConcurrentHashMap?>>() - private val cache = LruCache>(MAX_CACHED_SEEDS) - private val busy = LruCache(MAX_CACHED_SEEDS) + private val cache = LruCache(MAX_CACHED_SEEDS) @Volatile private var store: ArtworkSeedStore? = null - fun install(store: ArtworkSeedStore) { + fun install(store: ArtworkSeedStore?) { this.store = store } - fun cached(cacheKey: String): Pair? = cache.get(cacheKey) + fun cached(cacheKey: String): Pair? = cache.get(cacheKey)?.let { it.primary to it.secondary } - fun cachedBusy(cacheKey: String): Boolean? = busy.get(cacheKey) + fun cachedBusy(cacheKey: String): Boolean? = cache.get(cacheKey)?.busy suspend fun seeds( context: Context, ref: CoverArtRef, ): Pair? { - cache.get(ref.cacheKey)?.let { return it } + cache.get(ref.cacheKey)?.let { return it.primary to it.secondary } val job = jobs.computeIfAbsent(ref.cacheKey) { scope.async { resolve(context, ref) } } job.invokeOnCompletion { jobs.remove(ref.cacheKey, job) } return job.await() @@ -77,32 +85,31 @@ object ArtworkSeedCache { context: Context, ref: CoverArtRef, ): Boolean { - busy.get(ref.cacheKey)?.let { return it } + cache.get(ref.cacheKey)?.busy?.let { return it } seeds(context, ref) - return busy.get(ref.cacheKey) ?: false + return cache.get(ref.cacheKey)?.busy ?: false } @VisibleForTesting fun clear() { jobs.clear() cache.evictAll() - busy.evictAll() } private suspend fun resolve( context: Context, ref: CoverArtRef, ): Pair? { - ignoreFailure { store?.seed(ref.cacheKey) }?.let { stored -> - cache.put(ref.cacheKey, stored.primary to stored.secondary) - stored.nameBusy?.let { busy.put(ref.cacheKey, it) } - if (stored.nameBusy != null) return stored.primary to stored.secondary + val stored = ignoreFailure { store?.seed(ref.cacheKey) } + val storedSeeds = stored?.let { it.primary to it.secondary } + if (stored != null) { + cache.put(ref.cacheKey, Seed(stored.primary, stored.secondary, stored.nameBusy)) + if (stored.nameBusy != null) return storedSeeds } - val bitmap = decode(context, ref) ?: return null - val seeds = seedsFrom(bitmap) ?: return null + val bitmap = decode(context, ref) ?: return storedSeeds + val seeds = seedsFrom(bitmap) ?: return storedSeeds val nameBusy = bottomBandBusy(bitmap) - cache.put(ref.cacheKey, seeds) - busy.put(ref.cacheKey, nameBusy) + cache.put(ref.cacheKey, Seed(seeds.first, seeds.second, nameBusy)) ignoreFailure { store?.save(ArtworkSeed(ref.cacheKey, seeds.first, seeds.second, nameBusy)) } return seeds } @@ -139,6 +146,7 @@ object ArtworkSeedCache { @VisibleForTesting internal fun bottomBandBusy(bitmap: Bitmap): Boolean { + if (bitmap.width == 0 || bitmap.height == 0) return false val firstRow = (bitmap.height * BUSY_BAND_TOP).toInt().coerceIn(0, bitmap.height - 1) val width = bitmap.width var previousRow: FloatArray? = null @@ -220,7 +228,9 @@ fun PrefetchArtworkSeeds(ref: CoverArtRef?) { fun rememberOverlaidNameBusy(ref: CoverArtRef?): Boolean { val context = LocalPlatformContext.current val key = ref?.cacheKey - return produceState(key?.let(ArtworkSeedCache::cachedBusy) ?: false, key) { - value = ref?.let { ArtworkSeedCache.overlaidNameBusy(context, it) } ?: false - }.value + var busy by remember(key) { mutableStateOf(key?.let(ArtworkSeedCache::cachedBusy) ?: false) } + LaunchedEffect(key) { + busy = ref?.let { ArtworkSeedCache.overlaidNameBusy(context, it) } ?: false + } + return busy } diff --git a/app/src/test/kotlin/com/subtracks/ui/theme/ArtworkExtractionTest.kt b/app/src/test/kotlin/com/subtracks/ui/theme/ArtworkExtractionTest.kt index 068192f6..a4e02c27 100644 --- a/app/src/test/kotlin/com/subtracks/ui/theme/ArtworkExtractionTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/theme/ArtworkExtractionTest.kt @@ -17,7 +17,10 @@ import coil3.SingletonImageLoader import coil3.annotation.DelicateCoilApi import coil3.asImage import coil3.test.FakeImageLoaderEngine +import com.subtracks.data.model.ArtworkSeed import com.subtracks.data.model.CoverArtRef +import com.subtracks.data.repo.ArtworkSeedStore +import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull @@ -55,6 +58,7 @@ class ArtworkExtractionTest { @After fun resetImageLoader() { ArtworkSeedCache.clear() + ArtworkSeedCache.install(null) SingletonImageLoader.reset() } @@ -87,4 +91,20 @@ class ArtworkExtractionTest { composeRule.waitForIdle() assertEquals(42, state.value) } + + @Test + fun keepsTheStoredSeedWhenTheArtCannotBeDecoded() { + val key = "missing-${System.nanoTime()}" + val stored = ArtworkSeed(key, 0xFF3A7BD5.toInt(), 0xFFD53A3A.toInt(), null) + ArtworkSeedCache.install( + object : ArtworkSeedStore { + override suspend fun seed(cacheKey: String): ArtworkSeed? = stored.takeIf { it.cacheKey == cacheKey } + + override suspend fun save(seed: ArtworkSeed) = Unit + }, + ) + val ref = CoverArtRef(url = key, cacheKey = key) + val seeds = runBlocking { ArtworkSeedCache.seeds(ApplicationProvider.getApplicationContext(), ref) } + assertEquals(stored.primary, seeds?.first) + } } diff --git a/docs/architecture.md b/docs/architecture.md index 4cb34959..e25c76d7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -82,7 +82,7 @@ Audio plays through an `ExoPlayer` owned by `PlaybackService` (a `MediaSessionSe The queue lives in SQL, not memory. `queue_entries` is an ordered list of *references* — a whole playlist, an album, or a single song — each with an optional inclusive ordinal range (`rangeStart`/`rangeEnd`; both null means the whole referenced list). A queue can therefore be "album X tracks 1-5, song Y, then album X tracks 6-10" without copying a row per track, and inserting, moving or removing an entry is a single row. A one-row `playback_cursor` table holds the current position; both are written as playback advances. -`QueueRepository` resolves a queue position to a song lazily: it walks the entries summing their lengths (a `COUNT(*)` per entry) and then fetches one row with `LIMIT 1 OFFSET`. `PlaybackController` keeps only the resolved entry lengths and the cursor in memory — never the songs — and hands the player a bounded window (currently `2 * 25 + 1` items) centred on the cursor. A player index maps back to a queue position as `windowStart + index`. The window is loaded once with `setMediaItems` and then shifted incrementally with `addMediaItems`/`removeMediaItem` as playback advances; a shift never replaces the playlist, so the current item and its prepared next source survive and transitions stay gapless. `hasNext`/`hasPrevious` come from the SQL queue size, not the player's playlist. The snapshot is re-read whenever the window shifts and on skip, so a sync that shrinks the referenced album or playlist cannot strand the window on stale lengths; if the active source changes under a loaded queue, playback stops rather than resolving the old queue's stream URLs against the new server. +`QueueRepository` resolves a queue position to a song lazily: it walks the entries summing their lengths (a `COUNT(*)` per entry) and then fetches one row with `LIMIT 1 OFFSET`. `PlaybackController` keeps only the resolved entry lengths and the cursor in memory — never the songs — and hands the player a bounded window (currently `2 * 25 + 1` items) centred on the cursor. A player index maps back to a queue position as `windowStart + index`. The window is loaded once with `setMediaItems` and then shifted incrementally with `addMediaItems`/`removeMediaItem` as playback advances; a shift never replaces the playlist, so the current item and its prepared next source survive and transitions stay gapless. `hasNext`/`hasPrevious` are intentionally always true rather than derived from the SQL queue size: the previous/next buttons stay usable at both ends of the queue so repeat mode and restart-the-track keep working, and `next()`/`previous()` decide the actual end-of-queue action (advance, wrap when repeating, restart, or no-op). The snapshot is re-read whenever the window shifts and on skip, so a sync that shrinks the referenced album or playlist cannot strand the window on stale lengths; if the active source changes under a loaded queue, playback stops rather than resolving the old queue's stream URLs against the new server. Because the resolver reports *which entry* a position falls in, `PlaybackState` also carries the current queue context (`kind` and `refId`). That is what scopes the playing-track indicator in the album and playlist lists: a song that appears in both only lights up in the list the queue was actually started from. @@ -116,7 +116,7 @@ Sync is not atomic. Each batch commits on its own, so a failure leaves the batch ## Search -Search is a case-insensitive substring filter on the library queries (`instr(lower(column), lower(:search)) > 0` in `LibraryDao`), scoped to the active tab and with the matching rows ordered by the tab's sort. There is no separate search index; the filter runs directly against the library tables. +Search is a case-insensitive substring filter on the library queries (`instr(lower(column), lower(:search)) > 0` in `LibraryDao`), scoped to the active tab and with the matching rows ordered by the tab's sort. There is no separate search index; the filter runs directly against the library tables. An FTS5 trigram shadow table was trialled and dropped: it was wired to a separate search screen that went unused, and it indexed only `title`, so it did not cover the per-field substring the inline filter does. The decision is reversible if infix search over a very large library ever needs an index, because `tokenize='trigram'` is the only SQLite built-in that can index an arbitrary substring match. A revival must index every field the filter scans (`title`/`artist`/`album` for songs, `name`/`albumArtist` for albums, `name` for artists and playlists), be kept current incrementally per sync batch rather than cleared and rebuilt, and accept a three-character minimum (shorter queries fall back to the scan); sorting and paging would still come from the normal indexed library queries. ## Networking and auth diff --git a/docs/implementation-plan.md b/docs/implementation-plan.md index 14cdcd77..f7c34da8 100644 --- a/docs/implementation-plan.md +++ b/docs/implementation-plan.md @@ -52,6 +52,20 @@ Not done yet in this slice (deliberately): cover-art tonal colour extraction, lo - Signing, versioning and Fastlane metadata. - F-Droid submission. Their buildserver must have Gradle >= 9.6 and SDK 37, which this project targets; do not rely on the Gradle wrapper there (fdroidserver deletes `gradlew` and uses its own Gradle). +## Scale hardening backlog + +Follow-up work for very large libraries, playlists and long sessions, grouped so each block can be one PR. + +- **Library read path.** Index the normalized sort keys (a denormalized `albumArtist` on `songs` plus `NOCASE` indices on `albums`/`artists`/`playlists`) and replace `LIMIT 1 OFFSET` with keyset/cursor seeks in `QueueRepository`/`QueueDao`; compute the tapped-song ordinal with a SQL rank instead of materializing the whole sorted id list. Shared dependency for the read-side fixes; can split into "indices" then "keyset" if too large for one review. +- **Shuffle.** Drop `shuffle_order` and the retained whole-order/whole-id arrays; derive the permutation from `(seed, size)` and resolve positions through the same positional lookup. Recommended after the read-path PR, since it shares `QueueRepository`. +- **Sync pipeline.** Prune with a memory-bounded id set, remove `fetchRanks` and the write-only rank columns, fetch playlists/albums with bounded concurrency and skip unchanged ones, and raise/make configurable the page cap with a clear "library too large" failure. Independent of the read path. +- **Queue view and edits.** Bound the queue view's loaded rows (or page over the queue ordinal) and scope its ViewModel to the overlay; compact adjacent same-ref ranges after an edit and cache resolved entry lengths. The view half is independent; the edit half shares `QueueRepository` with the read-path PR. +- **Artwork.** Give `artwork_seeds` a `sourceId` with `ON DELETE CASCADE` and an LRU/TTL (or drop the table and rely on Coil's disk cache); prefetch the thumbnail, not the original, on now-playing transitions. Independent. +- **FTS5 search.** Deferred; the revival requirements are recorded in `architecture.md` (per-entity trigram tables indexing every scanned field, incremental maintenance, three-character floor). +- **UI/flow overhead.** Subscribe only the active tab's paging flow; move the position ticker off the shared `PlaybackState`. Independent. + +A `syncGen` column was considered for the sync prune and rejected: it is part of the upsert's update set, so its always-changing value defeats `upsertChanged`'s changed-only `WHERE` predicate and rewrites every row on every full sync. The prune should instead keep the two-phase diff with a memory-bounded id set (primitive long hashes) or diff a staging `seen` table in SQL. + ## Deferred decisions - Whether to drop the plaintext-auth fallback or keep it behind an "insecure server" toggle.