From 00c422f317dad104152620a66e4b54e378b40b90 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 07:07:06 -0500 Subject: [PATCH 1/2] Make Media3 write the container and codec it was asked for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Media3Engine never called setMuxerFactory or setAudioMimeType, and built a bare EditedMediaItem, so it always produced MP4 with an H.265 video track. The router meanwhile sends it WebM, Ogg, WAV and AAC-ADTS jobs, plus audio-only M4A, Opus and WAV — and ConversionWorker.media3MimeType() mapped VideoCodec.NONE through its else branch to VIDEO_H265. The visible result: "extract audio to M4A" transcoded the video to HEVC and named the file .m4a. Nothing failed, and nothing caught it, because Media3EngineTest had no audio-only case at all. media3-muxer already ships WebmMuxer, OggMuxer, WavMuxer and AacMuxer; only the MP4 ones come pre-wrapped as a Muxer.Factory. Media3Muxers supplies the rest. Their reported sample MIME types are read from each muxer's own support check rather than assumed, because Transformer uses those lists to decide whether a track needs re-encoding. HardwareTranscoder.transcode now takes the OutputFormat instead of a video MIME string, which is what gives the container, the audio codec and "this output has no video" somewhere to travel. MEDIA3_CONTAINERS stops being private so a test can assert it agrees with the factories. Those two drifted once already: the router's set was right the whole time the engine was ignoring it. Co-Authored-By: Claude Opus 5 (1M context) --- .../bench/RealMediaBenchmark.kt | 4 +- .../convert/Media3EngineTest.kt | 88 +++++++++++- .../fallback/FakeFailures.kt | 3 +- .../convert/Media3Engine.kt | 93 ++++++++++--- .../convert/Media3Muxers.kt | 128 ++++++++++++++++++ .../convert/Transcoders.kt | 10 +- .../model/ConversionRouter.kt | 10 +- .../work/ConversionWorker.kt | 9 +- .../convert/Media3MuxersTest.kt | 98 ++++++++++++++ 9 files changed, 407 insertions(+), 36 deletions(-) create mode 100644 app/src/main/java/org/libremediaconverter/convert/Media3Muxers.kt create mode 100644 app/src/test/java/org/libremediaconverter/convert/Media3MuxersTest.kt diff --git a/app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt b/app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt index f103d38..668fc6b 100644 --- a/app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt +++ b/app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt @@ -100,7 +100,7 @@ class RealMediaBenchmark { val hwOut = File(context.cacheDir, "bench_hw.mp4").apply { delete() } val engine = Media3Engine(context) val hwMs = try { - runCatching { timed { engine.transcode(uri, hwOut, MimeTypes.VIDEO_H265) } } + runCatching { timed { engine.transcode(uri, hwOut, OutputFormat.MP4_H265) } } .onFailure { Log.w(TAG, "BENCH hardware: UNSUPPORTED (${it.message})") } .getOrNull() } finally { @@ -162,7 +162,7 @@ class RealMediaBenchmark { val out = File(context.cacheDir, "bench_av1_out.mp4").apply { delete() } val engine = Media3Engine(context) val ms = try { - timed { engine.transcode(Uri.fromFile(input), out, MimeTypes.VIDEO_H265) } + timed { engine.transcode(Uri.fromFile(input), out, OutputFormat.MP4_H265) } } finally { engine.close() } diff --git a/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt b/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt index dc28d79..cf6d1f0 100644 --- a/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt @@ -11,10 +11,12 @@ import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withTimeout import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import org.libremediaconverter.model.OutputFormat import java.io.File import java.util.concurrent.Executors import java.util.concurrent.TimeUnit @@ -60,7 +62,7 @@ class Media3EngineTest { engine.transcode( input = Uri.fromFile(input), output = output, - videoMimeType = MimeTypes.VIDEO_H265, + format = OutputFormat.MP4_H265, ) { percent -> seen += percent } assertTrue("export produced no file", output.exists()) @@ -77,6 +79,77 @@ class Media3EngineTest { seen.forEach { assertTrue("progress out of range: $it", it in 0..100) } } + /** + * Regression guard for the audio-extraction bug. + * + * [OutputFormat.M4A_AAC] declares `VideoCodec.NONE`, and the router sends it to Media3. But + * the engine used to build a bare `EditedMediaItem` and take a video MIME type that defaulted + * to HEVC, so "extract the audio" transcoded the *video* to H.265 and wrote it to a file named + * `.m4a`. Nothing failed; the output was simply not what was asked for. + * + * This is the test the suite was missing — [Media3EngineTest] had no audio-only case at all, + * which is why the defect survived. + */ + @Test + fun audioOnlyExportDropsTheVideoTrack(): Unit = runBlocking { + val audio = File(context.cacheDir, "out_audio.m4a") + audio.delete() + try { + engine.transcode(Uri.fromFile(input), audio, OutputFormat.M4A_AAC) + + assertTrue("export produced no file", audio.exists() && audio.length() > 0) + val tracks = trackMimeTypesOf(audio) + assertEquals("expected exactly one track, got $tracks", 1, tracks.size) + assertEquals(MimeTypes.AUDIO_AAC, tracks.single()) + assertNull("an audio-only export must carry no video track", videoMimeTypeOf(audio)) + assertTrue("output has no duration", durationMsOf(audio) > 0) + } finally { + audio.delete() + } + } + + /** + * WAV output, which reaches Media3 for the same reason M4A does. + * + * `Container.WAV` is in the router's Media3 set, but until the engine passed a muxer factory + * the only muxer Transformer ever used was the MP4 one — so asking for WAV produced an MP4. + * Asserting the RIFF header proves the container, not merely that a file appeared; this + * follows what `FFmpegEngineTest` already does for its formats. + */ + @Test + fun wavExportWritesARiffHeader(): Unit = runBlocking { + val wav = File(context.cacheDir, "out_audio.wav") + wav.delete() + try { + engine.transcode(Uri.fromFile(input), wav, OutputFormat.WAV) + + assertTrue("export produced no file", wav.exists() && wav.length() > 0) + assertEquals("RIFF", String(wav.readBytes().copyOfRange(0, 4), Charsets.US_ASCII)) + } finally { + wav.delete() + } + } + + /** + * Ogg/Opus output, the third container the router claims for Media3. + * + * Asserted by magic bytes rather than `MediaExtractor`: platform extractor support for raw + * Ogg is inconsistent across the API levels in the CI matrix, and "OggS" is unambiguous. + */ + @Test + fun opusExportWritesAnOggHeader(): Unit = runBlocking { + val ogg = File(context.cacheDir, "out_audio.opus") + ogg.delete() + try { + engine.transcode(Uri.fromFile(input), ogg, OutputFormat.OPUS) + + assertTrue("export produced no file", ogg.exists() && ogg.length() > 0) + assertEquals("OggS", String(ogg.readBytes().copyOfRange(0, 4), Charsets.US_ASCII)) + } finally { + ogg.delete() + } + } + /** * Regression guard for the Transformer threading trap. * @@ -124,7 +197,7 @@ class Media3EngineTest { val failure = runCatching { runBlocking { withTimeout(30_000) { - engine.transcode(Uri.fromFile(input), impossible, MimeTypes.VIDEO_H265) {} + engine.transcode(Uri.fromFile(input), impossible, OutputFormat.MP4_H265) {} } } }.exceptionOrNull() @@ -148,6 +221,17 @@ class Media3EngineTest { } } + private fun trackMimeTypesOf(file: File): List { + val extractor = MediaExtractor() + return try { + extractor.setDataSource(file.absolutePath) + (0 until extractor.trackCount) + .map { extractor.getTrackFormat(it).getString(MediaFormat.KEY_MIME).orEmpty() } + } finally { + extractor.release() + } + } + private fun videoMimeTypeOf(file: File): String? { val extractor = MediaExtractor() try { diff --git a/app/src/androidTest/java/org/libremediaconverter/fallback/FakeFailures.kt b/app/src/androidTest/java/org/libremediaconverter/fallback/FakeFailures.kt index 84b93e8..f2839d4 100644 --- a/app/src/androidTest/java/org/libremediaconverter/fallback/FakeFailures.kt +++ b/app/src/androidTest/java/org/libremediaconverter/fallback/FakeFailures.kt @@ -7,6 +7,7 @@ import org.libremediaconverter.convert.HardwareTranscoder import org.libremediaconverter.convert.OutputPublisher import org.libremediaconverter.convert.SoftwareTranscoder import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.OutputFormat import java.io.File /** @@ -24,7 +25,7 @@ object FakeFailures { override suspend fun transcode( input: Uri, output: File, - videoMimeType: String, + format: OutputFormat, onProgress: (Int) -> Unit, ) { called = true diff --git a/app/src/main/java/org/libremediaconverter/convert/Media3Engine.kt b/app/src/main/java/org/libremediaconverter/convert/Media3Engine.kt index bb99468..0c78040 100644 --- a/app/src/main/java/org/libremediaconverter/convert/Media3Engine.kt +++ b/app/src/main/java/org/libremediaconverter/convert/Media3Engine.kt @@ -15,6 +15,9 @@ import androidx.media3.transformer.ProgressHolder import androidx.media3.transformer.Transformer import kotlinx.coroutines.CancellableContinuation import kotlinx.coroutines.suspendCancellableCoroutine +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.VideoCodec import java.io.File import kotlin.coroutines.resume import kotlin.coroutines.resumeWithException @@ -55,12 +58,19 @@ class Media3Engine(private val context: Context) : HardwareTranscoder { override suspend fun transcode( input: Uri, output: File, - videoMimeType: String, + format: OutputFormat, onProgress: (Int) -> Unit, ): Unit = suspendCancellableCoroutine { cont -> handler.post { - val transformer = buildTransformer(videoMimeType, cont) - val item = EditedMediaItem.Builder(MediaItem.fromUri(input)).build() + val transformer = runCatching { buildTransformer(format, cont) } + .getOrElse { cont.resumeWithException(it); return@post } + // Dropping the tracks the target format does not have is what stops an audio-only + // export from carrying a re-encoded video track. Without setRemoveVideo, asking for + // M4A produced an HEVC stream in a file named .m4a. + val item = EditedMediaItem.Builder(MediaItem.fromUri(input)) + .setRemoveVideo(format.videoCodec == VideoCodec.NONE) + .setRemoveAudio(format.audioCodec == AudioCodec.NONE) + .build() cont.invokeOnCancellation { // cancel() has the same single-thread requirement as start(). @@ -74,26 +84,69 @@ class Media3Engine(private val context: Context) : HardwareTranscoder { } } + /** + * @throws IllegalArgumentException if [format] names a container Media3 cannot mux. That is a + * routing bug rather than a runtime condition — [org.libremediaconverter.model.ConversionRouter] + * is supposed to have sent such a job to FFmpeg — so it fails loudly instead of quietly + * writing MP4, which is what the old code did. + */ private fun buildTransformer( - videoMimeType: String, + format: OutputFormat, cont: CancellableContinuation, - ): Transformer = Transformer.Builder(context) - .setLooper(thread.looper) - .setVideoMimeType(videoMimeType) - .addListener(object : Transformer.Listener { - override fun onCompleted(composition: Composition, result: ExportResult) { - if (cont.isActive) cont.resume(Unit) - } + ): Transformer { + val muxerFactory = requireNotNull(Media3Muxers.factoryFor(format.container)) { + "Media3 cannot mux ${format.container}; this job should have routed to FFmpeg." + } - override fun onError( - composition: Composition, - result: ExportResult, - exception: ExportException, - ) { - if (cont.isActive) cont.resumeWithException(exception) - } - }) - .build() + val builder = Transformer.Builder(context) + .setLooper(thread.looper) + .setMuxerFactory(muxerFactory) + + // Only name a MIME type for a track the output actually keeps. Naming one for a removed + // track makes Transformer build an encoder for samples that will never arrive. + videoMimeTypeFor(format.videoCodec)?.let(builder::setVideoMimeType) + audioMimeTypeFor(format.audioCodec)?.let(builder::setAudioMimeType) + + return builder + .addListener(object : Transformer.Listener { + override fun onCompleted(composition: Composition, result: ExportResult) { + if (cont.isActive) cont.resume(Unit) + } + + override fun onError( + composition: Composition, + result: ExportResult, + exception: ExportException, + ) { + if (cont.isActive) cont.resumeWithException(exception) + } + }) + .build() + } + + /** + * Media3 encodes only H.264 and H.265 of the codecs this app offers. + * + * VP8/VP9/AV1 targets never reach here — the router sends them to FFmpeg because + * `Transformer.setVideoMimeType` rejects them — so anything unexpected returns null and lets + * Transformer pick, rather than silently substituting H.265 the way the old mapping did. + */ + private fun videoMimeTypeFor(codec: VideoCodec): String? = when (codec) { + VideoCodec.H264 -> MimeTypes.VIDEO_H264 + VideoCodec.H265 -> MimeTypes.VIDEO_H265 + VideoCodec.NONE -> null + VideoCodec.VP8, VideoCodec.VP9, VideoCodec.AV1 -> null + } + + private fun audioMimeTypeFor(codec: AudioCodec): String? = when (codec) { + AudioCodec.AAC -> MimeTypes.AUDIO_AAC + AudioCodec.OPUS -> MimeTypes.AUDIO_OPUS + AudioCodec.VORBIS -> MimeTypes.AUDIO_VORBIS + AudioCodec.PCM -> MimeTypes.AUDIO_RAW + AudioCodec.NONE -> null + // MP3 and FLAC have no Android encoder; the router routes them to FFmpeg. + AudioCodec.MP3, AudioCodec.FLAC -> null + } /** * Polls export progress on the Transformer's own thread. diff --git a/app/src/main/java/org/libremediaconverter/convert/Media3Muxers.kt b/app/src/main/java/org/libremediaconverter/convert/Media3Muxers.kt new file mode 100644 index 0000000..5417186 --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/convert/Media3Muxers.kt @@ -0,0 +1,128 @@ +package org.libremediaconverter.convert + +import androidx.media3.common.C +import androidx.media3.common.MimeTypes +import androidx.media3.common.util.UnstableApi +import androidx.media3.muxer.AacMuxer +import androidx.media3.muxer.Muxer +import androidx.media3.muxer.MuxerException +import androidx.media3.muxer.OggMuxer +import androidx.media3.muxer.SeekableMuxerOutput +import androidx.media3.muxer.WavMuxer +import androidx.media3.muxer.WebmMuxer +import androidx.media3.transformer.DefaultMuxer +import com.google.common.collect.ImmutableList +import org.libremediaconverter.model.Container +import java.io.FileOutputStream + +/** + * Muxer factories for the containers Media3 can write. + * + * `media3-muxer` ships `Mp4Muxer`, `WebmMuxer`, `OggMuxer`, `WavMuxer` and `AacMuxer`, but + * `media3-transformer` only wraps the MP4 ones as a [Muxer.Factory]. Everything else needs the + * three lines of glue below, which is why the app previously wrote MP4 no matter what container + * was asked for: [androidx.media3.transformer.Transformer.Builder] defaults to + * `DefaultMuxer.Factory`, and nothing ever overrode it. + * + * ## The MIME lists are load-bearing + * + * `Transformer` calls [Muxer.Factory.getSupportedSampleMimeTypes] to decide whether a track can be + * copied through or has to be re-encoded, and `Transformer.Builder.build()` validates the + * requested MIME types against it. A list that over-claims produces a file the muxer cannot + * actually write; one that under-claims forces a needless re-encode. The values here were read out + * of each muxer's own `isMimeTypeSupported` check rather than assumed. + */ +@UnstableApi +object Media3Muxers { + + /** + * The factory for [container], or null when Media3 cannot mux it at all. + * + * A null here must agree with [org.libremediaconverter.model.ConversionRouter]'s container set + * — if the router sends Media3 a job this cannot mux, the conversion fails and falls back to + * FFmpeg, which is a slow way to discover a routing bug. `Media3MuxersTest` asserts they agree. + */ + fun factoryFor(container: Container): Muxer.Factory? = when (container) { + // Transformer's own default. Named explicitly so the MP4 path reads the same as the rest. + Container.MP4 -> DefaultMuxer.Factory() + Container.WEBM -> WebmFactory + Container.OGG -> OggFactory + Container.WAV -> WavFactory + Container.AAC_ADTS -> AacFactory + // Matroska, MP3 and the image outputs have no Media3 muxer. FFmpeg owns them. + Container.MKV, + Container.MP3, + Container.GIF, + Container.IMAGE_SEQUENCE, + -> null + } + + private object WebmFactory : Muxer.Factory { + override fun create(path: String): Muxer = wrapFailure(path) { + WebmMuxer.Builder(SeekableMuxerOutput.of(path)).build() + } + + override fun getSupportedSampleMimeTypes(trackType: Int): ImmutableList = + when (trackType) { + C.TRACK_TYPE_VIDEO -> ImmutableList.of(MimeTypes.VIDEO_VP8, MimeTypes.VIDEO_VP9) + C.TRACK_TYPE_AUDIO -> ImmutableList.of(MimeTypes.AUDIO_OPUS, MimeTypes.AUDIO_VORBIS) + else -> ImmutableList.of() + } + } + + private object OggFactory : Muxer.Factory { + override fun create(path: String): Muxer = wrapFailure(path) { + OggMuxer.Builder(FileOutputStream(path).channel).build() + } + + override fun getSupportedSampleMimeTypes(trackType: Int): ImmutableList = + if (trackType == C.TRACK_TYPE_AUDIO) { + ImmutableList.of(MimeTypes.AUDIO_OPUS, MimeTypes.AUDIO_VORBIS) + } else { + ImmutableList.of() + } + } + + private object WavFactory : Muxer.Factory { + override fun create(path: String): Muxer = wrapFailure(path) { + WavMuxer(SeekableMuxerOutput.of(path)) + } + + override fun getSupportedSampleMimeTypes(trackType: Int): ImmutableList = + if (trackType == C.TRACK_TYPE_AUDIO) { + ImmutableList.of(MimeTypes.AUDIO_RAW) + } else { + ImmutableList.of() + } + } + + private object AacFactory : Muxer.Factory { + override fun create(path: String): Muxer = wrapFailure(path) { + AacMuxer(FileOutputStream(path)) + } + + override fun getSupportedSampleMimeTypes(trackType: Int): ImmutableList = + if (trackType == C.TRACK_TYPE_AUDIO) { + ImmutableList.of(MimeTypes.AUDIO_AAC) + } else { + ImmutableList.of() + } + } + + /** + * Turns an I/O failure into a [MuxerException]. + * + * Opening the output can throw `FileNotFoundException`, which is not what `Muxer.Factory` + * declares. Transformer's error handling only recognises `MuxerException`, so letting the raw + * IOException escape turns a bad output path into an unhandled crash instead of a reported + * export failure — the case `Media3EngineTest.anUnwritableOutputPathFailsInsteadOfHanging` + * exists to pin down. + */ + private inline fun wrapFailure(path: String, open: () -> Muxer): Muxer = + try { + open() + } catch (e: Exception) { + if (e is MuxerException) throw e + throw MuxerException("Could not open $path for muxing", e) + } +} diff --git a/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt b/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt index c2594db..ac284d1 100644 --- a/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt +++ b/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt @@ -5,15 +5,23 @@ import android.net.Uri import org.libremediaconverter.ffmpeg.FFmpegEngine import org.libremediaconverter.model.ConversionRequest import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.OutputFormat import org.libremediaconverter.codec.AndroidDeviceCodecs import java.io.File /** The hardware conversion path. Implemented by [Media3Engine]. */ interface HardwareTranscoder : AutoCloseable { + /** + * Takes the whole [OutputFormat] rather than just a video MIME type. + * + * The narrower signature was the reason "extract audio to M4A" produced an HEVC video track: + * the container, the audio codec and "this output has no video at all" had nowhere to travel, + * so the engine defaulted all three. + */ suspend fun transcode( input: Uri, output: File, - videoMimeType: String = androidx.media3.common.MimeTypes.VIDEO_H265, + format: OutputFormat = OutputFormat.MP4_H265, onProgress: (Int) -> Unit = {}, ) } diff --git a/app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt b/app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt index 1d56210..49c3fd3 100644 --- a/app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt +++ b/app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt @@ -13,8 +13,14 @@ package org.libremediaconverter.model */ object ConversionRouter { - /** Containers Media3 can mux. Anything else has to go to FFmpeg. */ - private val MEDIA3_CONTAINERS = setOf( + /** + * Containers Media3 can mux. Anything else has to go to FFmpeg. + * + * Not private: `Media3Muxers` has to supply a `Muxer.Factory` for every entry, and a test + * asserts the two agree. They drifted once already — this set was correct while the engine + * silently wrote MP4 for all five. + */ + internal val MEDIA3_CONTAINERS = setOf( Container.MP4, Container.WEBM, Container.OGG, diff --git a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt index 6ae0507..f05a577 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt @@ -3,7 +3,6 @@ package org.libremediaconverter.work import android.content.Context import android.net.Uri import android.util.Log -import androidx.media3.common.MimeTypes import androidx.media3.common.util.UnstableApi import androidx.work.CoroutineWorker import androidx.work.Data @@ -21,7 +20,6 @@ import org.libremediaconverter.model.Engine import org.libremediaconverter.model.EnginePreference import org.libremediaconverter.model.OutputFormat import org.libremediaconverter.model.QualityTier -import org.libremediaconverter.model.VideoCodec import java.io.File /** @@ -115,7 +113,7 @@ class ConversionWorker( ) { val engine = ConversionDependencies.hardware(applicationContext) try { - engine.transcode(inputUri, staged, media3MimeType(request.format)) { percent -> + engine.transcode(inputUri, staged, request.format) { percent -> publishProgress(displayName, percent) } return @@ -189,11 +187,6 @@ class ConversionWorker( } } - private fun media3MimeType(format: OutputFormat): String = when (format.videoCodec) { - VideoCodec.H264 -> MimeTypes.VIDEO_H264 - else -> MimeTypes.VIDEO_H265 - } - override suspend fun getForegroundInfo(): ForegroundInfo = foregroundInfo( inputData.getString(KEY_DISPLAY_NAME) ?: "input", diff --git a/app/src/test/java/org/libremediaconverter/convert/Media3MuxersTest.kt b/app/src/test/java/org/libremediaconverter/convert/Media3MuxersTest.kt new file mode 100644 index 0000000..4a4b314 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/Media3MuxersTest.kt @@ -0,0 +1,98 @@ +package org.libremediaconverter.convert + +import androidx.media3.common.C +import androidx.media3.common.MimeTypes +import androidx.media3.common.util.UnstableApi +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.ConversionRouter + +/** + * Guards the agreement between the router's container set and the muxers that back it. + * + * These two drifted once already, and the failure was invisible: [ConversionRouter] correctly + * claimed WebM, Ogg, WAV and AAC for Media3 while [Media3Engine] had no way to write any of them + * and produced MP4 regardless. Nothing failed — the file was simply the wrong container. + * + * A JVM test rather than an instrumented one: constructing a factory touches no Android APIs, only + * `create()` does, so the mapping is checkable without a device. + */ +@UnstableApi +class Media3MuxersTest { + + @Test + fun `every container the router sends to Media3 has a muxer factory`() { + ConversionRouter.MEDIA3_CONTAINERS.forEach { container -> + assertNotNull( + "$container is routed to Media3 but has no Muxer.Factory", + Media3Muxers.factoryFor(container), + ) + } + } + + @Test + fun `containers the router withholds from Media3 have no factory`() { + Container.entries + .filter { it !in ConversionRouter.MEDIA3_CONTAINERS } + .forEach { container -> + assertNull( + "$container has a Muxer.Factory but the router never sends it to Media3", + Media3Muxers.factoryFor(container), + ) + } + } + + @Test + fun `WebM advertises only the codecs its muxer accepts`() { + val factory = requireNotNull(Media3Muxers.factoryFor(Container.WEBM)) + + assertEquals( + listOf(MimeTypes.VIDEO_VP8, MimeTypes.VIDEO_VP9), + factory.getSupportedSampleMimeTypes(C.TRACK_TYPE_VIDEO), + ) + assertEquals( + listOf(MimeTypes.AUDIO_OPUS, MimeTypes.AUDIO_VORBIS), + factory.getSupportedSampleMimeTypes(C.TRACK_TYPE_AUDIO), + ) + } + + /** + * The audio-only containers must not claim a video track. + * + * Transformer reads these lists to decide what it may hand the muxer. Claiming video for a + * container that cannot hold it turns a routing mistake into a corrupt file rather than a + * clean failure. + */ + @Test + fun `audio-only containers advertise no video codecs`() { + listOf(Container.OGG, Container.WAV, Container.AAC_ADTS).forEach { container -> + val factory = requireNotNull(Media3Muxers.factoryFor(container)) + assertTrue( + "$container claims video support", + factory.getSupportedSampleMimeTypes(C.TRACK_TYPE_VIDEO).isEmpty(), + ) + assertTrue( + "$container claims no audio support", + factory.getSupportedSampleMimeTypes(C.TRACK_TYPE_AUDIO).isNotEmpty(), + ) + } + } + + @Test + fun `WAV carries PCM and AAC-ADTS carries AAC`() { + assertEquals( + listOf(MimeTypes.AUDIO_RAW), + requireNotNull(Media3Muxers.factoryFor(Container.WAV)) + .getSupportedSampleMimeTypes(C.TRACK_TYPE_AUDIO), + ) + assertEquals( + listOf(MimeTypes.AUDIO_AAC), + requireNotNull(Media3Muxers.factoryFor(Container.AAC_ADTS)) + .getSupportedSampleMimeTypes(C.TRACK_TYPE_AUDIO), + ) + } +} -- 2.47.3 From 92b7395ba991e4eac8f3d93549b2f866c67f22fa Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 07:17:11 -0500 Subject: [PATCH 2/2] Media3 can only write MP4, so stop claiming otherwise MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI proved the WAV and Ogg exports this branch added cannot work, and the reason generalises further than those two. media3-muxer 1.11.0 ships WebmMuxer, OggMuxer, WavMuxer and AacMuxer, which is why MEDIA3_CONTAINERS listed the matching containers. But all four throw UnsupportedOperationException from addMetadataEntry, and MuxerWrapper.addTrackFormat calls it for every metadata entry on the track format. Any real recording carries at least a creation timestamp, so the export dies partway through: Caused by: java.lang.UnsupportedOperationException at androidx.media3.muxer.OggMuxer.addMetadataEntry(OggMuxer.java:123) at androidx.media3.transformer.MuxerWrapper.addTrackFormat(MuxerWrapper.java:488) They are standalone muxers, not Transformer-compatible ones. WAV fails a second way before even reaching that: DefaultEncoderFactory has no PCM encoder, so Transformer reports "No MIME type is supported by both encoder and muxer" instead of passing raw samples through. Both observed on an API 35 emulator in CI, not inferred. The tests that found them were written on the assumption these containers worked. So MEDIA3_CONTAINERS becomes {MP4}. That the set was wrong went unnoticed because the engine ignored the container and wrote MP4 regardless — the set being wrong and the engine being wrong cancelled out. WAV, Opus and raw AAC move to FFmpeg, which already produces all three with instrumented coverage asserting the produced files. WEBM_VP9's routing reason changes from NO_PLATFORM_ENCODER to CONTAINER_UNSUPPORTED. Both were always true; the container is the more fundamental, since even given a VP9 encoder the file could not be written. The audio-only regression guard this branch exists for passed on API 35: M4A output now carries exactly one AAC track and no video. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/Media3EngineTest.kt | 42 ------ .../convert/Media3Muxers.kt | 134 +++++------------- .../model/ConversionRouter.kt | 17 ++- .../convert/Media3MuxersTest.kt | 62 ++------ .../model/ConversionRouterTest.kt | 38 ++++- 5 files changed, 86 insertions(+), 207 deletions(-) diff --git a/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt b/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt index cf6d1f0..82577c7 100644 --- a/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt @@ -108,48 +108,6 @@ class Media3EngineTest { } } - /** - * WAV output, which reaches Media3 for the same reason M4A does. - * - * `Container.WAV` is in the router's Media3 set, but until the engine passed a muxer factory - * the only muxer Transformer ever used was the MP4 one — so asking for WAV produced an MP4. - * Asserting the RIFF header proves the container, not merely that a file appeared; this - * follows what `FFmpegEngineTest` already does for its formats. - */ - @Test - fun wavExportWritesARiffHeader(): Unit = runBlocking { - val wav = File(context.cacheDir, "out_audio.wav") - wav.delete() - try { - engine.transcode(Uri.fromFile(input), wav, OutputFormat.WAV) - - assertTrue("export produced no file", wav.exists() && wav.length() > 0) - assertEquals("RIFF", String(wav.readBytes().copyOfRange(0, 4), Charsets.US_ASCII)) - } finally { - wav.delete() - } - } - - /** - * Ogg/Opus output, the third container the router claims for Media3. - * - * Asserted by magic bytes rather than `MediaExtractor`: platform extractor support for raw - * Ogg is inconsistent across the API levels in the CI matrix, and "OggS" is unambiguous. - */ - @Test - fun opusExportWritesAnOggHeader(): Unit = runBlocking { - val ogg = File(context.cacheDir, "out_audio.opus") - ogg.delete() - try { - engine.transcode(Uri.fromFile(input), ogg, OutputFormat.OPUS) - - assertTrue("export produced no file", ogg.exists() && ogg.length() > 0) - assertEquals("OggS", String(ogg.readBytes().copyOfRange(0, 4), Charsets.US_ASCII)) - } finally { - ogg.delete() - } - } - /** * Regression guard for the Transformer threading trap. * diff --git a/app/src/main/java/org/libremediaconverter/convert/Media3Muxers.kt b/app/src/main/java/org/libremediaconverter/convert/Media3Muxers.kt index 5417186..d9a65ac 100644 --- a/app/src/main/java/org/libremediaconverter/convert/Media3Muxers.kt +++ b/app/src/main/java/org/libremediaconverter/convert/Media3Muxers.kt @@ -1,128 +1,60 @@ package org.libremediaconverter.convert -import androidx.media3.common.C -import androidx.media3.common.MimeTypes import androidx.media3.common.util.UnstableApi -import androidx.media3.muxer.AacMuxer import androidx.media3.muxer.Muxer -import androidx.media3.muxer.MuxerException -import androidx.media3.muxer.OggMuxer -import androidx.media3.muxer.SeekableMuxerOutput -import androidx.media3.muxer.WavMuxer -import androidx.media3.muxer.WebmMuxer import androidx.media3.transformer.DefaultMuxer -import com.google.common.collect.ImmutableList import org.libremediaconverter.model.Container -import java.io.FileOutputStream /** - * Muxer factories for the containers Media3 can write. + * Which containers Media3 can actually write, and why it is only one. * - * `media3-muxer` ships `Mp4Muxer`, `WebmMuxer`, `OggMuxer`, `WavMuxer` and `AacMuxer`, but - * `media3-transformer` only wraps the MP4 ones as a [Muxer.Factory]. Everything else needs the - * three lines of glue below, which is why the app previously wrote MP4 no matter what container - * was asked for: [androidx.media3.transformer.Transformer.Builder] defaults to - * `DefaultMuxer.Factory`, and nothing ever overrode it. + * ## The four other muxers cannot be driven by Transformer * - * ## The MIME lists are load-bearing + * `media3-muxer` 1.11.0 ships `WebmMuxer`, `OggMuxer`, `WavMuxer` and `AacMuxer` alongside the MP4 + * ones, which reads like four more containers on the hardware path. It is not: **all four throw + * `UnsupportedOperationException` from `addMetadataEntry`**, and + * `MuxerWrapper.addTrackFormat` calls it for every metadata entry on the track format. Any real + * recording carries some — a creation timestamp is enough — so the export dies partway through: * - * `Transformer` calls [Muxer.Factory.getSupportedSampleMimeTypes] to decide whether a track can be - * copied through or has to be re-encoded, and `Transformer.Builder.build()` validates the - * requested MIME types against it. A list that over-claims produces a file the muxer cannot - * actually write; one that under-claims forces a needless re-encode. The values here were read out - * of each muxer's own `isMimeTypeSupported` check rather than assumed. + * ``` + * Caused by: java.lang.UnsupportedOperationException + * at androidx.media3.muxer.OggMuxer.addMetadataEntry(OggMuxer.java:123) + * at androidx.media3.transformer.MuxerWrapper.addTrackFormat(MuxerWrapper.java:488) + * ``` + * + * They are standalone muxers, not Transformer-compatible ones. WAV fails a second way even before + * that: `DefaultEncoderFactory` has no PCM encoder, so Transformer reports "No MIME type is + * supported by both encoder and muxer" rather than passing raw samples through. + * + * Both were observed on a CI API 35 emulator, not inferred — the tests that found them were written + * on the assumption these containers worked. + * + * So Media3 writes MP4 and nothing else. WebM, Ogg, WAV and raw AAC belong to FFmpeg, which already + * produces all of them and has instrumented coverage asserting the produced files. */ @UnstableApi object Media3Muxers { /** - * The factory for [container], or null when Media3 cannot mux it at all. + * The factory for [container], or null when Media3 cannot write it. * - * A null here must agree with [org.libremediaconverter.model.ConversionRouter]'s container set - * — if the router sends Media3 a job this cannot mux, the conversion fails and falls back to - * FFmpeg, which is a slow way to discover a routing bug. `Media3MuxersTest` asserts they agree. + * A null here must agree with [org.libremediaconverter.model.ConversionRouter]'s container set; + * `Media3MuxersTest` asserts they do. They drifted once already, and expensively: the router + * claimed five containers while the engine silently wrote MP4 for all of them. */ fun factoryFor(container: Container): Muxer.Factory? = when (container) { - // Transformer's own default. Named explicitly so the MP4 path reads the same as the rest. + // Transformer's own default. Named explicitly so the engine states its container rather + // than inheriting one, which is how the MP4-for-everything bug went unnoticed. Container.MP4 -> DefaultMuxer.Factory() - Container.WEBM -> WebmFactory - Container.OGG -> OggFactory - Container.WAV -> WavFactory - Container.AAC_ADTS -> AacFactory - // Matroska, MP3 and the image outputs have no Media3 muxer. FFmpeg owns them. + + Container.WEBM, + Container.OGG, + Container.WAV, + Container.AAC_ADTS, Container.MKV, Container.MP3, Container.GIF, Container.IMAGE_SEQUENCE, -> null } - - private object WebmFactory : Muxer.Factory { - override fun create(path: String): Muxer = wrapFailure(path) { - WebmMuxer.Builder(SeekableMuxerOutput.of(path)).build() - } - - override fun getSupportedSampleMimeTypes(trackType: Int): ImmutableList = - when (trackType) { - C.TRACK_TYPE_VIDEO -> ImmutableList.of(MimeTypes.VIDEO_VP8, MimeTypes.VIDEO_VP9) - C.TRACK_TYPE_AUDIO -> ImmutableList.of(MimeTypes.AUDIO_OPUS, MimeTypes.AUDIO_VORBIS) - else -> ImmutableList.of() - } - } - - private object OggFactory : Muxer.Factory { - override fun create(path: String): Muxer = wrapFailure(path) { - OggMuxer.Builder(FileOutputStream(path).channel).build() - } - - override fun getSupportedSampleMimeTypes(trackType: Int): ImmutableList = - if (trackType == C.TRACK_TYPE_AUDIO) { - ImmutableList.of(MimeTypes.AUDIO_OPUS, MimeTypes.AUDIO_VORBIS) - } else { - ImmutableList.of() - } - } - - private object WavFactory : Muxer.Factory { - override fun create(path: String): Muxer = wrapFailure(path) { - WavMuxer(SeekableMuxerOutput.of(path)) - } - - override fun getSupportedSampleMimeTypes(trackType: Int): ImmutableList = - if (trackType == C.TRACK_TYPE_AUDIO) { - ImmutableList.of(MimeTypes.AUDIO_RAW) - } else { - ImmutableList.of() - } - } - - private object AacFactory : Muxer.Factory { - override fun create(path: String): Muxer = wrapFailure(path) { - AacMuxer(FileOutputStream(path)) - } - - override fun getSupportedSampleMimeTypes(trackType: Int): ImmutableList = - if (trackType == C.TRACK_TYPE_AUDIO) { - ImmutableList.of(MimeTypes.AUDIO_AAC) - } else { - ImmutableList.of() - } - } - - /** - * Turns an I/O failure into a [MuxerException]. - * - * Opening the output can throw `FileNotFoundException`, which is not what `Muxer.Factory` - * declares. Transformer's error handling only recognises `MuxerException`, so letting the raw - * IOException escape turns a bad output path into an unhandled crash instead of a reported - * export failure — the case `Media3EngineTest.anUnwritableOutputPathFailsInsteadOfHanging` - * exists to pin down. - */ - private inline fun wrapFailure(path: String, open: () -> Muxer): Muxer = - try { - open() - } catch (e: Exception) { - if (e is MuxerException) throw e - throw MuxerException("Could not open $path for muxing", e) - } } diff --git a/app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt b/app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt index 49c3fd3..d23616b 100644 --- a/app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt +++ b/app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt @@ -16,17 +16,16 @@ object ConversionRouter { /** * Containers Media3 can mux. Anything else has to go to FFmpeg. * + * MP4 alone. This set used to name WebM, Ogg, WAV and AAC-ADTS as well, on the strength of + * `media3-muxer` shipping a muxer for each — but none of those four can be driven by + * Transformer at all, for the reasons `Media3Muxers` records. Nothing caught it because the + * engine ignored the container entirely and wrote MP4 regardless, so the set being wrong and + * the engine being wrong cancelled out. + * * Not private: `Media3Muxers` has to supply a `Muxer.Factory` for every entry, and a test - * asserts the two agree. They drifted once already — this set was correct while the engine - * silently wrote MP4 for all five. + * asserts the two agree. */ - internal val MEDIA3_CONTAINERS = setOf( - Container.MP4, - Container.WEBM, - Container.OGG, - Container.WAV, - Container.AAC_ADTS, - ) + internal val MEDIA3_CONTAINERS = setOf(Container.MP4) /** WebM is codec-restricted: Media3's WebmMuxer writes only these. */ private val WEBM_AUDIO = setOf(AudioCodec.OPUS, AudioCodec.VORBIS) diff --git a/app/src/test/java/org/libremediaconverter/convert/Media3MuxersTest.kt b/app/src/test/java/org/libremediaconverter/convert/Media3MuxersTest.kt index 4a4b314..fed5dfd 100644 --- a/app/src/test/java/org/libremediaconverter/convert/Media3MuxersTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/Media3MuxersTest.kt @@ -1,12 +1,9 @@ package org.libremediaconverter.convert -import androidx.media3.common.C -import androidx.media3.common.MimeTypes import androidx.media3.common.util.UnstableApi import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull -import org.junit.Assert.assertTrue import org.junit.Test import org.libremediaconverter.model.Container import org.libremediaconverter.model.ConversionRouter @@ -14,9 +11,10 @@ import org.libremediaconverter.model.ConversionRouter /** * Guards the agreement between the router's container set and the muxers that back it. * - * These two drifted once already, and the failure was invisible: [ConversionRouter] correctly - * claimed WebM, Ogg, WAV and AAC for Media3 while [Media3Engine] had no way to write any of them - * and produced MP4 regardless. Nothing failed — the file was simply the wrong container. + * These two drifted once already, and the failure was invisible in both directions: the router + * claimed WebM, Ogg, WAV and AAC-ADTS for Media3 — none of which Transformer can actually write — + * while [Media3Engine] ignored the container and produced MP4 for all of them. Two bugs that + * cancelled out, so nothing failed and the file was simply the wrong container. * * A JVM test rather than an instrumented one: constructing a factory touches no Android APIs, only * `create()` does, so the mapping is checkable without a device. @@ -46,53 +44,17 @@ class Media3MuxersTest { } } - @Test - fun `WebM advertises only the codecs its muxer accepts`() { - val factory = requireNotNull(Media3Muxers.factoryFor(Container.WEBM)) - - assertEquals( - listOf(MimeTypes.VIDEO_VP8, MimeTypes.VIDEO_VP9), - factory.getSupportedSampleMimeTypes(C.TRACK_TYPE_VIDEO), - ) - assertEquals( - listOf(MimeTypes.AUDIO_OPUS, MimeTypes.AUDIO_VORBIS), - factory.getSupportedSampleMimeTypes(C.TRACK_TYPE_AUDIO), - ) - } - /** - * The audio-only containers must not claim a video track. + * MP4 is the whole of it. * - * Transformer reads these lists to decide what it may hand the muxer. Claiming video for a - * container that cannot hold it turns a routing mistake into a corrupt file rather than a - * clean failure. + * Pinned as a value rather than left implicit, because `media3-muxer` shipping a `WebmMuxer`, + * `OggMuxer`, `WavMuxer` and `AacMuxer` makes it look like there should be five. Those four + * throw `UnsupportedOperationException` from `addMetadataEntry`, which `MuxerWrapper` calls + * unconditionally — see [Media3Muxers]. If a future Media3 release fixes that, this test is + * where the decision to widen the set gets made deliberately. */ @Test - fun `audio-only containers advertise no video codecs`() { - listOf(Container.OGG, Container.WAV, Container.AAC_ADTS).forEach { container -> - val factory = requireNotNull(Media3Muxers.factoryFor(container)) - assertTrue( - "$container claims video support", - factory.getSupportedSampleMimeTypes(C.TRACK_TYPE_VIDEO).isEmpty(), - ) - assertTrue( - "$container claims no audio support", - factory.getSupportedSampleMimeTypes(C.TRACK_TYPE_AUDIO).isNotEmpty(), - ) - } - } - - @Test - fun `WAV carries PCM and AAC-ADTS carries AAC`() { - assertEquals( - listOf(MimeTypes.AUDIO_RAW), - requireNotNull(Media3Muxers.factoryFor(Container.WAV)) - .getSupportedSampleMimeTypes(C.TRACK_TYPE_AUDIO), - ) - assertEquals( - listOf(MimeTypes.AUDIO_AAC), - requireNotNull(Media3Muxers.factoryFor(Container.AAC_ADTS)) - .getSupportedSampleMimeTypes(C.TRACK_TYPE_AUDIO), - ) + fun `Media3 can write MP4 and nothing else`() { + assertEquals(setOf(Container.MP4), ConversionRouter.MEDIA3_CONTAINERS) } } diff --git a/app/src/test/java/org/libremediaconverter/model/ConversionRouterTest.kt b/app/src/test/java/org/libremediaconverter/model/ConversionRouterTest.kt index 36ab736..a00432b 100644 --- a/app/src/test/java/org/libremediaconverter/model/ConversionRouterTest.kt +++ b/app/src/test/java/org/libremediaconverter/model/ConversionRouterTest.kt @@ -39,13 +39,16 @@ class ConversionRouterTest { } @Test - fun `webm vp9 routes to ffmpeg because media3 cannot encode vp9`() { - // Transformer.setVideoMimeType accepts only H.263/H.264/H.265/MP4V. The WebM - // muxer exists, but there is no VP9 *encoder* behind it, so producing WebM - // video is FFmpeg's job even though the container is nominally supported. + fun `webm vp9 routes to ffmpeg`() { + // Two independent reasons, either of which is sufficient: Transformer.setVideoMimeType + // accepts only H.263/H.264/H.265/MP4V, so there is no VP9 encoder — and Media3 cannot mux + // WebM at all, which it was previously credited with. The container check runs first and + // is the more fundamental of the two: even given a VP9 encoder, the file could not be + // written. This asserted NO_PLATFORM_ENCODER while the container was wrongly believed to + // be supported. val d = route(OutputFormat.WEBM_VP9) assertEquals(Engine.FFMPEG, d.engine) - assertEquals(Reason.NO_PLATFORM_ENCODER, d.reason) + assertEquals(Reason.CONTAINER_UNSUPPORTED, d.reason) } @Test @@ -173,6 +176,31 @@ class ConversionRouterTest { assertEquals(Reason.USER_FORCED_SOFTWARE, d.reason) } + // --- containers Media3 cannot write ------------------------------------- + + /** + * WAV, Opus and raw AAC used to be claimed for Media3 and are not any more. + * + * `media3-muxer` ships a muxer for each, which is why they were listed — but none can be driven + * by Transformer, so the export dies at the muxer. They were never actually produced on the + * hardware path: the engine ignored the container and wrote MP4 into a file named `.wav`. + * FFmpeg produces all three, and `FFmpegEngineTest` asserts the produced files. + */ + @Test + fun `audio containers Media3 cannot mux route to FFmpeg`() { + listOf(OutputFormat.WAV, OutputFormat.OPUS).forEach { format -> + val d = route(format) + assertEquals("$format should need FFmpeg", Engine.FFMPEG, d.engine) + assertEquals(Reason.CONTAINER_UNSUPPORTED, d.reason) + } + } + + /** M4A is the audio format that does stay on hardware, because its container is MP4. */ + @Test + fun `m4a stays on hardware because MP4 is a container Media3 can write`() { + assertEquals(Engine.MEDIA3, route(OutputFormat.M4A_AAC).engine) + } + // --- exhaustiveness ---------------------------------------------------- @Test -- 2.47.3