Code review:

- fold audioExt into videoExt and share the classifier
This commit is contained in:
davotoula
2026-07-26 20:26:52 +02:00
parent b2b2adf076
commit 482be60e4a
4 changed files with 53 additions and 51 deletions
@@ -55,18 +55,19 @@ class AudioPlayerBoxOverflowTest {
private class Bounds {
var top = 0f
var bottom = 0f
var width = 0
var height = 0
fun modifier() =
Modifier.onGloballyPositioned {
top = it.positionInRoot().y
width = it.size.width
height = it.size.height
bottom = top + height
}
}
private fun measure(
mimeType: String?,
url: String,
square: Boolean,
): Pair<Bounds, Bounds> {
@@ -76,7 +77,7 @@ class AudioPlayerBoxOverflowTest {
rule.setContent {
Box(Modifier.size(width = 400.dp, height = 1200.dp)) {
Box(
modifier = mediaSizingModifier(unknownMediaAspectRatio(mimeType, url), ContentScale.Fit).then(box.modifier()),
modifier = mediaSizingModifier(unknownMediaAspectRatio(null, url), ContentScale.Fit).then(box.modifier()),
contentAlignment = Alignment.Center,
) {
Box((if (square) Modifier.audioSquare() else Modifier).then(player.modifier()))
@@ -90,7 +91,7 @@ class AudioPlayerBoxOverflowTest {
@Test
fun squareAudioPlayerStaysInsideItsMediaBox() {
val (box, player) = measure(null, "https://haven.sdbitcoiners.com/f28a5a2e.mp3", square = true)
val (box, player) = measure("https://haven.sdbitcoiners.com/f28a5a2e.mp3", square = true)
assertTrue(
"player overflows above the media box by ${box.top - player.top}px",
@@ -103,23 +104,10 @@ class AudioPlayerBoxOverflowTest {
assertEquals("the box should wrap the square", player.height, box.height)
}
@Test
fun audioWithAnExplicitMimeAlsoStaysInside() {
val (box, player) = measure("audio/mpeg", "https://example.com/download?id=7", square = true)
assertTrue(player.top >= box.top && player.bottom <= box.bottom)
assertEquals(player.height, box.height)
}
@Test
fun videoOfUnknownSizeKeeps16by9() {
val (box, _) = measure(null, "https://example.com/a.mp4", square = false)
val (box, _) = measure("https://example.com/a.mp4", square = false)
// 400.dp wide at 16:9. Rounding lands within a pixel either way.
val expected = box.height * 16f / 9f
assertTrue(
"expected a 16:9 box, got height=${box.height} for width=$expected",
kotlin.math.abs(expected - 400 * rule.density.density) <= 1f,
)
assertEquals(16f / 9f, box.width.toFloat() / box.height, 0.01f)
}
}
@@ -160,12 +160,7 @@ private const val DEFAULT_VIDEO_ASPECT_RATIO = 16f / 9f
internal fun unknownMediaAspectRatio(
mimeType: String?,
url: String,
): Float? =
when {
mimeType != null -> if (mimeType.startsWith("audio/")) null else DEFAULT_VIDEO_ASPECT_RATIO
RichTextParser.isAudioUrl(url) -> null
else -> DEFAULT_VIDEO_ASPECT_RATIO
}
): Float? = if (RichTextParser.isAudioContent(mimeType, url)) null else DEFAULT_VIDEO_ASPECT_RATIO
@Composable
fun ZoomableContentView(
@@ -226,10 +221,16 @@ fun ZoomableContentView(
}
is MediaUrlVideo -> {
// The fallback classifies the URL string, so compute it once per content — for audio
// the cache miss is permanent and this branch re-runs on every recomposition.
val fallbackRatio =
remember(content.url, content.mimeType) {
unknownMediaAspectRatio(content.mimeType, content.url)
}
val ratio =
content.dim?.aspectRatio()
?: MediaAspectRatioCache.get(content.url)
?: unknownMediaAspectRatio(content.mimeType, content.url)
?: fallbackRatio
val bridgedUrl =
remember(content.url, useLocalBlossomBridge) {
content.toCoilModel(useLocalBlossomBridge)
@@ -495,13 +495,20 @@ class RichTextParser {
)
val imageExt = listOf("png", "jpg", "gif", "bmp", "jpeg", "webp", "svg", "avif")
val videoExt = listOf("mp4", "avi", "wmv", "mpg", "amv", "webm", "mov", "mp3", "m3u8", "ogg", "wav", "flac", "aac", "opus", "m4a", "f4a")
val pdfExt = listOf("pdf")
// The audio-only members of [videoExt] both play through the same video pipeline, but audio
// has no picture, so anything that reasons about the shape of the media (aspect ratios, player
// sizing) has to tell them apart. `m3u8` stays out: a playlist carries either.
// Audio is folded into [videoExt] because both play through the same video pipeline but
// audio has no picture, so anything that reasons about the shape of the media (aspect
// ratios, player sizing) has to tell them apart. `m3u8` stays video-only: a playlist
// carries either.
//
// Composing [videoExt] out of [audioExt] is what keeps "every audio extension is also a
// video extension" true by construction rather than by convention — the two lists cannot
// drift apart. It is also why [audioExt] is declared first; the compiler rejects the
// reverse order outright ("Variable 'audioExt' must be initialized"), so this note is
// intent, not a guard rail.
val audioExt = listOf("mp3", "ogg", "wav", "flac", "aac", "opus", "m4a", "f4a")
val videoExt = listOf("mp4", "avi", "wmv", "mpg", "amv", "webm", "mov", "m3u8") + audioExt
val pdfExt = listOf("pdf")
val imageExtensions = imageExt + imageExt.map { it.uppercase() }
val videoExtensions = videoExt + videoExt.map { it.uppercase() }
@@ -518,14 +525,13 @@ class RichTextParser {
it.uppercase()
}
private fun removeQueryParamsForExtensionComparison(fullUrl: String): String =
if (fullUrl.contains("?")) {
fullUrl.split("?")[0]
} else if (fullUrl.contains("#")) {
fullUrl.split("#")[0]
} else {
fullUrl
}
private fun removeQueryParamsForExtensionComparison(fullUrl: String): String {
// Called per URL during feed render — substringBefore allocates nothing when the
// separator is absent, unlike split().
val queryStart = fullUrl.indexOf('?')
if (queryStart >= 0) return fullUrl.substring(0, queryStart)
return fullUrl.substringBefore('#')
}
fun isImageExtension(ext: String) = imageExtensions.any { it == ext }
@@ -553,6 +559,13 @@ class RichTextParser {
return audioExtensions.any { removedParamsFromUrl.endsWith(it) }
}
// A declared MIME type is authoritative when present; the URL extension is only a fallback
// for the common bare-URL case.
fun isAudioContent(
mimeType: String?,
url: String,
): Boolean = mimeType?.startsWith("audio/") ?: isAudioUrl(url)
// Mirrors the canonical HLS-playlist MIME list also kept in MediaItemCache.toExoPlayerMimeType.
// Called per URL during feed render — uses `equals(ignoreCase)` instead of `lowercase()` to
// avoid a per-call String allocation on the common non-HLS path.
@@ -31,14 +31,10 @@ class RichTextParserAudioUrlTest {
}
@Test
fun otherAudioContainersAreAudio() {
assertTrue(RichTextParser.isAudioUrl("https://example.com/a.wav"))
assertTrue(RichTextParser.isAudioUrl("https://example.com/a.flac"))
assertTrue(RichTextParser.isAudioUrl("https://example.com/a.aac"))
assertTrue(RichTextParser.isAudioUrl("https://example.com/a.opus"))
assertTrue(RichTextParser.isAudioUrl("https://example.com/a.m4a"))
assertTrue(RichTextParser.isAudioUrl("https://example.com/a.f4a"))
assertTrue(RichTextParser.isAudioUrl("https://example.com/a.ogg"))
fun everyAudioContainerIsAudio() {
RichTextParser.audioExt.forEach {
assertTrue(RichTextParser.isAudioUrl("https://example.com/a.$it"), it)
}
}
@Test
@@ -72,10 +68,14 @@ class RichTextParserAudioUrlTest {
}
@Test
fun audioExtensionsAreASubsetOfVideoExtensions() {
// Audio arrives as MediaUrlVideo precisely because videoExt lumps the two together.
RichTextParser.audioExt.forEach {
assertTrue(RichTextParser.videoExt.contains(it), "videoExt must still contain $it")
}
fun mimeTypeIsAuthoritativeOverTheUrl() {
assertTrue(RichTextParser.isAudioContent("audio/mpeg", "https://example.com/download?id=7"))
assertFalse(RichTextParser.isAudioContent("video/mp4", "https://example.com/a.mp3"))
}
@Test
fun urlExtensionIsTheFallbackWithoutAMimeType() {
assertTrue(RichTextParser.isAudioContent(null, "https://example.com/a.mp3"))
assertFalse(RichTextParser.isAudioContent(null, "https://example.com/a.mp4"))
}
}