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
This commit is contained in:
austinried
2026-10-04 00:30:00 +00:00
parent ebaa486126
commit a0647deaae
9 changed files with 27 additions and 50 deletions
@@ -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)
}
@@ -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 }
}
@@ -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)
}
@@ -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(),
@@ -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,
@@ -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()
}
@@ -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
@@ -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)