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/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/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/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/AppRootRestorationTest.kt b/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt index 8219b58..4b0ec82 100644 --- a/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt +++ b/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt @@ -34,9 +34,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/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/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), + ) + } +} 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..7f43e5f --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MediaProbeImageFormatTest.kt @@ -0,0 +1,91 @@ +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. + * + * 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 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`() { + 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`() { + assertIsImage("png_pipe") + assertIsImage("jpeg_pipe") + assertIsImage("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`() { + assertNotImage("yuv4mpegpipe") + } + + /** The ordinary media containers, which is what the false answer is mostly for. */ + @Test + fun `a real container is not an image`() { + assertNotImage("mov,mp4,m4a,3gp,3g2,mj2") + assertNotImage("matroska,webm") + assertNotImage("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`() { + 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`() { + assertNotImage("") + } + + private fun assertIsImage(formatName: String) = + assertTrue("isImageFormat(\"$formatName\")", MediaProbe.isImageFormat(formatName)) + + private fun assertNotImage(formatName: String) = + assertFalse("isImageFormat(\"$formatName\")", MediaProbe.isImageFormat(formatName)) +} 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 + } +} 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 { 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)) + } }