JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

AndroidDeviceCodecs' enumeration rules have no test, and its failure fallback contradicts its own log line #194

New Issue
Closed
opened 2026-09-02 12:46:11 +00:00 by JMR-dev · 0 comments
JMR-dev commented 2026-09-02 12:46:11 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Wave 4, filed from a coverage read on main @ 54ca2dd, 2026-09-02: 92.8% line (2183/2352), 81.3% branch (1091/1342), 584 JVM tests in 87 classes. This ticket carries the wave's shared filter note, at the bottom.

This is not a reopen of #86 or #133 — read that first

AndroidDeviceCodecs.probe() (app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt:49-76) is 20 never-executed lines, the biggest single block on the report. It has been considered twice and left out twice, and both closes were right about what they were closing:

  • #86 ruled it device-bound: "probe() queries the real MediaCodecList … that is the class's whole purpose and it cannot be answered on the JVM."
  • #133 re-checked that with ShadowMediaCodecList in hand and still declined, for a reason worth quoting: MediaCodecInfoBuilder "has no setIsAlias and no setCanonicalName, so the alias skip (:58) and the canonical-name dedup (:59) — the two things the class's KDoc calls out as easy to get wrong — are not reachable through it. What is reachable is the isHardwareAccelerated && !isSoftwareOnly filter and the video/non-video split, which is enumeration bookkeeping."

That objection is specific to the shadow. A pure seam does not have it, and #133 did not consider one — it evaluated the shadow route and closed it. This ticket is the other mechanism, and it makes reachable exactly the half #133 named as unreachable.

If you are reading this and about to re-argue ShadowMediaCodecList: don't. That spike has been run twice and #133 exists to stop a third.

The seam

internal data class CodecEntry(
    val canonicalName: String,
    val isAlias: Boolean,
    val isEncoder: Boolean,
    val isHardwareAccelerated: Boolean,
    val isSoftwareOnly: Boolean,
    val supportedTypes: List<String>,
)

internal fun capabilitiesFrom(enumerate: () -> List<CodecEntry>): AndroidDeviceCodecs

The thin edge keeps only the mapping from MediaCodecList(REGULAR_CODECS).codecInfos onto CodecEntry. Everything else — the alias skip, the canonical-name dedup, the video/ prefix filter, the encoder/decoder split, the isHardwareAccelerated && !isSoftwareOnly predicate, and the runCatching — moves inside.

The supplier shape is load-bearing, not a preference

A seam taking List<CodecEntry> cannot provoke the failure path at all: the runCatching would stay in the edge, and capabilitiesFrom(emptyList()) would pin empty → restrictive, not failure → restrictive. A test KDoc claiming the latter would be a passing test with a wrong explanation — precisely what wave 3 caught on probeForConcat, where the KDoc claimed to drive a catch arm that turned out to be unreachable.

If the supplier shape proves awkward in practice, say so in the PR and narrow the claim: the test pins empty → restrictive, and the failure → empty line stays uncovered in the edge. Do not write the broader KDoc over the narrower test.

The code change that goes with it: the fallback says the opposite of what it does

:72:

}.onFailure { Log.w(TAG, "Codec enumeration failed; assuming permissive.", it) }
...
return AndroidDeviceCodecs(encoders, decoders)   // both empty on failure

With empty sets, canEncode(H264) resolves mimeFor to video/avc, asks "video/avc" in emptySet() and answers false; canDecode("h264") likewise. So an enumeration failure routes everything to FFmpeg. That is the restrictive fallback, not the permissive one the log line and the class KDoc at :20-23 both imply.

Decided: the code is right and the message is wrong. All-FFmpeg on an unreadable codec list is the safe answer — FFmpeg can do everything Media3 can, only slower. Keep AndroidDeviceCodecs(emptySet(), emptySet()); rewrite the log line to say it routes everything to FFmpeg, and correct the class KDoc to match. The test then pins that as deliberate rather than accidental.

