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