diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index db71918..ff8b54a 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -272,7 +272,7 @@ jobs: # has to begin after them, not between them. The name is stale and kept: # read .github/scripts/e2e-run.sh's header, which carries the measurements. # - # notAnnotation below keeps four tests off this row, not two, and one of + # notAnnotation below keeps five tests off this row, and one of # them is new. SafPickerRoundTripTest's PICKER test was measured on # 2026-08-24 as passing here and was left on the leg; four gating logcats # read on 2026-09-05 show it aborting system_server from the task-snapshot diff --git a/CLAUDE.md b/CLAUDE.md index 75ef00e..2eb09c3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,19 +76,19 @@ days. Read it as the current answer, and see the git history if you need the old `angle_indirect` and `swangle_indirect` all boot, while `auto`, `off`, `guest` and `swiftshader_indirect` do not. `docs/local-emulator.md` has the evidence and the per-API renderer table. -- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Four** of the 60 instrumented - tests cannot be *run* on that image, for three unrelated reasons: two Media3 hardware transcodes - fail inside the emulator's own `c2.goldfish.h264.decoder`, one SAF test takes the framework down - when it rotates the display, and its sibling — the SAF picker round trip — aborts `system_server` - from the task-snapshot path whether it passes or not. All four carry `@FailsOnEmulatorApi37` and - run in a separate `continue-on-error` job; the gating leg runs the other 56. +- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Five** of the 68 instrumented + tests cannot be *run* on that image, for three unrelated reasons: three Media3 tests fail inside + the emulator's own `c2.goldfish.h264.decoder`, one SAF test takes the framework down when it + rotates the display, and its sibling — the SAF picker round trip — aborts `system_server` from + the task-snapshot path whether it passes or not. All five carry `@FailsOnEmulatorApi37` and run + in a separate `continue-on-error` job; the gating leg runs the other 63. **That third reason is why "cannot pass" became "cannot be run" on 2026-09-05.** Four gating runs were read logcat-first — 34006456986, 34001744574, 34001377499 and the green 34002313300 — and each carries exactly two `hasReadColorBufferDma` aborts before the suite (surfaceflinger, during boot and the SystemUI disable) and exactly **one** during it: `system_server`, thread `TaskSnapshotPer`, always inside the picker test's window, and nothing else in the gating set - reaches the mapper at all. Whether the leg went red was luck — one run passed the test and lost + reached the mapper at all. Whether the leg went red was luck — one run passed the test and lost the leg anyway with `failed: 0`, another passed it 0.6 s after the abort and went green. That is #108, it cost roughly a third of the gating legs over the wave-4 landings (#190), and a marker is what it needed. `docs/api-37-emulator-crash.md` has the timings. @@ -108,7 +108,7 @@ days. Read it as the current answer, and see the git history if you need the old describes everything in it. The name is kept deliberately — it is not a required context and people have learned to look for it — so **read the marker, not the name**, for what it holds. **It is red on every PR, by design**: do not read it as your change breaking something, and do - not read a green run as evidence those four tests pass. + not read a green run as evidence those five tests pass. `docs/api-37-emulator-crash.md` has the measurements. **That instruction is also why nobody looks, so the job now reports its own shape** — expected, @@ -128,7 +128,7 @@ days. Read it as the current answer, and see the git history if you need the old is gradle never returning, so the log it left says nothing about it. Still true, and the reason the advisory job is not simply deleted: **API 37 needs a manual check on -the Pixel 10 Pro XL before each release.** Those four tests are the one thing CI cannot answer +the Pixel 10 Pro XL before each release.** Those five tests are the one thing CI cannot answer for. On a device or emulator, build only the ABI it can execute: @@ -332,6 +332,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 d53d372..fd1c93f 100644 --- a/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt +++ b/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt @@ -9,9 +9,9 @@ package org.libremediaconverter * That is the whole reason there is one annotation rather than a pair of test lists: two lists * drift, and the drift is silent in both directions (a test that runs nowhere reads as green). * - * **"Cannot be run" covers two things, and it said only the first until 2026-09-05.** Three of the - * four carriers simply fail: two Media3 transcodes die in the image's own `c2.goldfish.h264 - * .decoder`, and the SAF rotation test takes the framework down with it. The fourth — + * **"Cannot be run" covers two things, and it said only the first until 2026-09-05.** Four of the + * five carriers simply fail: three Media3 tests die in the image's own `c2.goldfish.h264.decoder`, + * and the SAF rotation test takes the framework down with it. The fifth — * `SafPickerRoundTripTest.pickingAFileThroughTheSystemPickerFillsInTheFileCard` — **passes about * half the time and aborts `system_server` every time**, which is worse for a gating leg than an * honest failure: it fails the leg from the teardown, with no failing test to point at (#108). @@ -26,7 +26,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 four. + * 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 @@ -51,13 +51,14 @@ annotation class FailsOnEmulatorApi37 * A *smaller* failure count is the interesting direction: it means one of them now passes, which * is the trigger the KDoc above names for deleting the annotation. * - * **The fourth carrier is the one to read that sentence carefully for.** + * **The picker test is the one to read that sentence carefully for.** * `pickingAFileThroughTheSystemPickerFillsInTheFileCard` was marked on 2026-09-05 for aborting * `system_server` rather than for failing (#108), and on the gating leg it passed two runs of * four. It fails on the advisory leg because the rotation test runs before it and takes the * framework down first — measured, `api37-debug.yml` run 34008889182, which reports * `expected: 4, received: 4, failed: 4` with the four in the order Media3, Media3, rotation, - * picker. + * picker. (Those dispatches predate the third Media3 marker landing on `main`, so their totals + * are four rather than five; the ordering they establish is what matters here.) * * **But a second dispatch of the identical configuration reported 4/3/3**, having lost the last * test to the abort rather than to anything about the test list, and that is why @@ -77,4 +78,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 = 4 +const val FAILS_ON_EMULATOR_API37_BASELINE = 5 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 4a7d6cb..d059f4e 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 @@ -16,6 +26,7 @@ import org.libremediaconverter.convert.MediaProbe import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.ffmpeg.ConcatEngine import org.libremediaconverter.model.ConcatStrategy +import org.libremediaconverter.work.ConcatWorker import java.io.File /** @@ -50,6 +61,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 @@ -180,4 +267,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 959f8c3..c82e2ce 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/api-37-emulator-crash.md b/docs/api-37-emulator-crash.md index 4b8cc0d..bd37a73 100644 --- a/docs/api-37-emulator-crash.md +++ b/docs/api-37-emulator-crash.md @@ -443,7 +443,7 @@ Four gating API 37 runs read logcat-first — 34006456986, 34001744574, 34001377 | 34002313300 | 00:58:12.7 → 00:58:19.8, passed | 00:58:19.218 | green | Every one is `system_server`, thread `TaskSnapshotPer`, and every one lands inside that test's -window. Nothing else in the gating set of 57 reaches the mapper at all. So the picker test is +window. Nothing else in the gating set reached the mapper at all. So the picker test is **deterministic** in what it does to the image and a coin flip in what the leg reports: 34001377499 passed it and lost the leg from teardown with no failing test to name, and 34002313300 passed it 0.6 s after the abort and went green. 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