diff --git a/CLAUDE.md b/CLAUDE.md index fe48c69..d326d19 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -310,6 +310,46 @@ install for code that can never run — and on API 37 the full APK does not fit #218 and carries the unfixed scope. **Prefer a mutation that must go red to a repetition count** when a fix is for something intermittent. + **Every number above is `testDebugUnitTest` only, and on 2026-09-05 the instrumented suite got its + first read for that reason** — `docs/e2e-read-findings.md`, entries **E1-E6**, tickets + **#223-#230**. Four waves had been steered by a figure that **cannot see `app/src/androidTest` at + all**, so nothing had ever asked what those 60 device tests pin, only that they were green. + + **It found one test that passes while testing nothing, and it is the one that matters most.** + `HardwareFallbackTest` is the only automated check of the hardware→software fallback against a + *real* codec failure, and on run `34004304566` the API 33, 34, 35 and 37 legs each log + `Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER)` (API 36's logcat artifact on + that run is truncated, so it is unread rather than different): emulators expose no + hardware encoder, so the job never reaches Media3 and the `catch` it exists to prove is never + entered. Its two assertions — succeeded, output non-empty — are true anyway, and it finishes in + 448 ms. **Deleting that `catch` reddens nothing on any leg** (#223). + + Two things generalise from it. **A test can assert and still not reach**, which no coverage + number and no "does it assert something" review would catch — the filter that works is *does this + test's premise hold on the machine that runs it?*. And the codebase **already knew**: the sibling + `ForcedFailureTest` pins `DeviceCodecs.PERMISSIVE` against exactly this hazard and writes out why, + as does `ConversionWorkerTest`. The difference is that their assertions are about the *path*, so + without the pin they would fail loudly; `HardwareFallbackTest`'s are about the *output*, so it + passes quietly. **Prefer asserting the path over asserting the artefact** where the two differ. + + The read was a triage, not a test push, and six of its seven findings are prose rather than code — + the suite itself is in good shape. What had drifted is its self-description. + + **Working the tickets then found the thing the read could not: one production defect.** #238 — + joining files picked through the system picker failed outright on the stream-copy path. The + concat demuxer whitelists protocols separately from `-safe 0`, and `ffkitsaf` was not on the + list; only `STREAM_COPY` feeds it a list file, and every existing join test passed + `Uri.fromFile`, so **the one broken combination was the only one a user could reach**. Not a + missed line and not an unasserted value — two covered things no test put together, which is the + gap shape a coverage number is worst at. + + **E7 is the other reusable result**, because it re-scoped its own ticket. A real + `DocumentsProvider` cannot be reached without the picker: an unprotected one is refused at + install, instrumentation runs in the app's uid so the test APK's identity is no help, and shell + identity is denied too — each denial naming `ACTION_OPEN_DOCUMENT`. So #226 has no cheap headless + half. But the *input* bridge needs no documents provider at all, which is what kept #225 headless + and is how #238 surfaced. + - **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply — a change that is both needs both. diff --git a/app/src/androidTest/AndroidManifest.xml b/app/src/androidTest/AndroidManifest.xml index 2ed15ba..89753b2 100644 --- a/app/src/androidTest/AndroidManifest.xml +++ b/app/src/androidTest/AndroidManifest.xml @@ -42,6 +42,28 @@ + + + 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 diff --git a/app/src/androidTest/java/org/libremediaconverter/convert/ReattachOnLaunchTest.kt b/app/src/androidTest/java/org/libremediaconverter/convert/ReattachOnLaunchTest.kt index 6545315..3e6017b 100644 --- a/app/src/androidTest/java/org/libremediaconverter/convert/ReattachOnLaunchTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/convert/ReattachOnLaunchTest.kt @@ -13,6 +13,7 @@ import androidx.work.WorkManager import androidx.work.Worker import androidx.work.WorkerParameters import androidx.work.workDataOf +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withTimeout @@ -27,7 +28,10 @@ import org.junit.runner.RunWith import org.libremediaconverter.join.JoinState import org.libremediaconverter.join.JoinViewModel import org.libremediaconverter.model.ConcatStrategy +import org.libremediaconverter.model.ConversionRequest import org.libremediaconverter.model.Engine +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.QualityTier import org.libremediaconverter.work.ConcatWorker import org.libremediaconverter.work.ConversionWorker import org.libremediaconverter.work.JobTags @@ -64,6 +68,26 @@ class EchoWorker(context: Context, params: WorkerParameters) : Worker(context, p * path, foreground service included — into a synchronous test double, depending on class order. */ @UnstableApi +/** + * A [SoftwareTranscoder] that holds the worker in [WorkInfo.State.RUNNING] until released. + * + * Declared here rather than in `FakeFailures` because it is the only test that needs a job to stay + * live on demand, and the shape is specific to that: the others fake a *failure*, this fakes + * *duration*. + */ +private class BlockingTranscoder(private val released: CompletableDeferred) : SoftwareTranscoder { + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + released.await() + output.writeBytes(ByteArray(1_024)) + } +} + @RunWith(AndroidJUnit4::class) class ReattachOnLaunchTest { @@ -75,7 +99,13 @@ class ReattachOnLaunchTest { fun clearTheQueue() = emptyQueueAndStaging() @After - fun leaveNothingBehind() = emptyQueueAndStaging() + fun leaveNothingBehind() { + // The suite runs without Android Test Orchestrator, so every class shares one process and + // a swapped seam outlives the class that set it. Only one test here swaps one, but a + // BlockingTranscoder left in place would hang the next class that converts anything. + ConversionDependencies.reset() + emptyQueueAndStaging() + } /** * The claim the whole fix rests on, checked against the production request builder rather @@ -261,6 +291,69 @@ class ReattachOnLaunchTest { return request.id } + /** + * Reattaching to a conversion that is **running right now**, which nothing had ever driven. + * + * This class covers a job that finished, one whose staged file is gone, an ambiguous pair, one + * still queued, and one the user cancelled. [Reattachment.rank] gives + * [WorkInfo.State.RUNNING] the **highest** rank of all — "live work outranks a finished result + * because a running job is holding a foreground service" — and no test on either source set + * ever produced one. `ReattachmentTest` exercises the ranking as a pure function over + * fabricated snapshots; what was missing is a ViewModel meeting a real running job. + * + * It is also the likeliest reattachment there is: the user starts a conversion, leaves, and + * comes back while it is still going. + * + * ## Why the engine is a fake here, and why that is not a weakening + * + * The job has to still be running when the ViewModel is built, and every real conversion in + * this suite finishes in about a second — racing that is what made the cancellation tests flaky + * enough to need retries (#224). A [SoftwareTranscoder] that blocks until released removes the + * race outright: the job is `RUNNING` for exactly as long as the test wants. + * + * Nothing about reattachment depends on which engine is transcoding. What is under test is the + * tag query, [Reattachment.choose] over live WorkManager state, and `observe` mapping it to + * [ConversionState.Converting] — all of which run identically whatever is doing the work. + * + * ## What this does not do, and cannot (#230) + * + * It does not kill the process. `docs/defect-audit.md` D3/D13 record that `am kill` refuses a + * process holding a foreground service, and there is a more basic obstacle: **instrumentation + * runs in the app's own process**, so any route that really killed it would take the test + * runner with it and there would be nothing left to assert with. A relaunch-and-observe test + * needs two instrumentation runs, which the runner does not provide. + * + * So process death stays device-manual, and this is the closest observable analogue: a fresh + * ViewModel, with no memory of the work, meeting a job that is genuinely mid-flight. + */ + @Test + fun reattachesToAConversionThatIsStillRunning(): Unit = runBlocking { + val released = CompletableDeferred() + ConversionDependencies.software = { BlockingTranscoder(released) } + + val request = ConversionWorker.request( + inputUri = Uri.fromFile(stage("running_input.mp3")), + displayName = RUNNING_NAME, + sizeBytes = RUNNING_SIZE, + spec = OutputFormat.MP3.spec, + quality = QualityTier.FAST, + ) + workManager.enqueue(request).result.get() + + // Deterministic: the worker cannot finish until this test lets it. + withTimeout(TIMEOUT_MS) { + workManager.getWorkInfoByIdFlow(request.id).first { it?.state == WorkInfo.State.RUNNING } + } + + val reattached = awaitConversion() + + assertEquals(RUNNING_NAME, reattached.input.displayName) + assertEquals(RUNNING_SIZE, reattached.input.sizeBytes) + + released.complete(Unit) + workManager.cancelWorkById(request.id).result.get() + } + /** * Enqueues a job that stays [WorkInfo.State.ENQUEUED]. The delay is what holds it there: it * is long enough that nothing can run it during a test, and it is cancelled either way. @@ -326,5 +419,9 @@ class ReattachOnLaunchTest { * against WorkManager's database, so this is generous rather than tuned. */ const val SETTLE_MS = 5_000L + + /** Read back off the job's tags by the reattaching ViewModel, so both have to survive. */ + const val RUNNING_NAME = "still_running.mp3" + const val RUNNING_SIZE = 4_242L } } diff --git a/app/src/androidTest/java/org/libremediaconverter/fallback/HardwareFallbackTest.kt b/app/src/androidTest/java/org/libremediaconverter/fallback/HardwareFallbackTest.kt index b05f5b9..04a61cd 100644 --- a/app/src/androidTest/java/org/libremediaconverter/fallback/HardwareFallbackTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/fallback/HardwareFallbackTest.kt @@ -12,11 +12,17 @@ import kotlinx.coroutines.withTimeout import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue +import org.junit.Assume.assumeTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import org.libremediaconverter.codec.AndroidDeviceCodecs +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.ConversionRouter +import org.libremediaconverter.model.Engine import org.libremediaconverter.model.OutputFormat import org.libremediaconverter.model.QualityTier +import org.libremediaconverter.model.VideoCodec import org.libremediaconverter.work.ConversionWorker import java.io.File @@ -34,6 +40,47 @@ import java.io.File * hand — a regression test that silently skips is worse than no test, because the count * still reads as coverage. * + * ## Why this skips on emulators, and why that is the honest answer (#223) + * + * **This test used to pass everywhere while proving nothing.** Two independent facts stop the + * fallback happening on an emulator, and both were measured rather than reasoned: + * + * 1. **The router never sends the job to Media3.** A Fast MP4/H.265 job goes to the hardware path + * only when `device.canEncode(H265)`, and emulators expose no hardware encoder — every leg of + * run `34004304566` logged + * `Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER)`. The whole test + * finished in 448 ms, which is not long enough to fail an export and then re-encode. + * 2. **Forcing it to Media3 does not help either, which is the part that settles it.** Pinning + * `ConversionDependencies.deviceCodecs` to [DeviceCodecs.PERMISSIVE] — the trick + * [ForcedFailureTest] uses — makes the router choose Media3, and the export then *succeeds*. + * Measured on a local API 34 emulator: `MediaCodecInfo` logs + * `NoSupport [codec.profileLevel, avc1.F4000C, video/avc]` for **both** + * `c2.goldfish.h264.decoder` and `c2.android.avc.decoder`, and ExoPlayer allocates the + * goldfish decoder anyway, which decodes the file regardless of the profile it declares. + * `c2.android.hevc.encoder` then encodes the result and the job reports `MEDIA3`. + * + * So the class KDoc above — "Media3 fails partway through the export on every device" — **is not + * true of the emulator images**, and no amount of routing pressure makes this fixture force a + * fallback there. The emulator cannot answer this question, so the test says so out loud instead + * of passing. + * + * That is why the gate is [assumeTrue] on the *production* premise (`canEncode(H265)`) rather than + * a pinned profile: pinning would also swap in software codecs, which is not the path a real + * device takes and is what made the forced run succeed. **This is now the third permanent skip**; + * the other two are [org.libremediaconverter.bench.RealMediaBenchmark]'s. + * + * `ForcedFailureTest.hardwareFailureFallsBackToSoftware` still covers the fallback *wiring* on + * every leg, with an `ExplodingHardware` double. What only a device with a real hardware encoder + * can show is two real engines disagreeing about a real file, and that is what this is for. + * + * ## Why the assertion is a pair + * + * `KEY_ENGINE_USED` is `FFMPEG` whether the fallback fired **or** the router went straight there, + * so asserting it alone would not have caught any of the above. The premise is asserted + * separately: [ConversionRouter.route] chooses `MEDIA3` for this request on this device. Static + * routing wanted hardware, the runtime result was software — together, and only together, that is + * the fallback. + * * The fixture was produced with x264, which the host toolchain cannot do (Fedora's * ffmpeg ships openh264, which is Constrained Baseline only): * @@ -66,6 +113,15 @@ class HardwareFallbackTest { @Test fun aFileMedia3CannotDecodeStillConvertsViaFfmpeg(): Unit = runBlocking { + // See "Why this skips on emulators" on the class. Without a real hardware encoder the + // router never chooses Media3, and forcing it makes the export succeed instead of fail -- + // so there is no fallback to observe and a green run would mean nothing. + assumeTrue( + "no hardware HEVC encoder, so the router cannot choose Media3 and there is no " + + "fallback to exercise", + AndroidDeviceCodecs.get().canEncode(VideoCodec.H265), + ) + val request = ConversionWorker.request( inputUri = Uri.fromFile(input), displayName = SAMPLE, @@ -75,6 +131,19 @@ class HardwareFallbackTest { // the tier where the fallback has to rescue the conversion. quality = QualityTier.FAST, ) + // The premise, asserted rather than assumed: this request is one the router wants to send + // to hardware on this device. Without it the test is green whether the fallback fired or + // the job never went near Media3, which is exactly how #223 stayed invisible. + val decision = ConversionRouter.route( + ConversionRequest(OutputFormat.MP4_H265.spec, quality = QualityTier.FAST), + AndroidDeviceCodecs.get(), + ) + assertEquals( + "this test only means something if the router sends this job to Media3", + Engine.MEDIA3, + decision.engine, + ) + workManager.enqueue(request).result.get() val terminal = withTimeout(TIMEOUT_MS) { @@ -88,6 +157,14 @@ class HardwareFallbackTest { terminal?.state, ) + // The outcome. Paired with the routing assertion above this is the fallback and nothing + // else: hardware was chosen, software is what ran. + assertEquals( + "the router chose Media3, so a successful job must have fallen back to FFmpeg", + Engine.FFMPEG.name, + terminal?.outputData?.getString(ConversionWorker.KEY_ENGINE_USED), + ) + val out = File(terminal!!.outputData.getString(ConversionWorker.KEY_OUTPUT_PATH)!!) assertTrue("no output produced", out.exists() && out.length() > 0) out.delete() diff --git a/app/src/androidTest/java/org/libremediaconverter/ffmpeg/FFmpegEngineTest.kt b/app/src/androidTest/java/org/libremediaconverter/ffmpeg/FFmpegEngineTest.kt index 79ad243..02a72a0 100644 --- a/app/src/androidTest/java/org/libremediaconverter/ffmpeg/FFmpegEngineTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/ffmpeg/FFmpegEngineTest.kt @@ -4,10 +4,20 @@ import android.media.MediaExtractor import android.media.MediaFormat 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 +import org.junit.Assert.fail import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -113,6 +123,11 @@ class FFmpegEngineTest { fun encodesFlacLosslessAudio() { val out = convert(OutputFormat.FLAC) assertTrue("no FLAC produced", out.exists() && out.length() > 0) + // "fLaC", the native FLAC stream marker. Without this the test passed on any non-empty + // file, so a builder arm emitting the wrong encoder into a .flac name shipped green + // (#228) -- the same shape the five assertions above already guard against. + val magic = out.inputStream().use { String(it.readNBytes(4), Charsets.US_ASCII) } + assertEquals("fLaC", magic) } @Test @@ -127,6 +142,165 @@ class FFmpegEngineTest { fun encodesOpus() { val out = convert(OutputFormat.OPUS) assertTrue("no Opus produced", out.exists() && out.length() > 0) + // OutputFormat.OPUS is Container.OGG, so the file is an Ogg stream: "OggS" (#228). + // Deliberately the container marker rather than the codec -- it is what the other + // container-level assertions in this class check, and it is four bytes at offset 0. + val magic = out.inputStream().use { String(it.readNBytes(4), Charsets.US_ASCII) } + assertEquals("OggS", magic) + } + + /** + * The percentage itself, which every other test in this class computes and none of them reads. + * + * `FFmpegEngine` derives progress as `stats.time / durationMs * 100`, and the statistics + * callback runs on every conversion here — but every call site omits `onProgress`, so until + * this test nothing on any source set had ever looked at the number (#229). #196 covered the + * *worker's* progress lambda, and did it with a fake engine that reports whatever the test + * tells it to; `ProgressNotificationTest` covers throttling the same way. The arithmetic was + * the one part with no reader. + * + * ## Why the duration is deliberately wrong + * + * `sample_h264.mp4` is exactly 3.000 s, and this passes **30 s** as the duration. So the + * conversion still encodes the whole clip, `stats.time` still climbs to about 3000 ms, and the + * reported percentage tops out around **10** rather than 100. + * + * That is what makes the assertion bite. A range check alone is worthless here: replacing + * `percent` with a constant `0` satisfies "every value is in 0..100" and "the values never go + * backwards", and so does a list of `[0, 100]`. Pinning the *band* rejects every constant, and + * — because the band is a tenth of the way up — it also rejects an implementation that ignores + * `durationMs`, which would report ~100 for the same run. + * + * The bound is deliberately loose (5..25 for an expected 10). The last statistics callback can + * land slightly before the final frame, so the peak is "about 3000 ms of a claimed 30 000", + * not exactly it. + */ + @Test + fun progressIsReportedAsAFractionOfTheDurationItWasGiven() { + val seen = mutableListOf() + val out = outputFor("out_progress.mp4") + runBlocking { + engine.run( + request = ConversionRequest(spec = OutputFormat.MP4_H264.spec, quality = QualityTier.BEST), + inputPath = input.absolutePath, + output = out, + // Ten times the fixture's real 3 s. See the KDoc. + durationMs = 30_000, + onProgress = { percent -> seen += percent }, + ) + } + + assertTrue("the statistics callback never reported progress", seen.isNotEmpty()) + assertTrue("progress out of range: $seen", seen.all { it in 0..100 }) + assertEquals("progress went backwards: $seen", seen.sorted(), seen) + // The band. Rejects any constant, and rejects ignoring durationMs (which would read ~100). + val peak = seen.max() + assertTrue( + "3 s of media against a claimed 30 s should peak near 10%, got $peak from $seen", + peak in 5..25, + ) + } + + /** + * Cancelling a *running* conversion actually stops the native session. + * + * Nothing on any source set did this before (#224). Every `cancel` in `app/src/androidTest` is + * `WorkManager.cancelWorkById` against work that is **queued or already finished** — the two in + * `ReattachOnLaunchTest` cancel a job carrying a one-hour initial delay, and one immediately + * after enqueue. On the JVM, `WorkerCancellationTest` and `HardwareFallbackTest`'s cancellation + * case drive a `SoftwareTranscoder` double that records the call. No test had ever asked a real + * native session to stop. This is `docs/defect-audit.md` **D10**'s forcing condition. + * + * It is the one path where cancelling wrong is silently expensive rather than loudly broken: a + * missed `FFmpegKit.cancel` leaves the native process encoding to completion while the UI says + * the job is cancelled, and nothing reports the battery and thermal cost. + * + * ## Why the assertion is the session's return code, not the output file + * + * The obvious assertion — the partial output is gone — **cannot fail**, so it would have been a + * vacuous test. `invokeOnCancellation` deletes the path, and on POSIX unlinking a file ffmpeg + * still holds open leaves ffmpeg writing to the unlinked inode; the path stays gone whether or + * not the cancel ever reached the session. Deleting `FFmpegKit.cancel` and keeping + * `output.delete()` passes that check every time. + * + * What distinguishes them is the session's own verdict: a cancelled session ends with the + * cancel return code, a completed one ends successfully. That is a fact about the session + * rather than about timing, so it is read *after* waiting for the session to leave + * [SessionState.RUNNING] rather than at a fixed delay. + * + * ## Why it cancels on RUNNING rather than on the first progress callback + * + * Measured. Cancelling from the first `onProgress` was tried first and **failed on a local API + * 34 emulator with `state=COMPLETED rc=0`** — every committed fixture is 2-3 s at 320x240, and + * the encode finishes before the first statistics callback has been delivered and acted on. The + * progress callback proves the session is running, but arrives too late to interrupt anything. + * `FFmpegKit.listSessions` shows the session [SessionState.RUNNING] far earlier. + * + * ## Why it retries, which is the part that took two attempts to get right + * + * Waiting for `RUNNING` is not on its own enough. With `MP4_H265` at [QualityTier.BEST] this + * passed four consecutive local runs and all five CI legs, then failed on the API 34 and 35 legs + * of the next PR with `state=COMPLETED rc=0`. Nothing had changed: on a loaded runner the thread + * that observed `RUNNING` can be descheduled long enough for a short encode to finish before it + * calls `cancel`. A longer timeout does not help — the wait already succeeded. + * + * Two changes together, because neither is sufficient: + * + * - **A slower encode.** `WEBM_VP9` at `BEST` is the slowest thing this builder emits: + * `libvpx-vp9 -crf 31 -b:v 0`, with `-deadline realtime` added **only** on + * [QualityTier.FAST]. Probed on an API 34 emulator, that session is still `RUNNING` at 1 s + * and finished by 2 s, against well under a second for x265 `-preset medium`. + * - **Retrying the attempt.** An attempt whose session finished before the cancel landed has + * not tested anything, so it is not a failure — it is a miss, and it is retried. Only + * exhausting [CANCEL_ATTEMPTS] is a failure, and its message says which case it hit. + * + * That keeps the mutation honest: with `FFmpegKit.cancel` removed **every** attempt ends + * `COMPLETED`, so the test still fails — it just takes [CANCEL_ATTEMPTS] tries to say so. + * + * The session is identified by diffing against the ids present before each attempt, because + * this class has already produced eight of them by the time this executes. + */ + @Test + fun cancellingARunningConversionCancelsTheNativeSession(): Unit = runBlocking { + val outcomes = mutableListOf() + + repeat(CANCEL_ATTEMPTS) { attempt -> + val before = FFmpegKit.listSessions().map { it.getSessionId() }.toSet() + val out = outputFor("out_cancelled_$attempt.webm") + + val job = launch(Dispatchers.IO) { + engine.run( + // The slowest target this builder emits -- see the KDoc. Not decoration: + // with a faster one this loses the race on a loaded CI runner. + request = ConversionRequest(spec = OutputFormat.WEBM_VP9.spec, quality = QualityTier.BEST), + inputPath = input.absolutePath, + output = out, + durationMs = 3_000, + ) + } + + val ours = withTimeout(TIMEOUT_MS) { + var found: FFmpegSession? = null + while (found == null) { + found = FFmpegKit.listSessions().firstOrNull { it.getSessionId() !in before } + if (found == null) delay(POLL_MS) + } + found + } + job.cancelAndJoin() + withTimeout(TIMEOUT_MS) { + while (ours.getState() == SessionState.RUNNING) delay(POLL_MS) + } + + if (ReturnCode.isCancel(ours.getReturnCode())) return@runBlocking + // The encode beat us to it. That attempt proved nothing either way, so try again. + outcomes += "state=${ours.getState()} rc=${ours.getReturnCode()}" + } + + fail( + "never interrupted a running session in $CANCEL_ATTEMPTS attempts, so either every " + + "encode finished first or cancellation does not reach it: $outcomes", + ) } // --- the quality tier the GPL licence was taken for -------------------- @@ -166,4 +340,19 @@ class FFmpegEngineTest { }.exceptionOrNull() assertTrue("expected an FFmpegException, got $failure", failure is FFmpegEngine.FFmpegException) } + + private companion object { + /** Generous: it bounds a hang, and every wait here normally settles in well under a second. */ + const val TIMEOUT_MS = 30_000L + const val POLL_MS = 50L + + /** + * How many times to try to catch the session mid-encode. + * + * Each miss costs about the length of one VP9 encode -- a second or two -- and a miss is + * the loaded-runner case rather than a defect. Five is enough that exhausting them means + * cancellation is not reaching the session, which is what the failure message says. + */ + const val CANCEL_ATTEMPTS = 5 + } } diff --git a/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt b/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt index 4e9e1a5..ecadf1c 100644 --- a/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt @@ -5,10 +5,20 @@ 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 +import org.junit.Assert.fail import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -17,6 +27,7 @@ import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.ffmpeg.ConcatEngine import org.libremediaconverter.ffmpeg.FFmpegEngine import org.libremediaconverter.model.ConcatStrategy +import org.libremediaconverter.work.ConcatWorker import java.io.File /** @@ -51,6 +62,82 @@ 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. + * + * **And the attempt is retried**, for the reason the conversion side measured the hard way: on + * a loaded runner the thread that observed `RUNNING` can be descheduled long enough for a short + * encode to finish before it calls `cancel`, which failed two CI legs there. An attempt whose + * session finished first has tested nothing, so it is a miss rather than a failure; only + * exhausting [CANCEL_ATTEMPTS] fails, and with `FFmpegKit.cancel` removed every attempt misses, + * so the mutation still bites. + * + * 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 outcomes = mutableListOf() + + repeat(CANCEL_ATTEMPTS) { attempt -> + val before = FFmpegKit.listSessions().map { it.getSessionId() }.toSet() + val out = output("cancelled_join_$attempt.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 == null) { + found = FFmpegKit.listSessions().firstOrNull { it.getSessionId() !in before } + if (found == null) delay(POLL_MS) + } + found + } + job.cancelAndJoin() + withTimeout(TIMEOUT_MS) { + while (ours.getState() == SessionState.RUNNING) delay(POLL_MS) + } + + if (ReturnCode.isCancel(ours.getReturnCode())) return@runBlocking + outcomes += "state=${ours.getState()} rc=${ours.getReturnCode()}" + } + + fail( + "never interrupted a running join in $CANCEL_ATTEMPTS attempts, so either every " + + "encode finished first or cancellation does not reach it: $outcomes", + ) + } + private fun copyAsset(name: String): File { val out = File(context.cacheDir, name) InstrumentationRegistry.getInstrumentation().context.assets @@ -230,4 +317,13 @@ 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 + + /** See the conversion side: a miss is the loaded-runner case, not a defect. */ + const val CANCEL_ATTEMPTS = 5 + } } diff --git a/app/src/androidTest/java/org/libremediaconverter/saf/ContentUriInputTest.kt b/app/src/androidTest/java/org/libremediaconverter/saf/ContentUriInputTest.kt new file mode 100644 index 0000000..cf84d36 --- /dev/null +++ b/app/src/androidTest/java/org/libremediaconverter/saf/ContentUriInputTest.kt @@ -0,0 +1,123 @@ +package org.libremediaconverter.saf + +import androidx.media3.common.util.UnstableApi +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import androidx.work.WorkInfo +import androidx.work.WorkManager +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.ffmpeg.ConcatEngine +import org.libremediaconverter.model.Engine +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.QualityTier +import org.libremediaconverter.work.ConversionWorker +import java.io.File + +/** + * A `content://` input reaching FFmpeg successfully, which nothing had ever driven (#225). + * + * `FFmpegKitConfig.getSafParameterForRead` stands between a SAF grant and the native process, and + * it is on **every real user conversion**. Every passing convert and join test in this suite hands + * the worker a `Uri.fromFile(...)`, which takes the `uri.path` arm instead — so the bridge was + * exercised only on its failure side, by `UnopenableUriTest` naming an authority that does not + * exist. That proves the error message, not the bridge. + * + * ## Why a plain provider rather than the documents one + * + * [FixtureDocumentsProvider] cannot be reached from the app, measured three ways on an API 34 + * emulator (#226): a `DOCUMENTS_PROVIDER` declared without `MANAGE_DOCUMENTS` is refused at install + * — *"Provider must be protected by MANAGE_DOCUMENTS"*; instrumentation runs in the **target app's + * process**, so `Instrumentation.getContext()` still carries the app's uid and is denied; and + * `adoptShellPermissionIdentity(MANAGE_DOCUMENTS)` is denied identically. The denial names the only + * way in: *"you obtain access using ACTION_OPEN_DOCUMENT or related APIs"*. + * + * The bridge does not need one. It opens a descriptor through the resolver and hands FFmpeg a + * `saf:` path, so any readable `content://` URI exercises it — and [FixtureContentProvider] is an + * ordinary provider, which may be exported without a permission. The whole class is headless: no + * DocumentsUI, and none of the flake #190 records. + * + * ## Why MP3 + * + * The bridge lives on the FFmpeg arm, and MP3 is the format the router sends there unconditionally + * — no platform encoder exists at any API level, so `ConversionWorkerTest.routesAnMp3JobToFfmpeg…` + * relies on the same fact. Choosing a video target would make the engine depend on the device's + * codecs, and #223 is what that costs. + * + * *Mutation:* make `getSafParameterForRead` return `uri.toString()`. FFmpeg cannot open it and both + * tests fail; nothing else in either suite notices. + */ +@UnstableApi +@RunWith(AndroidJUnit4::class) +class ContentUriInputTest { + + private val context = InstrumentationRegistry.getInstrumentation().targetContext + private val workManager = WorkManager.getInstance(context) + + @After + fun tearDown() { + File(context.cacheDir, "conversions").listFiles()?.forEach { it.delete() } + } + + @Test + fun aContentUriInputConvertsThroughTheSafBridge(): Unit = runBlocking { + val input = FixtureContentProvider.uriFor(SAMPLE) + val request = ConversionWorker.request( + inputUri = input, + displayName = SAMPLE, + sizeBytes = 0L, + spec = OutputFormat.MP3.spec, + quality = QualityTier.FAST, + ) + workManager.enqueue(request).result.get() + + val terminal = withTimeout(TIMEOUT_MS) { + workManager.getWorkInfoByIdFlow(request.id).first { it != null && it.state.isFinished } + } + + val error = terminal?.outputData?.getString(ConversionWorker.KEY_ERROR) + assertEquals( + "a content:// input must convert, but failed with: $error", + WorkInfo.State.SUCCEEDED, + terminal?.state, + ) + // The bridge is on the FFmpeg arm only, so this is part of the claim rather than colour. + assertEquals(Engine.FFMPEG.name, terminal?.outputData?.getString(ConversionWorker.KEY_ENGINE_USED)) + + val out = File(terminal!!.outputData.getString(ConversionWorker.KEY_OUTPUT_PATH)!!) + assertTrue("no output produced from a content:// input", out.exists() && out.length() > 0) + out.delete() + } + + /** + * The same bridge on the join path, which has its own copy of the call (`ConcatEngine:36`). + * + * Driven through the engine rather than `ConcatWorker` because the engine is where the branch + * is; the worker adds a foreground service and nothing else this is about. + */ + @Test + fun contentUriInputsJoinThroughTheSafBridge(): Unit = runBlocking { + val out = File(context.cacheDir, "joined_from_content.mp4").apply { delete() } + val result = ConcatEngine(context).join( + listOf(FixtureContentProvider.uriFor(CLIP_A), FixtureContentProvider.uriFor(CLIP_B)), + out, + OutputFormat.MP4_H264, + ) + + assertTrue("no output produced from content:// inputs", result.output.length() > 0) + out.delete() + } + + private companion object { + const val SAMPLE = "sample_h264.mp4" + const val CLIP_A = "clip_a.mp4" + const val CLIP_B = "clip_b.mp4" + const val TIMEOUT_MS = 300_000L + } +} diff --git a/app/src/androidTest/java/org/libremediaconverter/saf/FixtureContentProvider.java b/app/src/androidTest/java/org/libremediaconverter/saf/FixtureContentProvider.java new file mode 100644 index 0000000..79c0bc1 --- /dev/null +++ b/app/src/androidTest/java/org/libremediaconverter/saf/FixtureContentProvider.java @@ -0,0 +1,135 @@ +package org.libremediaconverter.saf; + +import android.content.ContentProvider; +import android.content.ContentValues; +import android.database.Cursor; +import android.database.MatrixCursor; +import android.net.Uri; +import android.os.ParcelFileDescriptor; +import android.provider.OpenableColumns; + +import java.io.File; +import java.io.FileNotFoundException; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; + +/** + * A plain {@link ContentProvider} serving the committed media fixtures over {@code content://}. + * + *

