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/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/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/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)) + } }