kitty/spotube
1
0
Fork 0
mirror of https://github.com/KRTirtho/spotube.git synced 2026-11-04 10:13:59 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

fix(downloads): respect download format/quality and never save manifests or video

Browse Source 
- Downloads resolve streams with the user's download format/quality via a
  new StreamRequest instead of the streaming settings, and bypass the
  playback stream-URL cache in both directions
- Downloads only consider progressive streams; DASH/HLS manifests were
  previously saved as-is instead of audio
- NewPipe: map AAC "mp4a.40.x"/"m4a" to the advertised "aac"/"mp4" so real
  AAC streams can match (previously the hard-coded aac/mp4 manifests always
  won), report each stream's real delivery method, skip streams without a
  format or non-URL content
- Fail the download on non-2xx responses, on a range request answered with
  a full 200, and on video/* content, with localized reasons, instead of
  writing the response body to disk
- Save AAC as .m4a
- Tests for request overrides, manifest exclusion and cache isolation
This commit is contained in:
Kingkor Roy Tirtho 2026-10-20 16:03:48 +06:00
parent 8bbefdfb7d
commit 08892c2aa0
5 changed files with 182 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
composeApp/src/androidJvmMain/kotlin/org/spotube/spotube/core/newpipe/NewPipeService.androidJvm.kt
View File

@ -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
}

4
composeApp/src/commonMain/composeResources/values/strings.xml
View File

@ -710,6 +710,9 @@
<string name="downloads_error_no_metadata">No track metadata</string>
<string name="downloads_error_stream_unresolved_short">Could not resolve stream URL</string>
<string name="downloads_error_stream_unresolved">Could not resolve stream URL. Try playing the track first.</string>
<string name="downloads_error_http">The server refused the download (HTTP %1$d)</string>
<string name="downloads_error_range_ignored">The server sent an incomplete download. Try again.</string>
<string name="downloads_error_video_only">Only a video is available for this track, so it wasn\'t downloaded</string>
<!-- Saved tracks -->
<string name="saved_tracks_title">Saved Tracks</string>
@ -962,7 +965,6 @@
<string name="app_window_close">Close</string>
<!-- Android playback notification -->
<string name="notification_playback_service_text">Playback service</string>
<string name="notification_playback_channel_name">Playback</string>
<string name="notification_playback_channel_description">Media playback controls</string>

60
composeApp/src/commonMain/kotlin/org/spotube/spotube/core/server/StreamingUrlRepository.kt
View File

@ -54,6 +54,16 @@ private data class CachedAlternatives(
val pluginId: String,
val sources: List<AudioSource>)
/**
* 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

57
composeApp/src/commonMain/kotlin/org/spotube/spotube/modules/downloads/DownloadManager.kt
View File

@ -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)

64
composeApp/src/commonTest/kotlin/org/spotube/spotube/core/server/StreamingUrlRepositoryBehaviorTest.kt
View File

@ -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"))
}
}