Why this exists alongside {@link FixtureDocumentsProvider}. Every passing convert and + * join test hands the worker a {@code Uri.fromFile(...)}, which takes the {@code uri.path} arm and + * never touches {@code FFmpegKitConfig.getSafParameterForRead}. That bridge is on 100% of real user + * conversions and was on 0% of tested ones; only its failure side was covered, by + * {@code UnopenableUriTest} pointing at an authority that does not exist. + * + *

Why not the documents provider. It cannot be reached. Measured three ways on an API 34 + * emulator: a {@code DOCUMENTS_PROVIDER} declared without {@code MANAGE_DOCUMENTS} is refused at + * install ("Provider must be protected by MANAGE_DOCUMENTS"); instrumentation runs in the target + * app's process, so {@code Instrumentation.getContext()} still carries the app's uid and is denied; + * and {@code adoptShellPermissionIdentity(MANAGE_DOCUMENTS)} is denied identically. The denial says + * what is required — "you obtain access using ACTION_OPEN_DOCUMENT or related APIs" — so a + * documents provider is reachable only through a picker-issued grant. See issue #226. + * + *

The bridge does not need one. {@code getSafParameterForRead} opens a file descriptor through + * the resolver and hands FFmpeg a {@code saf:} path; any readable {@code content://} URI exercises + * it. An ordinary provider may be exported without a permission, so this one is, and the whole test + * stays headless — no DocumentsUI, and none of the flake #190 records. + * + *

Unlike {@link FixtureDocumentsProvider} this may use {@code androidx} and Kotlin freely — it is + * loaded into the app process like any other provider, not into the bare test process. It is kept + * in Java anyway, next to its sibling, so the two read alike. + */ +public final class FixtureContentProvider extends ContentProvider { + + /** Authority. Distinct from the documents provider's, and from anything the app declares. */ + public static final String AUTHORITY = "org.libremediaconverter.test.content"; + + /** Builds a URI for one of this source set's committed assets, e.g. {@code sample_h264.mp4}. */ + public static Uri uriFor(String assetName) { + return new Uri.Builder().scheme("content").authority(AUTHORITY).appendPath(assetName).build(); + } + + @Override + public boolean onCreate() { + return true; + } + + @Override + public ParcelFileDescriptor openFile(Uri uri, String mode) throws FileNotFoundException { + if (!"r".equals(mode)) { + throw new FileNotFoundException("this provider is read-only: " + mode); + } + return ParcelFileDescriptor.open(unpack(assetOf(uri)), ParcelFileDescriptor.MODE_READ_ONLY); + } + + /** + * Enough of {@link OpenableColumns} for {@code InputQuery.describe} to name and size the input. + * + *

Without these the app reaches the "Size unknown" screen, which is a different test. + */ + @Override + public Cursor query(Uri uri, String[] projection, String selection, String[] args, String sort) { + String asset = assetOf(uri); + File file; + try { + file = unpack(asset); + } catch (FileNotFoundException e) { + return null; + } + MatrixCursor cursor = new MatrixCursor( + new String[] {OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE}); + cursor.newRow().add(OpenableColumns.DISPLAY_NAME, asset).add(OpenableColumns.SIZE, file.length()); + return cursor; + } + + @Override + public String getType(Uri uri) { + return assetOf(uri).endsWith(".m4a") ? "audio/mp4" : "video/mp4"; + } + + @Override + public Uri insert(Uri uri, ContentValues values) { + throw new UnsupportedOperationException("read-only fixture provider"); + } + + @Override + public int delete(Uri uri, String selection, String[] args) { + throw new UnsupportedOperationException("read-only fixture provider"); + } + + @Override + public int update(Uri uri, ContentValues values, String selection, String[] args) { + throw new UnsupportedOperationException("read-only fixture provider"); + } + + private static String assetOf(Uri uri) { + String asset = uri.getLastPathSegment(); + return asset == null ? "" : asset; + } + + /** + * The asset on disk, unpacked the first time anything asks. + * + *

Reported as {@link FileNotFoundException} rather than swallowed: a provider answering with + * a zero-byte file would fail the conversion for a reason nothing states. + */ + private File unpack(String asset) throws FileNotFoundException { + File file = new File(getContext().getCacheDir(), "provided_" + asset); + if (file.length() > 0L) { + return file; + } + try (InputStream source = getContext().getAssets().open(asset); + OutputStream sink = new FileOutputStream(file)) { + byte[] buffer = new byte[8192]; + int read; + while ((read = source.read(buffer)) != -1) { + sink.write(buffer, 0, read); + } + } catch (IOException e) { + throw new FileNotFoundException("could not unpack " + asset + ": " + e); + } + return file; + } +} diff --git a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt index ba08da1..48aef0d 100644 --- a/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/saf/SafPickerRoundTripTest.kt @@ -207,6 +207,8 @@ import java.util.concurrent.atomic.AtomicInteger * driven there at all. That is why this gap survived as long as it did. * `tools/local-emulator/run-e2e.sh` runs API 33-36 on the development host, and both tests pass * there: **59 / 0 / 0 / 2 at API 33 and again at API 36**, whole suite, 2026-08-24. + * (Since #223 the skip column reads 3 on an emulator — `HardwareFallbackTest` now announces + * that it cannot run without a hardware HEVC encoder rather than passing vacuously.) * * ### Why only the rotation test carries [FailsOnEmulatorApi37] * diff --git a/app/src/androidTest/java/org/libremediaconverter/work/NotificationCancelActionTest.kt b/app/src/androidTest/java/org/libremediaconverter/work/NotificationCancelActionTest.kt new file mode 100644 index 0000000..53b218a --- /dev/null +++ b/app/src/androidTest/java/org/libremediaconverter/work/NotificationCancelActionTest.kt @@ -0,0 +1,129 @@ +package org.libremediaconverter.work + +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import androidx.work.OneTimeWorkRequestBuilder +import androidx.work.WorkInfo +import androidx.work.WorkManager +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.QualityTier +import java.io.File +import java.util.concurrent.TimeUnit + +/** + * The Cancel button in the notification shade actually cancels the job. + * + * `ConversionNotifications.build` attaches one action, wired to + * `WorkManager.createCancelPendingIntent(id)`. Before this test `createCancelPendingIntent` had + * **no references anywhere outside its own declaration** — no JVM test, no instrumented test + * (#227). + * + * That matters more than an ordinary uncovered line. A conversion runs in a foreground service and + * the user is invited to leave the app; once they do, this action is the only way to stop it. If + * the `PendingIntent` carries the wrong id, the button does nothing, the notification stays, and + * the job runs to completion — with no error, no log, and no screen to look at. + * + * ## Why this fires the intent rather than reading the shade + * + * The obvious version asks `NotificationManager.getActiveNotifications()` for id 1001 and taps what + * it finds. That was rejected: the instrumented suite grants no runtime permissions, so + * `POST_NOTIFICATIONS` is denied throughout, and whether a suppressed foreground-service + * notification is returned there is a platform detail that varies — the test would be asserting + * something about notification *visibility* rather than about cancellation. + * + * The `PendingIntent` is the subject; where it is read from is incidental. Building the + * notification for a real, live work id and firing its action exercises exactly the thing that can + * be wrong — a real `PendingIntent` dispatch reaching real `WorkManager` — and does it the same way + * on every API level. + * + * ## Why the job is delayed rather than running + * + * A conversion of the committed 3 s fixture finishes in well under a second on an emulator + * (`HardwareFallbackTest` completed one in 448 ms), so racing a cancel against a running job would + * be flaky in the direction that fails. An initial delay keeps the job reliably `ENQUEUED`, which + * is a state `cancelWorkById` acts on identically — what is under test is whether firing the action + * reaches WorkManager with the right id, not which state it interrupts. + * + * *Mutation:* build the `PendingIntent` from `UUID.randomUUID()` instead of the request's id. The + * notification looks identical and the job is never cancelled. + */ +@UnstableApi +@RunWith(AndroidJUnit4::class) +class NotificationCancelActionTest { + + private val context = InstrumentationRegistry.getInstrumentation().targetContext + private val workManager = WorkManager.getInstance(context) + private lateinit var input: File + + @Before + fun setUp() { + input = File(context.cacheDir, "cancel_action_sample.mp4") + InstrumentationRegistry.getInstrumentation().context.assets + .open("sample_h264.mp4") + .use { asset -> input.outputStream().use { asset.copyTo(it) } } + } + + @After + fun tearDown() { + input.delete() + File(context.cacheDir, "conversions").listFiles()?.forEach { it.delete() } + } + + @Test + fun theNotificationsCancelActionCancelsThatJob(): Unit = runBlocking { + val request = ConversionWorker.request( + inputUri = Uri.fromFile(input), + displayName = input.name, + sizeBytes = input.length(), + spec = OutputFormat.MP4_H264.spec, + quality = QualityTier.FAST, + ).let { base -> + // Rebuild with a delay so the job stays ENQUEUED for the whole test. See the KDoc. + OneTimeWorkRequestBuilder() + .setInputData(base.workSpec.input) + .setInitialDelay(1, TimeUnit.HOURS) + .build() + } + workManager.enqueue(request).result.get() + + // The job is queued and waiting, which is the state the cancel has to interrupt. + assertEquals( + WorkInfo.State.ENQUEUED, + withTimeout(TIMEOUT_MS) { + workManager.getWorkInfoByIdFlow(request.id).first { it != null } + }?.state, + ) + + val notification = ConversionNotifications(context) + .build(request.id, title = input.name, percent = 0, indeterminate = true) + val action = notification.actions?.firstOrNull() + assertNotNull("the progress notification carries no action to cancel with", action) + + // The whole point: fire it the way the shade would, and see the job stop. + action!!.actionIntent.send() + + val terminal = withTimeout(TIMEOUT_MS) { + workManager.getWorkInfoByIdFlow(request.id).first { it != null && it.state.isFinished } + } + assertEquals( + "firing the notification's Cancel action must cancel the job it was built for", + WorkInfo.State.CANCELLED, + terminal?.state, + ) + } + + private companion object { + const val TIMEOUT_MS = 30_000L + } +} diff --git a/app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegConcatCommand.kt b/app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegConcatCommand.kt index ec939b6..68a92c6 100644 --- a/app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegConcatCommand.kt +++ b/app/src/main/java/org/libremediaconverter/ffmpeg/FFmpegConcatCommand.kt @@ -35,6 +35,21 @@ object FFmpegConcatCommand { add("concat") add("-safe") add("0") + // And -protocol_whitelist permits the *scheme* those paths carry, which is a + // separate gate (#238). Every input the user actually picks is a content:// URI -- + // JoinScreen uses OpenMultipleDocuments -- so ConcatEngine maps it through + // FFmpegKitConfig.getSafParameterForRead and writes an `ffkitsaf:` path into the + // list file. The concat demuxer applies its own whitelist, defaulting to + // "file,crypto,data", and refused every one of them: + // + // [ffkitsaf @ ...] Protocol 'ffkitsaf' not on whitelist 'file,crypto,data'! + // + // This only widens that default. It is on the stream-copy branch alone because it + // is the only one that feeds the demuxer a list file -- REENCODE passes each input + // with its own -i, where the whitelist does not apply, which is why joining over SAF + // worked for mismatched clips and failed for matching ones. + add("-protocol_whitelist") + add(PROTOCOL_WHITELIST) add("-i") add(listFile.absolutePath) add("-c") @@ -84,4 +99,12 @@ object FFmpegConcatCommand { add(output.absolutePath) } } + + /** + * The concat demuxer's protocol whitelist: FFmpeg's own default, plus ffmpeg-kit's SAF scheme. + * + * Spelled out rather than appended to an unknown default, because the default is FFmpeg's and + * could change under us; naming all four keeps the command self-describing. See #238. + */ + private const val PROTOCOL_WHITELIST = "file,crypto,data,ffkitsaf" } diff --git a/app/src/test/java/org/libremediaconverter/convert/LauncherWiringTest.kt b/app/src/test/java/org/libremediaconverter/convert/LauncherWiringTest.kt index 0e38b1e..b939fc3 100644 --- a/app/src/test/java/org/libremediaconverter/convert/LauncherWiringTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/LauncherWiringTest.kt @@ -6,6 +6,7 @@ import android.net.Uri import androidx.activity.ComponentActivity import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule +import androidx.compose.ui.test.onAllNodesWithTag import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.performClick import androidx.media3.common.util.UnstableApi @@ -77,6 +78,28 @@ class LauncherWiringTest { * The transposition guard. A picked file has to reach `onInputPicked`, which is observable as * the screen arriving at `Ready` with the file card showing — `save()` from `Idle` returns at * its own guard and leaves nothing behind. + * + * ## Why this waits rather than asserting straight away (#220) + * + * `onInputPicked` does not reach `Ready` on the calling thread. It hops twice — + * `withContext(pickDispatcher) { InputQuery.describe(...) }` and then the probe — and + * `pickDispatcher` defaults to `Dispatchers.IO`, a real background thread that Compose's + * idling does not know about. `deliver` therefore returns with the state still `Idle` more + * often than not, and asserting immediately was a race the test usually won. + * + * It lost five times on CI in one day, on PRs whose diffs were instrumented tests and + * documentation, which is what #220 was filed for. `waitUntil` polls through + * `waitForIdle`, so it drains the main looper each time round and sees the recomposition that + * the IO hop eventually posts back. + * + * **Injecting the dispatcher would be better and is not available here.** `pickDispatcher` is + * a constructor parameter precisely so a test can pin it, but this test composes the real + * `ConverterScreen`, which resolves its own ViewModel through `viewModel()` — the seam exists + * one layer below the thing under test. Pinning it would mean not testing the launcher edge, + * which is the whole point of this class. + * + * The wait does not weaken the assertion: transposing the two callbacks leaves the screen in + * `Idle` forever, so it fails on the timeout with the same meaning it failed with before. */ @Test fun `a picked document is loaded as input rather than saved to`() { @@ -85,6 +108,11 @@ class LauncherWiringTest { composeRule.onNodeWithTag(TestTags.Converter.CHOOSE_FILE).performClick() deliver(Uri.parse("content://test/holiday.mkv")) + composeRule.waitUntil(PICK_TIMEOUT_MS) { + composeRule.onAllNodesWithTag(TestTags.Converter.FILE_CARD_NAME) + .fetchSemanticsNodes() + .isNotEmpty() + } composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD_NAME).assertIsDisplayed() } @@ -138,4 +166,13 @@ class LauncherWiringTest { ) composeRule.waitForIdle() } + + private companion object { + /** + * Long enough that a slow CI runner is not the reason this fails, short enough that a + * genuinely transposed callback does not stall the suite. The pick normally lands in + * single-digit milliseconds. + */ + const val PICK_TIMEOUT_MS = 10_000L + } } diff --git a/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegConcatCommandTest.kt b/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegConcatCommandTest.kt index c5b853d..764e1bc 100644 --- a/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegConcatCommandTest.kt +++ b/app/src/test/java/org/libremediaconverter/ffmpeg/FFmpegConcatCommandTest.kt @@ -56,6 +56,33 @@ class FFmpegConcatCommandTest { assertEquals("0", args[args.indexOf("-safe") + 1]) } + /** + * The gate that `-safe 0` does not open, and the one every real join needs (#238). + * + * `-safe 0` permits absolute *paths*; the concat demuxer separately whitelists the *protocol*, + * defaulting to `file,crypto,data`. `JoinScreen` picks with `OpenMultipleDocuments`, so real + * inputs are `content://` and `ConcatEngine` writes `ffkitsaf:` paths into the list file — which + * the demuxer refused outright, failing every stream-copy join a user could actually start. + * + * The re-encode strategy has no equivalent assertion because it needs none: it passes each + * input with its own `-i` and never feeds the demuxer a list file. That asymmetry is exactly + * why the defect survived — joining mismatched clips over SAF worked. + */ + @Test + fun `stream copy whitelists the protocol its list file entries actually use`() { + val args = FFmpegConcatCommand.build( + ConcatStrategy.STREAM_COPY, + inputs, + listFile, + output, + OutputFormat.MP4_H264, + ) + val whitelist = args[args.indexOf("-protocol_whitelist") + 1].split(",") + assertTrue("ffmpeg-kit's SAF scheme must be permitted, got $whitelist", "ffkitsaf" in whitelist) + // The defaults have to survive too: the list file itself is opened over `file`. + assertTrue("the demuxer still reads the list file itself, got $whitelist", "file" in whitelist) + } + @Test fun `re-encode passes every input separately and builds a filter graph`() { val args = FFmpegConcatCommand.build( diff --git a/docs/e2e-read-findings.md b/docs/e2e-read-findings.md new file mode 100644 index 0000000..b1257d2 --- /dev/null +++ b/docs/e2e-read-findings.md @@ -0,0 +1,382 @@ +# E2E-read findings + +**Status:** seven findings; E4 fixed, the rest standing, none urgent — **plus one confirmed vacuous test, which is a +ticket rather than an entry here** (see [Not covered here](#not-covered-here)). `E1`–`E6` came from +the 2026-09-05 read of the instrumented suite. Every entry here is a *test-suite* observation — +something a new test would not fix, because the test already exists and the problem is what it +claims rather than what it runs. +**Scope:** what reading all 60 instrumented tests turned up that writing a 61st would not fix. +**Last verified:** `main` at `4b02294`, 2026-09-05. **60 `@Test` methods in 12 classes**, three +carrying `@FailsOnEmulatorApi37`, gating API 37 leg 57. + +## Why this document exists, and why it is separate from the other two + +`docs/coverage-read-findings.md` (`F1`–`F10`) came from reading a **JaCoCo report**, and JaCoCo +measures `testDebugUnitTest` only. So four waves of coverage work have been shaped by a number that +**cannot see `app/src/androidTest` at all**. The instrumented suite has never had the equivalent +read: nothing has asked what those 60 tests actually pin, only that they are green. + +That is the gap this read is in. It is a **triage, not a test push** — the same shape as wave 4's +read, which "moved no number at all, and that is its result". + +`docs/defect-audit.md` (`D1`–`D16`) is the record of things *wrong at runtime*. Nothing here is +wrong at runtime. These are tests whose names, KDoc or reputation overstate what they execute. + +Entry ids are `E1`–`E6` so they cannot be confused with `F1`–`F10` or `D1`–`D16`. + +## How to read the confidence labels + +Same vocabulary as the other two documents, deliberately: + +- **Confirmed by inspection** — the control flow is fully readable and the finding follows from it. +- **Confirmed by measurement** — observed in a CI artifact, with the run id recorded. +- **No action** — recorded because it looks like a finding and is not. + +## The method, and the one filter that found everything + +A coverage number is useless here by construction, so the read used a different question, applied +to every one of the 60 tests: + +> **If the behaviour this test is named for stopped working, would it go red?** + +Three answers, and only the third is a gap: + +- **yes** — the test bites. Most of the suite. +- **no, and that is deliberate and written down** — `RealMediaBenchmark` asserts nothing on purpose + (E2); `transcodesH264ToH265AndReportsProgress` declines to assert progress for a stated reason + (E3). These are entries here, not tickets. +- **no, and nothing says so** — the gap. One test, and it is the most important one in the suite. + +**The reusable part is the second filter**, because "does it assert something?" would have cleared +the vacuous test — it asserts two things. What it does not do is *reach the code it names*: + +> **Does the test's own premise hold on the machine that runs it?** + +`HardwareFallbackTest` asserts `SUCCEEDED` and a non-empty output, and both are true of a +conversion that never went near the path it exists to prove (**#223**). See +[Not covered here](#not-covered-here); it is filed rather than recorded here because a test fixes it. + +--- + +## E1 — `RemuxTest`'s class KDoc argues for engine assertions three of its tests do not make, and they are right not to + +**Severity: low · Confirmed by inspection · the KDoc is what is wrong, not the tests** + +``` +app/src/androidTest/java/org/libremediaconverter/convert/RemuxTest.kt:31-42 +``` + +The class KDoc is headed **"Why these assert the engine, not just the file"** and makes a specific +argument: + +> A remux routed to FFmpeg produces a perfectly correct file — `-c copy` moves the same samples +> into the same container. So an output-only assertion passes whether the hardware transmux path +> ran or never executed at all […] which makes "silently always FFmpeg" the most likely way for +> this feature to regress. + +Five of its seven tests run a conversion. **Three assert no engine at all:** + +| test | output container | asserts engine? | +|---|---|---| +| `mkvToMp4RemuxesOnHardware` | MP4 | **yes** — `MEDIA3` | +| `mp4ToMkvRemuxesOnFFmpeg` | MKV | **yes** — `FFMPEG` | +| `webmToMkvKeepsVp9WithoutReencoding` | MKV | no | +| `audioOnlySourceRemuxesIntoMka` | MKV (`.mka`) | no | +| `mp4ToMpegTsAndAviProduceTheirOwnContainers` | MPEG-TS, then AVI | **TS only**; the AVI half does not | + +### Why this is not a gap + +`ConversionRouter.MEDIA3_CONTAINERS = setOf(Container.MP4)` (`ConversionRouter.kt:37`), and every +one of the three produces MKV or AVI. **They can only ever be FFmpeg**, so the regression the KDoc +names — "silently always FFmpeg" — is not a thing that can happen to them. The two tests where the +hardware path is genuinely at risk are exactly the two that assert it. + +An engine assertion on the other three would be near-tautological given today's router. It would +catch one thing: somebody adding MKV or AVI to `MEDIA3_CONTAINERS` without a muxer to match — which +is what `Media3MuxersTest` is for, on the JVM, where it does not need a device. + +### Why it is recorded rather than dropped + +**This was the strongest-looking candidate of the whole read and it dissolved on tracing**, which +is the same shape as `F5` in the coverage document (filed as a test gap, and only stopped being one +when someone went looking for its callers). Recorded so the next read does not re-file it. + +**The fix is one line of KDoc**, not three tests: the class asserts the engine *where the engine is +in doubt*, which is a better rule than the one it currently states. + +--- + +## E2 — three of the 60 instrumented tests assert nothing, and two of them never run + +**Severity: n/a · No action — deliberate, documented, and load-bearing as documentation** + +``` +app/src/androidTest/java/org/libremediaconverter/bench/RealMediaBenchmark.kt:25-53 +``` + +`reportDeviceEncoderCapabilities` logs and asserts nothing. `hardwareVersusSoftwareOnRealVideo` and +`av1InputRoutesAccordingToDeviceDecodeSupport` are `assumeTrue`-guarded on media that is **not +committed** and must be staged by hand into the app's internal `filesDir`, so they skip in every +automated run — they are the "2 skipped" every green leg reports, and `docs/local-emulator.md:305` +says so. + +The class KDoc is unambiguous: *"This is a benchmark, not part of the automated suite […] Not a +correctness test — the assertions are deliberately loose."* + +**No action.** Recorded for one reason: **the suite's headline number is 60, and three of those 60 +are not tests.** Any future statement of the form "60 instrumented tests cover X" is off by three, +and two of the three have never executed on CI at all. + +**It is the opposite of E-nothing, though** — `reportDeviceEncoderCapabilities` runs on every leg +and logs `BENCH can-encode:`, and **that log line is what confirmed the vacuous test this read +found** (**#223**). An assertion-free test that prints the machine's capabilities turned out to be +the only oracle in the suite. See [Not covered here](#not-covered-here). + +--- + +## E3 — `transcodesH264ToH265AndReportsProgress` does not assert that progress was reported + +**Severity: low · No action on the test; the name is the inaccurate part** + +``` +app/src/androidTest/java/org/libremediaconverter/convert/Media3EngineTest.kt:73, :90-93 +``` + +```kotlin +// Deliberately NOT asserting that progress fired. Polling is on a 250 ms tick, +// and a 3 s 320x240 clip can finish inside one tick on fast hardware, which +// would make the assertion fail intermittently for no real defect. +seen.forEach { assertTrue("progress out of range: $it", it in 0..100) } +``` + +`seen` is empty-safe: `forEach` on an empty list asserts nothing, so replacing `onProgress` with a +no-op reddens nothing here. The reasoning is sound and the alternative really is a flaky test. + +**No action on the body.** The name says `AndReportsProgress` and the body says it does not check +that, which is the `probeForConcat` shape from `CLAUDE.md` — *a passing test with a wrong +explanation is its own failure mode* — in its mildest form, since here the KDoc immediately corrects +the name. + +**Contrast the FFmpeg side, which is a real gap and is filed as #229**: `FFmpegEngine`'s percentage +arithmetic is executed by every FFmpeg test and observed by none, because every call site omits +`onProgress` entirely. Media3's is unasserted; FFmpeg's is unobserved. Only the second is a ticket. + +--- + +## E4 — the marker's KDoc says removing it grows the gating leg by two; three tests carry it + +**Severity: low · Confirmed by inspection · one line** + +``` +app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt:20 +``` + +> Delete the annotation from the tests, and the advisory job goes empty and the gating one grows by +> **two**. + +Three tests carry it — `Media3EngineTest:72`, `Media3EngineTest:135`, `SafPickerRoundTripTest:320` — +and `FAILS_ON_EMULATOR_API37_BASELINE = 3` eleven lines further down the same file, where the count +is machine-checked by `.github/scripts/e2e-report-shape.sh`. + +The third marker was added when the SAF rotation test was excluded; the sentence was not updated +with it. **Everything that is checked is consistent at three**; only the prose says two, which is +exactly why it drifted — and a good argument for the baseline const being a const. + +--- + +## E5 — `coverage-read-findings.md`'s F7 calls covered code uncovered + +**Severity: low · Confirmed by inspection · half of F7 is stale** + +F7 says `probeWithExtractor`'s catch (`MediaProbe.kt:180-182`) is unreachable on Robolectric and +"stays device-only", measured across four URI shapes. **The unreachability claim is correct and +stands.** The implication readers take from it — that nothing exercises it — does not: + +``` +app/src/androidTest/java/org/libremediaconverter/convert/RemuxTest.kt:111 +``` + +`probeDistinguishesAudioFromImagesFromRubbish` feeds it a file of random bytes and asserts +`InputKind.UNPARSEABLE`, on a device, on every gating leg. + +**"Device-only" holds; "uncovered" does not** — and the difference matters, because F7 is one of the +six entries that document calls "no action", on the grounds that a test would not help. A test +already exists. The entry should say so. + +**This is the failure mode the split between the two documents was meant to prevent**, and it caught +this repo out: a JaCoCo-derived document cannot see `androidTest`, so it will keep re-deriving +"uncovered" for anything the instrumented suite covers. That is a structural reason for this +document to exist, not a one-off correction. + +--- + +## E6 — the suite's one device-capability assertion derives its expectation from the call it is testing + +**Severity: low · Confirmed by inspection · no independent oracle exists** + +``` +app/src/androidTest/java/org/libremediaconverter/work/ConversionWorkerTest.kt:151-152 +``` + +```kotlin +val hasHardwareHevc = AndroidDeviceCodecs.get().canEncode(VideoCodec.H265) +``` + +and then the expectation is `if (hasHardwareHevc) MEDIA3 else FFMPEG`. The test asks +`AndroidDeviceCodecs` what to expect and then checks that the router agreed with +`AndroidDeviceCodecs`. **If the whole enumeration returned empty, this would still pass** — and +empty is precisely what the `runCatching` fallback returns (the reason `#194` was worth cutting; +it logs "assuming permissive" while making `canEncode` answer *no* for everything). + +Its KDoc defends the choice, and the defence is good: + +> Asserting MEDIA3 unconditionally tests the test machine, not the router. + +That is true, and there is no third source of truth on a device: `MediaCodecList` is what +`AndroidDeviceCodecs` reads, so any oracle built from it is the same oracle. + +**No action, but read it with #223.** It is the same missing oracle that makes the +vacuous-test fix a judgement call rather than a one-liner — you cannot assert "this device has +hardware HEVC" from inside the suite without asking the class under test. The honest options are a +visible skip or a red test, and that decision is the ticket's. + +--- + +## E7 — a real `DocumentsProvider` cannot be reached without the picker, so there is no cheap SAF test + +**Severity: n/a · Confirmed by measurement · this is a platform rule, not a gap** + +Added 2026-09-06, from doing #225 and #226 rather than from reading. + +`OutputPublisher.publish`'s destination side is asserted only against Robolectric fakes — +`FakeSafProvider`, registered with `asDocumentsProvider = true`, which is the flag that *makes* +`DocumentsContract.isDocumentUri` answer true. #226 split that into a cheap headless half (drive a +real `DocumentsProvider` directly) and an expensive picker-driven half. + +**The cheap half does not exist.** Three approaches, all measured on an API 34 emulator: + +| approach | result | +|---|---| +| a second `DOCUMENTS_PROVIDER` declared **without** `MANAGE_DOCUMENTS` | refused at install: `SecurityException: Provider must be protected by MANAGE_DOCUMENTS` | +| create the document as the **test APK**, which owns the provider | denied — instrumentation runs *in the target app's process*, so it carries the app's uid whatever `Context` is asked | +| `uiAutomation.adoptShellPermissionIdentity(MANAGE_DOCUMENTS)` | denied identically | + +The denial names the only way in: + +> `Permission Denial: opening provider …FixtureDocumentsProvider from +> ProcessRecord{… org.libremediaconverter/u0a192} requires that you obtain access using +> ACTION_OPEN_DOCUMENT or related APIs` + +And the intent filter is not optional: without it `isDocumentUri` returns false, which is exactly +the branch guarding `deletePartialOutput` — so a provider without the filter tests nothing the +ticket is about. + +**So any test of `publish` against a real `DocumentsProvider` must drive DocumentsUI**, and pays +#190's flake tax. The work is one item at that cost, not two, and #226 was updated to say so. + +### What this does *not* block, which is the useful half + +`FFmpegKitConfig.getSafParameterForRead` — the bridge on every real conversion and join — needs no +documents provider. It opens a descriptor through the resolver, so **any readable `content://` URI +exercises it**, and an ordinary `ContentProvider` may be exported without a permission. That is what +`FixtureContentProvider` is, and it made #225 headless. + +**That distinction was worth the trouble**: the first test ever to hand the join path a real +`content://` input found #238, a defect that broke joining for every user who picks matched files. +The expensive gate protects the *destination* side; the *input* side never needed it. + +## Summary + +| ID | Finding | Severity | Evidence | Action | +|---|---|---|---|---| +| E1 | `RemuxTest`'s KDoc claims engine assertions three of its tests correctly omit | low | confirmed by inspection; traced through `MEDIA3_CONTAINERS` | **one line of KDoc** — the tests are right | +| E2 | Three of the 60 instrumented tests assert nothing; two never run | n/a | confirmed by inspection; `docs/local-emulator.md:305` | **no action** — deliberate; but 60 ≠ 60 | +| E3 | `…AndReportsProgress` does not assert progress fired | low | confirmed by inspection; reason inline | **no action** — the name overstates, the KDoc corrects it | +| E4 | The API 37 marker's KDoc says "two"; three tests carry it | low | confirmed by inspection; baseline const says 3 | **fixed** in #243 — it names the constant now | +| E5 | `coverage-read-findings.md` F7's "uncovered" half is stale | low | confirmed by inspection; `RemuxTest.kt:111` drives it | **amend F7** — "device-only" stands, "uncovered" does not | +| E6 | The device-capability assertion asks the class under test what to expect | low | confirmed by inspection; no third oracle exists on a device | **no action** — read with **#223** | +| E7 | A real `DocumentsProvider` is unreachable without the picker, so #226 has no cheap half | n/a | measured three ways on API 34; each denial names `ACTION_OPEN_DOCUMENT` | **no action** — it re-scoped #226 | + +**Six of the seven are prose, not code**, and that is the shape of this read. The instrumented suite +is in good condition: 57 of its 60 tests bite, the fixtures are committed with their generation +recipes, and the one class that asserts nothing says so in its first line. What this read found is +that **the suite's self-description has drifted from the suite** in five small places and one large +one. + +**The large one is not in this table**, because a test fixes it: **#223**. + +## Not covered here + +**The vacuous test.** `HardwareFallbackTest.aFileMedia3CannotDecodeStillConvertsViaFfmpeg` passes on +every CI leg without ever entering the fallback it exists to prove. It is **#223**, not an entry +here, because a test fixes it — and it is the reason this read happened rather than an aside from it. + +Measured, not inferred, on run **`34004304566`** (all legs green), from each leg's own +`e2e-diagnostics-api*` logcat: + +``` +I/AndroidDeviceCodecs: Hardware video encoders: [] +I/RealMediaBenchmark: BENCH can-encode: COPY=true, H264=false, H265=false, VP9=false, VP8=false, AV1=false +I/ConversionWorker: Routing sample_h264_444.mp4 -> OutputSpec(container=MP4, videoCodec=H265, + audioCodec=AAC) via FFMPEG (NO_HARDWARE_ENCODER) +``` + +Identical on **API 33, 34, 35 and 37**. (API 36's logcat artifact on that run is truncated to 838 KB +and carries no test output at all, so it is unread rather than different.) The job is routed +**straight to FFmpeg before Media3 is attempted**, the `catch` in `runMedia3OrFallBack` is never +entered, and the test's two assertions — `SUCCEEDED`, output non-empty — are true anyway. It ran in +448 ms. + +**The repository already knew.** `ForcedFailureTest.hardwareFailureFallsBackToSoftware`, in the same +package, pins `ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE }` and says why: + +> most emulators expose no hardware video encoder at all -- so the router would legitimately send +> the job straight to FFmpeg and the hardware path would never be attempted. Without this the test +> passes on a Pixel and fails on every emulator, which says nothing about the code under test. + +`ConversionWorkerTest.routesAFastMp4JobByDeviceCapability` records the same fact a third time. The +knowledge is in two sibling files; `HardwareFallbackTest` is the one that walked into it — and +because its assertions are about the *output* rather than the *path*, it passes where +`ForcedFailureTest` would have failed. **That asymmetry is why nobody noticed.** + +**State it precisely.** The fallback *wiring* is covered on every leg by `ForcedFailureTest`, with +fakes. What has never run on any emulator is a fallback triggered by a **real** mid-export codec +failure — which is the case `HardwareFallbackTest` exists for, and the only reason +`sample_h264_444.mp4` is committed at all. That fixture, generated with x264 because Fedora's +ffmpeg ships openh264 and cannot produce High 4:4:4, does nothing on any CI leg today. + +The fix is not one assertion. `KEY_ENGINE_USED` is `FFMPEG` **whether the fallback fired or the +router went straight there** — asserting it changes nothing. The vacuity guard is two facts +together: the router chose `MEDIA3` for this request on this device, *and* the worker reported +`FFMPEG`. Whether to reach that with `assumeTrue` (a visible skip on emulators, and the "2 skipped" +becomes 3) or with an assertion (red on emulators, announcing it cannot test what it claims) is a +decision, not a detail — see **E6** for why no third option exists — and **#223** leaves it open. + +**The other e2e gaps this read found are tickets too**, and are not repeated here: + +| # | Gap | +|---|---| +| # | Gap | Outcome | +|---|---|---| +| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg | closed — it skips instead of passing vacuously | +| **#224** | Cancelling a *running* native session, in any of the three engines | closed — all three engines | +| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge | closed, and it found **#238** | +| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider` | **open** — re-scoped by E7; one picker-driven item, not two | +| **#227** | The notification's Cancel action has never been fired | closed | +| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file | closed | +| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere | closed | +| **#230** | *(spike)* whether a running conversion's process can be killed | closed — it cannot; the runner shares the app's process | + +**The read's own result, once the tickets were worked: one production defect.** #238 — joining files +picked through the system picker failed outright on the stream-copy path, because the concat demuxer +whitelists protocols separately from `-safe 0` and `ffkitsaf` was not on the list. Only `STREAM_COPY` +feeds the demuxer a list file, and every existing join test passed `Uri.fromFile`, so the one broken +combination was the only one a user could reach. + +That is the argument for this kind of read in one line: the gap was not a missed line or an +unasserted value, it was **a combination of two covered things that no test put together**. + +**Nothing here was filed as a coverage delta.** Each names the mutation that has to go red, which is +the acceptance criterion wave 4 established and which caught two vacuous tests in that wave before +they shipped. #223 is the one that shows why the criterion matters: it has two passing assertions and +still tests nothing. diff --git a/docs/local-emulator.md b/docs/local-emulator.md index 2e1519c..c326f28 100644 --- a/docs/local-emulator.md +++ b/docs/local-emulator.md @@ -312,6 +312,18 @@ on sample media that is deliberately not committed. Its third test, `reportDeviceEncoderCapabilities`, has no such guard and runs. A level reporting 0 skipped would mean someone had staged sample files, not that something improved. +**Since #223 there is a third, and it is the interesting one.** +`HardwareFallbackTest.aFileMedia3CannotDecodeStillConvertsViaFfmpeg` is `assumeTrue`-guarded on +`AndroidDeviceCodecs.get().canEncode(H265)`, which is false on every emulator image — so it now +skips here and runs only on the Pixel. It used to *pass* on emulators without ever attempting the +hardware path, which is worse. **Expect `skipped="3"` locally**, and note the guard is a property +of the machine rather than of staged files: a level reporting 2 would mean an emulator image had +gained a hardware HEVC encoder, which is worth knowing. + +That test's KDoc carries the measurement, including the part that decides it: forcing the route to +Media3 anyway does *not* produce a fallback, because the goldfish decoder decodes the High 4:4:4 +fixture despite declaring `NoSupport` for its profile. + ### What the sweep adds, and what it does not **The renderer rule held four more times.** No boot log contains the string