diff --git a/app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt b/app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt index 0b0052a..016a77c 100644 --- a/app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt +++ b/app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt @@ -21,6 +21,11 @@ import org.libremediaconverter.model.VideoCodec * words, "cannot be tested for correctness". It is a hint, not a guarantee, which is * why the router treats a failed hardware export as a signal to fall back rather * than trusting this up front. + * - **An enumeration that fails answers no to everything**, which sends every job to + * FFmpeg. Empty sets are not a permissive default: `canEncode` looks a MIME type up in + * [hardwareEncodeMimes] and finds nothing there. That is the intended answer — FFmpeg + * can do whatever Media3 can, only slower — but it is the opposite of what this class + * said until #194, so it is written down rather than left to be re-derived. */ class AndroidDeviceCodecs private constructor( private val hardwareEncodeMimes: Set, @@ -46,22 +51,62 @@ class AndroidDeviceCodecs private constructor( fun get(): AndroidDeviceCodecs = cached ?: synchronized(this) { cached ?: probe().also { cached = it } } - private fun probe(): AndroidDeviceCodecs { + /** + * One entry of the platform's codec list, reduced to what the rules below read. + * + * The five booleans and the type list are the whole of what [capabilitiesFrom] needs, and + * none of them can be set on a `MediaCodecInfo` from a test: Robolectric ships + * `MediaCodecInfoBuilder`, but it has no `setIsAlias` and no `setCanonicalName`, which is + * exactly the objection #133 raised against reaching this code through + * `ShadowMediaCodecList`. That objection is about the shadow. It does not apply to a + * function that takes its own entry type, which is why this exists. + */ + internal data class CodecEntry( + val canonicalName: String, + val isAlias: Boolean, + val isEncoder: Boolean, + val isHardwareAccelerated: Boolean, + val isSoftwareOnly: Boolean, + val supportedTypes: List, + ) + + /** + * The enumeration rules, over entries a caller chooses. + * + * [probe] is the only production caller and supplies the real codec list; a test supplies + * its own, which is the point — the two rules this class's KDoc calls out as easy to get + * wrong, the alias skip and the canonical-name dedup, are unreachable any other way. + * + * **`enumerate` returns a `Sequence`, deliberately.** The `runCatching` has to wrap the + * *iteration* rather than a list built before it, because a `MediaCodecInfo` whose + * properties throw does so partway through — and when that happens the codecs already read + * are kept. Taking a `List` here would move that throw outside the loop and silently turn a + * partial answer into an empty one. That behaviour predates this seam; a `List` parameter + * would have changed it as a side effect of a refactor. + * + * **An enumeration that fails answers restrictively, and that is deliberate.** The sets + * come back empty, and `"video/avc" in emptySet()` is `false`, so [canEncode] and + * [canDecode] both answer no and every job routes to FFmpeg. FFmpeg can do everything + * Media3 can, only slower, so refusing the hardware path is the safe reading of "we could + * not find out what this device supports". This used to log "assuming permissive", which + * described the opposite of what the code does. + */ + internal fun capabilitiesFrom(enumerate: () -> Sequence): AndroidDeviceCodecs { val encoders = mutableSetOf() val decoders = mutableSetOf() val seen = mutableSetOf() runCatching { - MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos.forEach { info -> + enumerate().forEach { entry -> // Aliases point at the same underlying codec; counting both would // double-count capabilities. - if (info.isAlias) return@forEach - if (!seen.add(info.canonicalName)) return@forEach + if (entry.isAlias) return@forEach + if (!seen.add(entry.canonicalName)) return@forEach - info.supportedTypes.forEach { mime -> + entry.supportedTypes.forEach { mime -> if (!mime.startsWith("video/")) return@forEach - if (info.isEncoder) { - if (info.isHardwareAccelerated && !info.isSoftwareOnly) { + if (entry.isEncoder) { + if (entry.isHardwareAccelerated && !entry.isSoftwareOnly) { encoders += mime } } else { @@ -69,12 +114,32 @@ class AndroidDeviceCodecs private constructor( } } } - }.onFailure { Log.w(TAG, "Codec enumeration failed; assuming permissive.", it) } + }.onFailure { Log.w(TAG, "Codec enumeration failed; routing everything to FFmpeg.", it) } Log.i(TAG, "Hardware video encoders: $encoders") return AndroidDeviceCodecs(encoders, decoders) } + /** + * The thin edge: the real codec list, mapped onto [CodecEntry] one at a time. + * + * Lazily, so a property that throws does it inside [capabilitiesFrom]'s `runCatching` and + * on the entry that caused it — see that function's note on why the parameter is a + * `Sequence`. + */ + private fun probe(): AndroidDeviceCodecs = capabilitiesFrom { + MediaCodecList(MediaCodecList.REGULAR_CODECS).codecInfos.asSequence().map { info -> + CodecEntry( + canonicalName = info.canonicalName, + isAlias = info.isAlias, + isEncoder = info.isEncoder, + isHardwareAccelerated = info.isHardwareAccelerated, + isSoftwareOnly = info.isSoftwareOnly, + supportedTypes = info.supportedTypes.toList(), + ) + } + } + /** * `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. diff --git a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt index b226f95..97f6fd1 100644 --- a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt +++ b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt @@ -221,12 +221,39 @@ object MediaProbe { null } - private fun readMediaInformation(path: String): FFprobeInfo? { - // ffmpeg-kit-next is compiled from Kotlin with private backing fields, so these have to go - // through the Java getters rather than property syntax. - val info: MediaInformation = FFprobeKit.getMediaInformation(path).getMediaInformation() - ?: return null + /** + * The thin edge: spawn FFprobe, hand what it said to [ffprobeInfoFrom]. + * + * Everything device-bound is on this line and the null check under it. What FFprobe *said* is a + * `MediaInformation`, which is an ordinary object over a `JSONObject` — so the reading of it is + * a decision a test can choose the inputs for, and it lives below rather than here. + */ + private fun readMediaInformation(path: String): FFprobeInfo? = + FFprobeKit.getMediaInformation(path).getMediaInformation()?.let(::ffprobeInfoFrom) + /** + * What FFprobe's answer means, as a function of the answer alone. + * + * `internal` for the same reason [Extracted] and [FFprobeInfo] are: a test cannot name it + * otherwise, and the JVM test source set is a friend of `main`. + * + * **JVM-safe, verified rather than assumed.** `javap` over the committed AAR's runtime jar: + * `MediaInformation(JSONObject, List, List)` and + * `StreamInformation(JSONObject)` are plain public constructors, and neither class's `` + * touches the native library — so a test builds its own without `libffmpegkit` being present. + * That is the whole reason this split is worth making: `readMediaInformation` was 114 missed + * instructions and 24 missed branches, of which exactly one line needed a device. + * + * The subtle part is the **second argument to [containerFrom]**. `matroska,webm` is reported + * for both MKV and WebM — they share a demuxer — so the video codec is the only thing that + * separates them, and dropping it silently turns every VP9 WebM into an MKV. `containerFrom` + * has thirty-three covered branches of its own and none of them can notice that, because the + * mistake is at the call rather than in the callee. + * + * ffmpeg-kit-next is compiled from Kotlin with private backing fields, so these go through the + * Java getters rather than property syntax. + */ + internal fun ffprobeInfoFrom(info: MediaInformation): FFprobeInfo { val streams = info.getStreams().orEmpty() val video = streams.firstOrNull { it.getType() == "video" } val audio = streams.firstOrNull { it.getType() == "audio" } diff --git a/app/src/test/java/org/libremediaconverter/codec/CodecEnumerationTest.kt b/app/src/test/java/org/libremediaconverter/codec/CodecEnumerationTest.kt new file mode 100644 index 0000000..e12ca16 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/codec/CodecEnumerationTest.kt @@ -0,0 +1,201 @@ +package org.libremediaconverter.codec + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.VideoCodec +import org.robolectric.RobolectricTestRunner + +/** + * The rules `AndroidDeviceCodecs.probe()` applies to the platform's codec list. + * + * ## Why this is not a third run of the #86/#133 spike + * + * #86 closed `probe()` as device-bound. #133 re-opened the question with + * `ShadowMediaCodecList` in hand and closed it again, for a reason that was right about what it + * was answering: `MediaCodecInfoBuilder` "has no `setIsAlias` and no `setCanonicalName`, so the + * alias skip and the canonical-name dedup — the two things the class's KDoc calls out as easy to + * get wrong — are not reachable through it." + * + * **That objection is about the shadow.** It does not apply to a function that takes its own entry + * type, which is what `capabilitiesFrom` now does. The half #133 named as unreachable is the half + * this file spends most of its cases on. + * + * ## What made the seam worth cutting, which is not coverage + * + * The `runCatching` fallback logged *"assuming permissive"* and returned empty sets — and empty + * sets are **restrictive**: `"video/avc" in emptySet()` is `false`, so `canEncode` and `canDecode` + * both answer no and every job routes to FFmpeg. The code was right and the message described the + * opposite of it. That is pinned below, so whichever reading a future change takes, it has to say + * so out loud. + * + * Robolectric only because `capabilitiesFrom` logs what it found; the rules themselves are pure. + */ +@RunWith(RobolectricTestRunner::class) +class CodecEnumerationTest { + + /** + * The alias skip, in the one arrangement where it is observable — and finding that arrangement + * is the whole of this test. + * + * A first attempt listed the alias *after* the codec it aliases and passed with the skip + * deleted, because `canonicalName` is shared and the dedup below catches the second entry + * either way. The two rules overlap, so a fixture that does not separate them tests neither. + * + * What separates them is **order**. `MediaCodecInfo.getCanonicalName()` on an alias returns the + * underlying codec's name, so an alias arriving first claims that name in `seen` and has its + * own `supportedTypes` credited — and then the real codec is dropped by the dedup. Without the + * alias skip the device is described by whichever entry the platform happened to list first. + * + * That also says what the rule is worth. With a `Set` accumulator, an alias declaring the same + * types as its codec changes nothing whichever order they arrive in; the skip earns its place + * only when the two disagree, which is exactly when believing the wrong one matters. + */ + @Test + fun `an alias listed before the codec it aliases does not describe the device`() { + val codecs = capabilities( + entry("c2.qti.avc.encoder", encoder = true, types = listOf(HEVC), alias = true), + entry("c2.qti.avc.encoder", encoder = true, types = listOf(AVC)), + ) + + assertTrue("the real codec's types are the device's", codecs.canEncode(VideoCodec.H264)) + assertFalse( + "an alias must not be credited with types the codec it aliases never claimed", + codecs.canEncode(VideoCodec.H265), + ) + } + + @Test + fun `two entries sharing a canonical name are read once`() { + val codecs = capabilities( + entry("c2.qti.avc.encoder", encoder = true, types = listOf(AVC)), + entry("c2.qti.avc.encoder", encoder = true, types = listOf(HEVC)), + ) + + assertEquals(setOf(AVC), codecs.hardwareEncoders()) + } + + /** + * Both halves of the hardware predicate, one arm at a time. + * + * A vendor may declare a codec hardware-accelerated *and* software-only; the class KDoc is + * explicit that the first flag "cannot be tested for correctness", so the second is what stops + * a mislabelled software encoder being treated as the fast path. + */ + @Test + fun `an encoder counts as hardware only when it is accelerated and not software-only`() { + assertEquals( + setOf(AVC), + capabilities(entry("hw", encoder = true, accelerated = true, types = listOf(AVC))).hardwareEncoders(), + ) + assertEquals( + emptySet(), + capabilities(entry("sw", encoder = true, accelerated = false, types = listOf(AVC))).hardwareEncoders(), + ) + assertEquals( + "a codec claiming both must not be trusted as hardware", + emptySet(), + capabilities( + entry("both", encoder = true, accelerated = true, softwareOnly = true, types = listOf(AVC)), + ).hardwareEncoders(), + ) + } + + /** + * Decoders are collected regardless of the hardware flags, and that asymmetry is the design. + * + * `canDecode` asks whether the platform can read the input at all — a software decoder answers + * that as well as a hardware one. `canEncode` asks whether the *fast path* exists, which is a + * different question and why only encoders are filtered. + */ + @Test + fun `a software decoder still counts as something the platform can read`() { + val codecs = capabilities( + entry( + "c2.android.avc.decoder", + encoder = false, + accelerated = false, + softwareOnly = true, + types = listOf(AVC), + ), + ) + + assertTrue(codecs.canDecode("h264")) + } + + @Test + fun `audio types are ignored on both sides`() { + val codecs = capabilities( + entry("aac.encoder", encoder = true, accelerated = true, types = listOf("audio/mp4a-latm")), + entry("aac.decoder", encoder = false, types = listOf("audio/mp4a-latm")), + ) + + assertEquals(emptySet(), codecs.hardwareEncoders()) + // Not "the platform cannot decode AAC" -- `canDecode` is asked about *video* codec names, + // and an unknown name is answered permissively. The point is that nothing audio reached + // either set. + assertTrue("an unknown name stays permissive", codecs.canDecode("something-nobody-named")) + } + + /** + * The failure fallback, pinned as the restrictive answer it actually is. + * + * #194 decided this rather than assuming it: the code stays, the message changes. If a later + * change wants the permissive reading its old log line described, this test is what makes that + * a decision instead of a drift. + */ + @Test + fun `an enumeration that fails sends every job to FFmpeg`() { + val codecs = AndroidDeviceCodecs.capabilitiesFrom { error("MediaCodecList exploded") } + + assertFalse("a failed enumeration must not claim a hardware encoder", codecs.canEncode(VideoCodec.H264)) + assertFalse(codecs.canDecode("h264")) + assertEquals(emptySet(), codecs.hardwareEncoders()) + } + + /** + * A list that throws partway keeps what it already read. + * + * This predates the seam — `runCatching` has always wrapped the iteration rather than a list + * built before it — and it is asserted here because the seam is where it could quietly have + * been lost. Taking a `List` instead of a `Sequence` would move the throw outside the loop and + * turn this partial answer into an empty one, with no test to notice. + */ + @Test + fun `codecs read before a failing entry are kept`() { + val codecs = AndroidDeviceCodecs.capabilitiesFrom { + sequence { + yield(entry("good", encoder = true, accelerated = true, types = listOf(AVC))) + error("the sixth codec's properties threw") + } + } + + assertEquals(setOf(AVC), codecs.hardwareEncoders()) + } + + private fun capabilities(vararg entries: AndroidDeviceCodecs.Companion.CodecEntry) = + AndroidDeviceCodecs.capabilitiesFrom { entries.asSequence() } + + private fun entry( + canonicalName: String, + encoder: Boolean, + accelerated: Boolean = true, + softwareOnly: Boolean = false, + alias: Boolean = false, + types: List, + ) = AndroidDeviceCodecs.Companion.CodecEntry( + canonicalName = canonicalName, + isAlias = alias, + isEncoder = encoder, + isHardwareAccelerated = accelerated, + isSoftwareOnly = softwareOnly, + supportedTypes = types, + ) + + private companion object { + const val AVC = "video/avc" + const val HEVC = "video/hevc" + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/FFprobeMappingTest.kt b/app/src/test/java/org/libremediaconverter/convert/FFprobeMappingTest.kt new file mode 100644 index 0000000..850fa59 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/FFprobeMappingTest.kt @@ -0,0 +1,192 @@ +package org.libremediaconverter.convert + +import com.arthenica.ffmpegkit.MediaInformation +import com.arthenica.ffmpegkit.StreamInformation +import org.json.JSONObject +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.Container +import org.robolectric.RobolectricTestRunner + +/** + * What FFprobe's answer means, read as a function of the answer alone. + * + * `readMediaInformation` was 114 missed instructions and 24 missed branches — the second-biggest + * block on the wave-4 report — of which **exactly one line needed a device**: + * + * ```kotlin + * FFprobeKit.getMediaInformation(path).getMediaInformation() + * ``` + * + * Everything after it reads an ordinary object. `javap` over the committed AAR's runtime jar: + * `MediaInformation(JSONObject, List, List)` and + * `StreamInformation(JSONObject)` are plain public constructors, and neither class's `` + * loads the native library — so the fixtures below are built without `libffmpegkit` present. + * + * ## The one that matters + * + * `containerFrom(formatName, video?.getCodec())`. FFprobe reports `matroska,webm` for **both** MKV + * and WebM, because they share a demuxer, so the video codec is the only thing separating them. + * `containerFrom` has thirty-three covered branches of its own and not one of them can notice the + * argument being dropped — the mistake would be at the call, not in the callee, and every existing + * `containerFrom` test would stay green while every VP9 WebM quietly became an MKV. + * + * Robolectric only for `org.json`, which is a stub in a plain JVM test. + */ +@RunWith(RobolectricTestRunner::class) +class FFprobeMappingTest { + + @Test + fun `the video codec decides between matroska and webm`() { + assertEquals( + Container.WEBM, + MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("video", "vp9"))).container, + ) + assertEquals( + Container.MKV, + MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("video", "h264"))).container, + ) + } + + /** + * The same format name with no video stream at all, which is what makes the case above about + * the *argument* rather than about the format string. + */ + @Test + fun `a matroska container with no video track cannot be told from webm and is not guessed`() { + val read = MediaProbe.ffprobeInfoFrom(info("matroska,webm", stream("audio", "opus"))) + + assertEquals(Container.MKV, read.container) + assertNull(read.videoCodec) + } + + @Test + fun `the first stream of each type wins`() { + val read = MediaProbe.ffprobeInfoFrom( + info( + "mov,mp4,m4a,3gp,3g2,mj2", + stream("video", "h264", width = 1920, height = 1080), + stream("video", "hevc", width = 640, height = 480), + stream("audio", "aac"), + stream("audio", "mp3"), + ), + ) + + assertEquals("h264", read.videoCodec) + assertEquals("aac", read.audioCodec) + assertEquals(1920, read.width) + assertEquals(1080, read.height) + } + + /** + * Dimensions come from the stream the codec came from, not from whichever stream has some. + * + * The fixture is deliberately awkward: the chosen video stream carries **no** dimensions and a + * later one does. That is a real shape — FFprobe omits `width`/`height` for a stream it could + * not measure — and it is the only arrangement that separates the two readings. + * + * A first version of this file asserted the dimensions inside the case above, where the chosen + * stream was also the first one carrying any. Replacing `video?.getWidth()` with + * `streams.firstNotNullOfOrNull { it.getWidth() }` gave the same answer there and **the + * mutation survived**. It reddens here. + */ + @Test + fun `a video stream with no dimensions reports none rather than borrowing another stream's`() { + val read = MediaProbe.ffprobeInfoFrom( + info( + "mov,mp4,m4a,3gp,3g2,mj2", + stream("video", "h264"), + stream("video", "hevc", width = 640, height = 480), + ), + ) + + assertEquals("h264", read.videoCodec) + assertEquals(0, read.width) + assertEquals(0, read.height) + } + + /** + * Stream order is the file's, not a promise. An audio-first container must read the same as a + * video-first one. + */ + @Test + fun `an audio track listed first does not become the video track`() { + val read = MediaProbe.ffprobeInfoFrom( + info("mov,mp4,m4a,3gp,3g2,mj2", stream("audio", "aac"), stream("video", "h264")), + ) + + assertEquals("h264", read.videoCodec) + assertEquals("aac", read.audioCodec) + } + + @Test + fun `a duration in seconds becomes milliseconds`() { + assertEquals(12_345L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = "12.345")).durationMs) + } + + /** + * Both ways a duration can be absent, and neither may throw. + * + * FFprobe reports `"N/A"` for a stream it could not measure, and omits the key entirely for + * some containers. `toDoubleOrNull` is what keeps the second from being an exception on the + * file-pick path, where there is no user-visible failure to report it as. + */ + @Test + fun `a duration that is not a number is no duration rather than a crash`() { + assertEquals(0L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = "N/A")).durationMs) + assertEquals(0L, MediaProbe.ffprobeInfoFrom(info("mp4", duration = null)).durationMs) + } + + @Test + fun `a file with no streams reports nothing rather than defaults that look measured`() { + val read = MediaProbe.ffprobeInfoFrom(info("mp4")) + + assertNull(read.videoCodec) + assertNull(read.audioCodec) + assertEquals(0, read.width) + assertEquals(0, read.height) + } + + @Test + fun `an image format is reported as one`() { + assertTrue(MediaProbe.ffprobeInfoFrom(info("png_pipe", stream("video", "png"))).isImage) + assertFalse(MediaProbe.ffprobeInfoFrom(info("mp4", stream("video", "h264"))).isImage) + } + + private fun stream(type: String, codec: String, width: Int? = null, height: Int? = null) = StreamInformation( + JSONObject().apply { + put(StreamInformation.KEY_TYPE, type) + put(StreamInformation.KEY_CODEC, codec) + width?.let { put(StreamInformation.KEY_WIDTH, it) } + height?.let { put(StreamInformation.KEY_HEIGHT, it) } + }, + ) + + /** + * The format properties are **nested** under `"format"`, which is how FFprobe reports them and + * what `MediaInformation` reads: `getFormat()` resolves through `getStringFormatProperty`, not + * off the top-level object. A first version of this helper put the keys at the top level and + * every format-dependent case failed with a null container, which is worth recording here so + * the next fixture does not have to rediscover it. + * + * Streams are the other half and are *not* nested — they come from the constructor argument. + */ + private fun info(formatName: String, vararg streams: StreamInformation, duration: String? = "1.0") = + MediaInformation( + JSONObject().apply { + put( + MediaInformation.KEY_FORMAT_PROPERTIES, + JSONObject().apply { + put(MediaInformation.KEY_FORMAT, formatName) + duration?.let { put(MediaInformation.KEY_DURATION, it) } + }, + ) + }, + streams.toList(), + emptyList(), + ) +} diff --git a/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt b/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt index a6e8112..7826daa 100644 --- a/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/FileCardTest.kt @@ -205,6 +205,34 @@ class FileCardTest { assertNoRow("Length") } + /** + * A video the app knows a great deal about and cannot name the container of. + * + * Not an edge case. `InputProbe.container`'s own KDoc says `MediaExtractor` cannot report a + * container at all -- it comes from FFprobe -- so any run where FFprobe did not answer produces + * exactly this: real codec, real dimensions, real duration, `container = null`. + * + * **The twin was already tested and this one was not**, which is the argument for adding it. + * `FileCard` renders `probe.container?.label ?: "Unknown"` twice, once in the `AUDIO_ONLY` + * branch (`ConverterScreen.kt:660`) and once in the `VIDEO` branch (`:668`), and + * `an audio-only file nothing else could describe degrades one row at a time` drives only the + * first. Same expression, same fallback, one kind covered. That asymmetry is the same one + * `CLAUDE.md` records for including `ContainerCapabilities:94`. + * + * The other rows are asserted alongside so this is not a copy of the audio-only case: there, + * everything is unknown at once; here, one field is missing from a probe that is otherwise + * complete, and the rest must be unaffected by it. + */ + @Test + fun `a video file whose container nothing identified says so and keeps its other rows`() { + setFileCard(input(probe = VIDEO_PROBE.copy(container = null))) + + assertRow("Container", "Unknown") + assertRow("Video", "${VideoCodec.H264.label} · 1920×1080") + assertRow("Audio", AudioCodec.AAC.label) + assertRow("Length", "1:30") + } + /** * The row is one node, not a label node beside a value node. A test matching on `"Container"` * alone would pass against either shape. diff --git a/app/src/test/java/org/libremediaconverter/convert/MessagelessFailureTest.kt b/app/src/test/java/org/libremediaconverter/convert/MessagelessFailureTest.kt new file mode 100644 index 0000000..83ea383 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MessagelessFailureTest.kt @@ -0,0 +1,213 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.ListenableWorker +import androidx.work.testing.TestListenableWorkerBuilder +import androidx.work.workDataOf +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.join.JoinState +import org.libremediaconverter.join.JoinViewModel +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.work.ConcatWorker +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * A failure that says nothing still has to say something. + * + * Three sites, all `ci == 0` before this file, and all the same rule: + * + * ``` + * work/ConversionWorker.kt:316 cause.message ?: GENERIC_FAILURE_MESSAGE + * convert/ConversionViewModel.kt:631 e.message ?: SAVE_FAILED_MESSAGE + * join/JoinViewModel.kt:416 e.message ?: SAVE_FAILED_MESSAGE + * ``` + * + * Every existing test throws *with* a message, so the right-hand side had never been evaluated + * anywhere in the suite. A `Throwable` carrying none is not exotic — `RuntimeException()`, + * `IOException()` and most platform exceptions raised without an argument all have a null message. + * + * ## Held in one class, against the ticket's suggestion + * + * #193 proposed putting each case beside the behaviour it neighbours. They are together instead, + * because they are one rule at three layers and because the trap below has to be explained once + * rather than three times. `FailedSaveRetryTest` sets the precedent for both ViewModels in one + * file; this extends it by one worker. + * + * ## The trap, which is why the worker case asserts what it does + * + * `ConversionStateMappingTest`'s *"a failure with nothing said still says something"* looks like it + * already covers the worker site. It does not: it drives the **read** side, `map(FAILED, Data.EMPTY)`, + * and that side has a fallback of its own (`ConversionViewModel.kt:147-149`): + * + * ```kotlin + * update.outputData.getString(ConversionWorker.KEY_ERROR) + * ?.takeIf { it.isNotBlank() } + * ?: ConversionWorker.GENERIC_FAILURE_MESSAGE + * ``` + * + * So mutating the worker's fallback to `.orEmpty()` writes `KEY_ERROR to ""`, and the ViewModel + * turns that straight back into the same constant. **A test asserting on the resulting `Failed` + * state stays green under the mutation**, which is most likely why the write-side fallback survived + * three waves of test work. The worker case therefore reads `KEY_ERROR` off the worker's own + * `Result`, before anything downstream can repair it. + * + * The two save cases have no such second line: both write `_state.value` directly, so the state is + * the right thing to assert there. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class MessagelessFailureTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) } + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + /** + * The engine gives up without saying why, which is what a native crash looks like from here. + * + * Asserted on the worker's own output `Data` rather than on a screen — see the class KDoc. + */ + @Test + fun `a conversion that fails without a message still reports one`() { + installTestWorkManager(app, Data.EMPTY) + ConversionDependencies.software = { MessagelessTranscoder } + + val result = runBlocking { failingWorker().doWork() } + + assertTrue("the job must fail rather than retry, got $result", result is ListenableWorker.Result.Failure) + assertEquals( + "a failure with no message must still put something on screen", + ConversionWorker.GENERIC_FAILURE_MESSAGE, + (result as ListenableWorker.Result.Failure).outputData.getString(ConversionWorker.KEY_ERROR), + ) + } + + @Test + fun `a save that fails without a message still reports one`() { + installTestWorkManager(app, conversionOutput()) + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv")) + awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + viewModel.convert() + awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + + publisher.publishFailure = RuntimeException() + viewModel.save(DESTINATION) + + val failed = awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } as ConversionState.Failed + assertEquals(SAVE_FAILED_MESSAGE, failed.message) + // The handle travels even on the wordless path. Without this, a fallback that also dropped + // `pending` would pass -- and the file would be unreachable from the screen that just said + // the save failed. + assertNotNull("a wordless failure must still offer the file again", failed.retry) + } + + @Test + fun `a join save that fails without a message still reports one`() { + installTestWorkManager(app, joinOutput()) + val viewModel = JoinViewModel(app, Dispatchers.Unconfined) + viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4"))) + awaitState(viewModel.state, "Ready") { it is JoinState.Ready } + viewModel.join() + awaitState(viewModel.state, "Joined") { it is JoinState.Joined } + + publisher.publishFailure = RuntimeException() + viewModel.save(DESTINATION) + + val failed = awaitState(viewModel.state, "Failed") { it is JoinState.Failed } as JoinState.Failed + assertEquals(SAVE_FAILED_MESSAGE, failed.message) + assertNotNull("a wordless failure must still offer the file again", failed.retry) + } + + /** + * `FORCE_SOFTWARE` so the failure comes straight out of `runFFmpeg`. + * + * `AUTO` would enter `runMedia3OrFallBack`, whose catch runs the job a second time in software + * — the same exception would arrive, but through a path this test is not about and which + * `HardwareFallbackTest` already owns. + */ + private fun failingWorker(): ConversionWorker { + val spec = OutputFormat.MP4_H265.spec + return TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConversionWorker.KEY_INPUT_URI to "file:///tmp/holiday.mp4", + ConversionWorker.KEY_DISPLAY_NAME to "holiday.mp4", + ConversionWorker.KEY_CONTAINER to spec.container.name, + ConversionWorker.KEY_VIDEO_CODEC to spec.videoCodec.name, + ConversionWorker.KEY_AUDIO_CODEC to spec.audioCodec.name, + ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ), + runAttemptCount = 0, + ).setId(JOB_ID).build() + } + + private fun conversionOutput() = workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConversionWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME, + ConversionWorker.KEY_MIME_TYPE to JOB_MIME_TYPE, + ) + + private fun joinOutput() = workDataOf( + ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConcatWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME, + ConcatWorker.KEY_MIME_TYPE to JOB_MIME_TYPE, + ) + + private companion object { + val DESTINATION: Uri = Uri.parse("content://test/destination.mp4") + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-00000000019a") + const val SUGGESTED_NAME = "holiday.mp4" + const val JOB_MIME_TYPE = "video/mp4" + } +} + +/** + * An engine that gives up without saying why. + * + * `RuntimeException()` rather than a subclass with a blank message: `Throwable.message` is *null* + * here, which is the case the elvis exists for. A blank-but-present message takes the left-hand + * side and is a different path — `ConversionStateMappingTest` covers that one, on the read side. + */ +@UnstableApi +private object MessagelessTranscoder : SoftwareTranscoder { + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ): Unit = throw RuntimeException() +} diff --git a/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt b/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt index 9f9624d..1274d11 100644 --- a/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt +++ b/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegCommandBuilderTest.kt @@ -190,6 +190,30 @@ class FFmpegCommandBuilderTest { assertPair(cmd(OutputFormat.M4A_AAC), "-b:a", "192k") } + /** + * Turning audio off, which the Advanced picker offers and nothing had ever built a command for. + * + * `audioArgs`' `Drop` arm was `ci == 0`. The suite's only `-an` assertion is in + * `gif generates a palette to avoid banding and drops audio`, and that one comes from the image + * path (`FFmpegCommandBuilder.kt:79`/`:90`), which emits `-an` directly and never reaches + * `audioArgs`. Two sites, one string, one tested. + * + * It is a live path rather than defensive code: `AdvancedPicker` renders all of + * `AudioCodec.entries` including `NONE`, `ContainerCapabilities.validate` permits audio-off + * whenever the input has video, and MKV routes the job to FFmpeg. + * + * Both halves are asserted. `-an` alone would still pass if the arm fell through to the `else` + * and emitted an AAC encoder beside it -- a file that is silent because the flag won, carrying + * an encoder nobody asked for. + */ + @Test + fun `turning audio off drops the track instead of encoding one`() { + val args = cmd(OutputSpec(Container.MKV, VideoCodec.H264, AudioCodec.NONE)) + + assertTrue("audio turned off must emit -an, got $args", args.contains("-an")) + assertFalse("a dropped track must not also carry an encoder, got $args", args.contains("-c:a")) + } + @Test fun `audio only formats never carry a video encoder`() { listOf(OutputFormat.MP3, OutputFormat.FLAC, OutputFormat.WAV, OutputFormat.OPUS)