From a0647deaaecddb9b0038f3627af4d30db124c67e Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sat, 26 Sep 2026 19:08:10 +0000 Subject: [PATCH] address playback review findings - treat flac/wav/alac as lossless and ignore the bitrate for them, so the declared length never drops below the real encoded size and truncates a lossless transcode - read the effective stream quality once per media item and use it for both the URL and the clip, closing the window where they disagreed - honour a bounded DataSpec length in KnownLengthDataSource and reject non-positive length fragments - drop MetadataDurationPlayer: clipping already gives the timeline the real duration, and the forwarder could publish a wrong metadata duration for direct play - drop the dead PlaybackState.error/lastError now that errors are toast-only, and tighten the floor test to assert the exact value --- .../subtracks/data/repo/SourceRepository.kt | 5 +++-- .../com/subtracks/data/source/StreamLength.kt | 11 ++++++++-- .../playback/KnownLengthDataSource.kt | 1 + .../subtracks/playback/Media3PlayerHandle.kt | 5 +++-- .../subtracks/playback/PlaybackController.kt | 8 ------- .../com/subtracks/playback/PlaybackService.kt | 12 +--------- .../subtracks/data/source/StreamLengthTest.kt | 11 ++++++++-- .../playback/PlaybackControllerTest.kt | 22 ------------------- docs/architecture.md | 2 +- 9 files changed, 27 insertions(+), 50 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt index 9d03617a..2a7d14c8 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt @@ -86,10 +86,11 @@ class SourceRepository( fun streamUri( songId: String, - durationMs: Long? = null, + durationMs: Long?, + quality: StreamQuality, ): String? { val uri = active?.streamUri(songId)?.toString() ?: return null - val length = declaredStreamLength(durationMs, quality.value) ?: return uri + val length = declaredStreamLength(durationMs, quality) ?: return uri return uri + streamLengthSuffix(length) } diff --git a/app/src/main/kotlin/com/subtracks/data/source/StreamLength.kt b/app/src/main/kotlin/com/subtracks/data/source/StreamLength.kt index 67f3b236..5f7e6452 100644 --- a/app/src/main/kotlin/com/subtracks/data/source/StreamLength.kt +++ b/app/src/main/kotlin/com/subtracks/data/source/StreamLength.kt @@ -6,7 +6,9 @@ internal const val STREAM_LENGTH_FRAGMENT = "streamLength" private const val MIN_TRANSCODE_BPS = 64_000L private const val UNCAPPED_BPS = 2_000_000L +private const val LOSSLESS_CEILING_BPS = 9_216_000L private const val OVERHEAD_BYTES = 256L * 1024L +private val LOSSLESS_FORMATS = setOf("flac", "wav", "alac") fun declaredStreamLength( durationMs: Long?, @@ -14,7 +16,12 @@ fun declaredStreamLength( ): Long? { if (durationMs == null || durationMs <= 0) return null if (!quality.transcodes) return null - val bps = if (quality.maxBitrate > 0) maxOf(quality.maxBitrate * 1000L, MIN_TRANSCODE_BPS) else UNCAPPED_BPS + val bps = + when { + quality.format?.lowercase() in LOSSLESS_FORMATS -> LOSSLESS_CEILING_BPS + quality.maxBitrate > 0 -> maxOf(quality.maxBitrate * 1000L, MIN_TRANSCODE_BPS) + else -> UNCAPPED_BPS + } return durationMs / 1000 * bps / 8 * 5 / 4 + OVERHEAD_BYTES } @@ -23,5 +30,5 @@ internal fun streamLengthSuffix(length: Long): String = "#$STREAM_LENGTH_FRAGMEN internal fun declaredLengthFromFragment(fragment: String?): Long? { val marker = "$STREAM_LENGTH_FRAGMENT=" if (fragment == null || !fragment.startsWith(marker)) return null - return fragment.substring(marker.length).toLongOrNull() + return fragment.substring(marker.length).toLongOrNull()?.takeIf { it > 0 } } diff --git a/app/src/main/kotlin/com/subtracks/playback/KnownLengthDataSource.kt b/app/src/main/kotlin/com/subtracks/playback/KnownLengthDataSource.kt index 6b574b82..18d5f47e 100644 --- a/app/src/main/kotlin/com/subtracks/playback/KnownLengthDataSource.kt +++ b/app/src/main/kotlin/com/subtracks/playback/KnownLengthDataSource.kt @@ -15,6 +15,7 @@ internal class KnownLengthDataSource( override fun open(dataSpec: DataSpec): Long { val length = delegate.open(dataSpec) if (length != C.LENGTH_UNSET.toLong()) return length + if (dataSpec.length != C.LENGTH_UNSET.toLong()) return dataSpec.length val declared = declaredLengthFromFragment(dataSpec.uri.fragment) ?: return length return (declared - dataSpec.position).coerceAtLeast(0) } diff --git a/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt b/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt index 30e66feb..35d80062 100644 --- a/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt +++ b/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt @@ -143,13 +143,14 @@ class Media3PlayerHandle( item.durationMs?.let { putLong(EXTRA_DURATION_MS, it) } }, ).build() + val quality = sourceRepository.quality.value val builder = MediaItem .Builder() .setMediaId(item.id) - .setUri(sourceRepository.streamUri(item.id, item.durationMs)) + .setUri(sourceRepository.streamUri(item.id, item.durationMs, quality)) .setMediaMetadata(metadata) - if (sourceRepository.quality.value.transcodes) { + if (quality.transcodes) { item.durationMs?.takeIf { it > 0 }?.let { duration -> builder.setClippingConfiguration( ClippingConfiguration.Builder().setEndPositionMs(duration).build(), diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index 2fe0d1b8..cf72a35b 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -45,7 +45,6 @@ data class PlaybackState( val item: QueueItem? = null, val context: QueueContext? = null, val position: Long? = null, - val error: String? = null, val isBuffering: Boolean = false, val isPlaying: Boolean = false, val durationMs: Long = 0, @@ -119,7 +118,6 @@ class PlaybackController( private val startLock = Mutex() private var snapshot: QueueSnapshot? = null - private var lastError: String? = null private var queueSourceId: Long? = null private var windowStart = 0L private var windowEnd = -1L @@ -226,7 +224,6 @@ class PlaybackController( windowJob?.cancel() windowStart = 0 windowEnd = -1 - lastError = null lastEdit = null endedHandled = false val previous = snapshot @@ -602,7 +599,6 @@ class PlaybackController( windowJob?.cancel() windowStart = 0 windowEnd = -1 - lastError = null lastEdit = null endedHandled = false val modes = queueRepository.modes() @@ -676,7 +672,6 @@ class PlaybackController( bufferingJob = null showBuffering = false snapshot = null - lastError = null queueSourceId = null windowStart = 0 windowEnd = -1 @@ -858,7 +853,6 @@ class PlaybackController( private val playerListener = object : PlayerHandle.Listener { override fun onTransition() { - lastError = null endedHandled = false if (updating) return scope.launch { onPositionChanged() } @@ -870,7 +864,6 @@ class PlaybackController( } override fun onError(message: String) { - lastError = message showMessage(message) refresh() } @@ -939,7 +932,6 @@ class PlaybackController( item = player.currentItem, context = contextAt(position), position = position, - error = lastError, isBuffering = showBuffering, isPlaying = player.playWhenReady && !player.isIdle && !player.isEnded, durationMs = durationMs, diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt index 4149a5c0..f25a6f94 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt @@ -7,8 +7,6 @@ import android.util.Log import androidx.annotation.OptIn import androidx.media3.common.AudioAttributes import androidx.media3.common.C -import androidx.media3.common.ForwardingPlayer -import androidx.media3.common.Player import androidx.media3.common.util.ExperimentalApi import androidx.media3.common.util.UnstableApi import androidx.media3.datasource.okhttp.OkHttpDataSource @@ -47,7 +45,6 @@ class PlaybackService : MediaSessionService() { KnownLengthDataSourceFactory(OkHttpDataSource.Factory(streamingClient())), ), ).build() - val player = MetadataDurationPlayer(exoPlayer) val loader = CoverArtBitmapLoader( applicationContext, @@ -57,7 +54,7 @@ class PlaybackService : MediaSessionService() { artworkLoader = loader session = MediaSession - .Builder(this, player) + .Builder(this, exoPlayer) .setSessionActivity( PendingIntent.getActivity( this, @@ -106,10 +103,3 @@ class PlaybackService : MediaSessionService() { super.onDestroy() } } - -@OptIn(UnstableApi::class) -private class MetadataDurationPlayer( - player: Player, -) : ForwardingPlayer(player) { - override fun getDuration(): Long = currentMediaItem?.mediaMetadata?.durationMs?.takeIf { it > 0 } ?: super.getDuration() -} diff --git a/app/src/test/kotlin/com/subtracks/data/source/StreamLengthTest.kt b/app/src/test/kotlin/com/subtracks/data/source/StreamLengthTest.kt index f762a8a9..ea4991d1 100644 --- a/app/src/test/kotlin/com/subtracks/data/source/StreamLengthTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/source/StreamLengthTest.kt @@ -27,9 +27,16 @@ class StreamLengthTest { @Test fun aTinyBitrateStillClearsTheEncodersFloor() { - val length = declaredStreamLength(180_000, StreamQuality(24, "mp3"))!! + assertEquals(2_062_144L, declaredStreamLength(180_000, StreamQuality(24, "mp3"))) + } - assertTrue(length >= 180 * 32_000 / 8) + @Test + fun losslessFormatsIgnoreTheBitrateAndAllowForTheLargestStream() { + val cdQuality = declaredStreamLength(320_000, StreamQuality(64, "flac"))!! + val highBitrate = declaredStreamLength(320_000, StreamQuality(320, "flac"))!! + + assertEquals(cdQuality, highBitrate) + assertTrue(cdQuality >= 320 * 1_411_200 / 8) } @Test diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index 279d6813..be87c516 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -280,28 +280,6 @@ class PlaybackControllerTest { await { !controller.state.value.isBuffering } } - @Test - fun playbackFailuresAreSurfacedUntilTheNextTrack() { - seedAlbum(3, sourceId = 1) - - controller.playAlbum(1, "al1", 0) - await { - controller.state.value.item - ?.id == "s1" - } - - handle.fail("ERROR_CODE_IO_UNSPECIFIED") - await { controller.state.value.error == "ERROR_CODE_IO_UNSPECIFIED" } - - handle.advanceTo(handle.currentIndex + 1) - await { - controller.state.value.item - ?.id == "s2" - } - - assertEquals(null, controller.state.value.error) - } - @Test fun playbackErrorsAreShownAsAToast() { seedAlbum(3, sourceId = 1) diff --git a/docs/architecture.md b/docs/architecture.md index 8f0166e7..039eb3d9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -86,7 +86,7 @@ The queue lives in SQL, not memory. `queue_entries` is an ordered list of *refer 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. -Stream URLs come from `SourceRepository.streamUri`, which asks the active `SubsonicSource` for a freshly salted `stream` URL; because the URL changes every request, artwork is attached to the media item by identity (the `CoverArtRef` cache key) rather than by URL. The active source carries the stream quality (a max bitrate plus an optional transcode format) for the current network: `NetworkMonitor` reports Wi-Fi vs mobile (metered) and `UserPreferences` stores a separate `StreamQuality` per mode, so `SourceRepository` rebuilds the source when either the mode or a quality changes. `PlaybackController` observes that effective quality and reloads the window around the current position, preserving play/pause and the playback position, so a Wi-Fi↔mobile transition re-issues the stream URLs with the new settings (a no-op when the two modes share a quality). Transcoded responses are chunked with no length and (because ffmpeg cannot seek back in a stream) no MP3 duration header, so Media3 would treat them as live and ignore seeks; the server's `estimateContentLength` is no better, since its estimate under-counts and cuts the track short. So a transcoded URL carries a declared upper-bound length in its fragment (never sent to the server), `KnownLengthDataSource` hands that length to Media3 to keep the stream seekable without truncation, and `MetadataDurationPlayer` reports the library duration to the session so the notification and UI stay correct. +Stream URLs come from `SourceRepository.streamUri`, which asks the active `SubsonicSource` for a freshly salted `stream` URL; because the URL changes every request, artwork is attached to the media item by identity (the `CoverArtRef` cache key) rather than by URL. The active source carries the stream quality (a max bitrate plus an optional transcode format) for the current network: `NetworkMonitor` reports Wi-Fi vs mobile (metered) and `UserPreferences` stores a separate `StreamQuality` per mode, so `SourceRepository` rebuilds the source when either the mode or a quality changes. `PlaybackController` observes that effective quality and reloads the window around the current position, preserving play/pause and the playback position, so a Wi-Fi↔mobile transition re-issues the stream URLs with the new settings (a no-op when the two modes share a quality). Transcoded responses are chunked with no length and (because ffmpeg cannot seek back in a stream) no MP3 duration header, so Media3 would treat them as live and ignore seeks; the server's `estimateContentLength` is no better, since its estimate under-counts and cuts the track short. So a transcoded URL carries a declared upper-bound length in its fragment (never sent to the server), `KnownLengthDataSource` hands that length to Media3 to keep the stream seekable without truncation, and each transcoded item is clipped to the library duration so the timeline ends where the audio does and the next track still advances. The window is also the queue the media notification and Android Auto can browse — a deliberate trade-off, since the alternative is holding an entire large library in memory. The queue view resolves positions lazily too: it loads the queue around the cursor in chunks and extends by a chunk when you scroll near either end, so it never materializes the whole queue. Dragging uses `sh.calvin.reorderable`, which floats the lifted row and reorders the loaded list live (animating with `animateItem`), so editing is what a drag needs: the view reorders its loaded list while dragging and commits one move on drop. `queue_entries` is rewritten to edit: removing a position splits its entry into up to two ranges, and moving one turns the moved track into an explicit song entry inserted at the target index, so an edit costs a couple of rows rather than a copy of the queue. When the edit falls inside the player's window the controller mirrors it with `removeMediaItem`/`moveMediaItem` so playback is not interrupted; removing the playing track, or a move that crosses the window boundary, reloads the window around the cursor. The controller keeps the pre-edit entries and cursor, so the queue view can offer a one-step undo. Timed lyrics/gapless format preferences, scrobbling, downloads and adding to the queue from the library are not wired yet.