diff --git a/composeApp/src/androidJvmMain/kotlin/org/spotube/spotube/core/newpipe/NewPipeService.androidJvm.kt b/composeApp/src/androidJvmMain/kotlin/org/spotube/spotube/core/newpipe/NewPipeService.androidJvm.kt index 7e4311dc..a79f5a03 100644 --- a/composeApp/src/androidJvmMain/kotlin/org/spotube/spotube/core/newpipe/NewPipeService.androidJvm.kt +++ b/composeApp/src/androidJvmMain/kotlin/org/spotube/spotube/core/newpipe/NewPipeService.androidJvm.kt @@ -26,6 +26,7 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import org.schabi.newpipe.extractor.ServiceList.YouTube import org.schabi.newpipe.extractor.services.youtube.linkHandler.YoutubeSearchQueryHandlerFactory +import org.schabi.newpipe.extractor.stream.DeliveryMethod import org.schabi.newpipe.extractor.stream.StreamInfoItem /** NewPipe reports durations in seconds (-1 when unknown). */ @@ -88,11 +89,15 @@ actual class NewPipeService { val audioStreams = extractor.audioStreams .mapNotNull { stream -> + val format = stream.format ?: return@mapNotNull null + // Non-URL content is an inline manifest, not something a player or downloader can fetch. + if (!stream.isUrl) return@mapNotNull null AudioStream.Lossy( url = stream.content, - codec = stream.codec, - container = stream.format!!.suffix, - bitrate = stream.bitrate) + codec = normalizeCodec(stream.codec), + container = normalizeContainer(format.suffix), + bitrate = stream.bitrate, + protocol = stream.deliveryMethod.toProtocol()) } + buildList { if (extractor.dashMpdUrl.isNotBlank()) add( AudioStream.Lossy( @@ -151,3 +156,22 @@ actual class NewPipeService { } } } + +/** + * NewPipe names AAC "mp4a.40.x" in an "m4a" container, but the plugin advertises (and users pick) + * "aac"/"mp4". Without this a real AAC stream never matched the preference, and the hard-coded + * aac/mp4 DASH/HLS manifests always won instead. + */ +private fun normalizeCodec(codec: String?): String { + val value = codec.orEmpty() + return if (value.startsWith("mp4a", ignoreCase = true)) "aac" else value +} + +private fun normalizeContainer(suffix: String): String = + if (suffix.equals("m4a", ignoreCase = true)) "mp4" else suffix + +private fun DeliveryMethod.toProtocol(): StreamProtocol = when (this) { + DeliveryMethod.DASH -> StreamProtocol.DASH + DeliveryMethod.HLS -> StreamProtocol.HLS + else -> StreamProtocol.PROGRESSIVE +} diff --git a/composeApp/src/commonMain/composeResources/values/strings.xml b/composeApp/src/commonMain/composeResources/values/strings.xml index 9cfcb895..a5481e2e 100644 --- a/composeApp/src/commonMain/composeResources/values/strings.xml +++ b/composeApp/src/commonMain/composeResources/values/strings.xml @@ -710,6 +710,9 @@ No track metadata Could not resolve stream URL Could not resolve stream URL. Try playing the track first. + The server refused the download (HTTP %1$d) + The server sent an incomplete download. Try again. + Only a video is available for this track, so it wasn\'t downloaded Saved Tracks @@ -962,7 +965,6 @@ Close - Playback service Playback Media playback controls diff --git a/composeApp/src/commonMain/kotlin/org/spotube/spotube/core/server/StreamingUrlRepository.kt b/composeApp/src/commonMain/kotlin/org/spotube/spotube/core/server/StreamingUrlRepository.kt index 3a20b697..3ee50fa0 100644 --- a/composeApp/src/commonMain/kotlin/org/spotube/spotube/core/server/StreamingUrlRepository.kt +++ b/composeApp/src/commonMain/kotlin/org/spotube/spotube/core/server/StreamingUrlRepository.kt @@ -54,6 +54,16 @@ private data class CachedAlternatives( val pluginId: String, val sources: List) +/** + * Stream preferences for callers other than playback. The downloader passes the user's *download* + * format/quality and needs a file it can fetch with plain HTTP, so DASH/HLS manifests are excluded. + * Such requests bypass the playback stream-URL cache in both directions. + */ +data class StreamRequest( + val format: AudioFormat, + val quality: AudioQuality, + val progressiveOnly: Boolean) + data class StreamInfo( val url: String, val protocol: StreamProtocol, @@ -90,26 +100,44 @@ class StreamingUrlRepository( suspend fun resolveStreamInfo( track: MetadataTrack, - forceRefresh: Boolean = false + forceRefresh: Boolean = false, + request: StreamRequest? = null ): StreamInfo? { val trackId = track.id val settings = settingsRepository.userSettings.value - val preferredFormat = settings.streamingMusicFormat - val preferredQuality = settings.streamingMusicQuality + val preferredFormat = request?.format ?: settings.streamingMusicFormat + val preferredQuality = request?.quality ?: settings.streamingMusicQuality + // The URL cache holds what *playback* resolved; a download with other prefs must not + // read it, nor replace it with a stream playback didn't ask for. + val useUrlCache = request == null val audioPlugin = pluginManager.selectedAudioPlugin.value if (audioPlugin == null) { logger.w { "No audio plugin selected while resolving stream for track $trackId" } return null } - // NOTE: always the *streaming* preferences, also when called by the downloader. logger.i { "[Resolve $trackId] '${track.title}' plugin=${audioPlugin.pluginId} forceRefresh=$forceRefresh " + "prefs: format=${preferredFormat.codec}/${preferredFormat.container} quality=$preferredQuality " + - "(streamingMusicFormat/streamingMusicQuality)" + if (request == null) "(streaming settings)" else "(caller request, progressiveOnly=${request.progressiveOnly})" } - if (!forceRefresh) { + fun select(stream: AudioSource.Streamed, origin: String): AudioStream? { + val candidates = if (request?.progressiveOnly == true) { + stream.streams.filter { it.protocol == StreamProtocol.PROGRESSIVE }.also { kept -> + if (kept.size != stream.streams.size) logger.i { + "[Resolve $trackId] dropped ${stream.streams.size - kept.size} manifest (DASH/HLS) candidates; " + + "caller needs a directly downloadable file" + } + } + } else { + stream.streams + } + return selectPreferredAudioStream(candidates, preferredFormat, preferredQuality) + .also { logSelection(trackId, origin, candidates, preferredFormat, preferredQuality, it) } + } + + if (useUrlCache && !forceRefresh) { getCachedStreamUrl(trackId, preferredFormat, preferredQuality, audioPlugin.pluginId)?.let { cached -> logger.i { "[Resolve $trackId] cache hit: codec=${cached.codec} container=${cached.container} " + @@ -135,14 +163,12 @@ class StreamingUrlRepository( if (stream != null) { logger.d { "Resolved stream for track $trackId using cached source" } - val resolvedStream = selectPreferredAudioStream( - stream.streams, - preferredFormat, - preferredQuality) - logSelection(trackId, "matched source ${source.id}", stream.streams, preferredFormat, preferredQuality, resolvedStream) + val resolvedStream = select(stream, "matched source ${source.id}") if (resolvedStream != null) { val info = resolvedStream.toStreamInfo() - cacheStreamInfo(trackId, info, preferredFormat, preferredQuality, stream, resolvedStream, audioPlugin.pluginId) + if (useUrlCache) { + cacheStreamInfo(trackId, info, preferredFormat, preferredQuality, stream, resolvedStream, audioPlugin.pluginId) + } return info } } @@ -195,14 +221,12 @@ class StreamingUrlRepository( } logger.d { "Resolved stream for track $trackId via track lookup" } - val resolvedStream = selectPreferredAudioStream( - stream.streams, - preferredFormat, - preferredQuality) - logSelection(trackId, "track lookup -> ${stream.id}", stream.streams, preferredFormat, preferredQuality, resolvedStream) + val resolvedStream = select(stream, "track lookup -> ${stream.id}") if (resolvedStream != null) { val info = resolvedStream.toStreamInfo() - cacheStreamInfo(trackId, info, preferredFormat, preferredQuality, stream, resolvedStream, audioPlugin.pluginId) + if (useUrlCache) { + cacheStreamInfo(trackId, info, preferredFormat, preferredQuality, stream, resolvedStream, audioPlugin.pluginId) + } return info } return null diff --git a/composeApp/src/commonMain/kotlin/org/spotube/spotube/modules/downloads/DownloadManager.kt b/composeApp/src/commonMain/kotlin/org/spotube/spotube/modules/downloads/DownloadManager.kt index d2f91aae..9b1824a0 100644 --- a/composeApp/src/commonMain/kotlin/org/spotube/spotube/modules/downloads/DownloadManager.kt +++ b/composeApp/src/commonMain/kotlin/org/spotube/spotube/modules/downloads/DownloadManager.kt @@ -21,6 +21,7 @@ import co.touchlab.kermit.Logger import org.spotube.plugin_interfaces.plugin_apis.metadata.track.MetadataTrack import org.spotube.spotube.core.paths.Paths import org.spotube.spotube.core.server.StreamInfo +import org.spotube.spotube.core.server.StreamRequest import org.spotube.spotube.core.server.StreamingUrlRepository import org.spotube.spotube.core.server.urlSummary import org.spotube.plugin_interfaces.plugin_apis.audio.StreamProtocol @@ -32,6 +33,7 @@ import io.ktor.client.request.head import io.ktor.client.request.headers import io.ktor.client.statement.bodyAsChannel import io.ktor.http.HttpHeaders +import io.ktor.http.HttpStatusCode import io.ktor.http.contentLength import io.ktor.http.isSuccess import io.ktor.utils.io.readRemaining @@ -226,7 +228,14 @@ class DownloadManager( try { logger.i { "[Download ${item.id}] resolving stream for '${track.title}'" } - val streamInfo = streamingUrlRepository.resolveStreamInfo(track) + val settings = settingsRepository.userSettings.value + val streamInfo = streamingUrlRepository.resolveStreamInfo( + track, + request = StreamRequest( + format = settings.downloadMusicFormat, + quality = settings.downloadMusicQuality, + // A manifest URL would be saved as-is instead of the audio it points to. + progressiveOnly = true)) if (streamInfo == null) { logger.w { "Could not resolve stream URL for ${track.title}" } val unresolvedStatus = getString(Res.string.downloads_error_stream_unresolved_short) @@ -358,9 +367,15 @@ class DownloadManager( "HTTP ${response.status.value} type=${response.headers[HttpHeaders.ContentType]} " + "range=${response.headers[HttpHeaders.ContentRange]}" } - // expectSuccess = false, so an error page would otherwise be saved as audio. + // expectSuccess = false, so without these checks an error page, or the whole file once per + // segment (a 200 to a range request), would be written into the "audio" file. if (!response.status.isSuccess()) { - logger.w { "[Download ${item.id}] segment ${segment.index} got HTTP ${response.status}; its body will be written as audio" } + logger.w { "[Download ${item.id}] segment ${segment.index} got HTTP ${response.status}; failing download" } + throw DownloadException(getString(Res.string.downloads_error_http, response.status.value)) + } + if (response.status != HttpStatusCode.PartialContent) { + logger.w { "[Download ${item.id}] segment ${segment.index} got ${response.status} instead of 206; range ignored" } + throw DownloadException(getString(Res.string.downloads_error_range_ignored)) } val channel = response.bodyAsChannel() @@ -389,8 +404,10 @@ class DownloadManager( "type=${response.headers[HttpHeaders.ContentType]} length=$totalSize" } if (!response.status.isSuccess()) { - logger.w { "[Download ${item.id}] GET got HTTP ${response.status}; its body will be written as audio" } + logger.w { "[Download ${item.id}] GET got HTTP ${response.status}; failing download" } + throw DownloadException(getString(Res.string.downloads_error_http, response.status.value)) } + rejectVideo(item.id, response.headers[HttpHeaders.ContentType]) val channel = response.bodyAsChannel() fileSystem.sink(tempFile).buffer().use { sink -> @@ -433,16 +450,27 @@ class DownloadManager( "[Download $id] HEAD -> HTTP ${response.status.value} type=$contentType " + "length=$contentLength acceptRanges=$acceptRanges" } - if (contentType?.startsWith("video/") == true) { - logger.w { "[Download $id] server says this is VIDEO ($contentType), not audio" } - } + rejectVideo(id, contentType) Pair(contentLength, supportsRanges) + } catch (e: DownloadException) { + throw e } catch (e: Exception) { logger.w(e) { "[Download $id] HEAD request failed, falling back to single connection" } Pair(0L, false) } } + /** + * Some sources only offer muxed video (e.g. NewPipe falls back to video streams when a video + * has no audio-only ones). Saving that as a "song" is worse than failing with a clear reason. + */ + private suspend fun rejectVideo(id: String, contentType: String?) { + if (contentType?.startsWith("video/", ignoreCase = true) == true) { + logger.w { "[Download $id] server says this is VIDEO ($contentType), not audio; failing download" } + throw DownloadException(getString(Res.string.downloads_error_video_only)) + } + } + private fun logResolvedStream(item: DownloadItem, info: StreamInfo) { val settings = settingsRepository.userSettings.value val wanted = settings.downloadMusicFormat @@ -457,9 +485,8 @@ class DownloadManager( } if (!info.codec.equals(wanted.codec, ignoreCase = true) || !info.container.equals(wanted.container, ignoreCase = true)) { logger.w { - "[Download ${item.id}] DOWNLOAD FORMAT NOT RESPECTED: download setting is ${wanted.codec}/${wanted.container} " + - "${settings.downloadMusicQuality}, got ${info.codec}/${info.container} " + - "(resolver used streaming prefs ${settings.streamingMusicFormat.codec}/${settings.streamingMusicFormat.container})" + "[Download ${item.id}] FORMAT FALLBACK: download setting is ${wanted.codec}/${wanted.container} " + + "${settings.downloadMusicQuality}, but the closest downloadable stream offered is ${info.codec}/${info.container}" } } } @@ -525,13 +552,21 @@ class DownloadManager( private fun buildFilename(downloadItem: DownloadItem, streamInfo: StreamInfo): String { val safeTitle = downloadItem.title.sanitizeFilename() val safeArtist = downloadItem.artists.sanitizeFilename() - val extension = streamInfo.container + val extension = fileExtension(streamInfo) return "$safeArtist - $safeTitle.$extension" } + /** AAC in an MP4 container is audio-only, so give it the audio extension players expect. */ + private fun fileExtension(info: StreamInfo): String = + if (info.codec.equals("aac", ignoreCase = true) && info.container.equals("mp4", ignoreCase = true)) "m4a" + else info.container + private fun String.sanitizeFilename(): String { return this.replace(Regex("[^a-zA-Z0-9.\\-_' ]"), "_").take(200) } private data class Segment(val index: Int, val start: Long, val end: Long) } + +/** A download failure whose [message] is already localized and safe to show the user. */ +private class DownloadException(message: String) : Exception(message) diff --git a/composeApp/src/commonTest/kotlin/org/spotube/spotube/core/server/StreamingUrlRepositoryBehaviorTest.kt b/composeApp/src/commonTest/kotlin/org/spotube/spotube/core/server/StreamingUrlRepositoryBehaviorTest.kt index b956d423..d7bda94a 100644 --- a/composeApp/src/commonTest/kotlin/org/spotube/spotube/core/server/StreamingUrlRepositoryBehaviorTest.kt +++ b/composeApp/src/commonTest/kotlin/org/spotube/spotube/core/server/StreamingUrlRepositoryBehaviorTest.kt @@ -545,4 +545,68 @@ class StreamingUrlRepositoryBehaviorTest { // Fetching the entry doesn't call the plugin again. assertEquals(1, plugin.audioAPI.getStreamsByTrackCallCount) } + + // ---------- caller requests (downloads) ---------- + + private val aacRequest = StreamRequest( + format = org.spotube.plugin_interfaces.plugin_apis.audio.AudioFormat( + codec = "aac", container = "mp4", qualities = listOf(AudioQuality.Lossy(128_000))), + quality = AudioQuality.Lossy(128_000), + progressiveOnly = true) + + private fun aacStream(url: String, protocol: StreamProtocol = StreamProtocol.PROGRESSIVE) = AudioStream.Lossy( + url = url, codec = "aac", container = "mp4", protocol = protocol, bitrate = 128_000) + + @Test + fun `a request's format overrides the streaming settings`() = runTest { + val h = Harness() + val plugin = h.selectPlugin("pluginA") + plugin.audioAPI.onGetStreamsByTrack = { + listOf(streamedSource("src1", listOf(stream("https://example.com/a.opus"), aacStream("https://example.com/a.m4a")))) + } + + val info = h.repository.resolveStreamInfo(track("t1"), request = aacRequest) + + assertEquals("https://example.com/a.m4a", info?.url) + } + + @Test + fun `progressiveOnly never picks a manifest even when it matches best`() = runTest { + val h = Harness() + val plugin = h.selectPlugin("pluginA") + plugin.audioAPI.onGetStreamsByTrack = { + listOf(streamedSource("src1", listOf( + aacStream("https://example.com/manifest.mpd", StreamProtocol.DASH), + stream("https://example.com/a.opus")))) + } + + val info = h.repository.resolveStreamInfo(track("t1"), request = aacRequest) + + assertEquals("https://example.com/a.opus", info?.url) + assertEquals(StreamProtocol.PROGRESSIVE, info?.protocol) + } + + @Test + fun `progressiveOnly returns null when only manifests exist`() = runTest { + val h = Harness() + val plugin = h.selectPlugin("pluginA") + plugin.audioAPI.onGetStreamsByTrack = { + listOf(streamedSource("src1", listOf(aacStream("https://example.com/manifest.m3u8", StreamProtocol.HLS)))) + } + + assertNull(h.repository.resolveStreamInfo(track("t1"), request = aacRequest)) + } + + @Test + fun `a request does not write the playback url cache`() = runTest { + val h = Harness() + val plugin = h.selectPlugin("pluginA") + plugin.audioAPI.onGetStreamsByTrack = { + listOf(streamedSource("src1", listOf(aacStream("https://example.com/a.m4a")))) + } + + h.repository.resolveStreamInfo(track("t1"), request = aacRequest) + + assertNull(h.repository.getCachedStreamUrlEntry("t1")) + } }