From 92bcff8656b2ba78f92c7136e6e1b097b674b1c3 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 2 Sep 2026 18:02:39 -0500 Subject: [PATCH 1/4] Make a failure that says nothing still say something (#193) Three sites, all ci == 0 before this, 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, and a native engine that dies is exactly where one comes from. The worker case needed care, and the care is the reason it survived three waves. ConversionStateMappingTest's "a failure with nothing said still says something" looks like it covers that site and does not -- it drives the READ side, map(FAILED, Data.EMPTY), and that side has a fallback of its own at ConversionViewModel.kt:147-149 which turns a blank KEY_ERROR back into the same constant. So a test asserting on the resulting Failed state stays green while the worker's fallback is broken. Measured rather than reasoned: with :316 mutated to .orEmpty(), exactly ONE of 587 tests went red, and it was the new one. Everything else, including the test that appears to cover it, stayed green. So the worker case 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, and both also assert that `pending` still travels: a fallback that dropped the handle would leave the file unreachable from the very screen that just said the save failed. Held in one class against the ticket's suggestion of three. They are one rule at three layers, and the masking above has to be explained once rather than three times. FailedSaveRetryTest already sets the precedent for both ViewModels in one file; this adds one worker to that shape. FORCE_SOFTWARE in the worker fixture 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 arrives, but by a path this is not about and which HardwareFallbackTest owns. Mutations, all run and restored -- each site to .orEmpty(), never to a different constant, which would only prove the test reads a constant: ConversionWorker:316 1 of 587 red (this file) ConversionViewModel:631 red JoinViewModel:416 red Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/MessagelessFailureTest.kt | 213 ++++++++++++++++++ 1 file changed, 213 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/convert/MessagelessFailureTest.kt 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() +} -- 2.47.3 From cc215195ee2f4e9eed8b4773e86c03e3c942b321 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 2 Sep 2026 18:05:11 -0500 Subject: [PATCH 2/4] Build a command for the audio the user turned off (#198) audioArgs' Drop arm -- `AudioPlan.Drop -> listOf("-an")` -- was ci == 0. The suite's only -an assertion lives in "gif generates a palette to avoid banding and drops audio", and that one comes from the image path at 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 -- so "convert this and drop the soundtrack" is something a user can do today and nothing had built the command for. Both halves are asserted, and the second is not padding: -an alone still passes if the arm falls through to the else and emits an AAC encoder beside the flag, which is a file that is silent because the flag won while carrying an encoder nobody asked for. Three mutations, all run and restored. The third is the one that justifies the second assertion, since the first two break -an as a side effect and so cannot show it: Drop -> emptyList() red Drop -> the else arm's aac encoder red (loses -an as well) Drop -> listOf("-an", "-c:a", "aac") red -- -an intact, caught by assertFalse Co-Authored-By: Claude Opus 5 (1M context) --- .../ffmpeg/FFmpegCommandBuilderTest.kt | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) 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) -- 2.47.3 From da8d53851bd45c2b9086aaa497b35718836d300c Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 2 Sep 2026 18:07:43 -0500 Subject: [PATCH 3/4] Render the container row for a video nothing could name (#199) ConverterScreen.kt:668's null arm -- DetailRow("Container", probe.container?.label ?: "Unknown") in the VIDEO branch -- had never rendered. Every video case in FileCardTest uses VIDEO_PROBE, which carries container = MP4. The argument for adding it is the asymmetry, not the coverage. FileCard renders that exact expression twice, once in AUDIO_ONLY (:660) and once in VIDEO (: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 and one not -- which is the same argument CLAUDE.md records for including ContainerCapabilities:94. Nor is null an edge case here. InputProbe.container's own KDoc says MediaExtractor cannot report a container at all, so it comes from FFprobe alone: any run where FFprobe did not answer produces exactly this shape -- real codec, real dimensions, real duration, no container. An empty value in its place would read as a rendering bug rather than as a probe that got half its sources. The other three rows are asserted alongside, which is what keeps this from being 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 have to be unaffected by it. Mutation: `?: "Unknown"` -> `?: ""` at :668 only, run and restored. The AUDIO_ONLY twin at :660 is a separate expression, and mutating that one would redden the existing test instead -- which would prove nothing about this one. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/FileCardTest.kt | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) 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. -- 2.47.3 From 6d700f00143acc74810d23535cf04987bbb50646 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 2 Sep 2026 18:15:53 -0500 Subject: [PATCH 4/4] Cut a seam through the codec enumeration, and say what a failed one actually does (#194) probe() was 20 never-executed lines, the biggest single block on the report. It has been looked at twice and left out twice, and both closes were right about what they closed: #86 ruled it device-bound, and #133 re-checked that with ShadowMediaCodecList in hand and still declined, because 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 taking its own entry type, which #133 did not evaluate. capabilitiesFrom(enumerate: () -> Sequence) holds every rule; the edge keeps only the mapping from MediaCodecList onto CodecEntry. The parameter is a Sequence rather than a List on purpose. runCatching has always wrapped the *iteration*, so a MediaCodecInfo whose properties throw partway leaves the codecs already read in place. A List parameter would move that throw outside the loop and turn a partial answer into an empty one -- a behaviour change smuggled in as a refactor. There is now a test for the partial case, and swapping the Sequence for an eager toList() reddens it. The behaviour change this DOES make is one line of log, and it is the reason the seam was worth cutting at all. The fallback said "Codec enumeration failed; assuming permissive" and returned empty sets -- but "video/avc" in emptySet() is false, so canEncode and canDecode answer no to everything and every job routes to FFmpeg. That is the restrictive answer, and it is the right one: FFmpeg does whatever Media3 does, only slower. The code stays; the message and the class KDoc now describe it. Seven tests. One of them was wrong first and the mutation is what said so: the alias case originally listed the alias *after* the codec it aliases and passed with the skip deleted, because canonicalName is shared and the dedup catches the second entry either way. The two rules overlap, so a fixture that does not separate them tests neither. Order separates them -- an alias arriving first claims the canonical name in `seen` and gets its own types credited, and the real codec is then dropped by the dedup. That is now the test, and it also says what the rule is worth: with a Set accumulator, an alias declaring the same types as its codec changes nothing, so the skip earns its place only when the two disagree. Mutations, each run and restored, each reddening the test that owns it: drop !isSoftwareOnly from the encoder predicate 1 red remove the alias skip 1 red (0 before the fixture was fixed) remove the canonical-name dedup 1 red remove the video/ prefix filter 1 red apply the hardware predicate to decoders too 1 red make the failure fallback permissive 2 red eager toList() instead of the lazy Sequence 1 red Co-Authored-By: Claude Opus 5 (1M context) --- .../codec/AndroidDeviceCodecs.kt | 81 ++++++- .../codec/CodecEnumerationTest.kt | 201 ++++++++++++++++++ 2 files changed, 274 insertions(+), 8 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/codec/CodecEnumerationTest.kt 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" + } +} -- 2.47.3