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.