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

Two engines answer the same return code differently, and neither answer is tested #203

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

Wave 4, filed from a coverage read on main @ 54ca2dd, 2026-09-02. The shared filter note is on #194. This one changes a message the user can see, and the decision has been made — see below.

Two engines, one when, two different answers to the same question

app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegEngine.kt:54-67
app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt:69-79

// FFmpegEngine
else -> cont.resumeWithException(FFmpegException(
    "FFmpeg failed (${rc?.value}): " +
        completed.getFailStackTrace().orEmpty().ifBlank {
            completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty()
        },
))

// ConcatEngine
else -> cont.resumeWithException(FFmpegEngine.FFmpegException(
    "Joining failed (${rc?.value}): " +
        completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty(),
))

Near-duplicate blocks: rc 0 resumes, rc 255 cancels, anything else fails. The difference is the message — one prefers the fail stack trace and falls back to the log tail, the other only ever reads the log tail. Neither is tested on the JVM, because both live inside a callback handed to FFmpegKit, and the divergence is invisible while they are two blocks in two files.

The seam

internal fun sessionOutcome(
    rc: ReturnCode?,
    prefix: String,
    failStackTrace: String?,
    logTail: String?,
): SessionOutcome    // Success | Cancelled | Failed(message)

Each engine keeps its own prefix and maps the result onto cont.resume / cont.cancel / cont.resumeWithException. The coroutine wiring and invokeOnCancellation stay device-only, covered by FFmpegEngineTest and ConcatEngineTest.

Verified JVM-safe rather than assumed. javap over the AAR's transformed runtime jar: ReturnCode has a plain public ReturnCode(int) constructor, SUCCESS/CANCEL int constants, and pure static isSuccess/isCancel. Its static {} is constant initialisation. Constructing one loads no native library.

The decision, made: unify on the stack trace

Both engines now prefer the fail stack trace, falling back to the log tail. Join failures start carrying diagnostics they did not before.

This changes user-visible text, which is why it is a decision rather than a refactor. It breaks nothing in the suite today: grep -rn 'Joining failed\|FFmpeg failed' app/src/ returns four hits, all in main, none in test or androidTest — and ConcatWorker.GENERIC_FAILURE_MESSAGE ("Joining failed.") is a different constant this does not touch, as is SharedFailureMessagesTest's TOO_FEW_INPUTS_MESSAGE. Re-run that grep before starting, in case something landed in between.

Behaviour the tests assert

  • rc 0 resumes; rc 255 cancels rather than failing.
  • Any other rc fails with a message carrying the numeric code.
  • A non-blank fail stack trace is preferred; a blank one falls back to the log tail; both blank still yields a message with the code rather than a dangling colon.
  • Both prefixes survive: an FFmpeg failure still says "FFmpeg failed", a join still says "Joining failed". Unifying the strategy must not unify the prefix.

Acceptance: the mutation that must go red

Swap the ifBlank operands, so the log tail wins over the stack trace. Restore, confirm green.

Scope

androidTest must compile, and the join legs are worth running locally (tools/local-emulator/run-e2e.sh, API 33-36) because this touches a real failure path that only the device exercises end to end.

This is the weakest of the wave's four seams — it buys one function and a visible-message decision rather than a large uncovered block. If it looks like more churn than it is worth once opened, closing it with a written finding is a valid outcome; what is not valid is cutting the seam and quietly changing the join message without the tests.

_Wave 4, filed from a coverage read on `main` @ `54ca2dd`, 2026-09-02. The shared filter note is on #194. **This one changes a message the user can see**, and the decision has been made — see below._ ## Two engines, one `when`, two different answers to the same question ``` app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegEngine.kt:54-67 app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt:69-79 ``` ```kotlin // FFmpegEngine else -> cont.resumeWithException(FFmpegException( "FFmpeg failed (${rc?.value}): " + completed.getFailStackTrace().orEmpty().ifBlank { completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty() }, )) // ConcatEngine else -> cont.resumeWithException(FFmpegEngine.FFmpegException( "Joining failed (${rc?.value}): " + completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty(), )) ``` Near-duplicate blocks: rc 0 resumes, rc 255 cancels, anything else fails. The difference is the message — one prefers the fail stack trace and falls back to the log tail, the other only ever reads the log tail. Neither is tested on the JVM, because both live inside a callback handed to `FFmpegKit`, and **the divergence is invisible while they are two blocks in two files.** ## The seam ```kotlin internal fun sessionOutcome( rc: ReturnCode?, prefix: String, failStackTrace: String?, logTail: String?, ): SessionOutcome // Success | Cancelled | Failed(message) ``` Each engine keeps its own prefix and maps the result onto `cont.resume` / `cont.cancel` / `cont.resumeWithException`. The coroutine wiring and `invokeOnCancellation` stay device-only, covered by `FFmpegEngineTest` and `ConcatEngineTest`. **Verified JVM-safe rather than assumed.** `javap` over the AAR's transformed runtime jar: `ReturnCode` has a plain `public ReturnCode(int)` constructor, `SUCCESS`/`CANCEL` int constants, and pure static `isSuccess`/`isCancel`. Its `static {}` is constant initialisation. Constructing one loads no native library. ## The decision, made: unify on the stack trace Both engines now prefer the fail stack trace, falling back to the log tail. Join failures start carrying diagnostics they did not before. **This changes user-visible text**, which is why it is a decision rather than a refactor. It breaks nothing in the suite today: `grep -rn 'Joining failed\|FFmpeg failed' app/src/` returns four hits, all in `main`, none in `test` or `androidTest` — and `ConcatWorker.GENERIC_FAILURE_MESSAGE` (`"Joining failed."`) is a *different* constant this does not touch, as is `SharedFailureMessagesTest`'s `TOO_FEW_INPUTS_MESSAGE`. **Re-run that grep before starting**, in case something landed in between. ## Behaviour the tests assert - rc 0 resumes; rc 255 cancels rather than failing. - Any other rc fails with a message carrying the numeric code. - A non-blank fail stack trace is preferred; a blank one falls back to the log tail; both blank still yields a message with the code rather than a dangling colon. - Both prefixes survive: an FFmpeg failure still says "FFmpeg failed", a join still says "Joining failed". Unifying the *strategy* must not unify the *prefix*. ## Acceptance: the mutation that must go red Swap the `ifBlank` operands, so the log tail wins over the stack trace. Restore, confirm green. ## Scope `androidTest` must compile, and the join legs are worth running locally (`tools/local-emulator/run-e2e.sh`, API 33-36) because this touches a real failure path that only the device exercises end to end. This is the weakest of the wave's four seams — it buys one function and a visible-message decision rather than a large uncovered block. **If it looks like more churn than it is worth once opened, closing it with a written finding is a valid outcome**; what is not valid is cutting the seam and quietly changing the join message without the tests.
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#203