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" + } +}