diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index cbc1188..87338b0 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,18 +1,13 @@ -## MusicResync v1.6.0 +## MusicResync v1.6.1 -### New lyrics providers -- **LyricsPlus**: word-by-word synced lyrics from the community LyricsPlus service (6 mirrors tried in order, remembering the last one that worked). -- **BetterLyrics**: Apple-style TTML lyrics, converted straight into word-by-word timing. -- Both slot into the normal provider list and support the multi-person word-by-word format. +### Way faster lyrics search (fixes #9) +Searching for lyrics could take **up to ~10 minutes** when a song was only available on a provider late in the chain (like QQ Music): every provider was tried one after another, each with its own retries and long network timeouts. -### Provider order, your way -- New **Provider order** section in Settings: drag providers into the order you want them tried, and untick any you don't want queried at all. Both the single-song search and the batch download now walk that exact chain, one by one, until they find a match — instead of always falling back to a fixed order. +- **All providers are now queried at the same time.** The total wait is roughly the time of the fastest provider that has your lyrics — not the sum of every slow one before it. +- **Per-provider time budget (25s).** A hung or unresponsive provider is skipped instead of stalling the whole search. +- **Snappier retries.** Failed requests still retry with exponential backoff, just with shorter delays (0.5s → 1s → 2s). +- **Tighter network timeouts** (12s per request, 6s connect) so a dead provider fails fast. -### Fixed: lyrics embedded in the file weren't recognized -- The home page's lyrics detector only checked for a sidecar `.lrc` file. Songs with lyrics embedded directly in their tags (via "Embed lyrics") were showing up as "missing lyrics" even though they weren't. The detector now reads embedded tags too, so those songs correctly show as having lyrics. - -### Batch download, cleaner -- The batch download setup is now a full page instead of a cramped popup, with a clear "Start" button. -- On the progress screen, the Synced / Unsynced / Not found / Failed counters are now tappable — each opens a drawer listing exactly which songs landed there. It opens at half the screen and slides up to full height. +Your provider order from Settings still decides which match wins when several providers find the song, and requests to each individual provider remain sequential and politely delayed — so rate limits are respected exactly as before. Both the single-song search and the batch download benefit. Install over your existing app — it's signed with the same key, so it upgrades in place. diff --git a/app/build.gradle.kts b/app/build.gradle.kts index e32a0ec..e5601a6 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -15,8 +15,8 @@ android { minSdk = 21 //noinspection OldTargetApi targetSdk = 35 - versionCode = 160 - versionName = "1.6.0" + versionCode = 161 + versionName = "1.6.1" vectorDrawables { useSupportLibrary = true diff --git a/app/src/main/java/pl/lambada/songsync/data/remote/lyrics_providers/SmartLyricsMatcher.kt b/app/src/main/java/pl/lambada/songsync/data/remote/lyrics_providers/SmartLyricsMatcher.kt index d4e762e..2178f4b 100644 --- a/app/src/main/java/pl/lambada/songsync/data/remote/lyrics_providers/SmartLyricsMatcher.kt +++ b/app/src/main/java/pl/lambada/songsync/data/remote/lyrics_providers/SmartLyricsMatcher.kt @@ -1,7 +1,10 @@ package pl.lambada.songsync.data.remote.lyrics_providers import android.util.Log +import kotlinx.coroutines.async +import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.delay +import kotlinx.coroutines.withTimeoutOrNull import pl.lambada.songsync.data.remote.lyrics_providers.others.BetterLyricsAPI import pl.lambada.songsync.data.remote.lyrics_providers.others.LRCLibAPI import pl.lambada.songsync.data.remote.lyrics_providers.others.LastResortAPI @@ -38,6 +41,12 @@ data class MatchConfig( val maxRetries: Int = 3, val requestDelayMs: Long = 200, val maxCandidatesPerProvider: Int = 4, + /** Delay before the first retry of a failed request; doubles per attempt (500ms -> 1s -> 2s). */ + val retryBaseDelayMs: Long = 500, + /** Hard wall-clock budget for ONE provider's whole candidate walk (search + retries). Issue #9: without + * this, a single unresponsive provider could burn retries × timeouts × candidates before the next provider + * even got a chance. */ + val providerTimeoutMs: Long = 25_000, ) /** @@ -66,6 +75,15 @@ class SmartLyricsMatcher( /** * @return all scored hits, highest confidence first (may be empty). The first element, if any, is the best * match; inspect [ScoredHit.tier] to decide auto-accept / verify / manual. + * + * All providers are queried CONCURRENTLY (issue #9): the old strictly sequential ladder could stack + * 7 providers × retries × 20s network timeouts into a multi-minute wait before ever reaching the provider + * that actually had the lyrics. Requests to any single provider remain sequential with a polite delay, so + * per-provider rate limiting is unchanged — only the waiting overlaps. Each provider is additionally + * hard-bounded by [MatchConfig.providerTimeoutMs], so one hung host can never stall the whole search. + * + * [onSkipped] fires for providers whose in-flight search was cancelled by the early auto-accept stop — + * they were never really tried, so callers should not paint them as "no lyrics". */ suspend fun search( local: LocalTrack, @@ -73,47 +91,89 @@ class SmartLyricsMatcher( config: MatchConfig = MatchConfig(), log: (String) -> Unit = { Log.i(TAG, it) }, onAttempt: (provider: Providers) -> Unit = {}, - ): List { + onSkipped: (provider: Providers) -> Unit = {}, + ): List = coroutineScope { val displayName = listOfNotNull(local.artist, local.title).joinToString(" - ").ifBlank { "" } - log("[match] \"$displayName\" dur=${local.durationSec?.let { "${it.toInt()}s" } ?: "?"} candidates=${candidates.size}") - - val hits = LinkedHashMap() // dedupe by provider+normalized result - var best = 0.0 + log("[match] \"$displayName\" dur=${local.durationSec?.let { "${it.toInt()}s" } ?: "?"} candidates=${candidates.size} providers=${config.providerOrder.size} (parallel)") - outer@ for (provider in config.providerOrder) { + val jobs = config.providerOrder.map { provider -> onAttempt(provider) - for (cand in candidates) { - val query = cand.asSearchString() - if (query.isBlank()) continue - - val found = when (provider) { - Providers.LRCLIB -> searchLrcLib(query, local, cand, config, log) - Providers.NETEASE -> searchNetease(query, local, cand, config, log) - Providers.LYRICSPLUS, Providers.BETTERLYRICS -> searchDirect(provider, local, cand, config, log) - else -> searchGeneric(provider, local, cand, config, log) - } - - for (hit in found) { - val key = "${hit.provider}|${hit.result.title}|${hit.result.artist}|${hit.result.durationSec}" - val existing = hits[key] - if (existing == null || hit.confidence.score > existing.confidence.score) hits[key] = hit - if (hit.confidence.score > best) { - best = hit.confidence.score - log(" [${provider.displayName}] ${cand.strategy.label} -> ${hit.result.title} - ${hit.result.artist} (${hit.confidence.percent()}%${if (hit.confidence.durationMatched) ", duration✓" else ""})") - } + async { + withTimeoutOrNull(config.providerTimeoutMs) { + searchProvider(provider, local, candidates, config, log) + } ?: emptyList().also { + log(" [${provider.displayName}] no answer within ${config.providerTimeoutMs / 1000}s — skipped") } + } + } - delay(config.requestDelayMs) - if (best >= ConfidenceScorer.AUTO_ACCEPT_THRESHOLD) { - log(" [auto-accept] reached ${(best * 100).toInt()}% — stopping early") - break@outer + // Collect in the user's configured priority order so the early-stop semantics match the old ladder: + // once the chain so far holds an auto-accept match, lower-priority providers still in flight are + // cancelled. Ones that already finished are merged anyway — free extra manual-search suggestions. + val hits = LinkedHashMap() // dedupe by provider+normalized result + var best = 0.0 + var stopped = false + for ((index, deferred) in jobs.withIndex()) { + val provider = config.providerOrder[index] + if (stopped && !deferred.isCompleted) { + deferred.cancel() + onSkipped(provider) + continue + } + for (hit in deferred.await()) { + val key = "${hit.provider}|${hit.result.title}|${hit.result.artist}|${hit.result.durationSec}" + val existing = hits[key] + if (existing == null || hit.confidence.score > existing.confidence.score) hits[key] = hit + if (hit.confidence.score > best) { + best = hit.confidence.score + log(" [${provider.displayName}] ${hit.strategy.label} -> ${hit.result.title} - ${hit.result.artist} (${hit.confidence.percent()}%${if (hit.confidence.durationMatched) ", duration✓" else ""})") } } + if (!stopped && best >= ConfidenceScorer.AUTO_ACCEPT_THRESHOLD) { + log(" [auto-accept] reached ${(best * 100).toInt()}% — cancelling providers still in flight") + stopped = true + } } val ranked = hits.values.sortedByDescending { it.confidence.score } if (ranked.isEmpty()) log(" [no match] nothing found across ${config.providerOrder.joinToString { it.displayName }}") - return ranked + ranked + } + + /** + * One provider's full candidate walk. Within a provider the requests stay sequential and politely delayed + * (rate-limit friendly); stops as soon as this provider produced an auto-accept-grade hit — weaker + * candidates can't beat it. + */ + private suspend fun searchProvider( + provider: Providers, + local: LocalTrack, + candidates: List, + config: MatchConfig, + log: (String) -> Unit, + ): List { + val providerHits = LinkedHashMap() + var providerBest = 0.0 + for (cand in candidates) { + val query = cand.asSearchString() + if (query.isBlank()) continue + + val found = when (provider) { + Providers.LRCLIB -> searchLrcLib(query, local, cand, config, log) + Providers.NETEASE -> searchNetease(query, local, cand, config, log) + Providers.LYRICSPLUS, Providers.BETTERLYRICS -> searchDirect(provider, local, cand, config, log) + else -> searchGeneric(provider, local, cand, config, log) + } + for (hit in found) { + val key = "${hit.provider}|${hit.result.title}|${hit.result.artist}|${hit.result.durationSec}" + val existing = providerHits[key] + if (existing == null || hit.confidence.score > existing.confidence.score) providerHits[key] = hit + if (hit.confidence.score > providerBest) providerBest = hit.confidence.score + } + if (providerBest >= ConfidenceScorer.AUTO_ACCEPT_THRESHOLD) break + delay(config.requestDelayMs) + } + return providerHits.values.toList() } /** A best-effort PLAIN (unsynced) lyrics hit, used only when no provider has synced lyrics. */ @@ -129,12 +189,14 @@ class SmartLyricsMatcher( candidates: List, config: MatchConfig = MatchConfig(), log: (String) -> Unit = { Log.i(TAG, it) }, - ): PlainHit? { + ): PlainHit? = withTimeoutOrNull(config.providerTimeoutMs) { + // Same hard time budget as a regular provider (issue #9): this fallback runs after a failed search, so + // letting its retries stack would just re-create the long wait we removed. for (cand in candidates) { val query = cand.asSearchString() if (query.isBlank()) continue val results = runCatching { - withRetry(config.maxRetries) { lrcLib.searchCandidates(query) } + withRetry(config.maxRetries, config.retryBaseDelayMs) { lrcLib.searchCandidates(query) } }.onFailure { log(" [LRCLib/plain] search failed: ${it.message}") }.getOrDefault(emptyList()) val best = results @@ -148,11 +210,11 @@ class SmartLyricsMatcher( if (best != null) { log(" [plain fallback] ${best.first.title} - ${best.first.artist} (${best.second.percent()}%)") - return PlainHit(Providers.LRCLIB, best.first, best.third) + return@withTimeoutOrNull PlainHit(Providers.LRCLIB, best.first, best.third) } delay(config.requestDelayMs) } - return null + null } /** A rescued result from the last-resort path: lyrics (synced or plain) under a canonical title/artist. */ @@ -187,32 +249,42 @@ class SmartLyricsMatcher( // hand them to the scorer-based validator. The old code accepted the closest-duration hit (or just the // first when duration was unknown) with no similarity check, so a wrong song by the same artist could be // rescued. selectBestRescue rescps each hit against the original local track via ConfidenceScorer. - val rescue = ArrayList() - for (query in queries) { - val metas = runCatching { lastResortApi.canonicalize(query) } - .onFailure { log(" [last-resort] canonicalize failed: ${it.message}") } - .getOrDefault(emptyList()) - for (meta in metas) { - val results = runCatching { lrcLib.searchCandidates("${meta.artist} ${meta.title}") } - .getOrDefault(emptyList()) - results.forEach { r -> - rescue.add( - RescueCandidate( - result = ProviderResult( - title = r.trackName, - artist = r.artistName, - durationSec = r.duration, - album = r.albumName, - hasSyncedLyrics = !r.syncedLyrics.isNullOrBlank(), - ), - syncedLyrics = r.syncedLyrics, - plainLyrics = r.plainLyrics, - coverUrl = meta.coverUrl, - ) - ) + // Each cleaned query is rescued CONCURRENTLY (the same issue-#9 treatment as the main search) and each + // gets the regular per-provider time budget; on timeout its partial haul is still scored. The rescue + // only runs after everything already failed, so it must never stretch the total wait by minutes again. + val rescue = coroutineScope { + queries.map { query -> + async { + val collected = ArrayList() + withTimeoutOrNull(config.providerTimeoutMs) { + val metas = runCatching { lastResortApi.canonicalize(query) } + .onFailure { log(" [last-resort] canonicalize failed: ${it.message}") } + .getOrDefault(emptyList()) + for (meta in metas) { + val results = runCatching { lrcLib.searchCandidates("${meta.artist} ${meta.title}") } + .getOrDefault(emptyList()) + results.forEach { r -> + collected.add( + RescueCandidate( + result = ProviderResult( + title = r.trackName, + artist = r.artistName, + durationSec = r.duration, + album = r.albumName, + hasSyncedLyrics = !r.syncedLyrics.isNullOrBlank(), + ), + syncedLyrics = r.syncedLyrics, + plainLyrics = r.plainLyrics, + coverUrl = meta.coverUrl, + ) + ) + } + delay(config.requestDelayMs) + } + } ?: log(" [last-resort] \"$query\" ran out of time — scoring its ${collected.size} partial candidates") + collected } - delay(config.requestDelayMs) - } + }.flatMap { it.await() } } val localViews = buildList { @@ -229,7 +301,7 @@ class SmartLyricsMatcher( suspend fun fetchLyrics(hit: ScoredHit, config: MatchConfig = MatchConfig(), log: (String) -> Unit = { Log.i(TAG, it) }): String? { return when (hit.provider) { Providers.NETEASE -> hit.neteaseId?.let { id -> - runCatching { withRetry(config.maxRetries) { netease.getSyncedLyrics(id) } } + runCatching { withRetry(config.maxRetries, config.retryBaseDelayMs) { netease.getSyncedLyrics(id) } } .onFailure { log(" [Netease] lyric fetch failed: ${it.message}") } .getOrNull() } @@ -251,7 +323,7 @@ class SmartLyricsMatcher( val durationSec = local.durationSec?.toInt()?.takeIf { it > 0 } val lyrics = runCatching { - withRetry(config.maxRetries, onRetry = { a, d, e -> + withRetry(config.maxRetries, config.retryBaseDelayMs, onRetry = { a, d, e -> log(" [${provider.displayName}] retry $a in ${d}ms (${e.message})") }) { when (provider) { @@ -291,7 +363,7 @@ class SmartLyricsMatcher( for (offset in 0 until config.maxCandidatesPerProvider) { val info = runCatching { - withRetry(config.maxRetries, onRetry = { a, d, e -> + withRetry(config.maxRetries, config.retryBaseDelayMs, onRetry = { a, d, e -> log(" [${provider.displayName}] retry $a in ${d}ms (${e.message})") }) { service.getSongInfo(SongInfo(cand.title, cand.artist), offset, provider) @@ -303,7 +375,7 @@ class SmartLyricsMatcher( val pr = ProviderResult(info.songName, info.artistName, null, null, true) val conf = scoreFor(local, pr, cand.strategy) val lyrics = if (conf.score >= ConfidenceScorer.REVIEW_THRESHOLD) { - runCatching { withRetry(config.maxRetries) { service.getSyncedLyrics(info, provider) } }.getOrNull() + runCatching { withRetry(config.maxRetries, config.retryBaseDelayMs) { service.getSyncedLyrics(info, provider) } }.getOrNull() } else null val hit = ScoredHit(provider, cand.strategy, pr, conf, lyrics, null) @@ -332,7 +404,7 @@ class SmartLyricsMatcher( query: String, local: LocalTrack, cand: QueryCandidate, config: MatchConfig, log: (String) -> Unit ): List { val results = runCatching { - withRetry(config.maxRetries, onRetry = { a, d, e -> log(" [LRCLib] retry $a in ${d}ms (${e.message})") }) { + withRetry(config.maxRetries, config.retryBaseDelayMs, onRetry = { a, d, e -> log(" [LRCLib] retry $a in ${d}ms (${e.message})") }) { lrcLib.searchCandidates(query) } }.onFailure { log(" [LRCLib] search failed: ${it.message}") }.getOrDefault(emptyList()) @@ -354,7 +426,7 @@ class SmartLyricsMatcher( query: String, local: LocalTrack, cand: QueryCandidate, config: MatchConfig, log: (String) -> Unit ): List { val songs = runCatching { - withRetry(config.maxRetries, onRetry = { a, d, e -> log(" [Netease] retry $a in ${d}ms (${e.message})") }) { + withRetry(config.maxRetries, config.retryBaseDelayMs, onRetry = { a, d, e -> log(" [Netease] retry $a in ${d}ms (${e.message})") }) { netease.searchCandidates(query, config.maxCandidatesPerProvider) } }.onFailure { log(" [Netease] search failed: ${it.message}") }.getOrDefault(emptyList()) diff --git a/app/src/main/java/pl/lambada/songsync/data/remote/lyrics_providers/others/LastResortAPI.kt b/app/src/main/java/pl/lambada/songsync/data/remote/lyrics_providers/others/LastResortAPI.kt index 164479d..85ec178 100644 --- a/app/src/main/java/pl/lambada/songsync/data/remote/lyrics_providers/others/LastResortAPI.kt +++ b/app/src/main/java/pl/lambada/songsync/data/remote/lyrics_providers/others/LastResortAPI.kt @@ -3,6 +3,8 @@ package pl.lambada.songsync.data.remote.lyrics_providers.others import io.ktor.client.request.get import io.ktor.client.statement.bodyAsText import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.async +import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.withContext import kotlinx.serialization.Serializable import pl.lambada.songsync.util.networking.Ktor.client @@ -78,15 +80,22 @@ class LastResortAPI { */ suspend fun canonicalize(query: String, limit: Int = 3): List { if (query.isBlank()) return emptyList() + // iTunes and Deezer are independent services — query both at once (issue #9: sequential waits stack, + // and one slow endpoint used to delay the other's answer for nothing). iTunes results still win ties. + val (itunesItems, deezerItems) = coroutineScope { + val i = async { itunes(query, limit) } + val d = async { deezer(query, limit) } + i.await() to d.await() + } val out = LinkedHashMap() - for (item in itunes(query, limit)) { + for (item in itunesItems) { val title = item.trackName?.takeIf { it.isNotBlank() } ?: continue val artist = item.artistName?.takeIf { it.isNotBlank() } ?: continue val cover = item.artworkUrl100?.takeIf { it.isNotBlank() }?.let(::upscaleITunes) out.putIfAbsent("${artist.lowercase()}|${title.lowercase()}", Meta(title, artist, cover)) } - for (item in deezer(query, limit)) { + for (item in deezerItems) { val title = item.title?.takeIf { it.isNotBlank() } ?: continue val artist = item.artist?.name?.takeIf { it.isNotBlank() } ?: continue val cover = item.album?.cover_xl?.takeIf { it.isNotBlank() } ?: item.album?.cover_big diff --git a/app/src/main/java/pl/lambada/songsync/ui/screens/lyricsFetch/LyricsFetchViewModel.kt b/app/src/main/java/pl/lambada/songsync/ui/screens/lyricsFetch/LyricsFetchViewModel.kt index e830096..efff1ef 100644 --- a/app/src/main/java/pl/lambada/songsync/ui/screens/lyricsFetch/LyricsFetchViewModel.kt +++ b/app/src/main/java/pl/lambada/songsync/ui/screens/lyricsFetch/LyricsFetchViewModel.kt @@ -61,7 +61,8 @@ class LyricsFetchViewModel( /** The provider that actually produced the shown lyrics (may differ from the chosen one after fallback). */ var activeProvider by mutableStateOf(userSettingsController.selectedProvider) - /** Live "which provider are we trying right now" status, shown while searching. */ + /** Live provider status shown while searching. Providers are now queried in parallel, so this stays null + * (the screen shows the generic "Searching…" label) rather than naming one provider misleadingly. */ var searchStatus by mutableStateOf(null) /** Per-provider probe result for the cloud-icon dropdown: tried→HAS_SYNCED/NONE, or LOADING during a retry. */ @@ -126,12 +127,14 @@ class LyricsFetchViewModel( val order = (listOf(userSettingsController.selectedProvider) + userSettingsController.enabledProviderOrder).distinct() - // Track which providers the search actually reached. The ladder can stop early (auto-accept), so - // the rest are genuinely UNTRIED — not failures — and must not be painted with a red X. + // Track which providers the search actually reached. All providers now run in parallel, but the + // auto-accept early stop can still cancel slower ones mid-flight — those are genuinely UNTRIED, + // not failures, and must not be painted with a red X. val attempted = linkedSetOf() val hits = matcher.search( local, candidates, MatchConfig(providerOrder = order), - onAttempt = { provider -> searchStatus = provider; attempted.add(provider); providerProbes[provider] = ProviderProbe.LOADING } + onAttempt = { provider -> attempted.add(provider); providerProbes[provider] = ProviderProbe.LOADING }, + onSkipped = { provider -> attempted.remove(provider) }, ) searchStatus = null diff --git a/app/src/main/java/pl/lambada/songsync/util/LyricsUtils.kt b/app/src/main/java/pl/lambada/songsync/util/LyricsUtils.kt index b1d5963..969930e 100644 --- a/app/src/main/java/pl/lambada/songsync/util/LyricsUtils.kt +++ b/app/src/main/java/pl/lambada/songsync/util/LyricsUtils.kt @@ -435,7 +435,9 @@ suspend fun matchAndSaveSong( // song's provider list can later show "found / no match / not tried" per provider. val attempted = LinkedHashSet() - val hits = runCatching { matcher.search(local, candidates, config, onAttempt = { attempted.add(it) }) } + val hits = runCatching { + matcher.search(local, candidates, config, onAttempt = { attempted.add(it) }, onSkipped = { attempted.remove(it) }) + } .getOrElse { Log.e("MusicResync", "match failed for ${song.filePath}: ${it.message}") return SongMatchInfo(LyricState.FAILED, failedProviders = attempted.toList()) diff --git a/app/src/main/java/pl/lambada/songsync/util/networking/Ktor.kt b/app/src/main/java/pl/lambada/songsync/util/networking/Ktor.kt index 867aa91..02464b6 100644 --- a/app/src/main/java/pl/lambada/songsync/util/networking/Ktor.kt +++ b/app/src/main/java/pl/lambada/songsync/util/networking/Ktor.kt @@ -9,14 +9,15 @@ import kotlinx.serialization.json.Json object Ktor { val client = HttpClient(CIO.create { - requestTimeout = 20_000 + requestTimeout = 12_000 }) { // Bound every request so a slow/down provider (e.g. Spotify) can't make the search hang forever -- it - // fails fast, the retry/backoff handles transient blips, and the matcher moves on to the next provider. + // fails fast, the retry/backoff handles transient blips, and the matcher moves on. A healthy lyrics + // provider answers well under these bounds; tighter limits are what keeps the worst case short (#9). install(HttpTimeout) { - connectTimeoutMillis = 10_000 - requestTimeoutMillis = 20_000 - socketTimeoutMillis = 20_000 + connectTimeoutMillis = 6_000 + requestTimeoutMillis = 12_000 + socketTimeoutMillis = 12_000 } install(ContentNegotiation) { json(Json {