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

An unreadable clip in a join: the catch that degrades it, and the guard that makes the audio asymmetry safe #170

New Issue
Closed
opened 2026-09-02 02:14:56 +00:00 by JMR-dev · 1 comment
JMR-dev commented 2026-09-02 02:14:56 +00:00 (Migrated from github.com)
Copy Link
Copy Source

An unreadable clip in a join: the catch that degrades it, and the guard that makes it safe

Two coupled facts, neither tested.

1. MediaProbe.probeForConcat's catch arm has no test on any source set

app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt:295-306. The
catch (e: Exception) returning ConcatInput(null, null, 0, 0, 0) (:300-302) is exercised
only by androidTest/join/ConcatEngineTest.kt:169-170, and only against real committed fixtures —
never against a failing input.

Without the catch, one unreadable input in a join throws out of ConcatEngine instead of degrading
to a re-encode.

Nearly free to close: MediaProbeNativeLoadTest already establishes that
MediaExtractor.setDataSource(context, Uri.parse("content://test/holiday.mp4"), null) throws under
Robolectric for an unregistered authority. Same URI, handed to probeForConcat.

2. The audio null-guard asymmetry is correct, and untested

ConcatPlanner.plan guards video against a null codec
(app/src/main/java/org/libremediaconverter/model/ConcatStrategy.kt:51) and audio not at all
(:54). That asymmetry is correct — MediaProbe.shortName (MediaProbe.kt:375) returns a
non-null String, so in concatInputFrom a null audioCodec means the track is absent, not
unknown, and two clips with no audio genuinely match.

But it is only safe because the video guard fires first on the all-null ConcatInput that the
catch arm above returns. That coupling is written down nowhere and pinned by nothing.

Add one ConcatPlannerTest case: an unprobeable clip forces a re-encode.

Acceptance: the mutations that must go red

  • Delete the catch arm's ConcatInput(null, null, 0, 0, 0) return (make it rethrow) — the
    probe test fails.
  • Relax ConcatStrategy.kt:51's it.videoCodec == null || — the planner test fails, because an
    all-null pair would then stream-copy.
## An unreadable clip in a join: the catch that degrades it, and the guard that makes it safe Two coupled facts, neither tested. ### 1. `MediaProbe.probeForConcat`'s catch arm has no test on any source set `app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt:295-306`. The `catch (e: Exception)` returning `ConcatInput(null, null, 0, 0, 0)` (`:300-302`) is exercised only by `androidTest/join/ConcatEngineTest.kt:169-170`, and only against real committed fixtures — never against a failing input. Without the catch, one unreadable input in a join throws out of `ConcatEngine` instead of degrading to a re-encode. Nearly free to close: `MediaProbeNativeLoadTest` already establishes that `MediaExtractor.setDataSource(context, Uri.parse("content://test/holiday.mp4"), null)` throws under Robolectric for an unregistered authority. Same URI, handed to `probeForConcat`. ### 2. The audio null-guard asymmetry is correct, and untested `ConcatPlanner.plan` guards video against a null codec (`app/src/main/java/org/libremediaconverter/model/ConcatStrategy.kt:51`) and audio not at all (`:54`). **That asymmetry is correct** — `MediaProbe.shortName` (`MediaProbe.kt:375`) returns a non-null `String`, so in `concatInputFrom` a null `audioCodec` means the track is *absent*, not unknown, and two clips with no audio genuinely match. But it is only safe because the video guard fires first on the all-null `ConcatInput` that the catch arm above returns. That coupling is written down nowhere and pinned by nothing. Add one `ConcatPlannerTest` case: **an unprobeable clip forces a re-encode.** ## Acceptance: the mutations that must go red - Delete the `catch` arm's `ConcatInput(null, null, 0, 0, 0)` return (make it rethrow) — the probe test fails. - Relax `ConcatStrategy.kt:51`'s `it.videoCodec == null ||` — the planner test fails, because an all-null pair would then stream-copy.
JMR-dev commented 2026-09-02 03:09:57 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Correction from the implementation.

probeForConcat's catch arm is not reachable on the JVM, so this ticket's first half cannot be closed as written. Measured before rewriting the test: Robolectric's MediaExtractor never throws from setDataSource.

input result
unregistered content:// authority no throw, trackCount = 0
missing file:// no throw, trackCount = 0
a file of garbage bytes no throw, trackCount = 0
http:// URL no throw, trackCount = 0

A failed read therefore arrives as an empty track list rather than an exception, and reaches the same ConcatInput(null, null, 0, 0, 0) by the other road. The catch stays covered only by ConcatEngineTest on a device.

My first draft's KDoc claimed otherwise and the test passed — rethrowing from the catch left it green, which is what exposed it. #181 says this in the file rather than implying the arm is handled.

What #181 does close is the span: both halves were already covered separately (MediaProbeTrackWalkTest for the probe, ConcatPlannerTest for the planner) and nothing joined them, so the planner's safety rested on the probe really producing that shape with nothing checking that it does.

Correction from the implementation. **`probeForConcat`'s catch arm is not reachable on the JVM**, so this ticket's first half cannot be closed as written. Measured before rewriting the test: Robolectric's `MediaExtractor` never throws from `setDataSource`. | input | result | |---|---| | unregistered `content://` authority | no throw, `trackCount = 0` | | missing `file://` | no throw, `trackCount = 0` | | a file of garbage bytes | no throw, `trackCount = 0` | | `http://` URL | no throw, `trackCount = 0` | A failed read therefore arrives as an empty track list rather than an exception, and reaches the same `ConcatInput(null, null, 0, 0, 0)` by the other road. The catch stays covered only by `ConcatEngineTest` on a device. My first draft's KDoc claimed otherwise and the test passed — rethrowing from the catch left it green, which is what exposed it. #181 says this in the file rather than implying the arm is handled. What #181 does close is the **span**: both halves were already covered separately (`MediaProbeTrackWalkTest` for the probe, `ConcatPlannerTest` for the planner) and nothing joined them, so the planner's safety rested on the probe really producing that shape with nothing checking that it does.
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#170