From fd2bb1d889ab8ea22b9608df729c8c87eb5698d4 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 22:36:17 -0500 Subject: [PATCH 1/2] 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 + } +} -- 2.47.3 From 8ac6e2b1c2ac25599fa9e189c6968770586c91a2 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 22:43:10 -0500 Subject: [PATCH 2/2] 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)) } -- 2.47.3