diff --git a/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/components/AudioPlayerBoxOverflowTest.kt b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/components/AudioPlayerBoxOverflowTest.kt index 9cea5fca62..564715b2f9 100644 --- a/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/components/AudioPlayerBoxOverflowTest.kt +++ b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/components/AudioPlayerBoxOverflowTest.kt @@ -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 { @@ -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) } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt index 2164723756..3e560530cc 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ZoomableContentView.kt @@ -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) diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/richtext/RichTextParser.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/richtext/RichTextParser.kt index 6e8ddcbf18..60f04f7578 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/richtext/RichTextParser.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/richtext/RichTextParser.kt @@ -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. diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/richtext/RichTextParserAudioUrlTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/richtext/RichTextParserAudioUrlTest.kt index 298692a4e4..f8537790a1 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/richtext/RichTextParserAudioUrlTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/richtext/RichTextParserAudioUrlTest.kt @@ -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")) } }