From 31f249ae0492fe5e8f8b376c4af70c623d898a75 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sun, 6 Sep 2026 02:28:18 -0500 Subject: [PATCH] Cancel a running Media3 export, completing #224's third engine The two FFmpeg engines were done in ad2a75d and d293646. This is Media3Engine.transcode's invokeOnCancellation, which posts transformer.cancel() onto the engine's own HandlerThread because cancel() has the same single-thread requirement as start(). The assertion is the output file here, where it could not be for FFmpeg. That side deletes the partial on cancellation, and on POSIX ffmpeg keeps writing to the unlinked inode, so the path stays gone whether or not the cancel landed -- it asserts the session's return code instead. Media3Engine deletes nothing, the partial being ConversionWorker's to clean up, so the file is the evidence. A cancelled export reports itself two ways and both mean interrupted: no video track, or MediaExtractor refusing the file outright with "Failed to instantiate extractor" because there is no moov atom. The first version treated only the null as success and the exception failed the test, which is how that was measured. Only a playable file counts as a miss. The wait before reading is several times the export's own length, so a cancel that did not land has certainly finished by then: the failure direction is "the file became playable", never "we did not wait long enough". The attempt is retried for the reason the other two engines measured -- a 3 s 320x240 export outruns a naive cancel on a loaded runner -- and an export that never wrote a file at all is recorded as inconclusive rather than allowed to pass as a cancellation. It carries @FailsOnEmulatorApi37, so FAILS_ON_EMULATOR_API37_BASELINE moves 3 -> 4 in this diff. That file also said removing the marker would grow the gating leg "by two", which has been wrong since the third marker landed; it now names the constant instead of restating it. Verified on a local API 34 emulator: 68 tests, 0 failures, 3 skipped; and with transformer.cancel() removed all five attempts produce a playable video/hevc and the test fails, naming each one. Co-Authored-By: Claude Opus 5 (1M context) --- .../FailsOnEmulatorApi37.kt | 4 +- .../convert/Media3EngineTest.kt | 104 ++++++++++++++++++ 2 files changed, 106 insertions(+), 2 deletions(-) diff --git a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt index a816864..35d27d1 100644 --- a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt +++ b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt @@ -17,7 +17,7 @@ package org.libremediaconverter * * Removing it is the goal, and the trigger is written down: a new API 37.x system image, or an * ATD image for 37. Delete the annotation from the tests, and the advisory job goes empty and - * the gating one grows by two. + * the gating one grows by [FAILS_ON_EMULATOR_API37_BASELINE]. * * **How many tests carry it is committed below**, as [FAILS_ON_EMULATOR_API37_BASELINE], and the * advisory job checks the run against it. Adding or removing a marker means changing that number @@ -52,4 +52,4 @@ annotation class FailsOnEmulatorApi37 * `INSTRUMENTATION_ABORTED`, so the count is a number taken from a partial run. The report * records the truncation next to the counts for that reason. */ -const val FAILS_ON_EMULATOR_API37_BASELINE = 3 +const val FAILS_ON_EMULATOR_API37_BASELINE = 4 diff --git a/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt b/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt index 3ae4173..fe6e1ea 100644 --- a/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt @@ -7,6 +7,10 @@ import androidx.media3.common.MimeTypes import androidx.media3.common.util.UnstableApi import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry +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 @@ -14,6 +18,7 @@ import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertTrue +import org.junit.Assert.fail import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -262,6 +267,92 @@ class Media3EngineTest { } } + /** + * Cancelling a *running* export stops it, completing #224's third engine. + * + * The two FFmpeg engines were done first (`ad2a75d`, `d293646`); this is + * `Media3Engine.transcode`'s `invokeOnCancellation`, which posts `transformer.cancel()` onto the + * engine's own `HandlerThread` because `cancel()` has the same single-thread requirement as + * `start()`. + * + * ## Why the assertion is the output file here, and was not for FFmpeg + * + * The FFmpeg side could not use the file: `invokeOnCancellation` unlinks it, and on POSIX ffmpeg + * keeps writing to the unlinked inode, so the path stays gone whether or not the cancel landed. + * It asserted the session's return code instead. + * + * `Media3Engine` deletes nothing — the partial is `ConversionWorker`'s to clean up — so the file + * *is* the evidence. An export that was cancelled leaves no moov atom, so `MediaExtractor` + * either finds no video track or refuses the file outright with + * `IOException: Failed to instantiate extractor` — measured, and both mean interrupted. One + * that ran to completion leaves a playable HEVC file, which is the only outcome treated as a + * miss. The wait before + * reading it is deliberately several times the length of the export, so a *non*-cancelled export + * has certainly finished by then: the failure direction is "the file became valid", never "we + * did not wait long enough". + * + * ## Why it retries + * + * Same reason as the other two, measured there: the committed fixture is 3 s at 320x240 and the + * export outruns a naive cancel on a loaded runner. An attempt whose export finished before the + * cancel landed has tested nothing, so it is a miss and is retried; only exhausting + * [CANCEL_ATTEMPTS] fails. With `transformer.cancel()` removed every attempt produces a playable + * file, so the mutation still bites — it just takes five tries to say so. + * + * Progress having been reported is what proves the export really started, so a miss is + * distinguishable from an export that never ran at all — which matters on the API 37 image, + * where the decoder is what fails. + */ + @Test + @FailsOnEmulatorApi37 + fun cancellingARunningExportStopsIt(): Unit = runBlocking { + val outcomes = mutableListOf() + + repeat(CANCEL_ATTEMPTS) { attempt -> + val partial = File(context.cacheDir, "cancelled_export_$attempt.mp4").apply { delete() } + + val job = launch(Dispatchers.IO) { + engine.transcode( + input = Uri.fromFile(input), + output = partial, + request = ConversionRequest(OutputFormat.MP4_H265.spec), + ) + } + + // The muxer creating the file is proof the export really started, and it is the + // earliest such proof available -- earlier than the first progress tick. + withTimeout(TIMEOUT_MS) { + while (!partial.exists() && job.isActive) delay(POLL_MS) + } + val started = partial.exists() + job.cancelAndJoin() + + if (!started) { + // The export failed before writing anything. That is not a cancellation result + // either way, so it is not allowed to pass as one. + outcomes += "attempt $attempt never produced an output file to cancel" + return@repeat + } + + // Several times the export's own length, so a cancel that did not land has certainly + // finished. The failure direction is "the file became playable", never "too soon". + delay(SETTLE_MS) + + // A cancelled export reports itself two ways and both mean the same thing: no video + // track, or MediaExtractor refusing the file outright with "Failed to instantiate + // extractor" because there is no moov atom to read. Only a *playable* file is a miss. + val video = runCatching { videoMimeTypeOf(partial) }.getOrNull() + partial.delete() + if (video == null) return@runBlocking + outcomes += "attempt $attempt produced a playable $video" + } + + fail( + "never interrupted a running export in $CANCEL_ATTEMPTS attempts, so either every " + + "export finished first or cancellation does not reach the transformer: $outcomes", + ) + } + private fun videoMimeTypeOf(file: File): String? { val extractor = MediaExtractor() try { @@ -280,6 +371,19 @@ class Media3EngineTest { private companion object { const val TIMEOUT_SECONDS = 120L + /** Bounds the wait for the muxer to create the file; a hang here is a defect. */ + const val TIMEOUT_MS = 30_000L + const val POLL_MS = 25L + + /** + * How long to let a *failed* cancel finish. Several times the export's own length, so + * "the file is not playable" cannot mean "not yet". + */ + const val SETTLE_MS = 10_000L + + /** See the KDoc: a miss is the loaded-runner case, not a defect. */ + const val CANCEL_ATTEMPTS = 5 + /** * Short on purpose. Nothing is decoded or encoded on this path — the builder refuses the * input outright — so anything approaching this is a hang, which is what the test is -- 2.47.3