From a645442accd2da00ed6025486333f15ec427a048 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 21:42:27 -0500 Subject: [PATCH 1/2] A2 + A3 (#168, #169): the hardware fallback, the cancellation that must not take it, and the name a job may not have runMedia3OrFallBack was eleven lines at 0% and isCancellation had never been called by any JVM test -- ci=0, not merely a missed branch. The seam to reach it has existed the whole time: ConversionDependencies.hardware, which no unit test had ever set. What kept the path cold is that every worker test uses EnginePreference.FORCE_SOFTWARE, which never enters the function, and the probe defaults to UNPARSEABLE, which PERMISSIVE.canDecode refuses -- so even AUTO would have routed straight to FFmpeg for a reason no assertion mentioned. Both are now stated in setUp rather than inherited. Four behaviours, each with the mutation that proves it: hardware failure falls back to software delete the fallback call ... on a *clean* staging file delete staged.delete() before it cancellation is rethrown, not fallen back delete `if (isCancellation(e)) throw e` engine.close() runs either way empty the finally block the display-name fallback (#169) change "input" to anything else All five confirmed red, then restored. The cancellation one is the reason this ticket was first in the group. runMedia3OrFallBack catches Throwable, so without that re-throw a user cancelling a hardware transcode has the app quietly start a *second* conversion in software -- the one thing cancelling is for. ForcedFailureTest covers the failure half on a device and does not cover this half at all. The clean-staging assertion is made where it is observable rather than by reading the file: the software fake records whether the output existed when it was entered, so a missing delete shows up as FFmpeg finding a half-written hardware output at the path it is about to write. 556 -> 560 JVM tests, 0 failures. No production code changed. ConversionWorker: 22 -> 9 missed lines, 14 -> 7 missed branches. Line 2090/2348 -> 2103/2348; branch 1022/1340 -> 1029/1340. Co-Authored-By: Claude Opus 5 (1M context) --- .../work/HardwareFallbackTest.kt | 226 ++++++++++++++++++ 1 file changed, 226 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/work/HardwareFallbackTest.kt diff --git a/app/src/test/java/org/libremediaconverter/work/HardwareFallbackTest.kt b/app/src/test/java/org/libremediaconverter/work/HardwareFallbackTest.kt new file mode 100644 index 0000000..9d420f9 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/HardwareFallbackTest.kt @@ -0,0 +1,226 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.ListenableWorker +import androidx.work.testing.TestListenableWorkerBuilder +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.HardwareTranscoder +import org.libremediaconverter.convert.SoftwareTranscoder +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * What happens when the hardware engine does not finish the job. + * + * `runMedia3OrFallBack` was eleven lines at 0% on the JVM and `isCancellation` had never been + * called by any unit test at all. Its own KDoc calls the fallback the protection against vendor + * hardware encoders that "cannot be tested for correctness", so it is the branch most likely to + * matter on a device nobody here owns — and it was reachable the whole time through + * `ConversionDependencies.hardware`, which no unit test had ever used. + * + * The sharp one is cancellation. `runMedia3OrFallBack` catches `Throwable`, so without the + * `isCancellation` re-throw a user cancelling a hardware transcode would have the app quietly + * start a *second* conversion in software — the one thing cancelling is supposed to prevent. + * + * `ForcedFailureTest` covers the failure half on a device. It does not cover the cancellation half, + * and this host cannot run it either way. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class HardwareFallbackTest { + + private lateinit var app: Application + private lateinit var hardware: RecordingHardwareTranscoder + private lateinit var software: RecordingSoftwareTranscoder + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + hardware = RecordingHardwareTranscoder() + software = RecordingSoftwareTranscoder() + ConversionDependencies.publisher = { AlwaysRoomPublisher(app) } + ConversionDependencies.hardware = { hardware } + ConversionDependencies.software = { software } + // A probe with real codecs, not the default: `InputProbe()` reports UNPARSEABLE, which + // PERMISSIVE.canDecode refuses, and the router would send every job here straight to + // FFmpeg without any of these tests mentioning why. + ConversionDependencies.probe = { _, _ -> H264_SOURCE } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a hardware failure runs the job again in software, on a clean staging file`() { + hardware.failWith = { error("the vendor encoder produced nothing usable") } + + val result = runBlocking { worker().doWork() } + + assertTrue("the job should still succeed, got $result", result is ListenableWorker.Result.Success) + assertEquals("the hardware engine gets exactly one attempt", 1, hardware.attempts) + assertEquals("and the job then goes to software", 1, software.attempts) + // The `staged.delete()` between the two, asserted where it is observable: FFmpeg must not + // find a half-written hardware output sitting at the path it is about to write. + assertFalse( + "the partial hardware output must be gone before FFmpeg starts", + software.outputExistedOnEntry, + ) + assertEquals("the hardware engine is closed either way", 1, hardware.closes) + } + + @Test + fun `a cancelled hardware transcode is not quietly retried in software`() { + hardware.failWith = { throw CancellationException("the user pressed Cancel") } + + assertThrows(CancellationException::class.java) { runBlocking { worker().doWork() } } + + assertEquals("the hardware engine ran", 1, hardware.attempts) + assertEquals( + "cancelling must not start a second conversion -- that is the whole point of cancelling", + 0, + software.attempts, + ) + assertEquals("and the engine is still closed on the way out", 1, hardware.closes) + } + + @Test + fun `a hardware transcode that works never reaches the software engine`() { + val result = runBlocking { worker().doWork() } + + assertTrue("got $result", result is ListenableWorker.Result.Success) + assertEquals(1, hardware.attempts) + assertEquals("the fallback is a fallback, not a second pass", 0, software.attempts) + assertEquals(1, hardware.closes) + } + + /** + * #169: the display-name fallback, which reaches further than the notification title. + * + * `inputData.getString(KEY_DISPLAY_NAME) ?: "input"` had never taken its right-hand side. The + * value is not only the foreground notification's title: it feeds `outputNameFor`, so it is + * also the filename offered in the user's save dialog. A job enqueued by an older build, or + * built by hand, carries no such key. + */ + @Test + fun `a job that names no input file still suggests an output name`() { + val result = runBlocking { worker(displayName = null).doWork() } + + assertTrue("got $result", result is ListenableWorker.Result.Success) + val suggested = (result as ListenableWorker.Result.Success) + .outputData.getString(ConversionWorker.KEY_SUGGESTED_NAME) + assertTrue( + "expected a name built from the fallback, got $suggested", + suggested.orEmpty().startsWith("input"), + ) + } + + private fun worker(displayName: String? = DISPLAY_NAME): ConversionWorker { + val spec = OutputFormat.MP4_H265.spec + val entries = buildMap { + put(ConversionWorker.KEY_INPUT_URI, INPUT.toString()) + displayName?.let { put(ConversionWorker.KEY_DISPLAY_NAME, it) } + put(ConversionWorker.KEY_SIZE_BYTES, INPUT_BYTES) + put(ConversionWorker.KEY_CONTAINER, spec.container.name) + put(ConversionWorker.KEY_VIDEO_CODEC, spec.videoCodec.name) + put(ConversionWorker.KEY_AUDIO_CODEC, spec.audioCodec.name) + // AUTO rather than FORCE_SOFTWARE, which is what every other worker test uses and is + // exactly why this path had no coverage: forcing software never enters the function. + put(ConversionWorker.KEY_ENGINE_PREFERENCE, EnginePreference.AUTO.name) + } + return TestListenableWorkerBuilder( + context = app, + inputData = Data.Builder().putAll(entries).build(), + runAttemptCount = 0, + ).setId(JOB_ID).build() + } + + private companion object { + val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4") + const val DISPLAY_NAME = "holiday.mp4" + const val INPUT_BYTES = 1024L + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000009") + val H264_SOURCE = InputProbe( + videoCodec = "h264", + audioCodec = "aac", + container = Container.MP4, + durationMs = 1_000, + ) + } +} + +/** + * A hardware engine that writes something before it fails, and remembers being closed. + * + * Writing first is the point, exactly as it is for `PartialThenFailingTranscoder`: an engine that + * only threw would let a missing `staged.delete()` pass unnoticed. + */ +@UnstableApi +private class RecordingHardwareTranscoder : HardwareTranscoder { + + var attempts = 0 + var closes = 0 + var failWith: (() -> Unit)? = null + + override suspend fun transcode(input: Uri, output: File, request: ConversionRequest, onProgress: (Int) -> Unit) { + attempts++ + output.writeBytes(ByteArray(PARTIAL_BYTES)) + failWith?.invoke() + } + + override fun close() { + closes++ + } + + private companion object { + const val PARTIAL_BYTES = 2048 + } +} + +/** The software engine, recording whether the hardware attempt's leftovers were cleared first. */ +private class RecordingSoftwareTranscoder : SoftwareTranscoder { + + var attempts = 0 + var outputExistedOnEntry = false + + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + attempts++ + outputExistedOnEntry = output.exists() + output.writeBytes(ByteArray(OUTPUT_BYTES)) + } + + private companion object { + const val OUTPUT_BYTES = 512 + } +} -- 2.47.3 From 2fbc9571196771ea96ac18a13224358a5422695e Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 21:45:43 -0500 Subject: [PATCH 2/2] A5 (#171): fire the muxer guard that repairs "MP4 for everything", which had never fired Media3Muxers' KDoc names the defect this guards -- "the router claimed five containers while the engine silently wrote MP4 for all of them" -- and the repair itself was untested: Media3Engine$buildTransformer$3, the requireNotNull message lambda, was four lines and four branches at 0%. Nothing had ever driven a plan whose container Media3 cannot mux, and factoryFor answers null for fourteen of them. Weakening it does not crash. The wrong output is a playable file with the wrong container, which is why a test rather than a bug report is what would catch it. Same harness and the same two disciplines as Media3EngineEmptyCompositionTest, which is the sibling this joins: assert the plan really is the one the test needs before driving the engine, and rule out CancellationException so an unresumed continuation cannot read as a pass. Three premises are asserted here rather than assumed -- that the plan is still WebM by the time the engine sees it, that Media3 really has no muxer for WebM, and that neither track was dropped, since the empty-composition refusal fires earlier and is a different test's subject. The assertion is on the exception type *and* its message, and the ticket predicted why: replacing requireNotNull with `?: DefaultMuxer.Factory()` does not make the export succeed, it lets it run on and fail some other way. Measured -- that mutation fails the type assertion, so the guard is genuinely what this test is holding, and the message assertion stands behind it. 560 -> 561 JVM tests, 0 failures. No production code changed. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/Media3MuxerGuardTest.kt | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/convert/Media3MuxerGuardTest.kt diff --git a/app/src/test/java/org/libremediaconverter/convert/Media3MuxerGuardTest.kt b/app/src/test/java/org/libremediaconverter/convert/Media3MuxerGuardTest.kt new file mode 100644 index 0000000..8207986 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/Media3MuxerGuardTest.kt @@ -0,0 +1,99 @@ +package org.libremediaconverter.convert + +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.AudioPlan +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.CopyPlanner +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputSpec +import org.libremediaconverter.model.VideoCodec +import org.libremediaconverter.model.VideoPlan +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.concurrent.CancellationException + +/** + * A job that reached Media3 with a container Media3 cannot mux. + * + * [Media3Muxers]' own KDoc names the defect this guards: *"the router claimed five containers while + * the engine silently wrote MP4 for all of them."* `factoryFor` answers null for fourteen of the + * app's containers, and `buildTransformer` turns that null into a failed job rather than letting + * `Transformer` fall back to its default muxer. + * + * The guard had never fired. `Media3Engine$buildTransformer$3` -- the `requireNotNull` message + * lambda -- was four lines and four branches at 0%, which is to say the entire repair for a defect + * the codebase went to the trouble of writing down was untested. Weakening it would restore that + * bug silently, because the wrong output is a *playable file with the wrong container*, not a crash. + * + * Same harness and same two disciplines as [Media3EngineEmptyCompositionTest]: assert the plan + * really is the one the test needs before driving the engine, and rule out + * `CancellationException` so an unresumed continuation cannot read as a pass. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class Media3MuxerGuardTest { + + @Test + fun `a container Media3 cannot mux fails the job rather than silently writing MP4`() { + val context = RuntimeEnvironment.getApplication() + val engine = Media3Engine(context) + val request = ConversionRequest( + spec = OutputSpec(Container.WEBM, VideoCodec.VP9, AudioCodec.OPUS), + probe = InputProbe(videoCodec = "h264", audioCodec = "aac", container = Container.MP4), + ) + + // The premise, asserted rather than assumed -- three separate ways this test could pass + // over a path it never entered. + val plan = CopyPlanner.plan(request.spec, request.probe) + assertEquals("the plan has to still be WebM by the time the engine sees it", Container.WEBM, plan.container) + assertNull("...and Media3 really has no muxer for it", Media3Muxers.factoryFor(plan.container)) + // Not the empty-composition refusal, which fires earlier and is a different test's subject. + assertNotEquals(VideoPlan.Drop, plan.video) + assertNotEquals(AudioPlan.Drop, plan.audio) + + val failure = try { + runCatching { + runBlocking { + withTimeout(TIMEOUT_MS) { + engine.transcode(Uri.parse("file:///dev/null"), File(context.cacheDir, "guard.webm"), request) { + } + } + } + }.exceptionOrNull() + } finally { + engine.close() + } + + assertFalse( + "the continuation was never resumed -- the refusal escaped instead of failing the job: $failure", + failure is CancellationException, + ) + // Type *and* message, and the message half is the load-bearing one. Replacing the + // requireNotNull with a fallback factory does not make the export succeed here: it lets it + // run on and fail some other way, which a bare type assertion would happily accept. + assertTrue("expected the muxer guard to refuse the job, got $failure", failure is IllegalArgumentException) + assertTrue( + "the refusal has to name the container it could not mux, got: ${failure?.message}", + failure?.message.orEmpty().contains("cannot mux") && + failure?.message.orEmpty().contains(Container.WEBM.name), + ) + } + + private companion object { + /** Nothing is decoded or muxed on this path -- the guard refuses before any of that. */ + const val TIMEOUT_MS = 10_000L + } +} -- 2.47.3