From ec2cae256f40c3ae00f2d2d91d9a69d30e5bc904 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 2 Sep 2026 19:16:53 -0500 Subject: [PATCH] Give both engines one rc-to-outcome function, and unify the failure message (#203) FFmpegEngine and ConcatEngine each carried their own copy of the same `when`, and the copies had drifted: one preferred the fail stack trace and fell back to the log tail, the other only ever read the log tail. Neither was tested -- both live inside a callback handed to FFmpegKit, which does not run on the JVM -- so nothing could see that the two disagreed about what a failed session says. sessionOutcome() now holds the rule and each engine maps Success/Cancelled/Failed onto its continuation. Verified JVM-safe rather than assumed: javap over the committed AAR shows ReturnCode(int) as a plain public constructor with pure static isSuccess/isCancel and a that loads no native library. Per #203's decision this unifies on the stack trace, so a join failure now carries the diagnostics a conversion failure always did. The PREFIX stays per-engine: unifying the strategy must not unify the sentence, since a join reporting "FFmpeg failed" would be a worse message than the one it replaces. There is a test for exactly that. The two message sources are lambdas rather than values, and that is load-bearing. getFailStackTrace and getAllLogsAsString are calls onto a native session, and only the failure arm needs either; taking them by value would put both on the happy path of every successful conversion, which the shape this replaces did not -- it read them inside the else branch. Same reasoning as capabilitiesFrom taking a Sequence in #194: a seam should not change what runs when. There is a test that counts the reads, and the eager mutation reddens it. A null return code is a real input rather than a defensive one -- getReturnCode() is nullable and a session killed before reporting has none -- so it fails, with "null" where the number would be. Nothing asserted the old join text: `grep -rn 'Joining failed|FFmpeg failed' app/src/` returns only main, plus ConcatWorker.GENERIC_FAILURE_MESSAGE, which is a different constant this does not touch. Re-run immediately before committing, as the ticket asked. Mutations, all run and restored: swap the ifBlank operands 1 red treat cancellation as a failure 2 red read both message sources eagerly 1 red hardcode the prefix 1 red Co-Authored-By: Claude Opus 5 (1M context) --- .../ffmpeg/ConcatEngine.kt | 21 ++- .../ffmpeg/FFmpegEngine.kt | 24 ++-- .../ffmpeg/SessionOutcome.kt | 54 ++++++++ .../ffmpeg/SessionOutcomeTest.kt | 127 ++++++++++++++++++ 4 files changed, 201 insertions(+), 25 deletions(-) create mode 100644 app/src/main/java/org/libremediaconverter/ffmpeg/SessionOutcome.kt create mode 100644 app/src/test/java/org/libremediaconverter/ffmpeg/SessionOutcomeTest.kt diff --git a/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt b/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt index 932a32d..c8e9997 100644 --- a/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt +++ b/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt @@ -5,7 +5,6 @@ import android.net.Uri import android.util.Log import com.arthenica.ffmpegkit.FFmpegKit import com.arthenica.ffmpegkit.FFmpegKitConfig -import com.arthenica.ffmpegkit.ReturnCode import kotlinx.coroutines.suspendCancellableCoroutine import org.libremediaconverter.convert.ConcatJoiner import org.libremediaconverter.convert.MediaProbe @@ -66,16 +65,16 @@ class ConcatEngine(private val context: Context) : ConcatJoiner { private suspend fun execute(args: List) = suspendCancellableCoroutine { cont -> Log.i(TAG, "ffmpeg ${args.joinToString(" ")}") val session = FFmpegKit.executeWithArgumentsAsync(args.toTypedArray()) { completed -> - val rc = completed.getReturnCode() - when { - ReturnCode.isSuccess(rc) -> cont.resume(Unit) - ReturnCode.isCancel(rc) -> cont.cancel() - else -> cont.resumeWithException( - FFmpegEngine.FFmpegException( - "Joining failed (${rc?.value}): " + - completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty(), - ), - ) + val outcome = sessionOutcome( + rc = completed.getReturnCode(), + prefix = "Joining", + failStackTrace = { completed.getFailStackTrace() }, + logTail = { completed.getAllLogsAsString(LOG_TAIL_LIMIT) }, + ) + when (outcome) { + SessionOutcome.Success -> cont.resume(Unit) + SessionOutcome.Cancelled -> cont.cancel() + is SessionOutcome.Failed -> cont.resumeWithException(FFmpegEngine.FFmpegException(outcome.message)) } } cont.invokeOnCancellation { FFmpegKit.cancel(session.getSessionId()) } diff --git a/app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegEngine.kt b/app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegEngine.kt index f830ca8..d365aff 100644 --- a/app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegEngine.kt +++ b/app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegEngine.kt @@ -4,7 +4,6 @@ import android.util.Log import com.arthenica.ffmpegkit.FFmpegKit import com.arthenica.ffmpegkit.FFmpegKitConfig import com.arthenica.ffmpegkit.Level -import com.arthenica.ffmpegkit.ReturnCode import kotlinx.coroutines.suspendCancellableCoroutine import org.libremediaconverter.convert.SoftwareTranscoder import org.libremediaconverter.model.ConversionRequest @@ -51,19 +50,16 @@ class FFmpegEngine : SoftwareTranscoder { val session = FFmpegKit.executeWithArgumentsAsync( args.toTypedArray(), { completed -> - val rc = completed.getReturnCode() - when { - ReturnCode.isSuccess(rc) -> cont.resume(Unit) - ReturnCode.isCancel(rc) -> - cont.cancel() - else -> cont.resumeWithException( - FFmpegException( - "FFmpeg failed (${rc?.value}): " + - completed.getFailStackTrace().orEmpty().ifBlank { - completed.getAllLogsAsString(LOG_TAIL_LIMIT).orEmpty() - }, - ), - ) + val outcome = sessionOutcome( + rc = completed.getReturnCode(), + prefix = "FFmpeg", + failStackTrace = { completed.getFailStackTrace() }, + logTail = { completed.getAllLogsAsString(LOG_TAIL_LIMIT) }, + ) + when (outcome) { + SessionOutcome.Success -> cont.resume(Unit) + SessionOutcome.Cancelled -> cont.cancel() + is SessionOutcome.Failed -> cont.resumeWithException(FFmpegException(outcome.message)) } }, { log -> Log.d(TAG, log.message.trimEnd()) }, diff --git a/app/src/main/java/org/libremediaconverter/ffmpeg/SessionOutcome.kt b/app/src/main/java/org/libremediaconverter/ffmpeg/SessionOutcome.kt new file mode 100644 index 0000000..2a412ad --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/ffmpeg/SessionOutcome.kt @@ -0,0 +1,54 @@ +package org.libremediaconverter.ffmpeg + +import com.arthenica.ffmpegkit.ReturnCode + +/** + * What a finished FFmpegKit session means, as a function of its return code. + * + * Both engines had their own copy of this `when`, twelve lines apart in two files, and the copies + * had drifted: [FFmpegEngine] preferred the fail stack trace and fell back to the log tail, while + * [ConcatEngine] only ever read the log tail. Neither was tested — both live inside a callback + * handed to `FFmpegKit`, which does not run on the JVM — so the divergence was invisible. + * + * #203 decided to unify on the stack trace, so a join failure now carries the diagnostics a + * conversion failure always did. The *prefix* stays per-engine: unifying the strategy must not + * unify the sentence, since "FFmpeg failed" and "Joining failed" describe different jobs. + */ +internal sealed interface SessionOutcome { + + /** rc 0. The suspension resumes normally. */ + data object Success : SessionOutcome + + /** rc 255. The suspension is cancelled rather than failed — the user asked for this. */ + data object Cancelled : SessionOutcome + + /** Anything else, with the sentence the user is shown. */ + data class Failed(val message: String) : SessionOutcome +} + +/** + * Maps a return code onto the outcome, and builds the failure sentence when there is one. + * + * **The two message parts arrive as lambdas, deliberately.** `getAllLogsAsString` and + * `getFailStackTrace` are calls onto a native session, and only the failure arm needs either. Taking + * them by value would put both on the happy path of every successful conversion, which is a cost the + * shape this replaced did not have — the old code read them inside the `else` branch. That is the + * same reason [org.libremediaconverter.codec.AndroidDeviceCodecs.capabilitiesFrom] takes a + * `Sequence`: a seam should not change what runs when. + * + * A null [rc] is a real input rather than a defensive one — `getReturnCode()` is nullable, and a + * session killed before it reported anything has none. It is neither success nor cancellation, so + * it fails, and the sentence says `null` where the number would be. + */ +internal fun sessionOutcome( + rc: ReturnCode?, + prefix: String, + failStackTrace: () -> String?, + logTail: () -> String?, +): SessionOutcome = when { + ReturnCode.isSuccess(rc) -> SessionOutcome.Success + ReturnCode.isCancel(rc) -> SessionOutcome.Cancelled + else -> SessionOutcome.Failed( + "$prefix failed (${rc?.value}): " + failStackTrace().orEmpty().ifBlank { logTail().orEmpty() }, + ) +} diff --git a/app/src/test/java/org/libremediaconverter/ffmpeg/SessionOutcomeTest.kt b/app/src/test/java/org/libremediaconverter/ffmpeg/SessionOutcomeTest.kt new file mode 100644 index 0000000..3639c60 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/ffmpeg/SessionOutcomeTest.kt @@ -0,0 +1,127 @@ +package org.libremediaconverter.ffmpeg + +import com.arthenica.ffmpegkit.ReturnCode +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * What a finished FFmpegKit session means, for both engines at once. + * + * `FFmpegEngine` and `ConcatEngine` each carried their own copy of this `when`, and the copies had + * drifted: one preferred the fail stack trace and fell back to the log tail, the other only ever + * read the log tail. Neither was tested, because both live inside a callback handed to `FFmpegKit`, + * which does not run on the JVM — so nothing could see that the two disagreed. + * + * **JVM-safe, verified rather than assumed.** `javap` over the committed AAR's runtime jar shows + * `ReturnCode(int)` as a plain public constructor with `SUCCESS`/`CANCEL` int constants and pure + * static `isSuccess`/`isCancel`; its `` is constant initialisation and loads no native + * library. + * + * The unification is #203's decision, so the tests pin it as one: a join failure now carries the + * stack trace a conversion failure always did, while the two prefixes stay distinct. + */ +class SessionOutcomeTest { + + @Test + fun `a return code of zero is success`() { + assertEquals(SessionOutcome.Success, outcome(ReturnCode(ReturnCode.SUCCESS))) + } + + /** + * Cancellation is a separate outcome from failure, and the distinction is the point: the engines + * resume the continuation *cancelled* rather than exceptionally, so a user who pressed Cancel + * does not get an error card. + */ + @Test + fun `a return code of 255 is a cancellation, not a failure`() { + assertEquals(SessionOutcome.Cancelled, outcome(ReturnCode(ReturnCode.CANCEL))) + } + + @Test + fun `any other return code fails, and the sentence carries the number`() { + val failed = outcome(ReturnCode(1), stackTrace = "boom") as SessionOutcome.Failed + + assertTrue("the code belongs in the message, got: ${failed.message}", failed.message.contains("(1)")) + } + + /** + * The half that was different between the two engines before #203, now the same in both. + */ + @Test + fun `the stack trace is preferred over the log tail`() { + val failed = outcome(ReturnCode(1), stackTrace = "the real cause", logTail = "…noise…") + as SessionOutcome.Failed + + assertTrue(failed.message.contains("the real cause")) + assertTrue("the log tail must not be appended as well", !failed.message.contains("noise")) + } + + @Test + fun `a blank stack trace falls back to the log tail`() { + val blank = outcome(ReturnCode(1), stackTrace = " ", logTail = "the last few lines") as SessionOutcome.Failed + val absent = outcome(ReturnCode(1), stackTrace = null, logTail = "the last few lines") as SessionOutcome.Failed + + assertTrue(blank.message.contains("the last few lines")) + assertTrue("a null stack trace is a blank one", absent.message.contains("the last few lines")) + } + + /** + * Both sources empty still has to produce a sentence. A message ending in a dangling colon is + * thin, but it is what the user gets when FFmpeg said nothing at all, and it must not be an + * exception on the way to the screen. + */ + @Test + fun `a failure with nothing to say still names the code`() { + val failed = outcome(ReturnCode(1), stackTrace = null, logTail = null) as SessionOutcome.Failed + + assertEquals("FFmpeg failed (1): ", failed.message) + } + + /** + * `getReturnCode()` is nullable and a session killed before it reported anything has none. + * Neither success nor cancellation, so it fails — and the sentence says so rather than throwing. + */ + @Test + fun `a session with no return code at all fails`() { + val failed = outcome(null, logTail = "whatever was logged") as SessionOutcome.Failed + + assertTrue("got: ${failed.message}", failed.message.startsWith("FFmpeg failed (null): ")) + } + + /** + * Unifying the *strategy* must not unify the *sentence*: the two engines describe different + * jobs, and a join that reports "FFmpeg failed" is a worse message than the one it replaced. + */ + @Test + fun `each engine keeps its own prefix`() { + val join = sessionOutcome(ReturnCode(1), "Joining", { "cause" }, { null }) as SessionOutcome.Failed + + assertTrue(join.message.startsWith("Joining failed (1): ")) + } + + /** + * Neither message source is read unless the outcome is a failure. + * + * They are calls onto a native session, and reading them on the happy path is work every + * successful conversion would do for nothing — which the shape this replaced did not, since it + * read them inside the `else` branch. That is why the parameters are lambdas, and this is what + * would notice if they stopped being. + */ + @Test + fun `a session that succeeded reads neither the stack trace nor the log`() { + var reads = 0 + fun counted(): String? { + reads++ + return null + } + + sessionOutcome(ReturnCode(ReturnCode.SUCCESS), "FFmpeg", ::counted, ::counted) + sessionOutcome(ReturnCode(ReturnCode.CANCEL), "FFmpeg", ::counted, ::counted) + + assertEquals("neither source may be touched unless the session failed", 0, reads) + } + + private fun outcome(rc: ReturnCode?, stackTrace: String? = null, logTail: String? = null) = + sessionOutcome(rc, "FFmpeg", { stackTrace }, { logTail }) +}