From 5416788274e7a80d3d14819393041030f53e3193 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sun, 6 Sep 2026 00:02:52 -0500 Subject: [PATCH] Cancel a running join session too (#224) The FFmpegEngine half landed in ad2a75d; this is the same gap in ConcatEngine. Between them, a real native session being asked to stop is now covered on both FFmpeg paths. The assertion is the session's return code again, and here that is not merely the better choice but close to the only one: ConcatEngine does not delete its output on cancellation at all. Its invokeOnCancellation is FFmpegKit.cancel and nothing else, where FFmpegEngine's also deletes the partial. Whether that asymmetry is deliberate is a separate question, so this asserts what is true of both engines rather than depending on it. The cancel triggers on SessionState.RUNNING rather than on progress. ConcatWorker publishes no progress at all, so there is no callback to hang it on even in principle -- and the conversion side already measured the deeper reason, that the committed clips outrun a callback-triggered cancel. The inputs are the mismatched pair on purpose, so ConcatPlanner chooses REENCODE. A stream copy of two 2 s clips is close to instantaneous and would leave nothing to interrupt; re-encoding is also the case where a user would actually reach for Cancel. Verified on a local API 34 emulator: three runs green at 64/0/0/3, and replacing invokeOnCancellation's body with an empty block fails this test and nothing else, with state=COMPLETED rc=0. Media3Engine's transformer.cancel() is still uncovered and #224 stays open for it. Co-Authored-By: Claude Opus 5 (1M context) --- .../join/ConcatEngineTest.kt | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt b/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt index 4a7d6cb..25702d6 100644 --- a/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt @@ -5,7 +5,16 @@ import android.media.MediaFormat import android.net.Uri import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry +import com.arthenica.ffmpegkit.FFmpegKit +import com.arthenica.ffmpegkit.FFmpegSession +import com.arthenica.ffmpegkit.ReturnCode +import com.arthenica.ffmpegkit.SessionState +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue @@ -16,6 +25,7 @@ import org.libremediaconverter.convert.MediaProbe import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.ffmpeg.ConcatEngine import org.libremediaconverter.model.ConcatStrategy +import org.libremediaconverter.work.ConcatWorker import java.io.File /** @@ -50,6 +60,65 @@ class ConcatEngineTest { (staged + listOf(clipA, clipB, clipMismatched)).forEach { it.delete() } } + /** + * Cancelling a *running* join actually stops the native session. + * + * The `FFmpegEngine` half of #224 landed first (PR #236); this is the same gap in + * [ConcatEngine]. Before these two, no test on any source set had ever asked a real native + * session to stop — every `cancel` in `app/src/androidTest` targets WorkManager entries that + * are queued or already finished. + * + * ## Two things carried over from the conversion side, both measured there + * + * **The assertion is the session's return code.** A cancelled session ends with the cancel + * code, a completed one does not. The alternative — checking the output file — is even less + * available here than it was for conversions: [ConcatEngine] does not delete its output on + * cancellation at all. Its `invokeOnCancellation` is `FFmpegKit.cancel(...)` and nothing else, + * where [org.libremediaconverter.ffmpeg.FFmpegEngine]'s also deletes the partial. Whether that + * asymmetry is deliberate is a separate question from this test, which is why this asserts the + * thing that is true of both. + * + * **The cancel is triggered on [SessionState.RUNNING], not on progress.** `ConcatWorker` + * publishes no progress at all, so there is no callback to hang it on even in principle — but + * the conversion side established the deeper reason: the committed clips are 2 s at 320x240 and + * the encode outruns a callback-triggered cancel. + * + * The inputs are deliberately the **mismatched** pair, so [ConcatStrategy.REENCODE] is chosen. + * A stream copy of two short clips is close to instantaneous and would leave nothing to + * interrupt; re-encoding is the case where a user would actually reach for Cancel. + * + * *Mutation:* drop `FFmpegKit.cancel(session.getSessionId())` from `ConcatEngine`'s + * `invokeOnCancellation` — the session runs to completion and this fails. + */ + @Test + fun cancellingARunningJoinCancelsTheNativeSession(): Unit = runBlocking { + val before = FFmpegKit.listSessions().map { it.getSessionId() }.toSet() + val out = output("cancelled_join.mp4") + + val job = launch(Dispatchers.IO) { + engine.join(listOf(Uri.fromFile(clipA), Uri.fromFile(clipMismatched)), out, ConcatWorker.DEFAULT_FORMAT) + } + + val ours = withTimeout(TIMEOUT_MS) { + var found: FFmpegSession? = null + while (found?.getState() != SessionState.RUNNING) { + found = FFmpegKit.listSessions().firstOrNull { it.getSessionId() !in before } + if (found?.getState() != SessionState.RUNNING) delay(POLL_MS) + } + found + } + job.cancelAndJoin() + + withTimeout(TIMEOUT_MS) { + while (ours.getState() == SessionState.RUNNING) delay(POLL_MS) + } + + assertTrue( + "the native join session was not cancelled: state=${ours.getState()} rc=${ours.getReturnCode()}", + ReturnCode.isCancel(ours.getReturnCode()), + ) + } + private fun copyAsset(name: String): File { val out = File(context.cacheDir, name) InstrumentationRegistry.getInstrumentation().context.assets @@ -180,4 +249,10 @@ class ConcatEngineTest { a.width != mismatched.width || a.height != mismatched.height, ) } + + private companion object { + /** Generous: it bounds a hang, and both waits here normally settle in well under a second. */ + const val TIMEOUT_MS = 30_000L + const val POLL_MS = 50L + } } -- 2.47.3