From 610528cfcd759470eff63b5846892f240eb720e3 Mon Sep 17 00:00:00 2001 From: davotoula Date: Tue, 14 Apr 2026 23:13:42 +0200 Subject: [PATCH] refactor(hls): trust the upload server URL verbatim, drop withExtensionHint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Policy shift agreed with the server-side collaborator: the URL the NIP-96 / Blossom server returns is already the optimal form — clean Content-Type, correct cache keys, range-friendly — and the client should not second-guess it. Stripping extensions, appending hints or rewriting to bare sha256 all add round trips and cache misses for no real win now that the WordPress plugin returns clean .m3u8 URLs. Concretely: - HlsUploadPipeline drops the withExtensionHint helper entirely. Every combinedUrl / mediaPlaylistUrl / masterUrl flows straight from the uploader's MediaUploadResult.url into the playlist rewrite and the HlsUploadResult. - A small kdoc note at the top of the pipeline documents the policy ("if a server returns an unplayable URL, the fix is server-side"). Pipeline tests pruned: - appendsMp4HintToBareCombinedUrlInMediaPlaylist — removed, was testing the appender that no longer exists. - appendsM3u8HintToBarePlaylistUrlInMasterPlaylist — removed, same. - trailingDotUrlsAreSanitisedIntoCleanExtension — removed, the Nip96 fallback-extension map keeps the server from returning trailing-dot URLs in the first place. - doesNotDoubleAppendExtensionWhenAlreadyPresent — removed. Kept and reframed: - serverUrlsAlreadyWithExtensionPassThroughUntouched — the canonical contract: server returns clean .m3u8 / .mp4, pipeline forwards verbatim. - bareServerUrlsPassThroughVerbatim (new) — explicitly documents that even a bare-hash URL flows through unchanged; the pipeline does not try to make it playable. Co-Authored-By: Claude Opus 4.5 --- .../service/uploads/hls/HlsUploadPipeline.kt | 43 +++-------- .../uploads/hls/HlsUploadPipelineTest.kt | 73 +++---------------- 2 files changed, 20 insertions(+), 96 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/uploads/hls/HlsUploadPipeline.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/uploads/hls/HlsUploadPipeline.kt index 6946dca48..86783a93a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/uploads/hls/HlsUploadPipeline.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/uploads/hls/HlsUploadPipeline.kt @@ -58,6 +58,12 @@ data class HlsUploadedRendition( * Finally rewrites the master playlist to reference the per-rendition playlist URLs and uploads * the master. The resulting [HlsUploadResult] is what the publisher uses to build the NIP-71 * event. + * + * URL handling policy: the pipeline uses the URL the server returned verbatim. No extension is + * appended, no trailing dot stripped, no bare-hash rewriting. The server is responsible for + * returning a URL that the player can fetch as-is — that way we get the best cache coherence, + * correct Content-Type, clean range requests, and no double round trips. If a server returns an + * unplayable URL, the fix is server-side. */ class HlsUploadPipeline( private val uploader: HlsBlobUploader, @@ -75,10 +81,7 @@ class HlsUploadPipeline( val combined = uploader.upload(rendition.combinedFile, CONTENT_TYPE_VIDEO_MP4) onProgress(++done, total) val combinedUrl = - withExtensionHint( - combined.url ?: error("Uploader returned null URL for ${rendition.combinedFile.name}"), - CONTENT_TYPE_VIDEO_MP4, - ) + combined.url ?: error("Uploader returned null URL for ${rendition.combinedFile.name}") val rewrittenMedia = HlsPlaylistRewriter.rewrite( @@ -90,10 +93,7 @@ class HlsUploadPipeline( val mediaPlaylist = uploader.upload(mediaPlaylistFile, CONTENT_TYPE_HLS) onProgress(++done, total) val mediaPlaylistUrl = - withExtensionHint( - mediaPlaylist.url ?: error("Uploader returned null URL for media playlist ${rendition.label}"), - CONTENT_TYPE_HLS, - ) + mediaPlaylist.url ?: error("Uploader returned null URL for media playlist ${rendition.label}") HlsUploadedRendition( label = rendition.label, @@ -112,10 +112,7 @@ class HlsUploadPipeline( val master = uploader.upload(masterFile, CONTENT_TYPE_HLS) onProgress(++done, total) val masterUrl = - withExtensionHint( - master.url ?: error("Uploader returned null URL for master playlist"), - CONTENT_TYPE_HLS, - ) + master.url ?: error("Uploader returned null URL for master playlist") return HlsUploadResult( masterUrl = masterUrl, @@ -124,28 +121,6 @@ class HlsUploadPipeline( ) } - // Blossom servers typically return bare-hash URLs (https://server/), but HLS parsers - // and ExoPlayer's Util.inferContentType sniff the URL extension to pick the right source - // factory. Append a hint unless the upload server already baked one in. Trailing dots are - // trimmed and any pre-existing doubled extension (e.g. "..m3u8") collapsed, so a server that - // echoes our empty-extension upload filename does not produce unreachable URLs. - private fun withExtensionHint( - url: String, - contentType: String, - ): String { - val ext = - when (contentType) { - CONTENT_TYPE_VIDEO_MP4 -> ".mp4" - CONTENT_TYPE_HLS -> ".m3u8" - else -> return url - } - val sanitised = - url - .replace(Regex("""\.{2,}""" + Regex.escape(ext.trimStart('.')) + "$", RegexOption.IGNORE_CASE), ext) - .trimEnd('.') - return if (sanitised.endsWith(ext, ignoreCase = true)) sanitised else sanitised + ext - } - companion object { const val CONTENT_TYPE_VIDEO_MP4 = "video/mp4" const val CONTENT_TYPE_HLS = "application/vnd.apple.mpegurl" diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/uploads/hls/HlsUploadPipelineTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/uploads/hls/HlsUploadPipelineTest.kt index edac38b65..75b804855 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/uploads/hls/HlsUploadPipelineTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/uploads/hls/HlsUploadPipelineTest.kt @@ -180,36 +180,25 @@ class HlsUploadPipelineTest { } @Test - fun appendsMp4HintToBareCombinedUrlInMediaPlaylist() { - val bundle = createBundle(listOf("360p")) - val uploader = BareUrlUploader() - val pipeline = HlsUploadPipeline(uploader) - - runBlocking { pipeline.upload(bundle) } - - val uploadedMediaPlaylist = uploader.calls[1].third - assertTrue( - "media playlist should reference url with .mp4 hint", - uploadedMediaPlaylist.contains("https://blossom.test/bare-1.mp4"), - ) - assertTrue(!uploadedMediaPlaylist.contains("https://blossom.test/bare-1\"")) - } - - @Test - fun appendsM3u8HintToBarePlaylistUrlInMasterPlaylist() { + fun bareServerUrlsPassThroughVerbatim() { + // Policy: the pipeline uses whatever URL the server returned, unchanged. + // Even a bare-hash URL with no extension flows straight into the rewritten + // playlists. If it does not play, the fix is server-side (return a playable URL). val bundle = createBundle(listOf("360p")) val uploader = BareUrlUploader() val pipeline = HlsUploadPipeline(uploader) val result = runBlocking { pipeline.upload(bundle) } - val uploadedMaster = uploader.calls[2].third + assertEquals("https://blossom.test/bare-1", result.renditions[0].combinedUrl) + assertEquals("https://blossom.test/bare-2", result.renditions[0].playlistUrl) + assertEquals("https://blossom.test/bare-3", result.masterUrl) + // The rewritten media playlist that the server received must contain the bare url. + val uploadedMediaPlaylist = uploader.calls[1].third assertTrue( - "master playlist should reference playlist url with .m3u8 hint", - uploadedMaster.contains("https://blossom.test/bare-2.m3u8"), + "media playlist should reference bare url: $uploadedMediaPlaylist", + uploadedMediaPlaylist.contains("https://blossom.test/bare-1"), ) - assertEquals("https://blossom.test/bare-3.m3u8", result.masterUrl) - assertEquals("https://blossom.test/bare-1.mp4", result.renditions[0].combinedUrl) } @Test @@ -252,46 +241,6 @@ class HlsUploadPipelineTest { assertTrue(!result.renditions[0].playlistUrl.contains(".m3u8.m3u8")) } - @Test - fun trailingDotUrlsAreSanitisedIntoCleanExtension() { - val bundle = createBundle(listOf("360p")) - val uploader = - object : HlsBlobUploader { - var count = 0 - - override suspend fun upload( - file: File, - contentType: String, - ): MediaUploadResult { - count++ - return MediaUploadResult(url = "https://blossom.test/bare-$count.", sha256 = "sha-$count", size = file.length()) - } - } - val pipeline = HlsUploadPipeline(uploader) - - val result = runBlocking { pipeline.upload(bundle) } - - assertTrue("masterUrl must not contain '..': ${result.masterUrl}", !result.masterUrl.contains("..")) - assertTrue("masterUrl must not end with '.': ${result.masterUrl}", !result.masterUrl.endsWith(".")) - assertTrue(result.masterUrl.endsWith(".m3u8")) - assertTrue(!result.renditions[0].combinedUrl.contains("..")) - assertTrue(result.renditions[0].combinedUrl.endsWith(".mp4")) - } - - @Test - fun doesNotDoubleAppendExtensionWhenAlreadyPresent() { - val bundle = createBundle(listOf("360p")) - val uploader = FakeUploader() // returns urls ending in .mp4 / .m3u8 - val pipeline = HlsUploadPipeline(uploader) - - runBlocking { pipeline.upload(bundle) } - - val uploadedMediaPlaylist = uploader.calls[1].content - assertTrue(!uploadedMediaPlaylist.contains(".mp4.mp4")) - val uploadedMaster = uploader.calls[2].content - assertTrue(!uploadedMaster.contains(".m3u8.m3u8")) - } - @Test fun reportsUploadProgressPerStep() { val bundle = createBundle(listOf("360p", "540p"))