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..82577c7 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,35 @@ 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() + } + } + /** * Regression guard for the Transformer threading trap. * @@ -124,7 +155,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 +179,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..d9a65ac --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/convert/Media3Muxers.kt @@ -0,0 +1,60 @@ +package org.libremediaconverter.convert + +import androidx.media3.common.util.UnstableApi +import androidx.media3.muxer.Muxer +import androidx.media3.transformer.DefaultMuxer +import org.libremediaconverter.model.Container + +/** + * Which containers Media3 can actually write, and why it is only one. + * + * ## The four other muxers cannot be driven by Transformer + * + * `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: + * + * ``` + * 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 write it. + * + * 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 engine states its container rather + // than inheriting one, which is how the MP4-for-everything bug went unnoticed. + Container.MP4 -> DefaultMuxer.Factory() + + Container.WEBM, + Container.OGG, + Container.WAV, + Container.AAC_ADTS, + Container.MKV, + Container.MP3, + Container.GIF, + Container.IMAGE_SEQUENCE, + -> null + } +} 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..d23616b 100644 --- a/app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt +++ b/app/src/main/java/org/libremediaconverter/model/ConversionRouter.kt @@ -13,14 +13,19 @@ package org.libremediaconverter.model */ object ConversionRouter { - /** Containers Media3 can mux. Anything else has to go to FFmpeg. */ - private val MEDIA3_CONTAINERS = setOf( - Container.MP4, - Container.WEBM, - Container.OGG, - Container.WAV, - Container.AAC_ADTS, - ) + /** + * 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. + */ + 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/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..fed5dfd --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/Media3MuxersTest.kt @@ -0,0 +1,60 @@ +package org.libremediaconverter.convert + +import androidx.media3.common.util.UnstableApi +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +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 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. + */ +@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), + ) + } + } + + /** + * MP4 is the whole of it. + * + * 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 `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