From fd2bb1d889ab8ea22b9608df729c8c87eb5698d4 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 22:36:17 -0500 Subject: [PATCH 1/5] Test the three MediaProbe helpers nothing else would catch MediaProbe's MIME table, its image-demuxer rule and its Int reader are pure functions with no test at all, and each fails silently rather than loudly. shortName falls through to substringAfter('/') and reports a plausible-looking string that CodecNames may or may not still recognise, so a dropped arm turns a stream-copyable file into a re-encode. isImageFormat is checked before anything else in classify, so a wrong answer overrides both probes. intOr's runCatching is the only thing standing between a Float frame rate and losing every other track property the loop had read. Widen the three to internal, as #57 did, and say in each KDoc why the shape is what it is -- the _pipe suffix is not a substring test because yuv4mpegpipe is raw video, and getInteger casts rather than coerces. Every format name asserted came from ffprobe rather than from memory: a picked .png reports png_pipe, a .jpg reports jpeg_pipe, a .y4m reports yuv4mpegpipe. Co-Authored-By: Claude Opus 5 (1M context) --- .../libremediaconverter/convert/MediaProbe.kt | 46 +++++++- .../convert/MediaProbeImageFormatTest.kt | 81 +++++++++++++ .../convert/MediaProbeMimeNamesTest.kt | 108 ++++++++++++++++++ .../convert/MediaProbeTrackFieldsTest.kt | 79 +++++++++++++ 4 files changed, 309 insertions(+), 5 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/MediaProbeImageFormatTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/convert/MediaProbeMimeNamesTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/convert/MediaProbeTrackFieldsTest.kt diff --git a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt index f98cfec..de4ba3d 100644 --- a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt +++ b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt @@ -242,8 +242,20 @@ object MediaProbe { else -> Container.MKV } - /** FFprobe describes still images through the image demuxers rather than a media container. */ - private fun isImageFormat(formatName: String): Boolean { + /** + * FFprobe describes still images through the image demuxers rather than a media container. + * + * The two halves of the rule are not interchangeable. `image2` is a whole name — what FFprobe + * reports for a numbered image sequence — while `_pipe` has to be a *suffix* test, because the + * piped demuxers are named one per image codec: `png_pipe`, `jpeg_pipe`, `webp_pipe`, and + * thirty more. Relaxing that suffix to a substring would swallow `yuv4mpegpipe`, which is raw + * video, and `classify` checks this before anything else — so a false positive makes the + * source-info card describe a video as an image. + * + * `internal` so the unit tests can name both halves; the JVM test source set is a friend of + * `main`, so this stays invisible outside the module. + */ + internal fun isImageFormat(formatName: String): Boolean { val names = formatName.split(',').map { it.trim().lowercase() } return names.any { it == "image2" || it.endsWith("_pipe") } } @@ -284,11 +296,35 @@ object MediaProbe { } } - private fun MediaFormat.intOr(key: String, fallback: Int = 0): Int = + /** + * One track property as an Int, or [fallback] when the format has no Int to give. + * + * `containsKey` alone is not enough, because `MediaFormat` is a heterogeneous map: a key it + * holds as a Float answers `getInteger` with a `ClassCastException` rather than a coercion, and + * `KEY_FRAME_RATE` — which [probeForConcat] reads — is legitimately set either way. The + * `runCatching` is therefore load-bearing rather than defensive. Without it a single + * oddly-typed field throws past the whole track loop, and the catch there answers with an empty + * [ConcatInput], discarding the codec and dimensions that had already been read. + * + * `internal` for the unit tests, as [shortName]. + */ + internal fun MediaFormat.intOr(key: String, fallback: Int = 0): Int = if (containsKey(key)) runCatching { getInteger(key) }.getOrDefault(fallback) else fallback - /** MediaFormat MIME -> the short codec names the router and FFmpeg both speak. */ - private fun shortName(mime: String): String = when (mime) { + /** + * MediaFormat MIME -> the short codec names the router and FFmpeg both speak. + * + * A lookup table over platform constants is the shape that rots quietly. Most of these arms are + * translations rather than trimming — `video/avc` is `h264`, `audio/mp4a-latm` is `aac`, + * `video/x-vnd.on2.vp9` is `vp9` — so a dropped arm does not fail. It falls through to + * `substringAfter('/')` and reports a different, plausible-looking string that + * `CodecNames` may or may not still recognise, and an unrecognised codec is how a + * stream-copyable file quietly becomes a re-encode. + * + * `internal` so the unit tests can name every arm; the JVM test source set is a friend of + * `main`, so this stays invisible outside the module. + */ + internal fun shortName(mime: String): String = when (mime) { MediaFormat.MIMETYPE_VIDEO_AVC -> "h264" MediaFormat.MIMETYPE_VIDEO_HEVC -> "hevc" MediaFormat.MIMETYPE_VIDEO_VP8 -> "vp8" diff --git a/app/src/test/java/org/libremediaconverter/convert/MediaProbeImageFormatTest.kt b/app/src/test/java/org/libremediaconverter/convert/MediaProbeImageFormatTest.kt new file mode 100644 index 0000000..2fe9e78 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MediaProbeImageFormatTest.kt @@ -0,0 +1,81 @@ +package org.libremediaconverter.convert + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The image-demuxer rule, which looks arbitrary until it is read as a suffix. + * + * `MediaProbe.classify` asks [MediaProbe.isImageFormat] before anything else, so this one boolean + * overrides everything both probes found: true and the source-info card says "Image" and a size, + * false and it says container, codec and length. Neither mistake fails loudly. + * + * The rule has two halves and they are not the same shape. `image2` is a whole format name — + * FFprobe reports it for a numbered image sequence — while the piped demuxers are named one per + * image codec, so `_pipe` has to be matched as a *suffix*: `png_pipe`, `jpeg_pipe`, `webp_pipe` + * and some thirty more. Widening that suffix to a substring is the tempting simplification and it + * is wrong, because `yuv4mpegpipe` is raw video. + * + * Every format name asserted here was read back from `ffprobe -show_entries format=format_name` + * rather than guessed: a picked `.png` reports `png_pipe`, a picked `.jpg` reports `jpeg_pipe`, a + * `.y4m` reports `yuv4mpegpipe`, and `image2` needs the demuxer named explicitly. + * + * One known gap, deliberately not asserted either way: `image2pipe` is a real FFprobe format name + * for an image read from a stream, and this rule answers false for it — it is not `image2` and + * does not end in `_pipe`. Whether that is worth fixing is a question about picked-file behaviour + * on a device, not something to settle by pinning today's answer here. + */ +class MediaProbeImageFormatTest { + + @Test + fun `a numbered image sequence is an image`() { + assertTrue(MediaProbe.isImageFormat("image2")) + } + + /** What a picked PNG or JPEG actually reports, and the reason the suffix rule exists. */ + @Test + fun `the per-codec piped demuxers are images`() { + assertTrue(MediaProbe.isImageFormat("png_pipe")) + assertTrue(MediaProbe.isImageFormat("jpeg_pipe")) + assertTrue(MediaProbe.isImageFormat("webp_pipe")) + } + + /** + * The half that a substring match would break. + * + * `yuv4mpegpipe` contains `pipe` and is not an image: it is raw uncompressed video, and + * describing it as an image would hide its codec, its size and its length from the card while + * leaving the file perfectly convertible. + */ + @Test + fun `a format that merely contains pipe is not an image`() { + assertFalse(MediaProbe.isImageFormat("yuv4mpegpipe")) + } + + /** The ordinary media containers, which is what the false answer is mostly for. */ + @Test + fun `a real container is not an image`() { + assertFalse(MediaProbe.isImageFormat("mov,mp4,m4a,3gp,3g2,mj2")) + assertFalse(MediaProbe.isImageFormat("matroska,webm")) + assertFalse(MediaProbe.isImageFormat("mp3")) + } + + /** + * FFprobe names every format sharing the demuxer, so the entry that matters can be anywhere in + * the list — and the padding and case are normalised the same way [MediaProbe.containerFrom] + * normalises them. + */ + @Test + fun `an image entry is found anywhere in the list, whatever its spacing or case`() { + assertTrue(MediaProbe.isImageFormat("PNG_PIPE")) + assertTrue(MediaProbe.isImageFormat(" image2 ")) + assertTrue(MediaProbe.isImageFormat("something_else, tiff_pipe")) + } + + /** Nothing to go on is not an image; the card falls back to describing an unknown container. */ + @Test + fun `an empty format name is not an image`() { + assertFalse(MediaProbe.isImageFormat("")) + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/MediaProbeMimeNamesTest.kt b/app/src/test/java/org/libremediaconverter/convert/MediaProbeMimeNamesTest.kt new file mode 100644 index 0000000..d3953f4 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MediaProbeMimeNamesTest.kt @@ -0,0 +1,108 @@ +package org.libremediaconverter.convert + +import android.media.MediaFormat +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * The MIME -> short codec name table, which nothing downstream would notice going wrong. + * + * `MediaExtractor` answers in platform MIME spellings; the router, the copy planner and the + * source-info card all speak FFmpeg's short names. [MediaProbe.shortName] is the one place those + * two vocabularies meet, and most of its arms are translations rather than trimming — `video/avc` + * is `h264`, `audio/mp4a-latm` is `aac`, `video/x-vnd.on2.vp9` is `vp9`. + * + * So a dropped or mistyped arm does not throw. It falls through to `substringAfter('/')` and + * reports a different, entirely plausible-looking string. `CodecNames` carries alias lists that + * happen to rescue some of those (`avc`, `av01`, `raw`) and not others (`mp4a-latm`, + * `x-vnd.on2.vp9`), which is exactly why leaning on the rescue is not a plan: an unrecognised + * codec is how a stream-copyable file quietly becomes a re-encode, and how the card ends up naming + * a codec no user has heard of. This table is the only place those arms are pinned. + * + * A plain JVM test rather than Robolectric: `MediaFormat.MIMETYPE_*` are Java compile-time String + * constants, so this test and `MediaProbe` alike carry the literals in their own bytecode and the + * framework class is never loaded. + * + * Every case names its MIME in the failure message, because the MIME is the thing that has to be + * looked up when one of these goes red. + */ +class MediaProbeMimeNamesTest { + + @Test + fun `an AVC track is reported as h264, which is what everything downstream calls it`() { + assertShortName("h264", MediaFormat.MIMETYPE_VIDEO_AVC) + } + + /** On2's vendor MIME looks nothing like the codec name FFmpeg and the router use. */ + @Test + fun `the VP8 and VP9 vendor MIMEs are reported without their vendor prefix`() { + assertShortName("vp8", MediaFormat.MIMETYPE_VIDEO_VP8) + assertShortName("vp9", MediaFormat.MIMETYPE_VIDEO_VP9) + } + + @Test + fun `AV1 and MPEG-4 are reported by codec name rather than by MIME spelling`() { + assertShortName("av1", MediaFormat.MIMETYPE_VIDEO_AV1) + assertShortName("mpeg4", MediaFormat.MIMETYPE_VIDEO_MPEG4) + } + + @Test + fun `an AAC track is reported as aac, not as the mp4a-latm its MIME says`() { + assertShortName("aac", MediaFormat.MIMETYPE_AUDIO_AAC) + } + + @Test + fun `uncompressed audio is reported as pcm, which is not what its MIME says either`() { + assertShortName("pcm", MediaFormat.MIMETYPE_AUDIO_RAW) + } + + /** + * Four arms produce exactly what the fallback would produce anyway. + * + * `video/hevc` -> `hevc`, `audio/opus` -> `opus`, `audio/flac` -> `flac`, + * `audio/vorbis` -> `vorbis`: for these the `when` arm and `substringAfter('/')` agree, so + * deleting the arm changes no observable behaviour and no test can catch it. That is a + * property of the code rather than a gap here, and it is reported as such rather than dressed + * up as coverage. The assertions still earn their place — they pin the promise the router is + * given (`hevc`, whatever the MIME happens to spell) against a later edit that changes the + * mapping rather than deleting it. + */ + @Test + fun `the arms whose MIME subtype already is the short name still map to it`() { + assertShortName("hevc", MediaFormat.MIMETYPE_VIDEO_HEVC) + assertShortName("opus", MediaFormat.MIMETYPE_AUDIO_OPUS) + assertShortName("flac", MediaFormat.MIMETYPE_AUDIO_FLAC) + assertShortName("vorbis", MediaFormat.MIMETYPE_AUDIO_VORBIS) + } + + /** + * The fallback, which is what makes an unlisted codec describable at all. + * + * These are real `MediaFormat` MIMEs with no arm of their own. Dropping the subtype is the + * right guess far more often than reporting the whole MIME would be — FFprobe calls the first + * of these `ac3` too. + */ + @Test + fun `a MIME with no arm of its own falls back to its subtype`() { + assertShortName("ac3", MediaFormat.MIMETYPE_AUDIO_AC3) + assertShortName("mpeg2", MediaFormat.MIMETYPE_VIDEO_MPEG2) + assertShortName("dolby-vision", MediaFormat.MIMETYPE_VIDEO_DOLBY_VISION) + } + + /** + * The surprising half of `substringAfter`'s contract, pinned deliberately. + * + * With no `/` in the string it returns the whole input rather than the empty string. Today's + * callers gate on a `video/` or `audio/` prefix so they cannot reach this, but "report what + * you were given" rather than "report nothing" is what would keep a malformed MIME visible on + * the card instead of blank. + */ + @Test + fun `a MIME with no subtype separator is reported unchanged`() { + assertShortName("weird", "weird") + assertShortName("", "") + } + + private fun assertShortName(expected: String, mime: String) = + assertEquals("shortName(\"$mime\")", expected, MediaProbe.shortName(mime)) +} diff --git a/app/src/test/java/org/libremediaconverter/convert/MediaProbeTrackFieldsTest.kt b/app/src/test/java/org/libremediaconverter/convert/MediaProbeTrackFieldsTest.kt new file mode 100644 index 0000000..728ab27 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MediaProbeTrackFieldsTest.kt @@ -0,0 +1,79 @@ +package org.libremediaconverter.convert + +import android.media.MediaFormat +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Reading Int track properties out of a `MediaFormat`, which is a heterogeneous map. + * + * [MediaProbe.intOr] guards two different failures with one expression, and only one of them is + * obvious. A key the format does not carry is the easy half. The other is a key it *does* carry + * with a value of another type: `getInteger` casts rather than coerces, so a frame rate stored as + * a Float answers with a `ClassCastException`. `probeForConcat` reads `KEY_FRAME_RATE`, which the + * platform accepts either way, and its `catch` sits outside the track loop — so without the + * `runCatching` one oddly-typed field would discard the codec and dimensions already read from + * that file and the join would re-encode for no reason. + * + * Robolectric rather than a plain JVM test, unlike the two sibling `MediaProbe` helper tests: this + * one needs a real `MediaFormat` instance, not just its compile-time String constants. + */ +@RunWith(RobolectricTestRunner::class) +class MediaProbeTrackFieldsTest { + + @Test + fun `a property the format carries as an Int is read`() { + val format = videoFormat() + + assertEquals(1920, with(MediaProbe) { format.intOr(MediaFormat.KEY_WIDTH) }) + assertEquals(1080, with(MediaProbe) { format.intOr(MediaFormat.KEY_HEIGHT) }) + } + + /** + * A track that simply does not say. `MediaExtractor` omits `KEY_FRAME_RATE` for plenty of real + * files, and 0 is what `ConcatPlanner` reads as "cannot prove a match". + */ + @Test + fun `a key the format does not carry gives the fallback`() { + val format = videoFormat() + + assertEquals(0, with(MediaProbe) { format.intOr(MediaFormat.KEY_FRAME_RATE) }) + assertEquals(-1, with(MediaProbe) { format.intOr(MediaFormat.KEY_FRAME_RATE, -1) }) + } + + /** + * The premise of the `runCatching`, pinned against the platform rather than assumed. + * + * If `getInteger` coerced a Float instead of throwing, the guard below would be testing + * nothing at all — so the throw is asserted directly first. + */ + @Test + fun `getInteger refuses a Float rather than coercing it`() { + val format = videoFormat() + format.setFloat(MediaFormat.KEY_FRAME_RATE, NON_INTEGRAL_FRAME_RATE) + + val thrown = runCatching { format.getInteger(MediaFormat.KEY_FRAME_RATE) }.exceptionOrNull() + + assertTrue("expected getInteger to refuse a Float, got $thrown", thrown is ClassCastException) + } + + /** And that refusal is answered with the fallback, not passed on to the caller. */ + @Test + fun `a frame rate the format carries as a Float gives the fallback rather than throwing`() { + val format = videoFormat() + format.setFloat(MediaFormat.KEY_FRAME_RATE, NON_INTEGRAL_FRAME_RATE) + + assertEquals(0, with(MediaProbe) { format.intOr(MediaFormat.KEY_FRAME_RATE) }) + assertEquals(-1, with(MediaProbe) { format.intOr(MediaFormat.KEY_FRAME_RATE, -1) }) + } + + private fun videoFormat(): MediaFormat = MediaFormat.createVideoFormat(MediaFormat.MIMETYPE_VIDEO_AVC, 1920, 1080) + + private companion object { + /** NTSC's 30000/1001, the frame rate that cannot be stored as an Int in the first place. */ + const val NON_INTEGRAL_FRAME_RATE = 29.97f + } +} From 5ec2bba64b16c1939b85060c01b4df8825df67ab Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 22:43:05 -0500 Subject: [PATCH 2/5] Check the MIME types Media3Engine hands Transformer, and the claim above them Both tables decide what codec ends up in the user's file, and neither was exercised. Point H265 at VIDEO_H264 and every hardware HEVC export writes H.264 into a file the user asked to be H.265: Transformer does as told, the export succeeds, and the only symptom is a codec nobody chose. One arm carried an assertion rather than a value -- "Never reached: only an Encode plan consults this, and COPY/NONE are not Encode" -- which is a claim about callers parked in a branch of a callee. It is true, and nothing checked it, so it would have gone on reading as true after it stopped being. Proved instead: CopyPlanner answers both codecs before the Encode branch and its fallback draws from ContainerCapabilities.encodableVideo, which contains neither, so a sweep over every spec the planner can be handed asserts no Encode plan carries COPY or NONE. Counters guard the sweep, because `as? Encode ?: let` asserts nothing at all for a Drop or Copy plan. The audio sibling claim did not survive intact. "MP3 and FLAC have no Android encoder; the router routes them to FFmpeg" is true and incomplete: one rule, `audioEncode !in MEDIA3_AUDIO`, diverts Vorbis by identical logic, so three of the six encodable codecs never reach the table. VORBIS -> AUDIO_VORBIS is a correct mapping for a request Transformer is never given. The arm stays -- a right answer in unreachable code costs nothing -- and the comment now says so. The tables are asked of the router's decisions rather than of its codec sets, because the comments claim behaviour and a set can be right while the rule reading it is wrong. Both move to an internal companion object so a JVM test can reach them without constructing an engine, which would start a real HandlerThread to answer an enum lookup; #57's precedent, and the JVM test source set is a friend of main. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/Media3Engine.kt | 77 +++-- .../convert/Media3EngineMimeTypesTest.kt | 287 ++++++++++++++++++ 2 files changed, 338 insertions(+), 26 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/Media3EngineMimeTypesTest.kt diff --git a/app/src/main/java/org/libremediaconverter/convert/Media3Engine.kt b/app/src/main/java/org/libremediaconverter/convert/Media3Engine.kt index fb25f71..8ade749 100644 --- a/app/src/main/java/org/libremediaconverter/convert/Media3Engine.kt +++ b/app/src/main/java/org/libremediaconverter/convert/Media3Engine.kt @@ -138,31 +138,6 @@ class Media3Engine(private val context: Context) : HardwareTranscoder { .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 - // Never reached: only an Encode plan consults this, and COPY/NONE are not Encode. - VideoCodec.COPY, 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.COPY, 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. * @@ -192,7 +167,57 @@ class Media3Engine(private val context: Context) : HardwareTranscoder { thread.quitSafely() } - private companion object { + /** + * The progress interval, and the two enum-to-MIME tables. + * + * The tables are pure functions of a codec enum, so they sit here rather than on the instance: + * a JVM test can then exercise every arm without constructing an engine, which would start a + * real [HandlerThread] to answer a lookup. `internal` rather than `private` for the reason + * `MainActivity`'s `Destination` records — the JVM test source set is a friend of `main`, so + * these stay invisible to anything outside the module. + */ + internal companion object { const val PROGRESS_INTERVAL_MS = 250L + + /** + * 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 as the old mapping did. + */ + internal fun videoMimeTypeFor(codec: VideoCodec): String? = when (codec) { + VideoCodec.H264 -> MimeTypes.VIDEO_H264 + VideoCodec.H265 -> MimeTypes.VIDEO_H265 + // Never reached, and no longer only asserted: `Media3EngineMimeTypesTest` drives + // `CopyPlanner` over every spec it can be handed and shows that no Encode plan carries + // either, which is what turns "COPY/NONE are not Encode" into a checked claim. + VideoCodec.COPY, VideoCodec.NONE -> null + VideoCodec.VP8, VideoCodec.VP9, VideoCodec.AV1 -> null + } + + /** + * Media3 encodes AAC, Opus and PCM. Three arms below are dead, not two. + * + * The comment this replaces named MP3 and FLAC as the exceptions, which reads as though + * every other arm were live. **Vorbis is not.** A single router rule diverts every audio + * codec outside {AAC, Opus, PCM} to FFmpeg, and Vorbis is outside it, so + * `VORBIS -> AUDIO_VORBIS` names a MIME type Transformer is never actually asked for. + * + * The arm stays because the mapping is correct — deleting a right answer out of + * unreachable code buys nothing — but it is an entry waiting on a routing change rather + * than a live one. `Media3EngineMimeTypesTest` routes all six encodable codecs and asserts + * which three arrive, so if that set moves, the disagreement fails rather than surprises. + */ + internal 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.COPY, AudioCodec.NONE -> null + // MP3 and FLAC have no Android encoder at any API level, so the router sends them to + // FFmpeg before an encoder is ever asked for. + AudioCodec.MP3, AudioCodec.FLAC -> null + } } } diff --git a/app/src/test/java/org/libremediaconverter/convert/Media3EngineMimeTypesTest.kt b/app/src/test/java/org/libremediaconverter/convert/Media3EngineMimeTypesTest.kt new file mode 100644 index 0000000..7e4c2f4 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/Media3EngineMimeTypesTest.kt @@ -0,0 +1,287 @@ +package org.libremediaconverter.convert + +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.assertTrue +import org.junit.Test +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.AudioPlan +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.ConversionPlan +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.ConversionRouter +import org.libremediaconverter.model.CopyPlanner +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.Engine +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputSpec +import org.libremediaconverter.model.VideoCodec +import org.libremediaconverter.model.VideoPlan + +/** + * Guards [Media3Engine]'s two enum-to-MIME tables and the claims written above them. + * + * The defect: neither table was exercised at all, so nothing stood between a wrong entry and the + * user's file. Point `H265` at `VIDEO_H264` and every hardware HEVC export writes H.264 into a + * file the user asked to be H.265 — Transformer does exactly as told, the export succeeds, and + * the only symptom is a codec nobody chose. + * + * Worse, one arm carried an assertion instead of a value: + * + * ``` + * // Never reached: only an Encode plan consults this, and COPY/NONE are not Encode. + * ``` + * + * That is a claim about *callers* parked in a branch of a callee. It happens to be true, and + * nothing whatsoever checked it, so it would have gone on reading as true after it stopped being. + * + * Three kinds of test, because arm-by-arm equality alone would only pin today's answers: + * + * 1. Every arm of both tables, nulls included. + * 2. The "never reached" claim, proved over every plan [CopyPlanner] can produce. + * 3. The tables against [ConversionRouter]'s actual decisions rather than against its codec sets — + * the comments claim behaviour ("the router routes them to FFmpeg"), and a set can be right + * while the rule that reads it is wrong. + * + * A JVM test rather than an instrumented one: both tables take an enum and return a constant. + */ +@UnstableApi +class Media3EngineMimeTypesTest { + + @Test + fun `every video codec maps to the MIME type Transformer will be given`() { + assertEquals( + "EXPECTED_VIDEO_MIME must name every VideoCodec, so a new one cannot arrive untested", + VideoCodec.entries.toSet(), + EXPECTED_VIDEO_MIME.keys, + ) + VideoCodec.entries.forEach { codec -> + assertEquals( + "videoMimeTypeFor(${codec.label})", + EXPECTED_VIDEO_MIME.getValue(codec), + Media3Engine.videoMimeTypeFor(codec), + ) + } + } + + @Test + fun `every audio codec maps to the MIME type Transformer will be given`() { + assertEquals( + "EXPECTED_AUDIO_MIME must name every AudioCodec, so a new one cannot arrive untested", + AudioCodec.entries.toSet(), + EXPECTED_AUDIO_MIME.keys, + ) + AudioCodec.entries.forEach { codec -> + assertEquals( + "audioMimeTypeFor(${codec.label})", + EXPECTED_AUDIO_MIME.getValue(codec), + Media3Engine.audioMimeTypeFor(codec), + ) + } + } + + /** + * The "never reached" claim, proved rather than repeated. + * + * [Media3Engine] asks these tables only for `plan.video as? VideoPlan.Encode`, and every plan + * it sees comes from [CopyPlanner]. So the claim reduces to a property of the planner: over + * every spec it can be handed, an `Encode` never carries `COPY` or `NONE`. That holds because + * both codecs are answered before the `Encode` branch, and the fallback draws from + * `ContainerCapabilities.encodableVideo`, which contains neither — but this asserts it instead + * of trusting the reading. + * + * The counters are not decoration. `(plan.video as? VideoPlan.Encode)?.let { ... }` asserts + * nothing at all for a `Drop` or `Copy` plan, so a sweep that stopped producing `Encode` plans + * would stay green while checking nothing. + */ + @Test + fun `no plan CopyPlanner can produce carries COPY or NONE inside an Encode`() { + var videoEncodes = 0 + var audioEncodes = 0 + everyPlan().forEach { (spec, probe, plan) -> + (plan.video as? VideoPlan.Encode)?.let { + videoEncodes++ + assertTrue( + "CopyPlanner produced VideoPlan.Encode(${it.codec}) for $spec against $probe", + it.codec != VideoCodec.COPY && it.codec != VideoCodec.NONE, + ) + } + (plan.audio as? AudioPlan.Encode)?.let { + audioEncodes++ + assertTrue( + "CopyPlanner produced AudioPlan.Encode(${it.codec}) for $spec against $probe", + it.codec != AudioCodec.COPY && it.codec != AudioCodec.NONE, + ) + } + } + assertTrue("the sweep produced no video Encode plan, so it asserted nothing", videoEncodes > 0) + assertTrue("the sweep produced no audio Encode plan, so it asserted nothing", audioEncodes > 0) + } + + /** + * The video table's other claim: VP8, VP9 and AV1 targets "never reach here". + * + * Asked of the router rather than of its private codec set, so the rule is what is under test. + */ + @Test + fun `the router sends exactly H264 and H265 video encodes to Media3`() { + val onMedia3 = REAL_VIDEO_CODECS.filter { engineForVideoEncode(it) == Engine.MEDIA3 } + assertEquals(listOf(VideoCodec.H264, VideoCodec.H265), onMedia3) + } + + /** + * The audio table's sibling claim, and where it turned out to be incomplete. + * + * The comment named MP3 and FLAC. One rule — `audioEncode !in MEDIA3_AUDIO` — diverts Vorbis + * by exactly the same logic, so three of the six encodable codecs never reach the table, not + * two. Asserted as the whole set rather than as two memberships, which is what makes the + * omission visible. + */ + @Test + fun `the router keeps MP3 FLAC and Vorbis audio encodes off Media3`() { + val onMedia3 = REAL_AUDIO_CODECS.filter { engineForAudioEncode(it) == Engine.MEDIA3 } + assertEquals(listOf(AudioCodec.AAC, AudioCodec.OPUS, AudioCodec.PCM), onMedia3) + } + + /** + * The binding that makes the two halves above one test rather than two coincidences. + * + * A codec the router starts sending to Media3 must have a MIME type here, or Transformer is + * left to pick its own and the user gets a codec they did not choose. + */ + @Test + fun `every codec the router sends to Media3 has a MIME type`() { + REAL_VIDEO_CODECS.filter { engineForVideoEncode(it) == Engine.MEDIA3 }.forEach { codec -> + assertNotNull( + "${codec.label} is routed to Media3 but videoMimeTypeFor returns null", + Media3Engine.videoMimeTypeFor(codec), + ) + } + REAL_AUDIO_CODECS.filter { engineForAudioEncode(it) == Engine.MEDIA3 }.forEach { codec -> + assertNotNull( + "${codec.label} is routed to Media3 but audioMimeTypeFor returns null", + Media3Engine.audioMimeTypeFor(codec), + ) + } + } + + /** + * The reverse direction, which holds for video and not for audio. + * + * Every video codec the router withholds has a null entry, so that table is exactly the set of + * codecs Media3 is asked to encode. Audio has one entry more than the router will ever use: + * `VORBIS -> AUDIO_VORBIS` is correct and unreachable. Pinned deliberately — if a routing + * change makes Vorbis live, this is the test that says the arm above stopped being dead. + */ + @Test + fun `Vorbis is the one MIME type the router never asks for`() { + REAL_VIDEO_CODECS.filter { engineForVideoEncode(it) == Engine.FFMPEG }.forEach { codec -> + assertEquals( + "${codec.label} never reaches Media3, so it must not name a MIME type", + null, + Media3Engine.videoMimeTypeFor(codec), + ) + } + val namedButUnrouted = REAL_AUDIO_CODECS + .filter { Media3Engine.audioMimeTypeFor(it) != null } + .filter { engineForAudioEncode(it) == Engine.FFMPEG } + assertEquals(listOf(AudioCodec.VORBIS), namedButUnrouted) + assertEquals(MimeTypes.AUDIO_VORBIS, Media3Engine.audioMimeTypeFor(AudioCodec.VORBIS)) + } + + /** + * Routes a video-only re-encode to [codec] and reports the engine chosen. + * + * `mpeg2video` is the load-bearing detail: [CopyPlanner] upgrades a request to a stream copy + * when the source codec matches, and a `Copy` plan would answer a different question. A name + * `CodecNames` cannot resolve forces an `Encode` for every codec, which the assertion pins so + * that a planner change cannot quietly turn this sweep into a sweep of `Copy` plans. + */ + private fun engineForVideoEncode(codec: VideoCodec): Engine { + val request = ConversionRequest( + spec = OutputSpec(Container.MP4, codec, AudioCodec.NONE), + probe = InputProbe(videoCodec = "mpeg2video", container = Container.MKV), + ) + assertEquals( + "this request no longer plans a video Encode, so its engine says nothing about $codec", + VideoPlan.Encode(codec), + CopyPlanner.plan(request.spec, request.probe).video, + ) + return ConversionRouter.route(request, DeviceCodecs.PERMISSIVE).engine + } + + /** The audio counterpart. `ac3` is unresolvable for the same reason `mpeg2video` is. */ + private fun engineForAudioEncode(codec: AudioCodec): Engine { + val request = ConversionRequest( + spec = OutputSpec(Container.MP4, VideoCodec.NONE, codec), + probe = InputProbe(audioCodec = "ac3", hasVideo = false, container = Container.MKV), + ) + assertEquals( + "this request no longer plans an audio Encode, so its engine says nothing about $codec", + AudioPlan.Encode(codec), + CopyPlanner.plan(request.spec, request.probe).audio, + ) + return ConversionRouter.route(request, DeviceCodecs.PERMISSIVE).engine + } + + private fun everyPlan(): List> = + ALL_SPECS.flatMap { spec -> PROBES.map { Triple(spec, it, CopyPlanner.plan(spec, it)) } } + + private companion object { + + /** Every arm of `videoMimeTypeFor`, including the ones the tests above prove unreachable. */ + val EXPECTED_VIDEO_MIME: Map = mapOf( + VideoCodec.H264 to MimeTypes.VIDEO_H264, + VideoCodec.H265 to MimeTypes.VIDEO_H265, + VideoCodec.VP8 to null, + VideoCodec.VP9 to null, + VideoCodec.AV1 to null, + // Unreachable, and asserted anyway: the proof lives in another test, and a reader + // deleting these would leave the arms themselves unexercised. + VideoCodec.COPY to null, + VideoCodec.NONE to null, + ) + + val EXPECTED_AUDIO_MIME: Map = mapOf( + AudioCodec.AAC to MimeTypes.AUDIO_AAC, + AudioCodec.OPUS to MimeTypes.AUDIO_OPUS, + AudioCodec.VORBIS to MimeTypes.AUDIO_VORBIS, + AudioCodec.PCM to MimeTypes.AUDIO_RAW, + AudioCodec.MP3 to null, + AudioCodec.FLAC to null, + AudioCodec.COPY to null, + AudioCodec.NONE to null, + ) + + /** Codecs a user can actually ask to be produced: `COPY` and `NONE` are instructions. */ + val REAL_VIDEO_CODECS = VideoCodec.entries - VideoCodec.COPY - VideoCodec.NONE + val REAL_AUDIO_CODECS = AudioCodec.entries - AudioCodec.COPY - AudioCodec.NONE + + /** Every output a spec can name — 15 containers by 7 video codecs by 8 audio codecs. */ + val ALL_SPECS: List = Container.entries.flatMap { container -> + VideoCodec.entries.flatMap { video -> + AudioCodec.entries.map { audio -> OutputSpec(container, video, audio) } + } + } + + /** Inputs chosen to reach each of [CopyPlanner]'s branches. */ + val PROBES = listOf( + // Nothing known about the source at all. + InputProbe(), + // Identified, and the container changes: the copy upgrade applies. + InputProbe(videoCodec = "h264", audioCodec = "aac", container = Container.MKV), + // Identified, container unchanged: the copy upgrade deliberately does not apply. + InputProbe(videoCodec = "h264", audioCodec = "aac", container = Container.MP4), + // Copyable but not encodable by either engine — the fallback's reason for existing. + InputProbe(videoCodec = "av1", audioCodec = "flac", container = Container.MKV), + // Real codecs this app cannot name, so a copy is never proven safe. + InputProbe(videoCodec = "mpeg2video", audioCodec = "ac3", container = Container.AVI), + // The platform extractor could not open it. + InputProbe(videoCodec = InputProbe.UNPARSEABLE), + // Audio only. + InputProbe(videoCodec = null, audioCodec = "opus", hasVideo = false, container = Container.OGG), + ) + } +} From 7f951baf8f275c28103589f1551294aef8d3851f Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 22:44:55 -0500 Subject: [PATCH 3/5] Make the two codec tables answer for each other, and stop describeAudio printing a NUL The FFprobe codec vocabulary is written out in at least four places and none of them had a test. Two had already drifted apart. `x264`, `hev1`, `x265` and `vp09` resolved in `CodecNames.videoFromName` and returned null from `AndroidDeviceCodecs.mimeForCodecName`, so the app identified the codec for the source card and for routing and then ran the device capability check blind on the same string; `mpeg4` ran the other way and rendered as a raw name. Nothing could notice, and the reason is structural: a `when` cannot be enumerated, so no test can ask one table what the other one knows. Both are maps now, for that reason alone, and `CodecVocabularyTest` walks the two key sets. A name added to -- or removed from -- one side alone fails the build. The one legitimate asymmetry is listed rather than implied: `mpeg4` is decodable input with no `VideoCodec` to name it, so `CodecNames` is right not to carry it. That list is itself checked, because otherwise it is an escape hatch -- any future divergence could be waved through by adding the name to it, and adding `x265` to it now fails. THIS CHANGES BEHAVIOUR for `x264`, `hev1`, `x265` and `vp09`. A null from `mimeForCodecName` means "unknown to us: assume the platform can handle it and let a failed export trigger the FFmpeg fallback", which is the right policy for a name nobody recognises and the wrong one for a name recognised one file over. A device without the matching decoder now sends those four to FFmpeg up front instead of spending a doomed hardware attempt to discover it. No input loses hardware it could have used: each alias resolves to the MIME its canonical spelling already resolved to, so a device that has the decoder still answers true. `ConversionRouterTest` still passes and that is not evidence either way -- every `canDecode` in it is a hand-written stub that never reaches this table. #74 is the same family one level down. `describeVideo` answered "Unrecognised" for `InputProbe.UNPARSEABLE` and `describeAudio` had no such arm, so an unparseable audio codec would have fallen through to `?: name` -- and the sentinel opens with a NUL, so the source-info card would have rendered a `Text` beginning with U+0000. The two now share one body, which is what stops the next arm being added to one side only. Two corrections to that ticket, taken from the file rather than from the ticket, since it warns about exactly this: - It quotes `audioFromName` as opening with `null, InputProbe.UNPARSEABLE -> null`. It did not; it opened with `null -> null` and the sentinel reached `else`. Naming the sentinel in the shared lookup therefore changes no answer and is documentation, not the fix. - It says `describeVideo`'s arm has no test of its own. It did -- `descriptions stay readable for unknown and missing codecs` asserts it -- so deleting the shared arm now reddens three tests across both sides, not one. Mutations run, each on the full 386-test suite: add "avc3" to CodecNames only -> CodecVocabularyTest red on two counts, CodecNamesTest green: 8 tests, 0 failures, which is the ticket's point about per-table arm tests delete the UNPARSEABLE arm -> CodecNamesTest red on three, one of them quoting the NUL back add "x265" to DECODE_ONLY_NAMES -> CodecVocabularyTest red on the escape hatch delete "vp09" from the MIME map -> CodecVocabularyTest red on three, which is the state this commit is fixing Audio is not cross-checked, and that is a gap rather than a decision: the device capability check is video-only, so this module has no second audio table to compare `AUDIO_ALIASES` against. `Media3Engine.audioMimeTypeFor` is the other half and belongs to #85. `MediaProbe.shortName` (#84) is the fourth table and is untouched here for the same reason. Closes #87. Closes #74. Co-Authored-By: Claude Opus 5 (1M context) --- .../codec/AndroidDeviceCodecs.kt | 76 ++++++++-- .../libremediaconverter/model/CodecNames.kt | 113 ++++++++++---- .../codec/CodecVocabularyTest.kt | 143 ++++++++++++++++++ .../model/CodecNamesTest.kt | 59 ++++++++ 4 files changed, 351 insertions(+), 40 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/codec/CodecVocabularyTest.kt diff --git a/app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt b/app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt index 85475a3..0b0052a 100644 --- a/app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt +++ b/app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt @@ -75,7 +75,13 @@ class AndroidDeviceCodecs private constructor( return AndroidDeviceCodecs(encoders, decoders) } - private fun mimeFor(codec: VideoCodec): String? = when (codec) { + /** + * `internal` rather than `private` so the cross-check test can ask what a [VideoCodec] + * means here and compare it with what [NAME_TO_MIME] says the same codec's names mean. + * The JVM test source set is a friend of `main`, so this stays invisible outside the + * module — the precedent is `MainActivity`'s `Destination`. + */ + internal fun mimeFor(codec: VideoCodec): String? = when (codec) { VideoCodec.H264 -> MediaFormat.MIMETYPE_VIDEO_AVC VideoCodec.H265 -> MediaFormat.MIMETYPE_VIDEO_HEVC VideoCodec.VP8 -> MediaFormat.MIMETYPE_VIDEO_VP8 @@ -87,20 +93,62 @@ class AndroidDeviceCodecs private constructor( VideoCodec.COPY, VideoCodec.NONE -> null } - /** Maps an FFprobe-style codec name onto a MediaFormat MIME type. */ - private fun mimeForCodecName(name: String): String? = when (name.lowercase()) { - "h264", "avc", "avc1" -> MediaFormat.MIMETYPE_VIDEO_AVC - "hevc", "h265", "hvc1" -> MediaFormat.MIMETYPE_VIDEO_HEVC - "vp8" -> MediaFormat.MIMETYPE_VIDEO_VP8 - "vp9" -> MediaFormat.MIMETYPE_VIDEO_VP9 - "av1", "av01" -> MediaFormat.MIMETYPE_VIDEO_AV1 - "mpeg4" -> MediaFormat.MIMETYPE_VIDEO_MPEG4 - // Unknown to us: assume the platform can handle it and let a failed export - // trigger the FFmpeg fallback, rather than pre-emptively refusing hardware. - else -> null - } + /** + * FFprobe-style codec names, and the MediaFormat MIME type each one asks about. + * + * This is the same vocabulary `CodecNames.VIDEO_ALIASES` holds, written out a second time + * because this side has to answer in platform MIME types and `model` does not depend on + * Android. Two copies of one vocabulary drift, and these had: `x264`, `hev1`, `x265` and + * `vp09` resolved for display and routing and fell through to null here, so the app ran + * the capability check blind on inputs it had already identified (#87). They are listed + * now, which **changes behaviour** for those four names — see [mimeForCodecName]. + * + * A map rather than a `when` because a `when` cannot be enumerated, and `CodecVocabularyTest` + * has to walk both key sets to notice the next divergence. + */ + internal val NAME_TO_MIME: Map = mapOf( + "h264" to MediaFormat.MIMETYPE_VIDEO_AVC, + "avc" to MediaFormat.MIMETYPE_VIDEO_AVC, + "avc1" to MediaFormat.MIMETYPE_VIDEO_AVC, + "x264" to MediaFormat.MIMETYPE_VIDEO_AVC, + "hevc" to MediaFormat.MIMETYPE_VIDEO_HEVC, + "h265" to MediaFormat.MIMETYPE_VIDEO_HEVC, + "hvc1" to MediaFormat.MIMETYPE_VIDEO_HEVC, + "hev1" to MediaFormat.MIMETYPE_VIDEO_HEVC, + "x265" to MediaFormat.MIMETYPE_VIDEO_HEVC, + "vp8" to MediaFormat.MIMETYPE_VIDEO_VP8, + "vp9" to MediaFormat.MIMETYPE_VIDEO_VP9, + "vp09" to MediaFormat.MIMETYPE_VIDEO_VP9, + "av1" to MediaFormat.MIMETYPE_VIDEO_AV1, + "av01" to MediaFormat.MIMETYPE_VIDEO_AV1, + "mpeg4" to MediaFormat.MIMETYPE_VIDEO_MPEG4, + ) - /** Test seam: lets instrumented tests build a probe from explicit sets. */ + /** + * The names in [NAME_TO_MIME] that no [VideoCodec] member spells, and why. + * + * MPEG-4 Part 2 is decodable input the app never targets, so there is no enum for it and + * `CodecNames` is right not to carry it. That makes it the one place the two tables + * legitimately differ. It is listed rather than implied so the cross-check can tell a + * documented asymmetry from a fresh drift — and so the list itself is checked: a name here + * that `CodecNames` does resolve is a divergence being waved through, and the test fails on + * it. + */ + internal val DECODE_ONLY_NAMES: Set = setOf("mpeg4") + + /** + * Maps an FFprobe-style codec name onto a MediaFormat MIME type. + * + * Null keeps its documented meaning — unknown to us: assume the platform can handle it and + * let a failed export trigger the FFmpeg fallback, rather than pre-emptively refusing + * hardware. What changed with #87 is which names are unknown. Four that FFmpeg genuinely + * emits used to land here and be treated as unknown while the rest of the app knew exactly + * what they were; a device without the matching decoder now routes them to FFmpeg up front + * instead of spending a doomed hardware attempt to find out. + */ + internal fun mimeForCodecName(name: String): String? = NAME_TO_MIME[name.lowercase()] + + /** Test seam: lets a test build a probe from explicit sets, on a device or on the JVM. */ fun forTesting(encoders: Set, decoders: Set) = AndroidDeviceCodecs(encoders, decoders) } } diff --git a/app/src/main/java/org/libremediaconverter/model/CodecNames.kt b/app/src/main/java/org/libremediaconverter/model/CodecNames.kt index 3b68815..c14ba67 100644 --- a/app/src/main/java/org/libremediaconverter/model/CodecNames.kt +++ b/app/src/main/java/org/libremediaconverter/model/CodecNames.kt @@ -15,36 +15,97 @@ package org.libremediaconverter.model */ object CodecNames { - fun videoFromName(name: String?): VideoCodec? = when (name?.lowercase()) { - null, InputProbe.UNPARSEABLE -> null - "h264", "avc", "avc1", "x264" -> VideoCodec.H264 - "hevc", "h265", "hvc1", "hev1", "x265" -> VideoCodec.H265 - "vp8" -> VideoCodec.VP8 - "vp9", "vp09" -> VideoCodec.VP9 - "av1", "av01" -> VideoCodec.AV1 - else -> null - } + /** + * The video vocabulary, as data rather than a `when`. + * + * This is not the only place the app spells these names. `AndroidDeviceCodecs` reads the same + * FFprobe strings to decide what the device can decode, and answers in platform MIME types, + * which `model` cannot name without depending on Android. The two copies drifted apart: + * `x264`, `hev1`, `x265` and `vp09` resolved here and returned null there, so the app + * identified the codec for display and routing and then ran the device check blind, attempting + * a hardware path it had enough information to skip (#87). + * + * The reason this is a map is that **a `when` cannot be enumerated**, so nothing could compare + * the two tables. `CodecVocabularyTest` walks both key sets, so a name added to or removed + * from one side alone now fails the build rather than waiting for a wasted transcode to show + * it. + * + * Keys are lowercase; [videoFromName] lowercases before looking one up. + */ + internal val VIDEO_ALIASES: Map = mapOf( + "h264" to VideoCodec.H264, + "avc" to VideoCodec.H264, + "avc1" to VideoCodec.H264, + "x264" to VideoCodec.H264, + "hevc" to VideoCodec.H265, + "h265" to VideoCodec.H265, + "hvc1" to VideoCodec.H265, + "hev1" to VideoCodec.H265, + "x265" to VideoCodec.H265, + "vp8" to VideoCodec.VP8, + "vp9" to VideoCodec.VP9, + "vp09" to VideoCodec.VP9, + "av1" to VideoCodec.AV1, + "av01" to VideoCodec.AV1, + ) - fun audioFromName(name: String?): AudioCodec? = when (name?.lowercase()) { - null -> null - "aac", "mp4a", "aac_latm" -> AudioCodec.AAC - "opus" -> AudioCodec.OPUS - "vorbis" -> AudioCodec.VORBIS - "mp3", "mp3float", "mpga" -> AudioCodec.MP3 - "flac" -> AudioCodec.FLAC - "pcm", "raw", "pcm_s16le", "pcm_s24le", "pcm_f32le" -> AudioCodec.PCM - else -> null - } + /** + * The audio vocabulary, data for the same reason. + * + * Nothing cross-checks this one yet, and that is a gap rather than a decision: the device + * capability check is video-only, so this module holds no second audio table to compare it + * against. `Media3Engine.audioMimeTypeFor` is the other half, and #85 owns that file. + */ + internal val AUDIO_ALIASES: Map = mapOf( + "aac" to AudioCodec.AAC, + "mp4a" to AudioCodec.AAC, + "aac_latm" to AudioCodec.AAC, + "opus" to AudioCodec.OPUS, + "vorbis" to AudioCodec.VORBIS, + "mp3" to AudioCodec.MP3, + "mp3float" to AudioCodec.MP3, + "mpga" to AudioCodec.MP3, + "flac" to AudioCodec.FLAC, + "pcm" to AudioCodec.PCM, + "raw" to AudioCodec.PCM, + "pcm_s16le" to AudioCodec.PCM, + "pcm_s24le" to AudioCodec.PCM, + "pcm_f32le" to AudioCodec.PCM, + ) + + fun videoFromName(name: String?): VideoCodec? = asCodecName(name)?.let(VIDEO_ALIASES::get) + + fun audioFromName(name: String?): AudioCodec? = asCodecName(name)?.let(AUDIO_ALIASES::get) /** Human-readable name for the source-info card. Falls back to the raw probe string. */ - fun describeVideo(name: String?): String = when { + fun describeVideo(name: String?): String = describe(name) { videoFromName(it)?.label } + + fun describeAudio(name: String?): String = describe(name) { audioFromName(it)?.label } + + /** + * Lowercases a probe string, and answers null for the two inputs that are not codec names at + * all: absent, and the [InputProbe.UNPARSEABLE] sentinel. + * + * The sentinel would miss every key anyway, so naming it changes no answer. Naming it is still + * the point: `videoFromName` excluded it explicitly and `audioFromName` did not, which read as + * though the two disagreed about what the sentinel means — the same asymmetry as #74 one + * function further up. + */ + private fun asCodecName(name: String?): String? = + if (name == null || name == InputProbe.UNPARSEABLE) null else name.lowercase() + + /** + * The shared body of [describeVideo] and [describeAudio]. + * + * They are one function apiece over one vocabulary, and they had stopped matching: + * `describeVideo` answered "Unrecognised" for [InputProbe.UNPARSEABLE] and `describeAudio` fell + * through to `?: name` instead. The sentinel opens with a NUL, so that fallback would have put + * a U+0000 into a `Text` on the source-info card (#74). Sharing the arms is what stops the next + * one being added to one side only. + */ + private fun describe(name: String?, label: (String) -> String?): String = when { name == null -> "Unknown" name == InputProbe.UNPARSEABLE -> "Unrecognised" - else -> videoFromName(name)?.label ?: name - } - - fun describeAudio(name: String?): String = when { - name == null -> "Unknown" - else -> audioFromName(name)?.label ?: name + else -> label(name) ?: name } } diff --git a/app/src/test/java/org/libremediaconverter/codec/CodecVocabularyTest.kt b/app/src/test/java/org/libremediaconverter/codec/CodecVocabularyTest.kt new file mode 100644 index 0000000..bc4c045 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/codec/CodecVocabularyTest.kt @@ -0,0 +1,143 @@ +package org.libremediaconverter.codec + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.libremediaconverter.model.CodecNames +import org.libremediaconverter.model.VideoCodec + +/** + * Bites on #87: two tables read one codec vocabulary and had stopped agreeing. + * + * `CodecNames.VIDEO_ALIASES` answers "which enum is this FFprobe name", for the source-info card + * and for routing. `AndroidDeviceCodecs.NAME_TO_MIME` answers "which MIME do I ask this device + * about", for the capability check. On `ad28293` five names lived in one and not the other: `x264`, + * `hev1`, `x265` and `vp09` were identified for display and then fell through the device check as + * unknown, so the app attempted a hardware path it had enough information to skip; `mpeg4` ran the + * other way and rendered as a raw name on the card. + * + * Per-table arm tests would have passed on both tables and encoded the disagreement, which is why + * these walk the key sets instead. A name added to — or removed from — one side alone fails here. + */ +class CodecVocabularyTest { + + private val aliases = CodecNames.VIDEO_ALIASES + private val mimes = AndroidDeviceCodecs.NAME_TO_MIME + private val decodeOnly = AndroidDeviceCodecs.DECODE_ONLY_NAMES + + @Test + fun `no video codec name resolves for display without also resolving for the device check`() { + assertEquals( + "resolve in CodecNames but return null from mimeForCodecName, so the device check runs blind", + emptySet(), + aliases.keys - mimes.keys, + ) + } + + @Test + fun `no video codec name resolves for the device check without being a name the app can label`() { + assertEquals( + "resolve in AndroidDeviceCodecs but not in CodecNames, and are not listed as decode-only", + emptySet(), + mimes.keys - aliases.keys - decodeOnly, + ) + } + + /** + * Membership is not enough: `"x265" to MIMETYPE_VIDEO_AVC` would satisfy both key sets and + * still ask the device about the wrong codec. + */ + @Test + fun `the two tables agree on what each name means, not merely that they know it`() { + aliases.forEach { (name, codec) -> + val expected = AndroidDeviceCodecs.mimeFor(codec) + assertNotNull("$name maps to $codec, which has no MIME to ask about", expected) + assertEquals("$name is $codec in CodecNames", expected, mimes[name]) + } + } + + /** + * The exception list is the escape hatch: any future divergence could be waved through by + * adding the name to it. Guard both directions so it cannot be. + */ + @Test + fun `the decode-only names are genuinely decode-only`() { + decodeOnly.forEach { name -> + assertNotNull("$name is listed as decode-only but the device check cannot resolve it", mimes[name]) + assertNull( + "$name is listed as decode-only, but CodecNames does resolve it — that is a divergence " + + "being waved through rather than a documented exception", + CodecNames.videoFromName(name), + ) + } + } + + /** + * The five names #87 measured, pinned by name so the specific regression cannot come back + * quietly even if someone rewrites the tables above. + */ + @Test + fun `the names that used to resolve on one side only resolve on both`() { + mapOf( + "x264" to VideoCodec.H264, + "hev1" to VideoCodec.H265, + "x265" to VideoCodec.H265, + "vp09" to VideoCodec.VP9, + ).forEach { (name, codec) -> + assertEquals("$name is a name FFmpeg emits", codec, CodecNames.videoFromName(name)) + assertEquals( + "$name has to reach the device check too, or the app identifies it and then asks blind", + AndroidDeviceCodecs.mimeFor(codec), + AndroidDeviceCodecs.mimeForCodecName(name), + ) + } + // The one that runs the other way: decodable input with no enum to name it. + assertNull("mpeg4 is not an output the app can target", CodecNames.videoFromName("mpeg4")) + assertNotNull("mpeg4 is still decodable input", AndroidDeviceCodecs.mimeForCodecName("mpeg4")) + } + + /** + * Without this the agreement test above could pass on two nulls. + * + * `MediaFormat.MIMETYPE_VIDEO_AVC` is a Java compile-time constant, so it is inlined and the + * unit-test classpath's stubbed `android.jar` never has to supply it. If that ever stops being + * true, every MIME comparison here would be `null == null` and green — the vacuous-mutation + * failure this repo has counted before. Assert one literal so the stub fails loudly instead. + */ + @Test + fun `the MIME constants are real strings rather than stubs`() { + assertEquals("video/avc", AndroidDeviceCodecs.mimeForCodecName("h264")) + assertEquals("video/hevc", AndroidDeviceCodecs.mimeForCodecName("hevc")) + assertEquals("video/avc", AndroidDeviceCodecs.mimeFor(VideoCodec.H264)) + } + + @Test + fun `codec names are matched case-insensitively on both sides`() { + assertEquals(VideoCodec.H265, CodecNames.videoFromName("HEV1")) + assertEquals("video/hevc", AndroidDeviceCodecs.mimeForCodecName("HEV1")) + } + + @Test + fun `a name neither table knows still resolves to nothing`() { + assertNull(CodecNames.videoFromName("cinepak")) + assertNull(AndroidDeviceCodecs.mimeForCodecName("cinepak")) + } + + /** + * The behaviour #87 actually changes, at the seam that uses it. + * + * `canDecode` treats an unresolved name as "assume the platform copes". Before the alias + * landed, a device with no HEVC decoder answered true for `x265` and Media3 was handed a job it + * could not do; now the router sends it to FFmpeg without spending the attempt. + */ + @Test + fun `a device without the decoder now says so for the aliases it used to wave through`() { + val hevcOnly = AndroidDeviceCodecs.forTesting(encoders = emptySet(), decoders = setOf("video/hevc")) + assertTrue("x265 is HEVC by another name", hevcOnly.canDecode("x265")) + assertFalse("this device has no AVC decoder, and x264 is AVC", hevcOnly.canDecode("x264")) + assertTrue("a name nobody knows keeps the permissive answer", hevcOnly.canDecode("cinepak")) + } +} diff --git a/app/src/test/java/org/libremediaconverter/model/CodecNamesTest.kt b/app/src/test/java/org/libremediaconverter/model/CodecNamesTest.kt index 6f21931..1b9ceb5 100644 --- a/app/src/test/java/org/libremediaconverter/model/CodecNamesTest.kt +++ b/app/src/test/java/org/libremediaconverter/model/CodecNamesTest.kt @@ -1,6 +1,7 @@ package org.libremediaconverter.model import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Test @@ -10,6 +11,16 @@ import org.junit.Test * Three vocabularies meet: `MediaExtractor` MIME types, FFprobe `codec_name` strings, and the * enums. Stream copy depends on the round trip, so a missing alias here shows up as "we could not * identify the source codec" and silently costs the user a re-encode. + * + * Also bites on #74: `describeVideo` and `describeAudio` are one function apiece over one + * vocabulary and had stopped matching. Only the video side special-cased + * [InputProbe.UNPARSEABLE]; the audio side fell through to the raw name, and that sentinel opens + * with a NUL, so the source-info card would have rendered a control character. The arms are shared + * now, and the tests below assert both sides so the symmetric bug cannot reappear on the other one. + * + * The tables these read are cross-checked against the device capability check by + * `CodecVocabularyTest` (#87). Deliberately not repeated here: this file is what each name means, + * that one is whether the app's two copies of the vocabulary still agree. */ class CodecNamesTest { @@ -48,4 +59,52 @@ class CodecNamesTest { // An unrecognised but real codec name is more useful shown than hidden. assertEquals("cinepak", CodecNames.describeVideo("cinepak")) } + + /** The audio row of the same card, which had none of the above. */ + @Test + fun `audio descriptions degrade exactly the way video ones do`() { + assertEquals("AAC", CodecNames.describeAudio("mp4a")) + assertEquals("Unknown", CodecNames.describeAudio(null)) + assertEquals("Unrecognised", CodecNames.describeAudio(InputProbe.UNPARSEABLE)) + assertEquals("qdm2", CodecNames.describeAudio("qdm2")) + } + + /** + * #74's actual failure mode, stated as the thing the user would have seen. + * + * `InputProbe.UNPARSEABLE` is `"\u0000unparseable"`. Falling through to `?: name` does not + * mislabel the track, it puts U+0000 into a `Text`. + */ + @Test + fun `no description can put a control character on the card`() { + listOf(CodecNames.describeAudio(InputProbe.UNPARSEABLE), CodecNames.describeVideo(InputProbe.UNPARSEABLE)) + .forEach { assertFalse("$it leaks the sentinel", it.contains('\u0000')) } + } + + /** + * Every alias, pinned one at a time. + * + * The tables became maps so `CodecVocabularyTest` could enumerate them; this is what catches a + * key mistyped or a value pointing at the wrong enum while that rewrite happened. + */ + @Test + fun `every name in the tables resolves to the codec it spells`() { + CodecNames.VIDEO_ALIASES.forEach { (name, codec) -> + assertEquals(name, codec, CodecNames.videoFromName(name)) + } + CodecNames.AUDIO_ALIASES.forEach { (name, codec) -> + assertEquals(name, codec, CodecNames.audioFromName(name)) + } + assertEquals(VideoCodec.H264, CodecNames.videoFromName("x264")) + assertEquals(VideoCodec.VP9, CodecNames.videoFromName("vp09")) + assertEquals(AudioCodec.MP3, CodecNames.audioFromName("mpga")) + assertEquals(AudioCodec.OPUS, CodecNames.audioFromName("opus")) + } + + /** The audio lookup reads the sentinel the same way the video one does. */ + @Test + fun `the unparseable sentinel resolves to nothing on the audio side too`() { + assertNull(CodecNames.audioFromName(InputProbe.UNPARSEABLE)) + assertNull(CodecNames.audioFromName(null)) + } } From 4ff44be1d7942b0cc9b01ec5941ce03fa5f6d1d7 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 22:45:02 -0500 Subject: [PATCH 4/5] Give the Robolectric choice a reason that is still true Two test classes justified using Robolectric by asserting that the alternative does not exist: AppRootRestorationTest "The instrumented tests cannot run on the development host at all (see CLAUDE.md)" OutputPublisherStagingTest "The instrumented suite cannot run on the development host, so this is the only place [it] can be caught" Both were true when written and stopped being true on 2026-08-22, when the segfault was traced to SwiftShader's Reactor JIT against SELinux execheap rather than to the machine. tools/local-emulator/run-e2e.sh has run API 33-36 here since. The first one cites CLAUDE.md as its authority, and PR #73 corrected CLAUDE.md to say the opposite. So it was no longer merely stale: a reader who followed the reference found the contradiction, with the citation making the wrong half look verified. That is the worst version of this -- R14, R15, R20 and R25 were all the same defect, and this is the fifth. The choice itself was never wrong, which is why the fix is not to move these tests. Both belong on the JVM, and the honest reason is cost rather than impossibility: neither needs anything a device supplies, and both run inside the same ./gradlew invocation as every other unit test instead of booting an emulator. That argument survives the correction; the premise did not. The old line also has a second failure mode worth naming. "Nobody can execute this" invites a reader to skip the local run and let CI decide, which is the opposite of what the definition-of-done in #51 asks for. Verified: the string appears nowhere in app/src now, and testDebugUnitTest, ktlintCheck and detekt are green. Closes #46. --- .../org/libremediaconverter/AppRootRestorationTest.kt | 9 ++++++--- .../convert/OutputPublisherStagingTest.kt | 6 ++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt b/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt index 07dd1f0..5c125b7 100644 --- a/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt +++ b/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt @@ -33,9 +33,12 @@ import org.robolectric.RobolectricTestRunner * representation survives a `Bundle` round trip. A JVM round-trip test on the * saver covers the representation. * - * Robolectric rather than the instrumented suite, deliberately. The instrumented tests - * cannot run on the development host at all (see CLAUDE.md), and a red test nobody can - * execute is not a loop anyone can work in. + * Robolectric rather than the instrumented suite, deliberately -- but not because the + * instrumented suite is unavailable. It runs on this host for API 33-36 + * (`tools/local-emulator/run-e2e.sh`), and CI runs 33-37. The reason is cost: this test + * needs a composition and a saved-state round trip, nothing a device supplies, and it runs + * in the same `./gradlew` invocation as every other JVM test instead of booting an + * emulator. A loop measured in seconds is a loop people stay inside. */ @UnstableApi @RunWith(RobolectricTestRunner::class) diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt index 4419919..f778f5a 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt @@ -18,8 +18,10 @@ import java.util.UUID * the actual filesystem — the same calls `reset()` makes, without needing a ViewModel (both * of those construct a `WorkManager`, which is not initialised on the JVM classpath). * - * The instrumented suite cannot run on the development host, so this is the only place the - * "Start over leaks a full-size copy" defect can be caught before CI. + * The instrumented suite could also catch the "Start over leaks a full-size copy" defect -- + * it runs on this host for API 33-36 (`tools/local-emulator/run-e2e.sh`) and on CI for + * 33-37. Here rather than there because a real `cacheDir` is all the defect needs, and + * finding it costs an emulator boot there and a few seconds here. */ @RunWith(RobolectricTestRunner::class) class OutputPublisherStagingTest { From 8ac6e2b1c2ac25599fa9e189c6968770586c91a2 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 22:43:10 -0500 Subject: [PATCH 5/5] Name the format in the image-demuxer failures Bare assertTrue/assertFalse report java.lang.AssertionError and nothing else, so the mutation that proves this test bites -- relaxing the _pipe suffix to a substring -- went red saying only that a line failed. The format name is the one thing a reader needs, exactly as the MIME is in the sibling test. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/MediaProbeImageFormatTest.kt | 48 +++++++++++-------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/app/src/test/java/org/libremediaconverter/convert/MediaProbeImageFormatTest.kt b/app/src/test/java/org/libremediaconverter/convert/MediaProbeImageFormatTest.kt index 2fe9e78..7f43e5f 100644 --- a/app/src/test/java/org/libremediaconverter/convert/MediaProbeImageFormatTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/MediaProbeImageFormatTest.kt @@ -17,28 +17,32 @@ import org.junit.Test * and some thirty more. Widening that suffix to a substring is the tempting simplification and it * is wrong, because `yuv4mpegpipe` is raw video. * - * Every format name asserted here was read back from `ffprobe -show_entries format=format_name` - * rather than guessed: a picked `.png` reports `png_pipe`, a picked `.jpg` reports `jpeg_pipe`, a - * `.y4m` reports `yuv4mpegpipe`, and `image2` needs the demuxer named explicitly. + * The image names were measured rather than recalled. `ffprobe -show_entries format=format_name` + * reports `png_pipe` for a `.png`, `jpeg_pipe` for a `.jpg`, `yuv4mpegpipe` for a `.y4m`, and + * `image2` only when that demuxer is named explicitly. The container names come from + * [MediaProbeFormatTest], and the case and spacing variants are synthetic — those exercise the + * normalisation rather than anything FFprobe emits. * - * One known gap, deliberately not asserted either way: `image2pipe` is a real FFprobe format name - * for an image read from a stream, and this rule answers false for it — it is not `image2` and - * does not end in `_pipe`. Whether that is worth fixing is a question about picked-file behaviour - * on a device, not something to settle by pinning today's answer here. + * One real format name is deliberately not asserted either way. `image2pipe` gets a false answer + * here, being neither `image2` nor a `_pipe` suffix, and that is inert rather than a latent bug: + * FFprobe only selects it when the demuxer is named with `-f image2pipe`, while `probeWithFFprobe` + * forces no format at all, so a picked image arrives as `png_pipe` or its own codec's equivalent. + * Pinning today's answer for a name this app cannot receive would be a test about FFmpeg's command + * line rather than about this rule. */ class MediaProbeImageFormatTest { @Test fun `a numbered image sequence is an image`() { - assertTrue(MediaProbe.isImageFormat("image2")) + assertIsImage("image2") } /** What a picked PNG or JPEG actually reports, and the reason the suffix rule exists. */ @Test fun `the per-codec piped demuxers are images`() { - assertTrue(MediaProbe.isImageFormat("png_pipe")) - assertTrue(MediaProbe.isImageFormat("jpeg_pipe")) - assertTrue(MediaProbe.isImageFormat("webp_pipe")) + assertIsImage("png_pipe") + assertIsImage("jpeg_pipe") + assertIsImage("webp_pipe") } /** @@ -50,15 +54,15 @@ class MediaProbeImageFormatTest { */ @Test fun `a format that merely contains pipe is not an image`() { - assertFalse(MediaProbe.isImageFormat("yuv4mpegpipe")) + assertNotImage("yuv4mpegpipe") } /** The ordinary media containers, which is what the false answer is mostly for. */ @Test fun `a real container is not an image`() { - assertFalse(MediaProbe.isImageFormat("mov,mp4,m4a,3gp,3g2,mj2")) - assertFalse(MediaProbe.isImageFormat("matroska,webm")) - assertFalse(MediaProbe.isImageFormat("mp3")) + assertNotImage("mov,mp4,m4a,3gp,3g2,mj2") + assertNotImage("matroska,webm") + assertNotImage("mp3") } /** @@ -68,14 +72,20 @@ class MediaProbeImageFormatTest { */ @Test fun `an image entry is found anywhere in the list, whatever its spacing or case`() { - assertTrue(MediaProbe.isImageFormat("PNG_PIPE")) - assertTrue(MediaProbe.isImageFormat(" image2 ")) - assertTrue(MediaProbe.isImageFormat("something_else, tiff_pipe")) + assertIsImage("PNG_PIPE") + assertIsImage(" image2 ") + assertIsImage("something_else, tiff_pipe") } /** Nothing to go on is not an image; the card falls back to describing an unknown container. */ @Test fun `an empty format name is not an image`() { - assertFalse(MediaProbe.isImageFormat("")) + assertNotImage("") } + + private fun assertIsImage(formatName: String) = + assertTrue("isImageFormat(\"$formatName\")", MediaProbe.isImageFormat(formatName)) + + private fun assertNotImage(formatName: String) = + assertFalse("isImageFormat(\"$formatName\")", MediaProbe.isImageFormat(formatName)) }