From 5761faced6f9e0bddfc21aaffc84eb73000f217a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 21:37:45 -0500 Subject: [PATCH 1/2] A4 (#170): join the two halves of an unreadable join clip, and record why the catch arm stays device-only The ticket asked for two things. One of them is not reachable from the JVM, and saying so is most of the value here. **probeForConcat's catch arm cannot be provoked on this runtime.** Robolectric's MediaExtractor never throws from setDataSource -- measured across four input shapes: an unregistered content:// authority, a missing file://, a file of garbage bytes, and an http:// URL. All four returned normally with trackCount = 0. So a failed read arrives as an empty track list rather than as an exception and reaches the same ConcatInput(null, null, 0, 0, 0) by the other road. The catch stays covered only by ConcatEngineTest on a device. The test file says this rather than implying the arm is handled. **What is reachable, and was genuinely missing, is the span.** Both halves were already covered and neither reached the other: MediaProbeTrackWalkTest pins what concatInputFrom makes of a track list, ConcatPlannerTest's `an unknown codec is not treated as a match` pins what the planner does with a hand-built ConcatInput(video = null). The planner's safety rests on the probe really producing that shape, and the hand-built fixture would go on passing if it stopped. Measured rather than claimed: mutating concatInputFrom's initial `video` to a non-null placeholder leaves ConcatPlannerTest green and turns this red. Dropping the planner's video null guard turns both red -- so that half was already held, and this file does not claim credit for it. The coupling itself is worth writing down: ConcatPlanner guards video against a null codec and audio not at all, and that asymmetry is correct rather than an oversight -- MediaProbe.shortName returns a non-null String, so a null audioCodec means the track is absent and two clips with no audio really do match, while a null videoCodec means absent *or* unreadable. The audio check is safe because the video guard fires first on a clip nothing could read. Nothing held that. 555 -> 556 JVM tests, 0 failures. No production code changed. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/UnreadableJoinInputTest.kt | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/convert/UnreadableJoinInputTest.kt diff --git a/app/src/test/java/org/libremediaconverter/convert/UnreadableJoinInputTest.kt b/app/src/test/java/org/libremediaconverter/convert/UnreadableJoinInputTest.kt new file mode 100644 index 0000000..460a061 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/UnreadableJoinInputTest.kt @@ -0,0 +1,70 @@ +package org.libremediaconverter.convert + +import android.net.Uri +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.ConcatPlanner +import org.libremediaconverter.model.ConcatStrategy +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +/** + * A clip in a join that nothing could read, from the probe all the way to the strategy. + * + * Both halves of this are covered already, and separately: `MediaProbeTrackWalkTest` pins what + * `concatInputFrom` makes of a track list, and `ConcatPlannerTest`'s + * `an unknown codec is not treated as a match` pins what the planner does with a hand-built + * `ConcatInput(video = null)`. **Nothing spanned the two**, and the span is the load-bearing part: + * the planner's safety rests on the probe really producing that shape, and the hand-built fixture + * would go on passing if it stopped. + * + * Measured rather than asserted: mutating `concatInputFrom`'s initial `video` to a non-null + * placeholder leaves `ConcatPlannerTest` green and turns this red. + * + * ## The asymmetry this protects + * + * `ConcatPlanner` guards its video check against a null codec (`ConcatStrategy.kt:51`) and its + * audio check not at all (`:54`). **That is correct, not an oversight.** `MediaProbe.shortName` + * returns a non-null `String`, so in `concatInputFrom` a null `audioCodec` means the track is + * *absent* — and two clips with no audio genuinely do match. A null `videoCodec` carries both + * meanings, absent or unreadable, which is why only that one is guarded. + * + * So the audio check is safe *because* the video guard fires first on a clip nothing could read. + * Nothing wrote that coupling down and nothing held it. + * + * ## What this deliberately does not cover + * + * `probeForConcat`'s `catch` arm (`MediaProbe.kt:300-302`). It is **not reachable on the JVM**: + * Robolectric's `MediaExtractor` never throws from `setDataSource`, measured across an + * unregistered `content://` authority, a missing `file://`, a file of garbage bytes and an `http://` + * URL — all four returned normally with `trackCount = 0`. So the failure arrives here as an empty + * track list rather than as an exception, which reaches the same `ConcatInput(null, null, 0, 0, 0)` + * by the other road. The catch stays device-only, and this file does not pretend otherwise. + */ +@RunWith(RobolectricTestRunner::class) +class UnreadableJoinInputTest { + + @Test + fun `a clip nothing could read probes as unknown, and an unknown clip is re-encoded`() { + val unreadable = MediaProbe.probeForConcat(RuntimeEnvironment.getApplication(), UNREADABLE) + + assertNull("an unreadable clip proves nothing about its video codec", unreadable.videoCodec) + assertNull("nor about its audio codec", unreadable.audioCodec) + assertEquals("nor about its dimensions", 0, unreadable.width) + assertEquals(0, unreadable.height) + assertEquals(0, unreadable.frameRate) + + assertEquals( + "a clip nothing could read is not evidence of a match with anything", + ConcatStrategy.REENCODE, + ConcatPlanner.plan(listOf(unreadable, unreadable)), + ) + } + + private companion object { + /** `content://` so the probe takes the SAF branch a real pick takes. Nothing answers it. */ + val UNREADABLE: Uri = Uri.parse("content://test/vanished.mp4") + } +} -- 2.47.3 From a645442accd2da00ed6025486333f15ec427a048 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 1 Sep 2026 21:42:27 -0500 Subject: [PATCH 2/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