Behaviour the tests assert

  • An entry marked isAlias is skipped, and a second entry sharing a canonicalName is counted once — the two things the KDoc calls out and the two the shadow cannot reach.
  • A video/* encoder reaches hardwareEncoders only when hardware-accelerated and not software-only.
  • audio/* types are ignored entirely on both sides.
  • Decoders collect every video type regardless of the hardware flag.
  • An enumeration failure yields the restrictive profile: canEncode(H264) is false, so the router sends the job to FFmpeg.

Acceptance: the mutation that must go red

Drop !info.isSoftwareOnly from the encoder predicate at :64. Restore, confirm green.

Not the acceptance

The coverage number, for the reason CLAUDE.md and #84, #86 and #88 each give. The argument for cutting this seam is that it turns a device-bound behaviour into a decision a test can choose the inputs for — which is the bar CLAUDE.md sets — and that the failure fallback's behaviour contradicts its own comment today. A seam cut only to make a percentage move is worse than the uncovered line it replaces.

The wave's filter note

Wave 3 filtered candidates to mi > 0. That filter fails in both directions:

  • It over-reports on Compose. JoinScreen.kt:222 shows mi=10 — and also ci=38, and JoinStateAffordancesTest already clicks that Save button and asserts save:joined.mp4. The missed instructions are the synthesized $changed/$dirty recomposition-skip path — the same codegen CLAUDE.md already warns about for branch counts on these files, showing up in the instruction count too. Every onClick lambda body on the flagged screen lines is covered at method level.
  • It under-reports on warm methods with cold arms. ConversionViewModel.cancel() runs in the suite, so no line of it is missed — yet activeWorkId?.let(workManager::cancelWorkById) has only ever been entered on the null side. That is #192, the largest real gap in this wave, and no line-level filter finds it.

Use two filters together:

  1. ci == 0 — the line never executed. This is JaCoCo's own missed-line definition, so it totals exactly the reported 169.
  2. ci > 0 && mb > 0 at method level — a covered method with an arm nothing takes.

Of the 251 missed branches, only 18 sit on lines that do execute, so the branch gap and the line gap are largely the same gap; filter 2 is about which of them are reachable.

This pass was coverage-driven. Assertion gaps — wave 3's more productive find — were not hunted systematically. #200 fell out anyway; do not read this wave as "everything that is untested".

_Wave 4, filed from a coverage read on `main` @ `54ca2dd`, 2026-09-02: **92.8% line (2183/2352), 81.3% branch (1091/1342)**, 584 JVM tests in 87 classes. This ticket carries the wave's shared filter note, at the bottom._ ## This is not a reopen of #86 or #133 — read that first `AndroidDeviceCodecs.probe()` (`app/src/main/java/org/libremediaconverter/codec/AndroidDeviceCodecs.kt:49-76`) is **20 never-executed lines**, the biggest single block on the report. It has been considered twice and left out twice, and **both closes were right about what they were closing**: - **#86** ruled it device-bound: "`probe()` queries the real `MediaCodecList` … that is the class's whole purpose and it cannot be answered on the JVM." - **#133** re-checked that with `ShadowMediaCodecList` in hand and still declined, for a reason worth quoting: `MediaCodecInfoBuilder` "has no `setIsAlias` and no `setCanonicalName`, so the alias skip (`:58`) and the canonical-name dedup (`:59`) — the two things the class's KDoc calls out as easy to get wrong — are not reachable through it. What *is* reachable is the `isHardwareAccelerated && !isSoftwareOnly` filter and the video/non-video split, which is enumeration bookkeeping." **That objection is specific to the shadow.** A pure seam does not have it, and #133 did not consider one — it evaluated the shadow route and closed it. This ticket is the other mechanism, and it makes reachable exactly the half #133 named as unreachable. If you are reading this and about to re-argue `ShadowMediaCodecList`: don't. That spike has been run twice and #133 exists to stop a third. ## The seam ```kotlin internal data class CodecEntry( val canonicalName: String, val isAlias: Boolean, val isEncoder: Boolean, val isHardwareAccelerated: Boolean, val isSoftwareOnly: Boolean, val supportedTypes: List<String>, ) internal fun capabilitiesFrom(enumerate: () -> List<CodecEntry>): AndroidDeviceCodecs ``` The thin edge keeps only the mapping from `MediaCodecList(REGULAR_CODECS).codecInfos` onto `CodecEntry`. Everything else — the alias skip, the canonical-name dedup, the `video/` prefix filter, the encoder/decoder split, the `isHardwareAccelerated && !isSoftwareOnly` predicate, **and the `runCatching`** — moves inside. ### The supplier shape is load-bearing, not a preference A seam taking `List<CodecEntry>` cannot provoke the failure path at all: the `runCatching` would stay in the edge, and `capabilitiesFrom(emptyList())` would pin *empty → restrictive*, not *failure → restrictive*. A test KDoc claiming the latter would be a passing test with a wrong explanation — precisely what wave 3 caught on `probeForConcat`, where the KDoc claimed to drive a `catch` arm that turned out to be unreachable. If the supplier shape proves awkward in practice, **say so in the PR and narrow the claim**: the test pins empty → restrictive, and the failure → empty line stays uncovered in the edge. Do not write the broader KDoc over the narrower test. ## The code change that goes with it: the fallback says the opposite of what it does `:72`: ```kotlin }.onFailure { Log.w(TAG, "Codec enumeration failed; assuming permissive.", it) } ... return AndroidDeviceCodecs(encoders, decoders) // both empty on failure ``` With empty sets, `canEncode(H264)` resolves `mimeFor` to `video/avc`, asks `"video/avc" in emptySet()` and answers **false**; `canDecode("h264")` likewise. So an enumeration failure routes **everything to FFmpeg**. That is the *restrictive* fallback, not the permissive one the log line and the class KDoc at `:20-23` both imply. **Decided: the code is right and the message is wrong.** All-FFmpeg on an unreadable codec list is the safe answer — FFmpeg can do everything Media3 can, only slower. Keep `AndroidDeviceCodecs(emptySet(), emptySet())`; rewrite the log line to say it routes everything to FFmpeg, and correct the class KDoc to match. The test then pins that as deliberate rather than accidental. ## Behaviour the tests assert - An entry marked `isAlias` is skipped, and a second entry sharing a `canonicalName` is counted once — the two things the KDoc calls out and the two the shadow cannot reach. - A `video/*` encoder reaches `hardwareEncoders` only when hardware-accelerated **and** not software-only. - `audio/*` types are ignored entirely on both sides. - Decoders collect every video type regardless of the hardware flag. - An enumeration failure yields the restrictive profile: `canEncode(H264)` is `false`, so the router sends the job to FFmpeg. ## Acceptance: the mutation that must go red Drop `!info.isSoftwareOnly` from the encoder predicate at `:64`. Restore, confirm green. ## Not the acceptance The coverage number, for the reason `CLAUDE.md` and #84, #86 and #88 each give. The argument for cutting this seam is that it turns a device-bound behaviour into a decision a test can choose the inputs for — which is the bar `CLAUDE.md` sets — and that the failure fallback's behaviour contradicts its own comment today. A seam cut only to make a percentage move is worse than the uncovered line it replaces. --- ## The wave's filter note Wave 3 filtered candidates to `mi > 0`. That filter fails in both directions: - **It over-reports on Compose.** `JoinScreen.kt:222` shows `mi=10` — and also `ci=38`, and `JoinStateAffordancesTest` already clicks that Save button and asserts `save:joined.mp4`. The missed instructions are the synthesized `$changed`/`$dirty` recomposition-skip path — the same codegen `CLAUDE.md` already warns about for *branch* counts on these files, showing up in the instruction count too. Every `onClick` lambda body on the flagged screen lines is covered at method level. - **It under-reports on warm methods with cold arms.** `ConversionViewModel.cancel()` runs in the suite, so no line of it is missed — yet `activeWorkId?.let(workManager::cancelWorkById)` has only ever been entered on the **null** side. That is #192, the largest real gap in this wave, and no line-level filter finds it. Use two filters together: 1. **`ci == 0`** — the line never executed. This is JaCoCo's own missed-line definition, so it totals exactly the reported 169. 2. **`ci > 0 && mb > 0` at method level** — a covered method with an arm nothing takes. Of the 251 missed branches, only **18** sit on lines that do execute, so the branch gap and the line gap are largely the same gap; filter 2 is about which of them are reachable. **This pass was coverage-driven.** Assertion gaps — wave 3's more productive find — were not hunted systematically. #200 fell out anyway; do not read this wave as "everything that is untested".
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
above-cut
Worked autonomously overnight: local, JVM-verifiable, no product decision
 accessibility
Barrier affecting people with disabilities
 backlog
Held for manual review: product/UX call, CI/workflow, hardware, or unverifiable here
 bug
Something isn't working
 confirmed
Reviewer demonstrated the defect
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 plausible
Reviewer could not fully demonstrate it; treat as unproven
 question
Further information is requested
 sev:high
High severity
 sev:low
Low severity
 sev:medium
Medium severity
 wontfix
This will not be worked on
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
JMR-dev
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMediaConverter#194