Compare commits

...
Author SHA1 Message Date
Jason Ross 64d5cbf738 Merge pull request #272 from JMR-dev/fix/102-picker-back-press-overshoot
Stop the picker dismissal destroying MainActivity, and census what actually turns legs red (#102)
2026-09-07 17:47:18 -05:00
JMR-devandClaude Opus 5 842965a479 Say who closed the picker, because it was us and the KDoc denied it
The commit below describes "a back press aimed at a picker that had closed five seconds
earlier". The timing is right and the agency is wrong, and the agency is the interesting
half. Re-read out of the same logcat:

  20:59:35.686  UiDevice: Retrieving node ... [RES='android:id/button1'].
  20:59:35.689  UiObject2: Clicking on (927, 2274).
  20:59:36.033  MainActivity RESUMED
  20:59:36.350  VRI[PickActivity]: visibilityChanged ... newVisibility=false

`aerr_wait` and `aerr_close` both missed on that iteration and `dismissASystemErrorDialog`
fell through to `android:id/button1` -- the framework's generic AlertDialog positive button,
which is on every AlertDialog on the device. It hit one inside DocumentsUI, and that is what
closed the picker. The picker did not close on its own; this class closed it.

So the destroyed Activity and #271 are one incident rather than two findings that happened
to share a trace, and the loop's shape is three iterations rather than two: iteration 2
closes the picker through `button1` and then presses back into an app that is already in
front, iteration 3 dismisses the launcher's ANR dialog and presses again, and that press
finishes MainActivity.

It also sharpens what the fix does. With the re-read, iteration 2 returns -- the app is
focused within a second of the `button1` click -- so neither of the two presses that
followed it happens at all. The previous message implied the fix caught only the last one.

`requireAReadableScreen` now drops a Boolean return value, which this codebase treats as a
smell. It is correct there -- the `device.wait` on the next line is the re-probe -- and the
call site says so rather than leaving a reader to work out whether it was an oversight.

No behaviour change beyond the comment: the fix itself is unchanged and the sweep is re-run
because `app/src` is touched.

Refs #102, #271

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-07 17:18:23 -05:00
JMR-devandClaude Opus 5 89832563e6 Stop the picker dismissal destroying MainActivity, and census what turns legs red
`dismissThePicker` guarded its back presses on `Activity.hasWindowFocus`, which a system
app-error dialog makes false as well -- it is a fullscreen `system_server` window, which is
why `dismissASystemErrorDialog` exists at all. So the guard could not tell "the picker is
still up" from "a dialog is on top of an app that is already in front", and the loop
dismissed the dialog and then pressed back on the reading it had taken before doing so.

Measured on the API 35 gating leg of run 34161043035 attempt 1, whose head is #269's own
commit: the save picker returns at 20:59:36.033, the launcher's ANR dialog is dismissed at
20:59:41.169, a back press goes out at 20:59:41.713, the launcher is moved to the front 41 ms
later, and MainActivity is DESTROYED at 20:59:42.278. Everything after that in the test throws
`NullPointerException: Cannot run onActivity since Activity has been destroyed already`.

The fix re-reads the focus after a dialog is actually dismissed, and only then -- so it
removes a back press sent on a stale reading rather than retrying one. A picker genuinely in
front still leaves the app unfocused and still gets the press, so nothing about what this
class can catch changes; and on the ordinary path, with no dialog, nothing is re-read and
nothing is waited on. `requireAReadableScreen` twenty lines away has always re-probed after
dismissing a dialog; this is the same rule in the one place that did not follow it.

It cannot be demonstrated by re-running and the KDoc says so: the launcher ANR is ambient on
these runners and is not reproducible on demand, so a green sweep is not evidence for this.
The trace is.

`docs/ci-failure-modes.md` is the rest of it -- a census of every gating E2E leg-attempt in
the repo's history, 129 failures in 1489, classified by mode with a disposition each. It
closes out #102, whose own mode turns out to be the emulator's Codec2 HAL segfaulting: a null
dereference in `getClientUsage` inside `libcodec2_goldfish_common.so` kills
`c2.goldfish.h264.decoder`, and Media3's 25 s export watchdog then aborts the export. Six
occurrences, six carrying that crash in the same job's log, 0.5% of API 33-36 leg-attempts,
and the same vendor HAL `@FailsOnEmulatorApi37` already names.

Three counting rules are in the document and in CLAUDE.md because each was learned by getting
it wrong: count per leg-attempt rather than per run, since a re-run to green replaces the
conclusion; give every mode its own denominator, since the API 37 row filters seven tests out
and some tests are younger than the window; and capture the log before retrying, since
`gh run view --job <id> --log` resolves by run and serves the latest attempt.

Refs #102

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-07 17:14:57 -05:00
Jason Ross ef9d35ed40 Merge pull request #269 from JMR-dev/fix/268-saf-picker-determinism
Synchronise SafPickerRoundTripTest on state, not on timing (#268)
2026-09-07 16:21:34 -05:00
JMR-devandClaude Opus 5 b0b8b66d31 Name reattach's third outcome, which this KDoc denied existed
The determinism argument said no coroutine had a `_state` write left in
flight, because `reattach` "has either returned on its `_state.value !is Idle`
guard or found nothing". There is a third outcome: `pruneWork()` is async, so
`reattach` can find an unpruned job, pass that guard, and start an `observe()`
that is a live coroutine with writes ahead of it.

The conclusion survives, by a mechanism the paragraph did not mention.
`reattach` reads `ownership.current` before its query and hands that token to
`observe`, while `onInputPicked` calls `ownership.claim()` synchronously on the
pick -- so once a detail row exists that observation is superseded and every
emission returns at `stillHeldBy` before it writes. The claim is therefore
"every write in flight is landed or superseded", not "no other coroutine
started".

This is the KDoc a future reader opens to learn why the test cannot flake, and
CLAUDE.md records the same failure mode twice already -- E1/E3, and #226's KDoc
that described a draft rather than the code. A correct test with an incomplete
explanation is its own defect.

Comment only; no test logic changed. Committed with --no-verify on the repo
owner's explicit say-so: the gate's cache is keyed on the `app/src` tree hash,
so a comment costs a full 33-36 sweep, and CI is already green on the parent
commit. ktlintCheck, detekt and compileDebugAndroidTestKotlin were run by hand
first and pass -- those are what a comment edit can actually break.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-07 16:19:24 -05:00
JMR-devandClaude Opus 5 9fd96d08fd Synchronise SafPickerRoundTripTest on state, not on timing (#268)
Its two picker tests failed on roughly half of gating runs, by two measured
mechanisms. Both are removed here rather than re-tuned; the fix is in the test.

**A -- the Convert tap was lost in the post-probe relayout.**
`ConversionViewModel.onInputPicked` writes `_state` twice: name and size first,
then the probe. The second write grows the file card and moves the Convert
button. Compose computes the tap's coordinate from the semantics node and
dispatches afterwards, so a relayout in that gap hit-tests a stationary
coordinate against the new layout and the touch lands on whatever moved into
the button's place -- silently. Measured as the gap between the pick's FFprobe
closing and the tap: 319 ms and 421 ms passed; 46 ms, 98 ms and 124 ms did not.

`convertToTheDefaultFormat` now waits for the `Container` detail row before
tapping. That row is composed only under `input.probe != null`, so its presence
means both of `onInputPicked`'s writes have landed and been laid out -- and
nothing else in the ViewModel has a `_state` write in flight at that moment
(`reattach` returned on its non-Idle guard, `observe` starts inside `convert()`).
The card cannot change height again before the tap. That is a different claim
from waiting longer.

**B -- the app was not the focused window when Compose was queried.**
One failure had a 416 ms gap, so it was not A: the tap landed,
`GrantPermissionsActivity` started, back was pressed, and nothing was ever
enqueued. A back press goes to whichever window holds *input* focus, while
`Until.hasObject` answers about the accessibility tree -- which can carry the
dialog's nodes first -- so a back that arrives one window early lands on
`MainActivity` and finishes it.

`POST_NOTIFICATIONS` is now held before the tap instead of the dialog being
dismissed after it. `RequestPermission.getSynchronousResult` returns without
starting anything when the permission is already granted, so there is no
foreign window, no back press, and nothing the test injects can finish the
Activity. `@Before` asserts the grant rather than assuming it.

The class KDoc claimed granting "was tried first and did not take". Re-measured
at API 34, six consecutive runs: zero `REQUEST_PERMISSIONS` starts, zero
`GrantPermissionsActivity`, and exactly two `Scheduling work ID` lines per run
-- one per converting test, so neither tap was lost.

Also adds a fail-fast that says the Convert tap started nothing, instead of
spending the 300 s conversion budget and then naming `action.saveFile`. It is a
diagnostic, explicitly not the synchronisation.

Not fixed in production. The double write is deliberate, documented progressive
disclosure -- blocking the screen on an FFprobe process spawn reads as the app
ignoring the tap -- and a layout fix (pinning the button, reserving the card's
height) would make A less likely for one widget where waiting on the probe makes
it impossible for every tap. The ticket's argument that each added `OutputFormat`
widens A does not hold either: the format `FlowRow`'s height is fixed for a given
entry list and does not change when the probe lands. What displaces Convert is
the card growing, independent of chip count.

Mutation, run not predicted: deleting `publish`'s `if (destinationWasEmpty)
deletePartialOutput(...)` arm reddens `aFailedSaveDeletesTheDocumentItCouldNotWrite`
with "publish did not delete the document it could not write", and reddens
nothing else -- its sibling stays green, since the success path never enters
that catch.

Counts re-derived and unchanged: 72 androidTest tests, 7 markers, 65 gating,
FAILS_ON_EMULATOR_API37_BASELINE = 7. Both tests keep @FailsOnEmulatorApi37.

`NotificationCancelActionTest`'s KDoc said the suite grants no runtime
permissions; that is no longer true and it now says so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-07 15:44:20 -05:00
Jason Ross fa22bf3b13 Merge pull request #261 from JMR-dev/feat/ogg-vorbis-libvorbis
Rebuild the FFmpeg AAR with libvorbis, and make Ogg Vorbis reachable (#254)
2026-09-07 13:25:37 -05:00
Jason Ross 73482520aa Merge branch 'main' into feat/ogg-vorbis-libvorbis 2026-09-07 12:16:43 -05:00
Jason Ross 563ec33d94 Merge pull request #267 from JMR-dev/docs/e9-union-branch-tier
Read the union's branch tier, and decide 12 of its 22 sites (E9)
2026-09-07 12:06:19 -05:00
JMR-devandClaude Opus 5 a4ca93b00e Say what the artefact check measured, not what it implies
Two claims in E9 went further than the evidence, in an entry whose whole subject
is a figure that was quoted past its own.

"All 16 sat on that one line" was consistent with what was measured, not
established by it: `MediaProbe:321` accounts for a 16-branch difference and the
denominators now agree at 1338, but the other lines were never enumerated in both
reports, so a line that lost coverage elsewhere would have been invisible to the
check that was run. Says that now.

And `matroskaOrWebm` is not "the only string-literal `when` in that file" --
`shortName` at :429 is a second one. The parenthetical was never checked; it is
gone rather than repaired, since the mechanism was not what the measurement
established anyway.

The previous commit message carries the stronger wording. It is left alone rather
than rewritten, because that would need a force push.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-07 12:03:08 -05:00
JMR-devandClaude Opus 5 aaa1b64f87 Read the union's branch tier, and decide 12 of its 22 sites (E9)
E8 classified the 32 lines neither suite executes and stopped there. It never
asked which *arms* neither suite takes on lines both suites run, and that tier
is where what is left actually lives.

Rebuilt the union rather than reusing E8's artifact: a fresh jacocoTestReport
merged with E8's own API 34 .ec against one set of current class files. Union is
99.0% line, 90.1% branch. Controls confirm the device half applied -- 32 lines in
FFmpegEngine, 24 in Media3Engine, 15 in ConcatEngine, 9 in MainActivity that the
JVM suite never reaches.

Three things came out of it that are worth more than the count:

- **E8's denominator artefact is retired.** It warned the union's branch
  denominator ran 16 ahead "entirely inside MediaProbe" and told readers not to
  quote a MediaProbe branch figure raw. Rebuilt, both denominators are 1338 and
  MediaProbe:321 reads mb=0 cb=4. All 16 sat on that one line, so it was a
  property of how the report was built and never of the code.

- **CLAUDE.md's "18" is not stale.** It reproduces exactly under `mi == 0 && mb > 0`
  (19 branches on 18 lines) and not under `ci > 0`, which admits partially-executed
  signature lines and gives 139. Different metrics, not drift -- stated in E9 so
  the next read does not "correct" a figure that is right.

- **Tier 1 is 22, down from 32.** #252 closed the ten getForegroundInfo lines and
  added none. Two classes carry a one-line blind spot because #252 changed their
  bytecode and JaCoCo rightly rejected E8's .ec for them; the bound is measured,
  not assumed, and the line is not a gap.

Of the 22 branch sites, 12 are decided here -- compiler codegen, an F4/F6/F10
exemption already on record, or a thread race -- and recorded in E9 rather than as
new F-entries, since coverage-read-findings.md is being rewritten by #261. The
other 10 are filed as #262-#266, each naming the mutation that must go red or
saying that the read is the ticket.

FFmpegCommandBuilder:183's missed arm is VORBIS, which #261 is closing as it
lands, so the set is 21 the day it merges. E9 says to re-derive rather than edit
that sentence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-07 11:56:44 -05:00
JMR-dev e6ac84cd24 Merge remote-tracking branch 'origin/main' into feat/ogg-vorbis-libvorbis 2026-09-07 11:47:17 -05:00
Jason Ross c2cc9e2fc7 Merge pull request #259 from JMR-dev/feat/expedited-conversion-work
Expedite user-initiated work, and give both getForegroundInfo overrides a caller (#252)
2026-09-07 11:20:52 -05:00
Jason Ross 4d21996735 Merge branch 'main' into feat/expedited-conversion-work 2026-09-07 11:11:53 -05:00
JMR-devandClaude Opus 5 d45abe7409 Rebuild the FFmpeg AAR with libvorbis, and make Ogg Vorbis reachable (#254)
`FFmpegCommandBuilder` has emitted `-c:a libvorbis` since the day it was
written, and libvorbis was not in the AAR this app ships: the configure line
omitted `--enable-libvorbis`, and `strings` on both ABIs' `libavcodec.so`
named every other external encoder and not that one. The arm was unreachable
from both ends, so nobody ever hit it -- but the first user to pick Ogg
Vorbis would have got `Unknown encoder 'libvorbis'`. That is #238's shape
again: two individually-correct facts, a builder arm and a configure line,
that no test put together, and that no coverage number can see.

So the binary is rebuilt rather than the arm rewritten. FFmpeg's in-tree
`vorbis` encoder was already in there and was tried first; it is
experimental, stereo-only, and its quality knob spans 2x its floor against
libvorbis's 6x. Shipping it would have meant `-strict experimental`, a
forced `-ac 2` that silently upmixes every mono source, and a slider with
nowhere to go. What ships instead is the arm as originally written,
`-c:a libvorbis -q:a 5`, with `OGG_VORBIS` added to the presets, `VORBIS`
added to `ENCODABLE_AUDIO`, and Ogg's per-codec extension fixed so a Vorbis
file is not named `.opus`.

The flag is `--enable-libvorbis`, read out of ffmpeg-kit's
`get_library_name()` rather than guessed: the `--enable-lame` /
`--enable-opus` rule predicts `--enable-vorbis`, and that is not it. An
unrecognised `--enable-*` is ignored silently, so the artifact was checked
before `bin/README.md` was touched -- `libvorbis` present in both ABIs, the
configure line otherwise identical, FFmpeg still n8.1.2, 10 shared libraries
per ABI, every LOAD still `0x4000`.

Both mutations were run on API 34 rather than predicted. Pointing the arm at
`libopus` reddens the e2e test with `expected:<[audio/vorbis]> but
was:<[audio/opus]>` while its `OggS` assertion still passes, which is why
the track MIME is asserted and the container magic is not enough. Adding
`-ac 2` back reddens it with `expected:<[1]> but was:<[2]>`: this class's
own fixture is mono, so mono staying mono is an assertion rather than a
claim.

The unit test's load-bearing assertion inverts with this change and is
rewritten to say so. It used to assert that `libvorbis` was *absent*; it now
asserts the encoder name plus the two flags that must not be there. Nothing
on the JVM can tell a real encoder name from a fictional one -- which is
exactly how this survived four coverage waves -- so the e2e test is the only
thing that proves the positive.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 18:31:36 -05:00
Jason Ross 264b8027e4 Merge pull request #260 from JMR-dev/fix/gate-cache-in-worktrees
Resolve the gate's cache dir with --git-common-dir, and say when it cannot (#258)
2026-09-06 18:02:22 -05:00
JMR-devandClaude Opus 5 7d1d3191a9 Resolve the gate's cache dir with --git-common-dir, and say when it cannot (#258)
CACHE_DIR was the literal ".git/lmc-verify". In a linked worktree `.git` is a
FILE containing `gitdir: ...`, so `mkdir -p .git/lmc-verify` fails with "Not a
directory" -- and because the write is the last thing the script does, it failed
while the gate still printed green and exited 0. Every commit and push from a
worktree then re-swept API 33-36 for nothing, silently. That is the worst shape a
cache can fail in: invisible and expensive, and it was found by an agent paying
for it four times over rather than by the tool saying anything.

Measured both ways: in a worktree the old expression gives
`mkdir: cannot create directory '.git': Not a directory`, exit 1; `git rev-parse
--git-common-dir` gives the real path and exit 0.

--git-common-dir rather than --git-dir so the cache is SHARED between worktrees.
The key is the app/src tree hash, and identical content is identical content
whichever worktree produced it -- a sweep run in one is evidence for all of them.

The write also stops being silent. record_sweep() prints when it cannot record,
because a cache that never fills looks exactly like one that is working.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 17:54:12 -05:00
JMR-devandClaude Opus 5 6a8cc01862 Expedite user-initiated work, and give both getForegroundInfo overrides a caller (#252)
`ConversionWorker.request` and `ConcatWorker.request` now carry
`setExpedited(RUN_AS_NON_EXPEDITED_WORK_REQUEST)`. Conversions and joins are
started by a tap; the jobs that have to go back through JobScheduler because no
process is left to start them should not queue behind a background chore. The
class KDoc that said expedited was "deliberately not used" is replaced with what
was actually read out of work-runtime 2.11.2: retries are never expedited
(`SystemJobInfoConverter:135`), and a job the system stops mid-run is resolved as
`ResetWorkerStatus` and re-enqueued rather than answered by `FailureOutcome`.

#252's own premise does not survive measurement, and that is the second half of
this change. `getForegroundInfo()` is WorkManager's expedited-work hook, but
`WorkForeground.kt:38` opens the library's only caller with
`if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, and minSdk is 33 --
so `setExpedited` alone leaves both overrides exactly as cold as the first
instrumented coverage read found them. Measured on API 34 rather than argued:
with the flag set and `doWork` still building its own notification, both methods
report `missed 1 / covered 0` and all ten lines `ci=0`, and the whole
instrumented suite is green anyway at 71/71.

What makes them live is that each worker held two definitions of one
notification. `doWork` now posts the override's instead of an identical copy, so
`ConcatWorker`'s countless "Joining files" -- which nothing executed and which
was therefore free to drift from the "Joining N files" that ran -- is gone.
After: both `getForegroundInfo` report `LINE 0 missed / 5 covered`.

Five mutations were run and all five went red: dropping `setExpedited` from
either request, hard-coding the conversion title, moving its `percent` off zero,
and dropping the join's input count.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 17:06:00 -05:00
Jason Ross 68b863fbdb Merge pull request #257 from JMR-dev/chore/gate-runs-shellcheck
Run shellcheck in the local gate, at CI's exact pin
2026-09-06 16:28:29 -05:00
JMR-devandClaude Opus 5 f4174e5b06 Run actionlint in the gate too, at CI's exact pin
The other half of the hole the previous commit closed. `git ls-files '*.sh'` does
not match workflow `run:` blocks, and a good deal of this repo's bash lives
there -- so a workflow edit was still the case where the gate passed and CI's
Static analysis leg went red.

Pinned by digest, read out of status_check.yml rather than copied, for the reason
the shellcheck section gives and for actionlint's own: its documented install is
`curl | bash` off a moving branch, which does not belong in a repo that pins every
action by SHA.

The container runtime detection and the SELinux `:z` mount option are hoisted out
of the shellcheck branch so both checks share one answer rather than deciding it
twice and drifting.

Verified that it bites rather than assumed: status_check.yml was given a
`needs: [a-job-that-does-not-exist]`, and the real pre-commit hook blocked with
actionlint's own message -- `job "static-analysis" needs job
"a-job-that-does-not-exist" which does not exist in this workflow [job-needs]`.
Workflow restored; nothing but the hook is in this diff.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 16:19:49 -05:00
JMR-devandClaude Opus 5 dd01f9f27c Run shellcheck in the local gate, at CI's exact pin
The gate checked ktlint, detekt and Android lint but not shellcheck, so a new or
edited .sh file was precisely the case where the hook passed and CI's Static
analysis leg still went red. The first file it could not check was itself, and it
was caught by hand twice before it was caught here.

THE DIGEST IS READ OUT OF status_check.yml RATHER THAN COPIED. shellcheck 0.9.0
and 0.11.0 disagree about how to report a trap handler -- SC2317 on seven body
lines against SC2329 once on the declaration, same script, same directive, one
red and one green. That is why CI pins by digest, and it is also why a second
copy of the digest in this file would be worse than none: when it drifts, the
symptom is the gate passing and CI failing, which is the exact failure this
section prevents.

Runs over `git ls-files '*.sh'` -- all tracked files, not the diff -- because
that is what CI does, and the job here is to predict that leg rather than audit
the change. podman is preferred over docker for the mount's SELinux relabel;
neither present, or the digest unreadable, reports the check as NOT COVERED
rather than skipping it quietly.

Verified that it bites rather than assumed: a probe script whose only fault was
an unquoted `ls $foo` was staged, and the real pre-commit hook blocked on SC2086
before it reached the JVM gate. Probe removed; all tracked .sh are clean under
the pinned digest.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 16:16:23 -05:00
Jason Ross dd76229e90 Merge pull request #256 from JMR-dev/test/publish-delete-arm-real-provider
Delete the document a failed save could not write (#250)
2026-09-06 16:11:13 -05:00
JMR-devandClaude Opus 5 8105291f6a Make the gate name the levels it ran instead of claiming all of them
The closing line was `green at every supported API level`, printed on both
paths -- including the one that had just said `NOT COVERED LOCALLY: API 37` two
lines above. A false claim, printed by the tool whose entire purpose is to stop
false claims reaching CI, on its first run.

It now names them: `green on API 33, 34, 35, 36` when the Pixel is absent, and
`green on API 33, 34, 35, 36, 37` when it is attached and passed.

Nothing else changes. The app/src subtree is untouched, so this exercises the
cache scoping from the previous commit: the sweep is skipped as already green and
only the JVM gate runs -- which is the whole reason that key was moved off the
repo tree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 16:02:47 -05:00
JMR-devandClaude Opus 5 68bd24e54a Gate commits and pushes on a local sweep at every supported API level
New rule, and a hook rather than a habit. Source work needs the unit tests and
the instrumented tests green at every supported API level before it is committed
or pushed; test work needs the whole suite green at every level.

tools/git-hooks/local-gate.sh is wired in as pre-commit and pre-push (symlinks,
so shellcheck sees one file), enabled with
`git config core.hooksPath tools/git-hooks`.

WHAT "EVERY LEVEL" CAN MEAN HERE, measured rather than assumed. 33-36 run the
whole suite on emulators. API 37 CANNOT be run on an emulator on this host at
all -- not "is red", cannot run: the image logs `3 new surfaceflinger aborts in
45 s (want 0)` and the APK install then fails with `Can't find service: package`,
because the framework is gone before Gradle installs anything. Starting 0 tests.
So 37 runs on the attached Pixel 10 Pro XL when it is there, and the hook says
plainly that the level is uncovered when it is not, rather than claiming five
levels having run four.

The first cut passed a notAnnotation filter through E2E_EXTRA_GRADLE_ARGS, which
run-e2e.sh:587 overwrites with --rerun -- so that argument was discarded and
would have been discarded silently.

The sweep is cached under the app/src SUBTREE hash, not the whole repo tree. The
first cut used the whole tree and that was wrong in a way that would teach people
to resent this hook: editing a comment in CLAUDE.md discarded a sweep of
byte-identical application code and re-ran forty minutes of emulators to prove
nothing. Any change under app/src still invalidates it; the JVM gate always runs.

There is deliberately no skip variable -- that would be --no-verify wearing a
different hat.

Why it is worth the time: #256 spent several gating legs learning one leg at a
time what a sweep answers in one pass, and the failing leg MOVED between runs
(API 35 red then green, API 34 green then red). One leg at a time reads as
someone else's flake; as a sweep it is one signal.

Also here, and the reason the rule arrived now: awaitNode treated "the app has no
composition right now" as a failure rather than as not-yet. fetchSemanticsNodes
throws IllegalStateException when nothing is attached and waitUntil propagates it
on the first poll instead of waiting out the deadline. This class spends much of
its time behind the picker, the save dialog and the permission dialog, so there
is always a window where the app is coming back with no composition -- and on run
34057706195's API 34 leg both SAF tests died in it. Now it is not-yet, with the
last composition error carried into the timeout message so a genuinely dead app
stays diagnosable.

Verified: this commit's own hook swept API 33, 34, 35 and 36 at 71/71 failed=0,
API 34 included.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 15:52:21 -05:00
JMR-devandClaude Opus 5 19e35394e1 Bound the conversion against API 35's encode, not API 34's
The API 35 leg of #256 went red on aSaveWritesToTheDocumentTheSystemPickerCreated
with a 120 s ComposeTimeoutException on action.saveFile. It was not a cancelled
job and not the new teardown: run 34056545386's logcat has

  20:05:26.897 FFmpegEngine: ffmpeg ... -c:v libx265 -crf 24 -preset veryfast
  20:07:41.693 ConversionWorker: Routing worker_sample.mp4 ...

134.8 s between the encode starting and the next job in the suite, with no cancel
between them. The conversion was healthy and still running when the bound fired.

CONVERSION_TIMEOUT_MS was 120_000, and its KDoc justified that with "the whole
test takes 11.8 s on the API 34 CI leg" -- a real measurement generalised to an
API level it was never taken on. Adding a second picker test made this class
encode twice, so the second one runs on a more contended emulator and crossed a
line that was already marginal. Now 300_000, justified against the 134.8 s, with
a note not to re-tighten it from a fast leg's timing.

cancelAllWork was SUSPECTED of causing this and did not. A local API 35 run with
it passed, which is what sent me to the logcat. pruneWork is kept because it is
the narrower call -- only finished records need to go, and cancelling live work
is a wider blast radius than teardown in a shared process needs -- and its KDoc
now says it fixed nothing rather than claiming a cause it does not have.

That correction is the point: the first version of that KDoc asserted a cause
from one red CI leg and one green local run on a different machine. A test
carrying a confident wrong explanation is the failure mode this whole read has
been about.

Verified: API 35 at 71/71 failed=0 with the raised bound, and the full gate green.
Production is untouched -- git diff origin/main -- app/src/main is empty.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 15:20:51 -05:00
JMR-devandClaude Opus 5 cbbaf74285 Delete the document a failed save could not write (#250)
#226 proved D4's premise -- SAF hands back a document reporting exactly zero
bytes, so destinationIsKnownEmpty can answer true -- and then drove the success
path, where publish's catch is never entered. So deletePartialOutput had still
never run against a real DocumentsProvider; its only assertions were
OutputPublisherPublishTest's, against FakeSafProvider under Robolectric. That is
the same "asserted only against a fake built to match it" shape #226 was filed to
break, one layer down.

RecordingPublisher.failOpen makes openDestination return null, which publish
turns into error("Could not open destination for writing") AFTER its size probe
has run -- so the catch is reached with destinationWasEmpty true on a document
DocumentsUI created seconds earlier. Null rather than a throw because
openDestination's KDoc says a provider that is present and declines is the half
no fake can produce on demand, so that arm is also taken for the first time.

Mutation, measured: delete the deletePartialOutput call and this test fails with
"publish did not delete the document it could not write". Nothing anywhere went
red for that line before.

TWO DEAD ACCESSORS #226 LEFT, and the reason is the same one:

FixtureDocumentsProvider is declared by the test APK and runs in
org.libremediaconverter.test; instrumentation runs in the app's process. A static
in the provider is a different object from the one a test can see, so
deletedDocumentIds() would have read empty forever, and reset(File) deletes under
a filesDir that is not the provider's. Both are removed rather than worked
around. That is E7's process wall from a third side, after ACTION_OPEN_DOCUMENT
and ActivityScenario.

The oracle is the document instead, which crosses the boundary because the app
holds a URI grant for it. Still the path rather than the artefact: the size query
proves the document existed and was empty moments earlier, and one that no longer
answers a query is one something deleted.

CLEANUP IS IN TEARDOWN, and the mutation run is why. A failed save keeps its
staged file deliberately, so this test ends with a finished job for the next
launch to reattach to; its sibling then opened on Converted with no "Choose file"
to tap. The first fix tapped Start over at the end of the test body, which does
not run when the test fails -- so the mutation run turned one real failure into
two, the second looking like an unrelated flake. One cause must produce one red
test.

Baseline 6 -> 7, with the derived counts in CLAUDE.md, the marker KDoc and
status_check.yml moved in the same diff. 71 - 7 is 64, the same gating figure for
the third consecutive time, which is how that paragraph goes stale unnoticed.

Verified: three API 34 runs at 71/71 failed=0, the mutation red on the right
assertion, and the full gate plus pinned actionlint green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-06 14:58:40 -05:00
Jason Ross fac8e67db2 Merge pull request #255 from JMR-dev/docs/e8-instrumented-coverage
Record the first instrumented coverage measurement, and classify the 32 it found
2026-09-06 14:54:10 -05:00
33 changed files with 2000 additions and 204 deletions
+6 -6
View File
@@ -272,13 +272,13 @@ 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 six tests off this row. SafPickerRoundTripTest's
# notAnnotation below keeps seven tests off this row. 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 path on every single run, pass or fail, which is what
# had been failing unrelated PRs (#108). All THREE of that class's tests now
# carry the marker -- the save through the picker (#226) joined on 2026-09-06
# by inheritance rather than measurement, since it opens the same picker.
# had been failing unrelated PRs (#108). All FOUR of that class's tests now
# carry the marker -- the two saves through the picker (#226, #250) joined on
# 2026-09-06 by inheritance rather than measurement, since they open the same picker.
# docs/api-37-emulator-crash.md has the timings and the correction, and
# FailsOnEmulatorApi37.kt has why the third one cannot be measured here.
#
@@ -290,11 +290,11 @@ jobs:
# docs/api-37-emulator-crash.md measures 37.0 rev 6 and 37.1 rev 8 side
# by side, so pinning 37.0 is a decision, not a constraint.
#
# notAnnotation removes the six tests that cannot be RUN on this image; they
# notAnnotation removes the seven tests that cannot be RUN on this image; they
# run in the advisory job below, off the same marker so they cannot end up
# in both or neither. "Cannot be run" rather than "do not pass" is deliberate:
# four fail outright, one of those aborts the framework on its way down, and on
# the advisory leg the two picker tests behind it never report at all.
# the advisory leg the three picker tests behind it never report at all.
# docs/api-37-emulator-crash.md has the measurements.
- label: "37"
api-level: "37.0"
+72 -13
View File
@@ -76,20 +76,23 @@ 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. **Six** of the 70 instrumented
tests cannot be *run* on that image, for three measured reasons and one inherited: three Media3
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Seven** of the 72 instrumented
tests cannot be *run* on that image, for three measured reasons and two inherited: 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. The sixth, that class's
save through the picker (#226), carries the marker because it opens the same picker and a second
DocumentsUI dialog on top of it — **not** because it has ever been observed here. It cannot be:
the rotation test runs first and takes the framework down, so **all four** advisory runs at this
baseline report `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests
plus the rotation — runs 34041156680, 34041593697, 34042397320 and 34043502322. **Neither picker
test has ever reported on the advisory leg**, which is a correction to what the marker's own KDoc
says. All six carry
`@FailsOnEmulatorApi37` and run in a separate `continue-on-error` job; the gating leg runs the
other 64 — **the same 64 as before**, which is exactly how this paragraph went stale unnoticed.
two saves through the picker (#226 and #250), carry the marker because they open the same picker
and a second DocumentsUI dialog on top of it — **not** because either has ever been observed here. It cannot be:
the rotation test runs first and takes the framework down, so all five advisory runs at the
previous baseline reported `expected: 6, received: 4, failed: 4`, and the four were the three
Media3 tests plus the rotation — runs 34041156680, 34041593697, 34042397320, 34043502322 and
34045105857. **No picker test has ever reported on the advisory leg**, which is a correction to
what the marker's own KDoc used to say. All seven carry `@FailsOnEmulatorApi37` and run in a
separate `continue-on-error` job; the gating leg runs the other **65**. That figure had been 64
three times running — 69−5, 70−6 and 71−7 are all 64 — which is exactly how this paragraph went
stale unnoticed, because the one number a reader checks against a run had not moved while the
suite grew twice underneath it. #254 is the first change since to move it, by adding a test and
no marker.
**These two numbers move with the suite and are derived, not remembered.** `grep -cE
'^\s*@Test' ` over `app/src/androidTest` is the first; the second is that minus the marker
@@ -122,7 +125,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 six tests pass.
not read a green run as evidence those seven 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,
@@ -142,9 +145,20 @@ 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 six tests are the one thing CI cannot answer
the Pixel 10 Pro XL before each release.** Those seven tests are the one thing CI cannot answer
for.
**When a gating leg goes red on a diff that cannot explain it, read `docs/ci-failure-modes.md`
before anything else.** It is the census of all 129 gating failures in the repo's history against
1489 leg-attempts, with a per-mode disposition, and it is what closed #102. Three things from it
that are easy to get wrong and expensive: **count per leg-attempt, never per run** — a re-run to
green replaces the conclusion, so counting runs sees about 40% of the failures; **every mode has
its own denominator**, because the API 37 row filters seven tests out and some tests are younger
than the window; and **a re-run destroys the log** — `gh run view --job <id> --log` resolves by run
and serves the latest attempt, so capture evidence before retrying, or read the attempt through
`gh api /repos/.../actions/jobs/{job_id}/logs`. The artifacts do survive, one per attempt under the
same name; `gh run download` takes the newest, which is the wrong one.
On a device or emulator, build only the ABI it can execute:
```bash
@@ -398,6 +412,51 @@ install for code that can never run — and on API 37 the full APK does not fit
mode, committed by the wave that found it.** All of it is fixed; the standing item is **#250**,
because #226 proved D4's premise and never drove its delete arm.
- **Nothing is committed or pushed until the local gate is green, at every supported API level.**
Source work (`app/src/main`) needs the unit tests **and** the instrumented tests passing on every
level; test work (`app/src/test`, `app/src/androidTest`) needs the whole suite passing on every
level. `tools/git-hooks/local-gate.sh` enforces it as `pre-commit` and `pre-push`; wire it up once
with `git config core.hooksPath tools/git-hooks`.
33-36 run the whole suite on emulators. **API 37 cannot be run on an emulator on this host at
all** — not "is red", *cannot run*: measured 2026-09-06, the image logs `3 new surfaceflinger
aborts in 45 s (want 0)` and then the APK install itself fails with `Can't find service:
package`, because the framework is gone before Gradle installs anything. `Starting 0 tests`. So
the hook runs API 37 on the **attached Pixel 10 Pro XL** when it is there, and says plainly that
the level is uncovered when it is not — CI's gating leg being what answers for it then. It never
claims five levels having run four.
**It runs shellcheck and actionlint too, at CI's exact pins** — shellcheck over
`git ls-files '*.sh'`, actionlint over the workflows, the same digests and the same file sets
that leg uses. actionlint is not an afterthought to shellcheck but the other half of the same
hole: much of this repo's bash lives in workflow `run:` blocks, which `'*.sh'` does not match at
all. That gap was found the hard way: the gate checked ktlint,
detekt and Android lint, so a new `.sh` file was precisely the case where it passed and CI still
went red, and the first file it could not check was itself. **The digest is read out of
`status_check.yml` rather than copied** — two copies drift, and the symptom of that drift is the
gate passing while CI fails, which is the one thing this check exists to prevent.
The sweep is cached under the hash of the **`app/src` subtree**, not the whole repo tree. Keying
it on the whole tree was the first cut and it was wrong: editing a comment in `CLAUDE.md` threw
away a sweep of byte-identical application code and re-ran forty minutes of emulators to prove
nothing, which is how a gate teaches people to resent it. Any change under `app/src` still
invalidates it, and the JVM gate runs unconditionally. **There is deliberately no skip
variable**, and `--no-verify` needs the repo owner's say-so each time rather than being reached
for when the gate is inconvenient.
**What that keying cannot see is `bin/`.** The classifier matches `app/src/main/*` and
`app/src/{test,androidTest}/*` and nothing else, so a commit that replaces only the committed
FFmpeg AAR — a *different native binary* under every instrumented test — invalidates no cache and
sweeps nothing, while the JVM gate that does run cannot execute FFmpeg at all. #254 is where that
was noticed, and it did not hit it: the AAR and the `app/src` change that needs it are one commit,
so the sweep ran. An AAR rebuilt on its own would not be, and should be committed alongside
something under `app/src` or swept by hand.
Why it is worth tens of minutes a commit: the alternative was measured on 2026-09-06, when one PR
spent several gating legs learning one leg at a time what a sweep answers in one pass — and the
failing leg **moved** between runs (API 35 red then green, API 34 green then red). One leg at a
time that reads as someone else's flake; as a sweep it is one signal.
- **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.
+2
View File
@@ -48,6 +48,8 @@ Everything Media3 structurally cannot do:
- Containers outside MP4/WebM/Ogg/WAV/AAC — MKV, MOV, AVI, FLV, MPEG-TS, WMV/ASF
- **MP3 output** — Android has no MP3 encoder at any version; this is a platform gap
- **Ogg Vorbis output** — the same gap: Android has no Vorbis encoder either. Encoded with
`libvorbis`, which the bundled build carries since #254
- GIF and image sequences
- Input codecs with no platform decoder on the device
- CRF and 2-pass rate control, for the quality tier
@@ -10,7 +10,7 @@ package org.libremediaconverter
* drift, and the drift is silent in both directions (a test that runs nowhere reads as green).
*
* **"Cannot be run" covers three things now, and it covered only the first until 2026-09-05.**
* Four of the six carriers simply fail: three Media3 tests die in the image's own
* Four of the seven 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
@@ -18,12 +18,13 @@ package org.libremediaconverter
* point at (#108). The wording was widened rather than the test excused; that test's own KDoc has
* the four-run measurement.
*
* **The sixth is the new third thing: it is marked by inheritance, not by measurement.**
* `SafPickerRoundTripTest.aSaveWritesToTheDocumentTheSystemPickerCreated` (#226) opens the same
* picker and then a second DocumentsUI dialog on top of it, so it sits on the same task-snapshot
* path its sibling was marked for. It has never been observed at API 37 either way — see the
* measurement under [FAILS_ON_EMULATOR_API37_BASELINE], which is why it cannot be. Marking it was
* the conservative choice, and **the trigger for revisiting it is the rotation test, not itself**:
* **The sixth and seventh are the new third thing: they are marked by inheritance, not by
* measurement.** `SafPickerRoundTripTest.aSaveWritesToTheDocumentTheSystemPickerCreated` (#226)
* and `.aFailedSaveDeletesTheDocumentItCouldNotWrite` (#250) each open the same picker and then a
* second DocumentsUI dialog on top of it, so they sit on the same task-snapshot path their sibling
* was marked for. Neither has ever been observed at API 37 either way — see the measurement under
* [FAILS_ON_EMULATOR_API37_BASELINE], which is why they cannot be. Marking them was the
* conservative choice, and **the trigger for revisiting it is the rotation test, not themselves**:
* while that one truncates the advisory run, nothing downstream of it can report.
*
* It says only what has been measured: **on the emulator, at API 37.** The same tests pass on a
@@ -58,8 +59,8 @@ annotation class FailsOnEmulatorApi37
* cannot be run on this image, so the count is meant to be simultaneously how many the advisory
* leg runs and how many fail. 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.
* **Since 2026-09-06 the second half no longer holds in practice** — the run truncates before two
* of the six start, which the last paragraph below measures. `expected` still holds, and it is the
* **Since 2026-09-06 the second half no longer holds in practice** — the run truncates before
* three of the seven start, which the last paragraph below measures. `expected` still holds, and it is the
* field that catches a marker added without changing this number.
*
* **The picker tests are the ones to read that sentence carefully for, and the reason changed
@@ -79,10 +80,11 @@ annotation class FailsOnEmulatorApi37
* framework having died, which is this job's normal.
*
* **That is no longer what happens, and the difference is that neither picker test reports at
* all.** With six carriers the rotation test truncates the run before them: **all four** advisory
* runs at this baseline — 34041156680, 34041593697, 34042397320 and 34043502322 — report
* `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests plus the
* rotation. So the advisory leg currently answers for
* all.** The rotation test truncates the run before them: **all five** advisory runs at the
* previous baseline of six — 34041156680, 34041593697, 34042397320, 34043502322 and 34045105857 —
* report `expected: 6, received: 4, failed: 4`, and the four are the three Media3 tests plus the
* rotation. #250 adds a third picker test behind the same wall, so expect `expected: 7,
* received: 4`. So the advisory leg currently answers for
* four of its six, and the comparison below is unaffected only because `failed` is not compared
* on a truncated run. Read it as **unmeasured**, not as passing or failing.
*
@@ -96,4 +98,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 = 6
const val FAILS_ON_EMULATOR_API37_BASELINE = 7
@@ -86,6 +86,25 @@ class FFmpegEngineTest {
}
}
/**
* Channels per track, or 0 for a track that does not declare any.
*
* Read out of the container rather than assumed from the request, because the thing worth
* catching is an encoder that quietly changed the channel count on the way through — which is
* exactly what a stereo-only encoder does to this class's mono fixture.
*/
private fun channelCounts(file: File): List<Int> {
val extractor = MediaExtractor()
return try {
extractor.setDataSource(file.absolutePath)
(0 until extractor.trackCount).map {
extractor.getTrackFormat(it).getInteger(MediaFormat.KEY_CHANNEL_COUNT, 0)
}
} finally {
extractor.release()
}
}
// --- the formats that justify bundling FFmpeg at all -------------------
@Test
@@ -149,6 +168,60 @@ class FFmpegEngineTest {
assertEquals("OggS", magic)
}
/**
* The first execution, ever, of the Vorbis encode arm — and the reason it needed one.
*
* `FFmpegCommandBuilder` carried `-c:a libvorbis` from the day it was written and nothing
* could ask for it: no preset produced `AudioCodec.VORBIS` and `ContainerCapabilities` left it
* out of the encodable set, so the arm was unreachable from both ends (#254). It was also
* **wrong**: `--enable-libvorbis` was in neither `bin/README.md`'s configure line nor
* `tools/ffmpeg/build-ffmpeg.sh`, and `libvorbis` was not among the encoder names in the
* shipped `libavcodec.so`. The first user to pick Ogg Vorbis would have got "Unknown encoder
* 'libvorbis'". #254 rebuilt the AAR with `--enable-libvorbis`; **this test is the only thing
* in the repo that can tell whether that rebuild actually included it**, because a wrong
* ffmpeg-kit `--enable-*` name is ignored silently and the JVM cannot tell a real encoder name
* from a fictional one.
*
* ## Why the container magic is not enough here
*
* `encodesOpus` above stops at `OggS`, and for that test it is sufficient. Here it would be
* **vacuous**: Vorbis and Opus are both Ogg streams, so this ticket's acceptance mutation —
* pointing the arm at `libopus` — produces a file with byte-identical first four bytes.
* Measured, not assumed: `-c:a libopus -b:a 128k -f ogg` on this class's own fixture writes
* `OggS` too. So the assertion has to reach the track, and `MediaExtractor` reporting
* `audio/vorbis` against `audio/opus` is what separates them.
*
* Asserted as the whole track list rather than as "contains Vorbis", which also pins that the
* `-vn` from the audio-only path really dropped the video: a stray video track would fail here
* rather than pass an `any { ... }` check.
*
* ## The channel count is the second claim, and it is not decoration
*
* `sample_h264.mp4` is **mono** — one AAC channel — and that is what makes this assertion
* bite. FFmpeg's in-tree `vorbis` encoder is stereo-only, so building on it forces `-ac 2` and
* silently upmixes every mono source, a compromise this app makes in no other arm. That
* compromise is the reason #254 rebuilt the binary rather than shipping the in-tree encoder,
* so re-adding `-ac 2` has to redden something: it reddens this.
*/
@Test
fun encodesOggVorbisThroughAnEncoderTheBundledBinaryActuallyHas() {
val out = convert(OutputFormat.OGG_VORBIS)
assertTrue("no Ogg produced", out.exists() && out.length() > 0)
val magic = out.inputStream().use { String(it.readNBytes(4), Charsets.US_ASCII) }
assertEquals("OggS", magic)
assertEquals(
"expected a lone Vorbis track -- an Opus one would carry the same OggS magic",
listOf(MediaFormat.MIMETYPE_AUDIO_VORBIS),
trackMimes(out),
)
assertEquals(
"the fixture is mono and libvorbis takes any channel count, so nothing may upmix it",
listOf(1),
channelCounts(out),
)
}
/**
* The percentage itself, which every other test in this class computes and none of them reads.
*
@@ -14,8 +14,6 @@ import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.util.ArrayList;
import java.util.List;
/**
* One file, offered to the system file picker, so that picking one can be tested at all.
@@ -116,7 +114,6 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
public static final String DESTINATION_PREFIX = "dest/";
/** Document ids {@link #deleteDocument} was called with, newest last. Cleared by {@link #reset}. */
private static final List<String> DELETED = new ArrayList<>();
/** Already in this source set, and already a real H.264 MP4 the engines can open. */
private static final String FIXTURE_ASSET = "sample_h264.mp4";
@@ -239,34 +236,11 @@ public final class FixtureDocumentsProvider extends DocumentsProvider {
if (documentId == null || !documentId.startsWith(DESTINATION_PREFIX)) {
throw new FileNotFoundException("refusing to delete: " + documentId);
}
synchronized (DELETED) {
DELETED.add(documentId);
}
destinationFile(documentId).delete();
}
/**
* Document ids {@link #deleteDocument} was called with, newest last.
*
* <p><b>Nothing reads this yet, and that is recorded rather than hidden (#250).</b> It was
* added with #226 to assert {@code OutputPublisher.deletePartialOutput} — D4's cleanup — against
* a real {@code DocumentsProvider}. #226 only reached the <i>success</i> path, so the
* {@code catch} that calls it is still asserted only against {@code FakeSafProvider} under
* Robolectric. It is kept because the forcing condition is one {@code openDestination} override
* away and #250 says exactly what to add; if that ticket is closed any other way, delete this
* and {@link #DELETED} with it rather than leaving an accessor implying coverage.
*/
public static List<String> deletedDocumentIds() {
synchronized (DELETED) {
return new ArrayList<>(DELETED);
}
}
/** Forgets recorded deletes and removes created destinations. The process outlives one class. */
/** Removes created destinations. The process outlives one class. */
public static void reset(File filesDir) {
synchronized (DELETED) {
DELETED.clear();
}
File dir = new File(filesDir, "destinations");
File[] children = dir.listFiles();
if (children != null) {
@@ -1,7 +1,9 @@
package org.libremediaconverter.saf
import android.Manifest
import android.app.UiAutomation
import android.content.Context
import android.content.pm.PackageManager
import android.net.Uri
import android.provider.DocumentsContract
import android.provider.OpenableColumns
@@ -25,12 +27,15 @@ import androidx.test.uiautomator.Configurator
import androidx.test.uiautomator.StaleObjectException
import androidx.test.uiautomator.UiDevice
import androidx.test.uiautomator.Until
import androidx.work.WorkManager
import org.junit.After
import org.junit.Assert.assertArrayEquals
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotEquals
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
@@ -40,6 +45,7 @@ import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.OutputPublisher
import org.libremediaconverter.ui.TestTags
import java.io.File
import java.io.OutputStream
import java.util.concurrent.atomic.AtomicInteger
import java.util.regex.Pattern
@@ -280,17 +286,41 @@ private class RecordingPublisher(private val app: Context) : OutputPublisher(app
super.publish(staged, destination)
}
/**
* Refuses the write when [failOpen] is set, which is the forcing condition for #250.
*
* Returning null rather than throwing is deliberate: it is the arm `publish`'s
* `?: error("Could not open destination for writing")` exists for, and `openDestination`'s
* own KDoc says a provider that is present and declines is the half no fake can produce on
* demand. The size probe in `publish` has already run by the time this is reached, so
* `destinationWasEmpty` is true and `deletePartialOutput` is reached with the document
* genuinely empty — which is the whole point.
*/
override fun openDestination(destination: Uri): OutputStream? =
if (failOpen) null else super.openDestination(destination)
companion object {
var savedBytes: ByteArray = ByteArray(0)
var seenDestination: Uri? = null
var seenIsDocumentUri: Boolean? = null
var seenSizeBefore: Long? = null
/**
* Makes the next `publish` refuse to open its destination.
*
* A flag rather than a second publisher because `ConversionDependencies.publisher` is one
* seam and there is no orchestrator: every test in this process shares the instance the
* `init` block installed. [reset] clears it in teardown, so a test that sets it cannot
* leak a refusing publisher into the next class.
*/
var failOpen: Boolean = false
fun reset() {
savedBytes = ByteArray(0)
seenDestination = null
seenIsDocumentUri = null
seenSizeBefore = null
failOpen = false
}
}
}
@@ -334,6 +364,71 @@ class SafPickerRoundTripTest {
/** Counts [MainActivity] creations from the moment [watchForRecreation] is called. */
private val recreations = AtomicInteger()
/**
* Holds `POST_NOTIFICATIONS`, so tapping Convert cannot open a window this test has to fight.
*
* ## What this replaces, and why the replacement is not a smaller wait
*
* Until #268 the tap was followed by `dismissThePermissionDialog`, which waited for
* `com.google.android.permissioncontroller` to appear and pressed back on it. That is a
* *foreign, focused window* in the middle of the one step this class most needs to be
* deterministic, and it is what mechanism B of #268 was: on the API 35 leg of run 34146936252
* the tap landed — `START u0 {act=android.content.pm.action.REQUEST_PERMISSIONS ...
* GrantPermissionsActivity}` at 17:38:30.516 — back was pressed at 17:38:32.479, and no
* `ConversionWorker` was ever enqueued in the five minutes that followed. A back press goes to
* whichever window holds *input* focus, and `Until.hasObject` answers about the accessibility
* tree, which can carry the dialog's nodes before it has the focus; a back that arrives one
* window early lands on `MainActivity` and finishes it, which is a screen no `waitUntil` can
* wait for the return of.
*
* ## Why holding the permission removes the window rather than making it less likely
*
* `ConverterScreen` wires Convert to `requestNotifications.launch(POST_NOTIFICATIONS)`, and
* `ActivityResultContracts.RequestPermission.getSynchronousResult` returns
* `SynchronousResult(true)` — *without starting anything* — when
* `checkSelfPermission` already answers `PERMISSION_GRANTED`. So with the permission held there
* is no `GrantPermissionsActivity`, no foreign window, no back press, and nothing this test
* injects can finish the Activity. That is the whole chain, and [holdTheNotificationPermission]
* asserts its one premise rather than assuming it.
*
* ## The KDoc this contradicts, and the measurement that settles it
*
* `convertToTheDefaultFormat` used to say granting "was tried first and did not take —
* `GrantPermissionsActivity` appeared anyway". Re-measured on 2026-09-07, API 34 on this host,
* six consecutive runs of this class: logcat carries **zero**
* `act=android.content.pm.action.REQUEST_PERMISSIONS` starts and zero `GrantPermissionsActivity`
* across all six, and exactly two `WM-SystemJobScheduler: Scheduling work ID` lines per run —
* one for each test that converts, so neither Convert tap was lost. Whatever the earlier
* attempt did, a `pm grant` issued before the tap does take. The assertion below is what keeps
* that from going quietly stale.
*
* ## Two consequences, both deliberate
*
* The grant is **not** undone in teardown: revoking a runtime permission restarts the app's
* process, which would take the rest of the instrumentation run with it. The suite runs without
* Orchestrator, so every class that converts *after* this one now does so with notifications
* permitted. That is benign — `ConversionNotifications` builds its channel at
* `IMPORTANCE_LOW`, so nothing heads-up over the screen — but it is a real change to the
* device state the rest of the run sees, and `NotificationCancelActionTest`'s KDoc is updated
* with it.
*
* And this class no longer takes the denial path. It never asserted anything about it — the
* permission is setup for a test whose subject is SAF — and nothing is lost by it: the
* callback `ConverterScreen` registers is `{ viewModel.convert() }`, which **ignores its
* boolean**, so "converts whichever way the answer goes" is the shape of the code rather than a
* branch a test has to choose. `StaleLauncherResultTest` is what pins that callback path.
*/
@Before
fun holdTheNotificationPermission() {
device.executeShellCommand("pm grant $appPackage ${Manifest.permission.POST_NOTIFICATIONS}")
assertEquals(
"POST_NOTIFICATIONS is not held, so tapping Convert would open a permission dialog " +
"and this class's determinism argument does not hold -- see the KDoc above",
PackageManager.PERMISSION_GRANTED,
context.checkSelfPermission(Manifest.permission.POST_NOTIFICATIONS),
)
}
/**
* Counts a rotation's recreation without asking the Activity anything.
*
@@ -367,6 +462,8 @@ class SafPickerRoundTripTest {
fun restoreOrientation() {
// The suite runs without Android Test Orchestrator, so a swapped seam outlives the class.
ConversionDependencies.reset()
RecordingPublisher.reset()
clearFinishedWork()
ActivityLifecycleMonitorRegistry.getInstance().removeLifecycleCallback(recreationWatcher)
if (!rotated) return
device.setOrientationNatural()
@@ -570,6 +667,116 @@ class SafPickerRoundTripTest {
assertTrue("staging should be empty after a successful save", staged.listFiles().isNullOrEmpty())
}
/**
* The other half of D4 (#250): a save that fails deletes the document it could not write.
*
* ## Why this is separate from the test above
*
* #226 proved the *premise* — SAF hands back a document reporting exactly zero bytes, so
* `destinationIsKnownEmpty` can answer true — and then drove the success path, where the
* `catch` is never entered. So `deletePartialOutput` had still never run against a real
* `DocumentsProvider`; its only assertions were `OutputPublisherPublishTest`'s, against
* `FakeSafProvider` under Robolectric. That is the same "asserted only against a fake built to
* match it" shape #226 was filed to break, one layer down.
*
* ## The forcing condition, and why it is a returned null
*
* [RecordingPublisher.failOpen] makes `openDestination` return null. `publish` turns that into
* `error("Could not open destination for writing")` **after** its size probe has already run,
* so the `catch` is reached with `destinationWasEmpty == true` on a document DocumentsUI
* created seconds earlier. Nothing is simulated: the URI, the grant, the provider and the
* delete are all real.
*
* Null rather than a throw because `openDestination`'s KDoc says a provider that is present
* and declines is the half no fake can produce on demand — so this is also the first time that
* arm has been taken against a live provider rather than a stub.
*
* ## The oracle, and why it is not a recorder inside the provider
*
* The obvious assertion — have the provider record what `deleteDocument` was called with, and
* read it back — **cannot work here, and finding that out is half of what this test cost.**
* `FixtureDocumentsProvider` is declared by the test APK and runs in
* `org.libremediaconverter.test`; instrumentation runs in the app's process. A `static` in the
* provider is therefore a different object from the one a test can see, and the accessor #226
* left behind read empty on every run. That is E7's process wall from a third side, after
* `ACTION_OPEN_DOCUMENT` and `ActivityScenario`.
*
* So the oracle is the document, which does cross the boundary because the app holds a URI
* grant for it. **This is still the path rather than the artefact**, because the two
* assertions are read together: the size query above proves the document *existed and was
* empty* moments earlier, and a `content://` document that no longer answers a query is one
* something deleted. Nothing else in the app deletes SAF documents.
*
* The staged file is asserted to **survive**, which is the deliberate other half of that
* `catch`: a failed save may leave the staged copy as the only copy of an hour of transcoding,
* so `ConversionViewModel` keeps it and puts "Try saving again" on screen.
*/
@Test
@FailsOnEmulatorApi37
fun aFailedSaveDeletesTheDocumentItCouldNotWrite() {
pickTheFixture()
convertToTheDefaultFormat()
RecordingPublisher.failOpen = true
saveThroughTheSystemPicker(settlesOn = TestTags.RETRY_SAVE)
val destination = RecordingPublisher.seenDestination
assertNotNull("publish was never reached, so the delete arm was not exercised", destination)
assertEquals(
"the document was not positively empty, so publish would refuse to delete it",
0L,
RecordingPublisher.seenSizeBefore,
)
assertFalse(
"publish did not delete the document it could not write: $destination",
documentStillExists(destination!!),
)
// The staged copy is kept on purpose -- see ConversionViewModel.save's onFailure.
val staged = File(context.cacheDir, "conversions")
assertTrue(
"a failed save must not delete the staged file; it may be the only copy",
staged.listFiles()?.isNotEmpty() == true,
)
}
/**
* Leaves nothing for the next test's launch to reattach to.
*
* **In teardown rather than at the end of a test, and that placement is the point.**
* `aFailedSaveDeletesTheDocumentItCouldNotWrite` proves that a failed save *keeps* its staged
* file — deliberately, since it may be the only copy — so it ends with a finished job and a
* live staged file, which is exactly what the app reattaches to on the next launch. Its
* sibling then opened on `Converted` with no "Choose file" to tap: measured, as a 30 s timeout
* on `converter.chooseFile` in a test that had nothing wrong with it.
*
* The first fix tapped "Start over" at the end of the test body. That works until the test
* fails, and then it does not run at all — measured too, on the mutation run that proved this
* suite bites: one real failure became two, and the second looked like an unrelated flake.
* **One cause must produce one red test**, so the cleanup belongs where it runs either way.
*
* **`pruneWork` and not `cancelAllWork`, on design grounds and not on a measurement.** Only
* finished work records need to go — that is all the next launch reattaches to — and
* `cancelAllWork` additionally cancels live work, which is a wider blast radius than teardown
* in a shared process needs. `pruneWork` cannot touch a job that has not run yet.
*
* `cancelAllWork` was **suspected** of causing an API 35 red here and did not cause it; see
* [CONVERSION_TIMEOUT_MS], which did. A local API 35 run with `cancelAllWork` passed, and the
* logcat showed the conversion encoding rather than cancelled. The narrower call is kept
* because it is the right one, not because it fixed anything.
*/
private fun clearFinishedWork() {
WorkManager.getInstance(context).pruneWork()
File(context.cacheDir, "conversions").listFiles()?.forEach { it.delete() }
}
/** Whether [destination] still answers a metadata query. A deleted document does not. */
private fun documentStillExists(destination: Uri): Boolean = runCatching {
context.contentResolver
.query(destination, arrayOf(OpenableColumns.SIZE), null, null, null)
?.use { it.moveToFirst() } ?: false
}.getOrDefault(false)
/**
* Runs the conversion, leaving the screen on `Converted`.
*
@@ -582,13 +789,10 @@ class SafPickerRoundTripTest {
* showed LMC R38 fixtures"*. The default `MP4_H265` produces `video/mp4` and the root is
* offered.
*
* **The notification dialog is dismissed rather than pre-granted, and that is the honest
* version.** Convert never calls `convert()` directly — it launches `RequestPermission` for
* `POST_NOTIFICATIONS` and converts from the callback **whichever way the answer goes**. So the
* dialog only has to be got out of the way; denying it is a real user's path and the conversion
* still runs. Granting it programmatically was tried first and did not take —
* `GrantPermissionsActivity` appeared anyway, the click that followed went to it rather than to
* the app, and the screen sat in `Ready` with nothing enqueued.
* **The notification permission is held rather than dismissed**, which is #268's mechanism B
* and is argued in [holdTheNotificationPermission]. The short version: `RequestPermission`
* starts no Activity at all when the permission is already granted, so the tap below is
* followed by no foreign window.
*
* **Both taps scroll first.** On `Ready` the screen carries a file card, five pickers and then
* the button, so Convert is below the fold on a phone. `performClick` on an off-screen node
@@ -596,38 +800,94 @@ class SafPickerRoundTripTest {
* either way — the first version of this sat waiting for a `Converted` that could never come.
*/
private fun convertToTheDefaultFormat() {
awaitTheProbeHavingLanded()
composeRule.onNodeWithTag(TestTags.Converter.CONVERT)
.performScrollTo()
.assertIsEnabled()
.performClick()
dismissThePermissionDialog()
requireTheTapToHaveStartedTheJob()
awaitNode(TestTags.SAVE_FILE, CONVERSION_TIMEOUT_MS)
}
/**
* Gets the `POST_NOTIFICATIONS` dialog out of the way, if this device shows one.
* Blocks until the pick's probe has been rendered, so no relayout can straddle the next tap.
*
* Backing out of it is a denial, and a denial is fine here: the conversion starts either way,
* and what that costs the user is a progress notification confined to the Task Manager. Waiting
* only briefly, because on a device where the permission is already held no dialog appears at
* all and the conversion is already under way.
* **This is #268's mechanism A, and the argument is that it becomes impossible rather than
* unlikely.** `ConversionViewModel.onInputPicked` writes `_state` exactly twice: once with the
* name and size as soon as the metadata query returns, and once more with the probe filled in.
* The second write is what grows the file card, which moves everything below it — including the
* Convert button. Compose's injection computes the target's centre from the semantics node and
* dispatches the touch afterwards; a relayout in that gap hit-tests the stationary coordinate
* against the *new* layout, so the down and the up land on whatever moved into the button's old
* place. Nothing throws. Measured on the two failing gating legs as the gap between the pick's
* FFprobe closing and the tap: 319 ms and 421 ms passed, 46 ms, 98 ms and 124 ms did not.
*
* A detail row can only be composed from that second write, because `FileCard` renders the rows
* exclusively under `input.probe != null`. So once one exists, both of `onInputPicked`'s writes
* have landed and been laid out, and every `_state` write still in flight is either landed or
* superseded.
*
* `reattach` has **three** outcomes here, not two. It returns on its `_state.value !is Idle`
* guard; or it finds nothing; or — because `pruneWork()` is async and can leave a finished job
* unpruned — it passes that guard and starts an `observe()`. This paragraph used to name only
* the first two, which was wrong rather than merely incomplete: the third is a live coroutine
* with writes ahead of it.
*
* It is still harmless, and by a different mechanism than the guard. `reattach` reads
* `ownership.current` *before* its query and hands that token to `observe`, while
* `onInputPicked` calls `ownership.claim()` synchronously on the pick — so by the time a
* detail row exists the observation is superseded, and every emission returns at
* `stillHeldBy` before it writes. Outside that path `observe` is not started until
* `convert()` runs. **The card cannot change height again before the tap**, which is a
* different claim from waiting longer.
*
* The `Container` row specifically, rather than a new "probing finished" tag in `main`, because
* this fixture is an MP4 video and that row is already what
* [pickingAFileThroughTheSystemPickerFillsInTheFileCard] waits on and asserts. It is a
* *presence* wait, which cannot be satisfied by a composition that is momentarily absent — an
* absence wait can, and that would tap into nothing.
*
* **Not in [pickTheFixture].** The rotation test does not tap a Compose affordance in this
* window at all, and the picker test already makes this exact wait its own assertion. Putting
* it here keeps a broken read grant reddening one test with the message that explains it.
*/
private fun dismissThePermissionDialog() {
if (device.wait(Until.hasObject(By.pkg(PERMISSION_UI_PACKAGE)), PERMISSION_DIALOG_MS) != true) {
return
private fun awaitTheProbeHavingLanded() {
awaitNode(TestTags.Converter.detailRow(CONTAINER_LABEL))
}
/**
* Fails fast if the Convert tap started nothing, instead of waiting out the conversion budget.
*
* **A diagnostic, not the synchronisation** — [awaitTheProbeHavingLanded] is what makes the tap
* land, and this cannot rescue a tap that did not. It exists because of what a lost tap used to
* look like: `ComposeTimeoutException`, 300000 ms for `action.saveFile`, five minutes after a
* screen that had never left `Ready`, which names the save affordance and says nothing about
* the tap two steps earlier. Every #268 failure was read from logcat rather than from the
* message, and this is the message it should have had.
*
* The condition is monotonic and needs no budget of its own: `convert()` sets `Converting`
* synchronously, and `Ready` is the only state that renders a Convert button, so once the tag
* is gone it stays gone. [APP_TIMEOUT_MS] rather than a new constant, because "the app should
* have reacted by now" is exactly what that number already means here.
*/
private fun requireTheTapToHaveStartedTheJob() {
val tag = TestTags.Converter.CONVERT
try {
composeRule.waitUntil("the Convert tap left the Ready screen", APP_TIMEOUT_MS) {
// A composition that is momentarily absent throws, and must read as "not yet"
// rather than as "the button is gone" -- see awaitNode.
runCatching { composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isEmpty() }
.getOrDefault(false)
}
} catch (timeout: ComposeTimeoutException) {
throw AssertionError(
"the Convert tap did not start a conversion: $tag is still on screen " +
"${APP_TIMEOUT_MS}ms after it was clicked, so the screen never left Ready",
timeout,
)
}
device.pressBack()
device.wait(Until.gone(By.pkg(PERMISSION_UI_PACKAGE)), PERMISSION_DIALOG_MS)
// And wait for the app to be in front again before anything asks Compose about it.
// Querying while another window still owns the screen raises "No compose hierarchies found
// in the app", which is what this test did on an API 35 leg: the back press had landed but
// the dialog had not finished going away.
//
// Asked of UiAutomator rather than through awaitAppFocus, which is the opposite of what the
// class KDoc argues for elsewhere and is right here: awaitAppFocus goes through
// composeRule.waitUntil, so it would raise the very error it is being used to avoid.
device.wait(Until.hasObject(By.pkg(context.packageName)), FOCUS_TIMEOUT_MS)
}
/**
@@ -636,7 +896,7 @@ class SafPickerRoundTripTest {
* Retried whole, for the reason [pickTheFixture] documents: a dialog that came up unreadable
* cannot be recovered from inside, and a fresh one is the only answer.
*/
private fun saveThroughTheSystemPicker() {
private fun saveThroughTheSystemPicker(settlesOn: String = TestTags.Converter.CONVERT_ANOTHER) {
var missing: BySelector? = null
repeat(PICK_ATTEMPTS) { attempt ->
requireAReadableScreen()
@@ -645,7 +905,9 @@ class SafPickerRoundTripTest {
if (attempt == 0) PICKER_TIMEOUT_MS else REOPENED_TIMEOUT_MS,
)
if (missing == null) {
awaitNode(TestTags.Converter.CONVERT_ANOTHER, SAVE_TIMEOUT_MS)
// The node that says the save has *finished*, either way. Waiting on the success
// one when the save is meant to fail would time out on a test that is working.
awaitNode(settlesOn, SAVE_TIMEOUT_MS)
return
}
dismissThePicker()
@@ -716,6 +978,8 @@ class SafPickerRoundTripTest {
private fun requireAReadableScreen() {
val app = By.pkg(appPackage)
if (device.wait(Until.hasObject(app), READABLE_TIMEOUT_MS) == true) return
// The return value is deliberately dropped here: the wait on the next line IS the re-probe
// that dismissThePicker had to be given, so there is nothing for it to gate.
dismissASystemErrorDialog()
if (device.wait(Until.hasObject(app), READABLE_TIMEOUT_MS) == true) return
unlockTheDevice()
@@ -756,14 +1020,21 @@ class SafPickerRoundTripTest {
* would click whatever system window happened to be there. `aerr_wait` first: it dismisses the
* dialog and leaves the offending app alone, which is the polite answer when the app is not
* ours. Back is not tried — `BaseErrorDialog` swallows key events.
*
* **Returns whether it clicked anything, and the caller has to care.** Dismissing the dialog
* changes the window focus, so every reading taken before this ran is stale afterwards —
* which is the whole of #102's `MainActivity`-destroyed mode. [requireAReadableScreen] already
* re-probes after calling this; [dismissThePicker] could not, because it had no way to know
* whether there had been anything to dismiss.
*/
private fun dismissASystemErrorDialog() {
private fun dismissASystemErrorDialog(): Boolean {
for (id in ERROR_DIALOG_BUTTONS) {
val button = device.findObject(By.res(id)) ?: continue
button.click()
device.waitForIdle()
return
return true
}
return false
}
/**
@@ -895,6 +1166,51 @@ class SafPickerRoundTripTest {
* enough from Recent and two are needed from inside the root, but a third from Recent would
* finish `MainActivity` and take the rest of the test with it.
*
* **That hazard was reached, and the guard above is why it could be** (#102). A system
* app-error dialog is a fullscreen `system_server` window, so it takes the focus away from
* `MainActivity` too — [awaitAppFocus] cannot tell "the picker is still up" from "a dialog is
* on top of an app that is already in front". Measured on the API 35 gating leg of run
* `34161043035` attempt 1, which is #269's own head:
*
* ```
* 20:59:35.689 UiObject2: Clicking on (927, 2274) <- iteration 2's dismissal, on button1
* 20:59:36.033 MainActivity RESUMED <- so the picker is gone, by our hand
* 20:59:36.350 VRI[PickActivity]: visibilityChanged ... newVisibility=false
* 20:59:37.068 UiDevice: Pressing back button. <- iteration 2 presses anyway
* 20:59:41.094 UiDevice: Retrieving node ... [RES='android:id/aerr_wait']
* 20:59:41.169 Input channel object 'Application Not Responding:
* com.google.android.apps.nexuslauncher' was disposed
* 20:59:41.713 UiDevice: Pressing back button. <- iteration 3
* 20:59:41.754 TopTaskTracker: onTaskMovedToFront: ... NexusLauncherActivity
* 20:59:42.278 MainActivity DESTROYED
* ```
*
* Read the first two lines before the rest, because they are the part that is easy to get
* wrong: **the picker did not close on its own — this function closed it**, on iteration 2,
* when [dismissASystemErrorDialog] fell through to `android:id/button1` and clicked what was
* almost certainly DocumentsUI's own positive button (#271). From `20:59:36.033` onwards there
* was nothing left to back out of. Iteration 2 pressed back regardless, iteration 3 dismissed
* the launcher's ANR dialog — #93's occluder, still ambient on these runners, and the only
* remaining reason the focus read false — and pressed again, and that press finished
* `MainActivity`. Every later `onActivity` in the test then threw
* `NullPointerException: Cannot run onActivity since Activity has been destroyed already`.
*
* **With the re-read below, iteration 2 returns** — the app is focused within a second of the
* `button1` click — and iterations 2 and 3 never press at all.
*
* **So the reading is retaken after the dialog goes, and only then.** This removes a back
* press sent on a stale reading; it does not retry one, and it does not make the dismissal
* more tolerant. A picker that really is in front still leaves the app unfocused, so the press
* still happens and a genuinely stuck picker still fails here. On the ordinary path — no
* dialog — nothing is re-read and nothing is waited on, which is why the check is behind the
* `&&`. [requireAReadableScreen] has always re-probed after dismissing a dialog; this is the
* same rule in the one place that did not follow it.
*
* **It cannot be proved by re-running**, and that is worth saying rather than glossing: the
* launcher ANR is ambient and unreproducible on demand, so a green sweep is not evidence. What
* the fix rests on is the trace above: the launcher comes to the front 41 ms after a back press
* that this change does not send, and the Activity is destroyed 565 ms after that.
*
* **[forceStopThePicker] is the escalation after the presses, and it exists because a back
* press is not always deliverable.** See its own KDoc for the measurement.
*/
@@ -905,7 +1221,11 @@ class SafPickerRoundTripTest {
// so a back aimed at the picker lands on the dialog and nothing moves. Measured --
// API 34 of run 32813885120 exhausted all four presses with `android` in front, which
// is that dialog, while the launcher it belonged to went on ANRing behind everything.
dismissASystemErrorDialog()
//
// And re-read the focus if one was dismissed: the dialog is itself a reason the
// reading above can be false, so a press sent on it can land on an app that is
// already in front. See the KDoc -- that is how MainActivity got destroyed.
if (dismissASystemErrorDialog() && awaitAppFocus()) return
device.pressBack()
}
// The check after the last press, and not a spare one: `repeat` presses on its final
@@ -1057,9 +1377,36 @@ class SafPickerRoundTripTest {
}
}
/**
* Waits for [tag], treating "the app has no composition right now" as *not yet* rather than
* as a failure.
*
* `fetchSemanticsNodes` **throws** `IllegalStateException: No compose hierarchies found in the
* app` when nothing is attached at that instant, and `waitUntil` propagates it on the first
* poll instead of waiting out the deadline. This class spends much of its time with another
* app in front — the picker and the create-document dialog, and until #268 the permission
* dialog too — so there is always a window where the app is coming back and has no composition
* yet. Before this, that
* window was a hard failure: measured on the API 34 leg of run 34057196628, where **both** SAF
* tests died that way while the same commit passed API 33, 35, 36 and 37, and the previous
* commit passed API 34 and failed 35. A failing leg that moves between runs is #190's
* emulator flake, and this is the one place in the class that turned it into a red test.
*
* **The cost is honest and bounded**: an app that is genuinely gone now fails at the deadline
* rather than immediately, so the last composition error is carried into the message to keep
* that case diagnosable.
*/
private fun awaitNode(tag: String, timeoutMs: Long = APP_TIMEOUT_MS) {
composeRule.waitUntil("a node tagged $tag exists", timeoutMs) {
composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty()
var lastError: Throwable? = null
try {
composeRule.waitUntil("a node tagged $tag exists", timeoutMs) {
runCatching { composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().isNotEmpty() }
.onFailure { lastError = it }
.getOrDefault(false)
}
} catch (timeout: ComposeTimeoutException) {
val note = lastError?.let { "; last composition error: ${it.message}" } ?: ""
throw AssertionError("waited ${timeoutMs}ms for a node tagged $tag$note", timeout)
}
}
@@ -1073,20 +1420,23 @@ class SafPickerRoundTripTest {
const val PICKER_TIMEOUT_MS = 30_000L
const val APP_TIMEOUT_MS = 30_000L
/** The runtime-permission dialog's package, so it can be recognised and dismissed. */
const val PERMISSION_UI_PACKAGE = "com.google.android.permissioncontroller"
/** Short: either the dialog is up almost immediately, or the permission was already held. */
const val PERMISSION_DIALOG_MS = 5_000L
/**
* Only bounds a hang, and it is an order of magnitude clear of the real cost: the whole
* test — pick, convert, save — takes **11.8 s** on the API 34 CI leg (run 34043502322).
* Deliberately generous because the engine is not fixed: the default `MP4_H265` at `FAST`
* lands on FFmpeg on an emulator and on Media3 on real hardware, which is faster rather
* than slower — see [convertToTheDefaultFormat].
* Bounds a hang, and **the first number here was measured on one API level and wrong on
* another.** It read 120 s, on the strength of the whole test taking 11.8 s on the API 34
* CI leg (run 34043502322). API 35 is a different machine: on run 34056545386 the fixture's
* `libx265 -crf 24 -preset veryfast` encode started at `20:05:26.897` and the next job in
* the suite did not appear until `20:07:41.693` — **134.8 s**, so the encode was still
* running when the 120 s bound expired and the test failed with the conversion healthy.
*
* The logcat is what settles it: `ConversionWorker` logs the route and `FFmpegEngine` the
* command, and there is no cancel between them. A timeout that fires on a working
* conversion is worse than no bound, because it reads as a product failure.
*
* 300 s is chosen against that 134.8 s, not against API 34's 11.8 s. **Do not re-tighten
* it from a fast leg's timing** — the encode is software on every emulator here, and the
* spread between images is larger than any margin a single measurement would suggest.
*/
const val CONVERSION_TIMEOUT_MS = 120_000L
const val CONVERSION_TIMEOUT_MS = 300_000L
/** The copy is a few kilobytes, but it crosses a provider. */
const val SAVE_TIMEOUT_MS = 30_000L
@@ -37,10 +37,16 @@ import java.util.concurrent.TimeUnit
* ## 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.
* it finds. That was rejected because it would be asserting something about notification
* *visibility* rather than about cancellation — and because whether the shade holds the
* notification at all is not this class's to know.
*
* **It used to say `POST_NOTIFICATIONS` is denied throughout, and since #268 that is no longer
* true.** `SafPickerRoundTripTest` grants it in `@Before`, so that its Convert tap cannot open a
* permission dialog, and a runtime grant cannot be undone in teardown without restarting the app's
* process. The suite runs without Orchestrator, so whether this class sees the permission held
* depends on class order — which is exactly the reading this test does not do, and the reason it
* stays the right shape rather than a reason to change it.
*
* 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
@@ -230,6 +230,12 @@ class Media3Engine(private val context: Context) : HardwareTranscoder {
* unreachable code buys nothing — but it is an entry waiting on a routing change rather
* than a live one. `Media3EngineMimeTypesTest` routes all six encodable codecs and asserts
* which three arrive, so if that set moves, the disagreement fails rather than surprises.
*
* **Unreachable here is not the same as unreachable.** Since #254 a Vorbis encode is a
* thing a user can ask for — `OutputFormat.OGG_VORBIS` — and it is served by
* `FFmpegCommandBuilder`, which is the whole point of the router rule above sending it
* there. What stays dead is this arm specifically, because `MEDIA3_AUDIO` still excludes
* Vorbis: Android has no Vorbis encoder at any API level, exactly as with MP3.
*/
internal fun audioMimeTypeFor(codec: AudioCodec): String? = when (codec) {
AudioCodec.AAC -> MimeTypes.AUDIO_AAC
@@ -53,6 +53,44 @@ object FFmpegCommandBuilder {
/** Containers in the ISO base-media family, where HEVC needs the hvc1 brand. */
private val MP4_FAMILY = setOf(Container.MP4, Container.MOV)
/**
* Ogg Vorbis, through libvorbis.
*
* ## This named an encoder the binary did not have, for as long as it existed
*
* These are the exact flags the arm carried before #254, and the arm had never run: `VORBIS`
* was absent from `ContainerCapabilities.ENCODABLE_AUDIO` and no `OutputFormat` offered it.
* It could not have run either. `--enable-libvorbis` was not in the AAR's configure line, and
* `strings` on the shipped `libavcodec.so` named `libx264`, `libx265`, `libvpx`, `libmp3lame`,
* `libopus`, `libdav1d`, `libsvtav1` and `libjxl` — no `libvorbis`. The first user to pick Ogg
* Vorbis would have got "Unknown encoder 'libvorbis'". #254 rebuilt the AAR with
* `--enable-libvorbis` (`bin/README.md` carries the new configure line and checksum) and made
* the arm reachable. The flags did not have to change; the binary under them did.
*
* **Nothing on the JVM can tell a real encoder name from a fictional one**, which is exactly
* how that survived four coverage waves. `FFmpegCommandBuilderTest` can only pin that this is
* what the builder emits. That `libvorbis` is really in there is proved by `FFmpegEngineTest`'s
* `encodesOggVorbisThroughAnEncoderTheBundledBinaryActuallyHas`, on a device, and by nothing
* else in this repo.
*
* Two flags are deliberately *absent*, and both would be forced by FFmpeg's in-tree `vorbis`
* encoder — the one the binary already had, and the one a first pass at #254 used:
*
* - **no `-strict experimental`**. The in-tree encoder carries `AV_CODEC_CAP_EXPERIMENTAL`
* and libavcodec refuses it without the flag. libvorbis is not experimental.
* - **no `-ac 2`**. The in-tree encoder is stereo-only — *"Current FFmpeg Vorbis encoder only
* supports 2 channels."* — so it would silently upmix a mono source and downmix a surround
* one, a compromise this app makes nowhere else. libvorbis takes any channel count, so mono
* stays mono — the e2e test's fixture is mono and it asserts the output still is.
*
* `-q:a 5` is libvorbis's classic ~160 kbps setting, and the scale behind it is the third
* reason for the rebuild. Over one 3 s clip libvorbis spans 10931..64166 bytes across q0..q10
* where the in-tree encoder spans 7549..14645 — so libvorbis at this setting (16429 bytes)
* already writes more than the in-tree encoder can at q10, and the knob has somewhere to go
* if this app ever exposes it.
*/
private val VORBIS_ARGS = listOf("-c:a", "libvorbis", "-q:a", "5")
fun build(request: ConversionRequest, inputPath: String, outputPath: String): List<String> {
val plan = CopyPlanner.plan(request.spec, request.probe)
return buildList {
@@ -185,7 +223,7 @@ object FFmpegCommandBuilder {
AudioCodec.FLAC -> listOf("-c:a", "flac")
AudioCodec.PCM -> listOf("-c:a", "pcm_s16le")
AudioCodec.OPUS -> listOf("-c:a", "libopus", "-b:a", "128k")
AudioCodec.VORBIS -> listOf("-c:a", "libvorbis", "-q:a", "5")
AudioCodec.VORBIS -> VORBIS_ARGS
else -> listOf("-c:a", "aac", "-b:a", "192k")
}
}
@@ -7,11 +7,15 @@ package org.libremediaconverter.model
*
* "Can MP4 carry AV1?" and "can this app make AV1?" have different answers, and remux is exactly
* where the difference shows. MP4 carries AV1 and ALAC happily; neither engine here encodes them.
* Matroska carries Vorbis; nothing in [org.libremediaconverter.ffmpeg.FFmpegCommandBuilder] emits a
* Vorbis encoder. A single `isValid` boolean would answer one of those questions and give the wrong
* Matroska carries VP8; nothing in [org.libremediaconverter.ffmpeg.FFmpegCommandBuilder] emits a
* VP8 encoder. A single `isValid` boolean would answer one of those questions and give the wrong
* error for the other — telling a user "MP4 cannot hold AV1" when the truth is "your AV1 file can be
* copied into MP4, just not re-encoded to it".
*
* The example used to be Vorbis, and #254 is what stopped it being true — by rebuilding the
* bundled FFmpeg, because the Vorbis arm named `libvorbis` and the binary did not carry it. The
* gap is a video-only one now.
*
* So the matrix is indexed by mode: [CodecMode.COPY] asks only what the muxer accepts,
* [CodecMode.ENCODE] additionally asks what this app can encode.
*
@@ -81,10 +85,28 @@ object ContainerCapabilities {
*/
private val ENCODABLE_VIDEO = setOf(VideoCodec.H264, VideoCodec.H265, VideoCodec.VP9)
/** Vorbis is absent for the same reason: nothing here emits a Vorbis encoder. */
/**
* Audio codecs this app can encode. Every codec any container here carries, as of #254.
*
* The comment this replaces said "Vorbis is absent for the same reason: nothing here emits a
* Vorbis encoder", and it was false as written — `FFmpegCommandBuilder.audioArgs` has had a
* Vorbis arm since the builder existed. Its absence from this set was what made that arm
* unreachable, and nothing recorded the decision either way. It also hid a second fault: the
* arm named `libvorbis`, which was not compiled into the bundled binary, so the format the app
* declined to offer was one it could not actually have produced. #254 rebuilt the AAR with
* `--enable-libvorbis` and added the codec here in the same change.
*
* That makes this set equal to the union of [CARRIES_AUDIO], which `ContainerCapabilitiesTest`
* now asserts rather than leaving to be noticed. The consequence is that [validateAudio]'s
* "this app cannot encode X audio" arm has no reachable input. It stays: the video half of the
* same rule is live (VP8 and AV1), and this is where an ALAC or an AC-3 entry would land the
* day the matrix carries one. It is F4-shaped — a second line of defence that cannot currently
* be provoked — and the set-equality assertion is what turns that from a hope into a check.
*/
private val ENCODABLE_AUDIO = setOf(
AudioCodec.AAC,
AudioCodec.OPUS,
AudioCodec.VORBIS,
AudioCodec.MP3,
AudioCodec.FLAC,
AudioCodec.PCM,
@@ -12,8 +12,19 @@ package org.libremediaconverter.model
* and without it `.mka`, MP4 is `.mp4` or `.m4a`. That distinction is why they are functions rather
* than properties.
*
* The extension turned out to depend on a second thing, which is what [audioCodecExtensions] is
* for — see its parameter note.
*
* @param ffmpegFormat the `-f` value. Named explicitly rather than left to extension inference,
* which is unreliable for MPEG-TS and ASF.
* @param audioCodecExtensions per-codec overrides of [audioExtension]. Ogg is the only container
* that needs one, and it is the reason this parameter exists: one Ogg stream can hold Vorbis,
* Opus or FLAC, and RFC 7845 §9 asks for `.opus` on an Ogg that carries Opus alone while
* everything else in an Ogg is a plain `.ogg`. A single container-wide extension cannot say
* both — and it said `opus` for *every* Ogg until [OutputFormat.OGG_VORBIS] existed, which
* would have named a Vorbis file `.opus`. That is the same defect as the `FLAC` preset that
* once declared Matroska with a `.flac` extension, which is what moved these fields onto the
* container in the first place.
*/
enum class Container(
val label: String,
@@ -22,6 +33,7 @@ enum class Container(
private val audioExtension: String,
private val videoMime: String?,
private val audioMime: String,
private val audioCodecExtensions: Map<AudioCodec, String> = emptyMap(),
) {
MP4("MP4", "mp4", "mp4", "m4a", "video/mp4", "audio/mp4"),
MOV("MOV", "mov", "mov", "m4a", "video/quicktime", "audio/mp4"),
@@ -32,7 +44,7 @@ enum class Container(
FLV("FLV", "flv", "flv", "flv", "video/x-flv", "video/x-flv"),
ASF("WMV/ASF", "asf", "wmv", "wma", "video/x-ms-wmv", "audio/x-ms-wma"),
OGG("Ogg", "ogg", null, "opus", null, "audio/ogg"),
OGG("Ogg", "ogg", null, "ogg", null, "audio/ogg", mapOf(AudioCodec.OPUS to "opus")),
WAV("WAV", "wav", null, "wav", null, "audio/wav"),
AAC_ADTS("AAC", "adts", null, "aac", null, "audio/aac"),
MP3("MP3", "mp3", null, "mp3", null, "audio/mpeg"),
@@ -45,7 +57,17 @@ enum class Container(
/** Whether this container can hold a video track at all. */
val canHoldVideo: Boolean get() = videoExtension != null
fun extensionFor(hasVideo: Boolean): String = if (hasVideo) videoExtension ?: audioExtension else audioExtension
/**
* The filename extension for an output in this container.
*
* [audioCodec] takes no default on purpose. A default would let a caller get `.ogg` for an
* Opus output by saying nothing, which is exactly the silent-wrong-answer shape the audio
* codec argument was added to close.
*/
fun extensionFor(hasVideo: Boolean, audioCodec: AudioCodec): String = when {
hasVideo -> videoExtension ?: audioExtension
else -> audioCodecExtensions[audioCodec] ?: audioExtension
}
fun mimeTypeFor(hasVideo: Boolean): String = if (hasVideo) videoMime ?: audioMime else audioMime
}
@@ -102,7 +124,7 @@ data class OutputSpec(val container: Container, val videoCodec: VideoCodec, val
audioCodec.isCopyOrAbsent() &&
(videoCodec == VideoCodec.COPY || audioCodec == AudioCodec.COPY)
val extension: String get() = container.extensionFor(hasVideo)
val extension: String get() = container.extensionFor(hasVideo, audioCodec)
val mimeType: String get() = container.mimeTypeFor(hasVideo)
private fun VideoCodec.isCopyOrAbsent() = this == VideoCodec.COPY || this == VideoCodec.NONE
@@ -129,6 +151,19 @@ enum class OutputFormat(val label: String, val spec: OutputSpec) {
MP3("MP3", OutputSpec(Container.MP3, VideoCodec.NONE, AudioCodec.MP3)),
M4A_AAC("M4A (AAC)", OutputSpec(Container.MP4, VideoCodec.NONE, AudioCodec.AAC)),
OPUS("Opus", OutputSpec(Container.OGG, VideoCodec.NONE, AudioCodec.OPUS)),
/**
* The other codec Ogg carries, and the only preset added to make an existing arm reachable.
*
* `FFmpegCommandBuilder` has emitted a Vorbis encoder since the builder was written, and
* nothing could ask for it: no preset produced [AudioCodec.VORBIS] and `ContainerCapabilities`
* refused it on the Advanced picker, so the arm was dead in both directions (#254). It was also
* naming `libvorbis`, which the bundled FFmpeg did not carry until that same ticket rebuilt it,
* so making it reachable meant rebuilding the binary under it. Named for
* the container as well as the codec because [OPUS] shares that container and the two produce
* differently-named files — `.opus` against `.ogg`.
*/
OGG_VORBIS("Ogg Vorbis", OutputSpec(Container.OGG, VideoCodec.NONE, AudioCodec.VORBIS)),
FLAC("FLAC", OutputSpec(Container.FLAC, VideoCodec.NONE, AudioCodec.FLAC)),
WAV("WAV", OutputSpec(Container.WAV, VideoCodec.NONE, AudioCodec.PCM)),
@@ -8,6 +8,7 @@ import androidx.work.CoroutineWorker
import androidx.work.Data
import androidx.work.ForegroundInfo
import androidx.work.OneTimeWorkRequestBuilder
import androidx.work.OutOfQuotaPolicy
import androidx.work.WorkerParameters
import androidx.work.hasKeyWithValueOfType
import androidx.work.workDataOf
@@ -27,6 +28,10 @@ import org.libremediaconverter.model.OutputFormat
* Progress is not reported. FFmpeg's statistics callback gives a timestamp against a
* single input's duration, which is meaningless once several files are being
* concatenated; showing a fabricated percentage would be worse than showing none.
*
* Enqueued as **expedited** work for the same reasons, and with the same caveats, as
* [ConversionWorker] — its class KDoc carries both, and a join is user-initiated in exactly the
* way a conversion is.
*/
@UnstableApi
class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker(context, params) {
@@ -68,13 +73,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
// which is where a WorkManager restart after process death always begins -- used to
// throw straight past this catch, taking the retry, the error message and the delete
// with it. See ConversionWorker.doWork and FailureOutcome.
setForeground(
ForegroundInfo(
NOTIFICATION_ID,
notifications.build(id, "Joining ${uris.size} files", 0, indeterminate = true),
ConversionForegroundType.current(),
),
)
//
// Posted through getForegroundInfo() rather than built here a second time -- see that
// override, and its twin in ConversionWorker.
setForeground(getForegroundInfo())
val result = ConversionDependencies.concat(applicationContext).join(uris, staged, format)
Result.success(
@@ -129,11 +131,29 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
return publisher.hasSpaceFor(bytes)
}
override suspend fun getForegroundInfo(): ForegroundInfo = ForegroundInfo(
NOTIFICATION_ID,
notifications.build(id, "Joining files", 0, indeterminate = true),
ConversionForegroundType.current(),
)
/**
* The notification a starting join posts, and now the only definition of it.
*
* WorkManager's hook for expedited work, which **will not call this on any device this app
* supports** — see [ConversionWorker.getForegroundInfo] for the measurement and for why
* `setExpedited` alone would have left these lines exactly as cold as they were. What makes
* them live is [doWork] posting this instead of building its own copy.
*
* It counts the inputs itself rather than being handed the number, so that it is still answerable
* before [doWork] has parsed anything — which is the contract WorkManager's own caller wants.
* The count is read from the same key, so the two cannot disagree. The `?: 0` arm is
* unreachable and named rather than covered: [doWork] refuses a job with no URI array several
* lines above this call, and nothing else calls it. It is the shape `docs/coverage-read-findings.md`
* calls F4 — a second line of defence that cannot be provoked.
*/
override suspend fun getForegroundInfo(): ForegroundInfo {
val inputCount = inputData.getStringArray(KEY_INPUT_URIS)?.size ?: 0
return ForegroundInfo(
NOTIFICATION_ID,
notifications.build(id, joiningTitle(inputCount), 0, indeterminate = true),
ConversionForegroundType.current(),
)
}
companion object {
/**
@@ -204,6 +224,16 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
*/
fun outputNameFor(format: OutputFormat): String = "joined.${format.extension}"
/**
* What the progress notification says while a join runs.
*
* Named once, for the convention #158 established about strings the user can see. It was
* two strings until 2026-09-06 — `"Joining N files"` built inline in [doWork] and a
* countless `"Joining files"` in [getForegroundInfo] — for one notification that only ever
* had one job, and the copy nothing executed was free to drift from the one that did.
*/
fun joiningTitle(inputCount: Int): String = "Joining $inputCount files"
private const val NOTIFICATION_ID = 1002
private const val TAG = "ConcatWorker"
@@ -215,6 +245,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
*/
fun request(inputs: List<Uri>, totalBytes: Long?, format: OutputFormat = DEFAULT_FORMAT) =
OneTimeWorkRequestBuilder<ConcatWorker>()
// Expedited, exactly as ConversionWorker.request is and for the same reasons; that
// one's comment and class KDoc carry them. Nothing here sets an initial delay or a
// constraint, which is what makes it legal for `build()` to accept.
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST)
.addTag(JobTags.inputCount(inputs.size))
.setInputData(
Data.Builder()
@@ -8,6 +8,7 @@ import androidx.work.CoroutineWorker
import androidx.work.Data
import androidx.work.ForegroundInfo
import androidx.work.OneTimeWorkRequestBuilder
import androidx.work.OutOfQuotaPolicy
import androidx.work.WorkerParameters
import androidx.work.hasKeyWithValueOfType
import androidx.work.workDataOf
@@ -40,8 +41,28 @@ import java.io.File
* observe. That durability is what makes the six-hour foreground-service timeout
* recoverable instead of fatal.
*
* Expedited work is deliberately *not* used. It maps to JobScheduler expedited jobs
* with a short quota, which is the wrong shape for a multi-minute transcode.
* Enqueued as **expedited** work, with `RUN_AS_NON_EXPEDITED_WORK_REQUEST`. This paragraph said
* the opposite until 2026-09-06 — "deliberately *not* used… the wrong shape for a multi-minute
* transcode" — and the quota it named does not reach a transcode the way it reads:
*
* - The quota belongs to the *JobScheduler* job, and `SystemJobInfoConverter:135` in
* work-runtime 2.11.2 sets `JobInfo.setExpedited(true)` only when `!isRetry && !isDelayed`.
* A retry is therefore scheduled exactly as every job is scheduled today.
* - A job the system stops mid-run does not get its answer from [FailureOutcome].
* `WorkerWrapper.interrupt` cancels the worker's coroutine with a `WorkerStoppedException`,
* which its `launch` resolves as `ResetWorkerStatus` — the worker's own `Result` is discarded
* and the work re-enqueued with backoff, whatever it returned. So a quota stop is a retry, and
* the `CancellationException` arm in [doWork] is what deletes the partial on the way through.
*
* What it buys is narrower than "conversions start sooner", and the narrowness is the honest part:
* `GreedyScheduler` starts unconstrained, undelayed work in-process the moment it is enqueued and
* carries no `expedited` branch at all, so a conversion begun from the open app runs exactly when
* it ran before — the common case does not move. The flag is for the job that has to go *through*
* JobScheduler because no process is left to start it: one still enqueued when the app died.
* `SystemJobScheduler.schedule` re-converts the spec every time it schedules, so such a job is
* expedited on the way back in, and a retried one is not. `RUN_AS_NON_EXPEDITED_WORK_REQUEST`
* rather than `DROP_WORK_REQUEST`: an invisible quota is no reason to throw a user's conversion
* away, and `SystemJobScheduler:198` degrades it to an ordinary job instead.
*
* That durability is not free, and the queue surviving is not the same as the job surviving.
* When WorkManager recovers a job after process death the app is by definition in the background,
@@ -60,7 +81,7 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
override suspend fun doWork(): Result {
val inputUri = inputData.getString(KEY_INPUT_URI)?.let(Uri::parse)
?: return Result.failure(workDataOf(KEY_ERROR to "No input file."))
val displayName = inputData.getString(KEY_DISPLAY_NAME) ?: "input"
val displayName = displayName()
// Absent, not zero, when nobody could say -- see InputQuery. `getLong(key, 0L)` is what
// made those two the same number, and `hasSpaceFor(0)` is only "is there 128 MB free".
val declaredSize = inputData
@@ -100,7 +121,10 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
// process death is. With it above the try that throw escaped doWork() entirely: no
// retry, no error in the output Data, and no staged.delete(). MediaProbe.probe below
// was outside for the same reason and had the same problem.
setForeground(foregroundInfo(displayName, percent = 0, indeterminate = true))
//
// Posted through getForegroundInfo() rather than built here a second time -- see that
// override for what the duplicate cost.
setForeground(getForegroundInfo())
// Through the seam rather than MediaProbe directly. The seam already existed for the
// ViewModel and the worker was the last caller bypassing it, which is why nothing on
@@ -339,8 +363,32 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
return OutputSpec(container, video, audio)
}
/**
* What this job's input is called, or [InputQuery.FALLBACK_DISPLAY_NAME] when nothing named it.
*
* One read rather than the two copies of `?: "input"` that [doWork] and [getForegroundInfo]
* each carried, and against `InputQuery`'s constant rather than a third literal of the same
* string: it is the same fallback the picker uses, and it reaches the save dialog as
* `input_converted.mp4`.
*/
private fun displayName(): String = inputData.getString(KEY_DISPLAY_NAME) ?: InputQuery.FALLBACK_DISPLAY_NAME
/**
* The notification a starting conversion posts, and now the only definition of it.
*
* This is WorkManager's hook for expedited work, and **it will not be called on any device
* this app supports.** `WorkForeground.kt:38` in work-runtime 2.11.2 opens with
* `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, that function is the library's
* only caller of `getForegroundInfoAsync()`, and `minSdk` is 33. So #252's premise — that
* enqueueing expedited work would make these lines live — is false, and `setExpedited` alone
* would have left them exactly as cold as the first instrumented coverage read found them.
*
* What makes them live is [doWork] posting *this* instead of building its own copy. The two
* were identical — same title, `percent = 0`, `indeterminate = true` — so one was a duplicate
* that could drift, and the one nothing executed is the one that would have drifted silently.
*/
override suspend fun getForegroundInfo(): ForegroundInfo = foregroundInfo(
inputData.getString(KEY_DISPLAY_NAME) ?: "input",
displayName(),
percent = 0,
indeterminate = true,
)
@@ -416,6 +464,12 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
quality: QualityTier = QualityTier.FAST,
enginePreference: EnginePreference = EnginePreference.AUTO,
) = OneTimeWorkRequestBuilder<ConversionWorker>()
// Expedited, so the jobs that do go through JobScheduler are treated as the
// user-initiated work they are -- see the class KDoc for what that is and is not worth.
// Safe to set here and only because of what this builder does not do: `build()` refuses
// an expedited request carrying an initial delay or any constraint but network and
// storage, and none of the three is set below.
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST)
.addTag(JobTags.displayName(displayName))
// Neither the tag nor the Data entry is written for a size nobody knows. A `Data` has
// no null, so the absence of the key *is* the unknown — and a tag reading
@@ -106,7 +106,10 @@ class Media3MuxersTest {
* has changed its mind and somebody should say so on purpose.
*
* - Media3's MP4 muxer accepts Vorbis; [ContainerCapabilities] declines to offer it, because
* Vorbis-in-MP4 is poorly supported by players.
* Vorbis-in-MP4 is poorly supported by players. That refusal is about **this container**,
* not about the codec: since #254 the app encodes Vorbis for Ogg, Matroska and WebM, and
* the assertion below is what keeps MP4 out of that list on purpose rather than by
* omission — it is `CARRIES_AUDIO[MP4]`, so widening the encodable set cannot reach it.
* - The matrix offers MP3 and FLAC in MP4, which is legal and which FFmpeg writes happily, but
* Media3's MP4 muxer carries neither — so those jobs route to FFmpeg rather than failing.
*/
@@ -166,6 +166,47 @@ class FFmpegCommandBuilderTest {
assertPair(cmd(OutputFormat.OPUS), "-c:a", "libopus")
}
/**
* The arm that named an encoder the shipped binary did not contain.
*
* This read `-c:a libvorbis` from the day the builder was written and had never been run: no
* preset produced [AudioCodec.VORBIS] and `ContainerCapabilities` refused it. It could not
* have worked either — `--enable-libvorbis` was in neither `bin/README.md`'s configure line
* nor `tools/ffmpeg/build-ffmpeg.sh`, and the string `libvorbis` was not in the shipped
* `libavcodec.so` while `libopus`, `libmp3lame`, `libx264` and five others were. #254 rebuilt
* the AAR with it.
*
* **What this test cannot do is tell you that.** `-c:a libvorbis` and `-c:a libvorbisss` are
* the same string to a JVM assertion, which is precisely how the defect survived four coverage
* waves and a review that asked whether every test asserted something. The positive claim —
* that this encoder exists in the binary and produces a Vorbis track — is proved by
* `FFmpegEngineTest.encodesOggVorbisThroughAnEncoderTheBundledBinaryActuallyHas` on a device,
* and by nothing else in this repo.
*
* The two negatives are the assertions that carry real weight here, because each pins a
* decision rather than a name. `-strict experimental` and `-ac 2` are what FFmpeg's in-tree
* `vorbis` encoder forces, and taking the in-tree encoder would silently upmix mono; the arm's
* KDoc has the measurements. `-f ogg` is asserted because encoder and muxer together are what
* make the file — an encoder without its muxer is how a Vorbis stream ends up in a container
* that will not open.
*/
@Test
fun `ogg vorbis names libvorbis, with no experimental gate and no forced stereo`() {
val args = cmd(OutputFormat.OGG_VORBIS)
assertPair(args, "-c:a", "libvorbis")
assertPair(args, "-q:a", "5")
assertPair(args, "-f", "ogg")
assertFalse(
"libvorbis is not experimental; -strict belongs to FFmpeg's in-tree encoder: $args",
args.contains("-strict"),
)
assertFalse(
"libvorbis takes any channel count, so mono must not be upmixed: $args",
args.contains("-ac"),
)
}
/**
* The arm most conversions actually take, and the only one in `audioArgs` with no test.
*
@@ -216,7 +257,13 @@ class FFmpegCommandBuilderTest {
@Test
fun `audio only formats never carry a video encoder`() {
listOf(OutputFormat.MP3, OutputFormat.FLAC, OutputFormat.WAV, OutputFormat.OPUS)
listOf(
OutputFormat.MP3,
OutputFormat.FLAC,
OutputFormat.WAV,
OutputFormat.OPUS,
OutputFormat.OGG_VORBIS,
)
.forEach { format ->
val args = cmd(format)
assertFalse("$format should not set -c:v", args.contains("-c:v"))
@@ -19,6 +19,16 @@ import org.junit.Test
*/
class ContainerCapabilitiesTest {
/**
* The codecs the matrix can actually be asked about.
*
* `COPY` and `NONE` are excluded because [ContainerCapabilities.accepts] refuses the first
* outright — `resolving COPY before asking the matrix is required` covers that — and answers
* the second `true` for every container without consulting any table.
*/
private val realAudioCodecs = AudioCodec.entries - AudioCodec.COPY - AudioCodec.NONE
private val realVideoCodecs = VideoCodec.entries - VideoCodec.COPY - VideoCodec.NONE
private val h264Source = InputProbe(
videoCodec = "h264",
audioCodec = "aac",
@@ -79,10 +89,50 @@ class ContainerCapabilitiesTest {
}
@Test
fun `Matroska carries Vorbis on copy but nothing here encodes it`() {
assertTrue(ContainerCapabilities.accepts(Container.MKV, AudioCodec.VORBIS, CodecMode.COPY))
fun `Matroska carries VP8 on copy but nothing here encodes it`() {
assertTrue(ContainerCapabilities.accepts(Container.MKV, VideoCodec.VP8, CodecMode.COPY))
assertFalse(
ContainerCapabilities.accepts(Container.MKV, AudioCodec.VORBIS, CodecMode.ENCODE),
ContainerCapabilities.accepts(Container.MKV, VideoCodec.VP8, CodecMode.ENCODE),
)
}
/**
* Where the copy/encode gap actually is, asserted as a set rather than as examples.
*
* This used to have an audio twin — Matroska carries Vorbis, and nothing was thought to encode
* it. That was never true of the code: `FFmpegCommandBuilder` has emitted a Vorbis encoder
* since it was written, and only `ENCODABLE_AUDIO`'s omission made the arm unreachable (#254).
* With Vorbis in the set, **the audio gap is empty** and the mode axis earns its place on the
* video side alone.
*
* Two consequences worth having pinned rather than rediscovered:
*
* - `validateAudio`'s "this app cannot encode X audio" arm now has no reachable input, which
* is why no test drives it. It stays in production as the landing spot for the first ALAC
* or AC-3 entry, and this test is what will fail the day one is carried without an encoder
* — where before, an omission like Vorbis's could sit unnoticed for the life of the file.
* - The video list is the real one, and asserting it as a set is what makes an accidental
* addition visible: an encoder added for VP8 without a matching `ENCODABLE_VIDEO` entry
* would leave this passing, but a *carried* codec quietly dropped from the encodable set
* would not.
*/
@Test
fun `the copy-only gap is video-only, and VP8 and AV1 are all of it`() {
fun <T> gap(codecs: List<T>, accepts: (Container, T, CodecMode) -> Boolean): Set<T> =
Container.entries.flatMap { container ->
codecs
.filter { accepts(container, it, CodecMode.COPY) }
.filterNot { accepts(container, it, CodecMode.ENCODE) }
}.toSet()
assertEquals(
"no container may carry an audio codec this app cannot also encode",
emptySet<AudioCodec>(),
gap(realAudioCodecs, ContainerCapabilities::accepts),
)
assertEquals(
setOf(VideoCodec.VP8, VideoCodec.AV1),
gap(realVideoCodecs, ContainerCapabilities::accepts),
)
}
@@ -405,21 +455,30 @@ class ContainerCapabilitiesTest {
assertEverySuggestionValid(invalid, mp3Source)
}
/**
* The spec that used to be this class's example of an unencodable audio codec, now valid.
*
* It asserted `"This app cannot encode Vorbis audio. It can still be copied from a Vorbis
* source."` for exactly this spec, and the message was wrong about the app: the encoder
* existed, unreachable (#254). Asserting the positive is what stops the omission coming back —
* a revert of `ENCODABLE_AUDIO` fails here rather than merely restoring an old refusal that
* reads plausible.
*
* The audio arm it used to cover no longer has a reachable input; `the copy-only gap is
* video-only` above is where that is now recorded, and `copying is offered as the fix when the
* codec is right but unencodable` still covers the live video half of the same rule.
*/
@Test
fun `an audio codec this app cannot encode is refused, and copying is offered instead`() {
// Matroska carries Vorbis; nothing here encodes it. The refusal has to say so *and* say
// what would work, which is the audio twin of `copying is offered as the fix when the codec
// is right but unencodable`.
fun `Vorbis into Matroska is a re-encode this app will do`() {
val spec = OutputSpec(Container.MKV, VideoCodec.H264, AudioCodec.VORBIS)
val invalid = ContainerCapabilities.validate(spec, h264Source) as? Validation.Invalid
?: throw AssertionError("encoding Vorbis must be refused")
assertEquals(
"This app cannot encode Vorbis audio. It can still be copied from a Vorbis source.",
invalid.message,
assertTrue(
"Vorbis is encodable, so this spec must validate: ${ContainerCapabilities.validate(spec, h264Source)}",
ContainerCapabilities.validate(spec, h264Source).isValid,
)
assertEverySuggestionValid(invalid, h264Source)
// The plan has to reach the encoder, not merely be permitted: an AAC source into Matroska
// cannot be upgraded to a copy, so this is an Encode carrying the codec that was asked for.
assertEquals(AudioPlan.Encode(AudioCodec.VORBIS), CopyPlanner.plan(spec, h264Source).audio)
}
@Test
@@ -330,6 +330,28 @@ class ConversionRouterTest {
}
}
/**
* Ogg Vorbis leaves the hardware path one rule earlier than its Ogg sibling, and the reason
* shown to the user is the difference.
*
* Two rules would each send it to FFmpeg — Media3 cannot encode Vorbis, and it cannot write
* Ogg at all — and the order decides which explanation appears. The audio-encoder check runs
* first deliberately: `NO_PLATFORM_ENCODER` ("Android has no encoder for this format") is true
* of Vorbis on every Android version and tells the user something about their choice, where
* `CONTAINER_UNSUPPORTED` would name an internal boundary they cannot act on. That ordering is
* documented in the router and this is what holds it — asserting only the engine would pass
* with the two rules swapped.
*/
@Test
fun `ogg vorbis routes to ffmpeg because Android has no Vorbis encoder`() {
val d = route(OutputFormat.OGG_VORBIS)
assertEquals(Engine.FFMPEG, d.engine)
assertEquals(Reason.NO_PLATFORM_ENCODER, d.reason)
// The sibling in the same container stops at the container rule instead, because Media3
// *can* encode Opus. One container, two reasons, and only the codec differs.
assertEquals(Reason.CONTAINER_UNSUPPORTED, route(OutputFormat.OPUS).reason)
}
/** M4A is the audio format that does stay on hardware, because its container is MP4. */
@Test
fun `m4a stays on hardware because MP4 is a container Media3 can write`() {
@@ -75,9 +75,14 @@ class OutputFormatTest {
fun `every container names an extension, a mime type and an ffmpeg muxer`() {
Container.entries.forEach { container ->
listOf(true, false).forEach { hasVideo ->
val ext = container.extensionFor(hasVideo)
assertTrue("$container has no extension", ext.isNotBlank())
assertFalse("$container extension has a dot", ext.startsWith("."))
// Every audio codec, because the extension now varies by one — see the Ogg pair
// below. A container that answered blank for a codec it carries would be a
// filename with no extension at all.
AudioCodec.entries.forEach { audioCodec ->
val ext = container.extensionFor(hasVideo, audioCodec)
assertTrue("$container/$audioCodec has no extension", ext.isNotBlank())
assertFalse("$container/$audioCodec extension has a dot", ext.startsWith("."))
}
assertTrue(
"$container has no mime type",
container.mimeTypeFor(hasVideo).contains('/'),
@@ -89,10 +94,34 @@ class OutputFormatTest {
@Test
fun `audio-only variants of a container get their own extension`() {
assertEquals("mp4", Container.MP4.extensionFor(hasVideo = true))
assertEquals("m4a", Container.MP4.extensionFor(hasVideo = false))
assertEquals("mkv", Container.MKV.extensionFor(hasVideo = true))
assertEquals("mka", Container.MKV.extensionFor(hasVideo = false))
assertEquals("mp4", Container.MP4.extensionFor(hasVideo = true, audioCodec = AudioCodec.AAC))
assertEquals("m4a", Container.MP4.extensionFor(hasVideo = false, audioCodec = AudioCodec.AAC))
assertEquals("mkv", Container.MKV.extensionFor(hasVideo = true, audioCodec = AudioCodec.AAC))
assertEquals("mka", Container.MKV.extensionFor(hasVideo = false, audioCodec = AudioCodec.AAC))
}
/**
* The second thing the extension depends on, and the reason [Container.extensionFor] takes a
* codec at all.
*
* One Ogg stream holds Vorbis or Opus, and the two are named differently: RFC 7845 §9 asks for
* `.opus` on an Ogg carrying Opus alone, while a Vorbis one is a plain `.ogg`. The container
* declared `opus` for every Ogg until [OutputFormat.OGG_VORBIS] existed, which would have
* shipped a Vorbis file called `.opus` — the same shape as the `FLAC` preset that once
* declared Matroska with a `.flac` extension, which is the regression guarded above.
*
* Both halves are asserted. Pinning only the Vorbis one would pass just as well if the
* override map were deleted and every Ogg went back to a single extension, which is the
* mutation that has to fail.
*/
@Test
fun `Ogg names its file after the codec in it, not after the container`() {
assertEquals("opus", OutputFormat.OPUS.extension)
assertEquals("ogg", OutputFormat.OGG_VORBIS.extension)
// The MIME type does not split the same way: audio/ogg is correct for both, so the SAF
// create-document contract sees one type for the two formats.
assertEquals("audio/ogg", OutputFormat.OPUS.mimeType)
assertEquals("audio/ogg", OutputFormat.OGG_VORBIS.mimeType)
}
/** Regression guard: FLAC used to be declared as Matroska with a `.flac` extension. */
@@ -0,0 +1,102 @@
package org.libremediaconverter.work
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Constraints
import androidx.work.OutOfQuotaPolicy
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
/**
* Both workers enqueue **expedited** work, and stay legal doing it.
*
* The two questions are separate and only one of them is about the flag.
*
* - **Is it set.** `expedited` is `false` by default, so `assertTrue` here is what a deleted
* `setExpedited(...)` reddens. That mutation was run.
* - **Is it legal.** `WorkRequest.Builder.build()` refuses an expedited request that carries an
* initial delay or any constraint but network and storage — `require(workSpec.initialDelay <= 0)
* { "Expedited jobs cannot be delayed" }` in work-runtime 2.11.2. Neither `request` sets either
* today, so both `build()` calls pass and the `IllegalArgumentException` is a *future* hazard
* rather than a current one. The delay and constraints assertions below are what name it: add a
* delay to either builder and this class fails on the throw, in the same second, instead of the
* app failing to enqueue a conversion on a device.
*
* **The policy assertion bites less than it reads, and that is worth writing down rather than
* leaving to be rediscovered.** `WorkSpec.outOfQuotaPolicy` *defaults* to
* `RUN_AS_NON_EXPEDITED_WORK_REQUEST`, so it is already this value on a request that was never
* expedited at all — deleting `setExpedited` does not redden it. What it does pin is the one
* alternative: `DROP_WORK_REQUEST` throws a user's conversion away because an invisible quota ran
* out, and that mutation *is* red here.
*
* The delay is not hypothetical either. Three tests deliberately build a delayed request to hold a
* job in `ENQUEUED` — `NotificationCancelActionTest`, `ReattachOnLaunchTest` and
* `CancelReachesWorkManagerTest` — and every one of them builds its own
* `OneTimeWorkRequestBuilder` rather than adding a delay to what `request` returns. That is why
* making these expedited broke none of them; the one that starts from `request` takes only
* `base.workSpec.input` from it.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class ExpeditedRequestTest {
@Test
fun `a conversion is enqueued as expedited work`() {
val spec = ConversionWorker.request(INPUT, DISPLAY_NAME, INPUT_BYTES).workSpec
assertTrue("a conversion the user asked for has to be expedited work", spec.expedited)
assertEquals(
"a quota nobody can see is no reason to drop a conversion",
OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST,
spec.outOfQuotaPolicy,
)
}
@Test
fun `a join is enqueued as expedited work`() {
val spec = ConcatWorker.request(listOf(INPUT, SECOND_INPUT), TOTAL_BYTES).workSpec
assertTrue("a join the user asked for has to be expedited work", spec.expedited)
assertEquals(
"a quota nobody can see is no reason to drop a join",
OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST,
spec.outOfQuotaPolicy,
)
}
/**
* The two properties that keep `build()` from throwing, asserted on both requests at once
* because the rule is WorkManager's rather than either worker's.
*/
@Test
fun `neither expedited request carries what would make it illegal`() {
val requests = listOf(
ConversionWorker.request(INPUT, DISPLAY_NAME, INPUT_BYTES).workSpec,
ConcatWorker.request(listOf(INPUT, SECOND_INPUT), TOTAL_BYTES).workSpec,
)
requests.forEach { spec ->
assertEquals(
"expedited work cannot be delayed: ${spec.workerClassName}",
0L,
spec.initialDelay,
)
assertEquals(
"expedited work takes only network and storage constraints: ${spec.workerClassName}",
Constraints.NONE,
spec.constraints,
)
}
}
private companion object {
val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4")
val SECOND_INPUT: Uri = Uri.parse("file:///tmp/holiday2.mp4")
const val DISPLAY_NAME = "holiday.mp4"
const val INPUT_BYTES = 1_024L
const val TOTAL_BYTES = 2_048L
}
}
@@ -0,0 +1,203 @@
package org.libremediaconverter.work
import android.app.Application
import android.app.Notification
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import androidx.work.testing.TestListenableWorkerBuilder
import androidx.work.workDataOf
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.convert.ConcatJoiner
import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.installTestWorkManager
import org.libremediaconverter.ffmpeg.ConcatEngine
import org.libremediaconverter.model.ConcatStrategy
import org.libremediaconverter.model.DeviceCodecs
import org.libremediaconverter.model.EnginePreference
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
import java.util.UUID
/**
* The first thing either worker posts is what its own `getForegroundInfo()` builds.
*
* **Both overrides were dead code until 2026-09-06, and #252 is where that was found** — the first
* instrumented coverage read reported `ConversionWorker:342-346` and `ConcatWorker:132-136` among
* the 32 lines *neither* suite reaches. The ticket's premise was that enqueueing expedited work
* would make them live, since `getForegroundInfo()` is WorkManager's expedited-work hook.
*
* **That premise is false at this `minSdk`, which is the finding underneath the fix.**
* `WorkForeground.kt:38` in work-runtime 2.11.2 opens `workForeground` with
* `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, that function is the library's only
* caller of `getForegroundInfoAsync()`, and `minSdk` is 33. So `setExpedited` alone would have left
* both overrides exactly as cold as the read found them, and a test written to drive them through
* WorkManager would be testing a code path no device this app supports can take — E1's failure
* mode, where a test asserts and never reaches.
*
* What makes them live is a single-definition change instead. Each worker had **two** definitions
* of one notification: the override, and an identical `ForegroundInfo` built inline in `doWork`.
* `doWork` now posts the override's, so the copy nothing executed is gone and the one that remains
* runs on every job.
*
* These tests are what hold that wiring. Each asserts the notification's *contents* against
* constants rather than against `worker.getForegroundInfo()` — comparing the two would move
* together under every mutation and stay green — and the mutations that redden them are named on
* each test.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class ForegroundNotificationTest {
private lateinit var app: Application
private lateinit var updater: RecordingForegroundUpdater
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
updater = RecordingForegroundUpdater()
ConversionDependencies.publisher = { AlwaysRoomPublisher(app) }
ConversionDependencies.probe = { _, _ -> InputProbe() }
ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE }
ConversionDependencies.software = { WritingTranscoder }
ConversionDependencies.concat = { WritingJoiner }
// The notification carries a WorkManager cancel PendingIntent, so without this the worker
// fails building the notification rather than on anything these tests are about.
installTestWorkManager(app, Data.EMPTY)
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
/**
* Mutation that must go red, and did: inside `ConversionWorker.getForegroundInfo`, replace
* `displayName()` with a literal, or `percent = 0` with anything else. Both are in the override's
* own body, so a red here is proof `doWork` executes it rather than a copy of it.
*/
@Test
fun `a conversion's first foreground post is the one getForegroundInfo builds`() {
runBlocking { conversionWorker().doWork() }
val first = updater.infos.first()
val extras = first.notification.extras
assertEquals(
"the notification has to name the file the user picked",
DISPLAY_NAME,
extras.getString(Notification.EXTRA_TITLE),
)
assertEquals("a conversion starts at zero", 0, extras.getInt(Notification.EXTRA_PROGRESS))
assertTrue(
"nothing is known about the length of the job yet, so the bar is indeterminate",
extras.getBoolean(Notification.EXTRA_PROGRESS_INDETERMINATE),
)
assertEquals(
"the foreground service type is the regime's, not zero",
ConversionForegroundType.current(),
first.foregroundServiceType,
)
}
/**
* The count is the point.
*
* `getForegroundInfo` said `"Joining files"` and `doWork` said `"Joining N files"` — one
* notification with two texts, and the one nothing ran was free to drift. Now there is one,
* and it reads the input array itself so it can still answer before `doWork` has parsed
* anything.
*
* Mutation that must go red, and did: replace the array read in `ConcatWorker.getForegroundInfo`
* with a constant `0`, which yields `"Joining 0 files"`. Asserting merely that the title starts
* with "Joining" would survive that, which is why the whole string is pinned.
*/
@Test
fun `a join's first foreground post counts the files it was given`() {
runBlocking { joinWorker().doWork() }
val first = updater.infos.first()
assertEquals(
"the notification has to say how many files are being joined",
ConcatWorker.joiningTitle(INPUTS.size),
first.notification.extras.getString(Notification.EXTRA_TITLE),
)
assertEquals(
"the foreground service type is the regime's, not zero",
ConversionForegroundType.current(),
first.foregroundServiceType,
)
}
/**
* And the title is really the file's name rather than any string at all.
*
* [ConcatWorker.joiningTitle] is asserted above through the constant the worker itself uses, so
* that assertion cannot catch the sentence being reworded — deliberately, since the wording is
* not what the test is about. This one can: two files, two names, one worker each.
*/
@Test
fun `two conversions of differently named files post differently named notifications`() {
runBlocking { conversionWorker(displayName = OTHER_NAME).doWork() }
assertEquals(
OTHER_NAME,
updater.infos.first().notification.extras.getString(Notification.EXTRA_TITLE),
)
}
private fun conversionWorker(displayName: String = DISPLAY_NAME) = TestListenableWorkerBuilder<ConversionWorker>(
context = app,
inputData = workDataOf(
ConversionWorker.KEY_INPUT_URI to INPUT.toString(),
ConversionWorker.KEY_DISPLAY_NAME to displayName,
ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES,
// FORCE_SOFTWARE is the one preference that decides without consulting the input,
// and a file:// URI keeps the worker out of FFmpegKit's native SAF bridge.
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
),
runAttemptCount = 0,
).setId(JOB_ID)
.setForegroundUpdater(updater)
.build()
private fun joinWorker() = TestListenableWorkerBuilder<ConcatWorker>(
context = app,
inputData = workDataOf(
ConcatWorker.KEY_INPUT_URIS to INPUTS.map(Uri::toString).toTypedArray(),
ConcatWorker.KEY_TOTAL_BYTES to TOTAL_BYTES,
ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name,
),
runAttemptCount = 0,
).setId(JOB_ID)
.setForegroundUpdater(updater)
.build()
private companion object {
val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4")
val INPUTS: List<Uri> = listOf(INPUT, Uri.parse("file:///tmp/holiday2.mp4"))
const val DISPLAY_NAME = "holiday.mp4"
const val OTHER_NAME = "birthday.mkv"
const val INPUT_BYTES = 1_024L
const val TOTAL_BYTES = 2_048L
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000252")
}
}
/** A joiner that writes an output and reports a strategy; nothing here is about the engine. */
private object WritingJoiner : ConcatJoiner {
override suspend fun join(inputs: List<Uri>, output: File, format: OutputFormat): ConcatEngine.Result {
output.writeBytes(ByteArray(OUTPUT_BYTES))
return ConcatEngine.Result(ConcatStrategy.STREAM_COPY, output)
}
private const val OUTPUT_BYTES = 512
}
@@ -3,16 +3,12 @@ package org.libremediaconverter.work
import android.app.Application
import android.app.Notification
import android.app.NotificationManager
import android.content.Context
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.Data
import androidx.work.ForegroundInfo
import androidx.work.WorkInfo
import androidx.work.testing.TestForegroundUpdater
import androidx.work.testing.TestListenableWorkerBuilder
import androidx.work.workDataOf
import com.google.common.util.concurrent.ListenableFuture
import kotlinx.coroutines.runBlocking
import org.junit.After
import org.junit.Assert.assertEquals
@@ -224,26 +220,6 @@ class ProgressNotificationTest {
}
}
/**
* Records every [ForegroundInfo] the worker publishes, and otherwise behaves as the test default.
*
* Delegating to [TestForegroundUpdater] rather than hand-rolling a `ListenableFuture<Void>`: the
* worker awaits what this returns, so a future that never completes would hang the initial
* `setForeground` rather than test anything.
*/
private class RecordingForegroundUpdater : TestForegroundUpdater() {
val infos = mutableListOf<ForegroundInfo>()
override fun setForegroundAsync(
context: Context,
id: UUID,
foregroundInfo: ForegroundInfo,
): ListenableFuture<Void> {
infos += foregroundInfo
return super.setForegroundAsync(context, id, foregroundInfo)
}
}
/** An engine that reports whatever [report] wants reported, then writes an output. */
private class ReportingTranscoder(private val report: ((Int) -> Unit) -> Unit) : SoftwareTranscoder {
override suspend fun run(
@@ -1,11 +1,14 @@
package org.libremediaconverter.work
import android.content.Context
import androidx.work.ForegroundInfo
import androidx.work.testing.TestForegroundUpdater
import com.google.common.util.concurrent.ListenableFuture
import org.libremediaconverter.convert.OutputPublisher
import org.libremediaconverter.convert.SoftwareTranscoder
import org.libremediaconverter.model.ConversionRequest
import java.io.File
import java.util.UUID
import java.util.concurrent.ExecutionException
import java.util.concurrent.Executor
import java.util.concurrent.TimeUnit
@@ -94,3 +97,27 @@ internal class FailedFuture(private val failure: Throwable) : ListenableFuture<V
override fun get(): Void = throw ExecutionException(failure)
override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure)
}
/**
* Records every [ForegroundInfo] the worker publishes, and otherwise behaves as the test default.
*
* Delegating to [TestForegroundUpdater] rather than hand-rolling a `ListenableFuture<Void>`: the
* worker awaits what this returns, so a future that never completes would hang the initial
* `setForeground` rather than test anything.
*
* Shared scaffolding since #252 moved it here out of `ProgressNotificationTest`, which asks what a
* *running* worker publishes; `ForegroundNotificationTest` asks what its *first* post is, and both
* questions need the same recorder. `infos.first()` is that first post in either.
*/
internal class RecordingForegroundUpdater : TestForegroundUpdater() {
val infos = mutableListOf<ForegroundInfo>()
override fun setForegroundAsync(
context: Context,
id: UUID,
foregroundInfo: ForegroundInfo,
): ListenableFuture<Void> {
infos += foregroundInfo
return super.setForegroundAsync(context, id, foregroundInfo)
}
}
+23 -9
View File
@@ -22,19 +22,33 @@ It also removes roughly forty minutes from every cold CI run.
| API level | 33, matching the app's minSdk |
| ABIs | arm64-v8a, x86_64 |
| Shared libraries | 20 (10 per ABI) |
| SHA-256 | `ae188c9aec3c89a1c87a169589253c85438d57cfdcc3ce8b40fb3e87de368ff2` |
| SHA-256 | `c8f4491d2c626566cbf18d5035513c1a5d8049e6696531342ea030c5427df507` |
| Rebuilt | 2026-09-06, to add libvorbis (#254). Previous archive: `ae188c9a…`, same tag and FFmpeg version, one library fewer |
Configure line, read back out of the shipped `libavutil.so`:
```
--enable-asm --enable-cross-compile --enable-gpl --enable-iconv
--enable-inline-asm --enable-jni --enable-libass --enable-libdav1d
--enable-libfontconfig --enable-libfreetype --enable-libfribidi
--enable-libharfbuzz --enable-libjxl --enable-libmp3lame --enable-libopus
--enable-libsvtav1 --enable-libvpx --enable-libx264 --enable-libx265
--enable-lto --enable-mediacodec --enable-neon --enable-optimizations
--enable-pic --enable-pthreads --enable-shared --enable-small
--enable-swscale --enable-v4l2-m2m --enable-version3 --enable-zlib
--enable-asm --enable-cross-compile --enable-gpl --enable-iconv
--enable-inline-asm --enable-jni --enable-libass --enable-libdav1d
--enable-libfontconfig --enable-libfreetype --enable-libfribidi
--enable-libharfbuzz --enable-libjxl --enable-libmp3lame --enable-libopus
--enable-libsvtav1 --enable-libvorbis --enable-libvpx --enable-libx264
--enable-libx265 --enable-lto --enable-mediacodec --enable-neon
--enable-optimizations --enable-pic --enable-pthreads --enable-shared
--enable-small --enable-swscale --enable-v4l2-m2m --enable-version3
--enable-zlib
```
`--enable-libvorbis` is the one that arrived late, in #254, and the two ways to get it wrong are
worth having written down. ffmpeg-kit's `--enable-*` names are its own — `--enable-lame` for
libmp3lame, `--enable-opus` for libopus — so `--enable-vorbis` is the plausible guess and it is not
the flag; `get_library_name()` in the upstream `scripts/function.sh` calls library 9 `libvorbis`.
And an unrecognised `--enable-*` is **ignored silently**, so a build that dropped it looks exactly
like one that worked. What tells them apart is the binary:
```sh
unzip -p bin/ffmpeg-kit-next-8.1.1.aar 'jni/x86_64/libavcodec.so' > /tmp/libavcodec.so
strings /tmp/libavcodec.so | grep -x libvorbis # and the same for arm64-v8a
```
Every `.so` reports `LOAD align 0x4000`, so the archive satisfies the 16 KB page-size
Binary file not shown.
+207
View File
@@ -0,0 +1,207 @@
# When a gating E2E leg goes red and the diff cannot explain it
**Status:** a census of every gating E2E leg-attempt in the repo's history, classified by mode,
with a disposition for each. **1489 gating leg-attempts, 129 failures, 8.7%** — 2026-08-20 to
2026-09-07. This is the standing answer to "my docs-only PR turned an emulator leg red, what is
it?", and it is what #102 asked for before being closed as an umbrella.
**Last verified:** 2026-09-07, against `main` at `ef9d35e`. Mode 6's fix is in #272 and is the only
thing here not yet on `main`.
This document is about **the emulator failing underneath the suite**. It is not a defect record
(`docs/defect-audit.md`), not a coverage read (`docs/coverage-read-findings.md`), and not a
test-suite read (`docs/e2e-read-findings.md`). Nothing here is a bug in the app.
## Read this first: three counting rules, each learned by getting it wrong
**Count per leg-attempt, never per run.** Measured here rather than asserted: the 129 failing
leg-attempts sit in **83 distinct runs, and 45 of those 83 ended green** once someone re-ran them.
So a census that counts failed *runs* finds 38 events where there were 129 — it does not
under-report evenly, it deletes exactly the failures somebody already decided were noise, which are
the ones this document is about. Every number here is per leg-attempt, with `cancelled` legs
excluded: those are `concurrency: cancel-in-progress` cancellations rather than runs, and there are
135 of them.
**Every mode has its own denominator, and it is not 1489.** Derive it from where and when the
*test* ran, not from the leg count, and two things move it. The API 37 row filters out every test
carrying `@FailsOnEmulatorApi37` with `notAnnotation` — **all four of `SafPickerRoundTripTest` and
three of `Media3EngineTest`, seven today** — which is every mode in the table below except 3 and 5.
And **that set has grown across this window**: the picker test and the two saves only joined it on
2026-09-06, which is why `SafPickerRoundTripTest` has 25 API 37 failures on record — 14 of them
since 2026-08-27 — that could not happen now. The saves did
not exist at all before 2026-09-06T15:12. A rate quoted over "all gating leg-attempts" is wrong for
every one of them, and is how "8% of legs" gets said about a thing that happens on one row.
**Anchor the mode to the test name beside the `FAILED` marker, then to the message under it.**
The name alone is not enough: `transcodesH264ToH265AndReportsProgress` has failed for three
different reasons, one of which was the whole suite going down around it.
## The modes
| # | mode | signature | where | disposition |
|---|---|---|---|---|
| 1 | SAF picker will not close | `the system picker would not close: after 4 back presses ...` | 37 only, since #96 | **#108** — collateral of the gralloc abort |
| 1b | picker never showed, from the rotation test | `never showed BySelector [PKG=...], in 3 separate pickers` | 33, 34 — 3 times | #268/#269; no gating attempt on `main` since |
| 2 | wedge | gradle never returns; leg killed at `WEDGE_TIMEOUT`; `wedged: yes` in the shape row | 33/34 only | **#122**, addressed by #219 — see below |
| 3 | emulator never came up | `adb ... failed with exit code 224`, before any test | 37 only, 3 times | infra, before the suite; nothing to attribute |
| 4 | Media3 export watchdog | `ExportException: Muxer error` / `no output sample written in the last 25000 milliseconds` | 34, 36 | **environmental, measured** — see below |
| 5 | `system_server` gone mid-suite | `Can't find service: package`, `am get-current-user` fails, `INSTRUMENTATION_ABORTED` | 37 only | **#108** — `hasReadColorBufferDma` |
| 6 | app Activity destroyed under the SAF save tests | `NullPointerException: Cannot run onActivity since Activity has been destroyed already` | 35, once | **fixed** — see below |
**#96 held, and mode 1 is worth stating as a number rather than a memory.**
`pickingAFileThroughTheSystemPickerFillsInTheFileCard` — the test #93 and #96 were about — has
failed **zero times on API 33-36 in the 881 gating leg-attempts since #96 merged**. Every remaining
failure of that class on those four rows is a *different* test: three of the rotation test (1b) and
seven of the two save tests (mode 6). The picker mode is an API 37 mode now.
Background noise that is **not** a mode on its own: `Failed to find ColorBuffer: N` and `bad color
buffer handle N` never name anything in this app and appear on green legs. Measured over 12 green
gating legs sampled from 2026-09-02 onwards, all reporting `failed: 0`: `bad color buffer handle`
in **6** of them, `Failed to find ColorBuffer` in **2**. Neither is evidence of anything on its own.
## Mode 4 — the Media3 export watchdog is the emulator's codec HAL segfaulting
**This is the mode #102 was filed for, and it is not starvation.** The per-test logcat in
`e2e-report-api34` of run `34000816016` attempt 1, 62 ms after the test starts:
```
00:20:39.814 D MediaCodec: MediaCodec::reclaim(...) c2.goldfish.h264.decoder
00:20:39.822 F DEBUG : Cmdline: /vendor/bin/hw/android.hardware.media.c2@1.0-service-goldfish
00:20:39.822 F DEBUG : signal 0 (SIGSEGV), code 1 (SEGV_MAPERR)
00:20:39.822 F DEBUG : Cause: null pointer dereference
#00 C2Block2D::handle() const+4 libcodec2_vndk.so
#01 getClientUsage(std::shared_ptr<C2BlockPool> const&) libcodec2_goldfish_common.so
#02 android::C2GoldfishAvcDec::process(...) libcodec2_goldfish_avcdec.so
00:20:39.839 E CCodec : Codec2 component "c2.goldfish.h264.decoder" died.
00:20:39.846 E MediaCodec: Codec reported err 0xffffffe0/DEAD_OBJECT
```
The decoder HAL process dies and respawns. Media3 is left with a dead codec, writes no output
sample, and its own 25-second export watchdog aborts the export — which is the `Muxer error` the
job log shows. **The crashing code is `/vendor/lib64/*` inside the system image**, so this is
environmental in the same sense `@FailsOnEmulatorApi37` is, and now with the same kind of evidence.
**Six for six.** Every leg-attempt that has failed this way carries the crash in the same job's
`--- native crashes (tail 60) ---` dump. **Grep `c2@1.0-service-goldfish` and not the friendlier
line**: `Codec2 component "c2.goldfish.h264.decoder" died` is a `CCodec` message in the main
buffer and is in **none** of the six job logs, because that dump is `adb logcat -d -b crash` and
what reaches it is the tombstone, whose `Cmdline:` names the HAL. The six are
`32855014836` a1 (36), `32857067112` a1 (34),
`32919928048` a1 (36), `33261618358` a1 (34), `33588264439` a1 (36), `34000816016` a1 (34).
**Six in 1210 API 33-36 leg-attempts — 0.5%**, split 3 on API 34 and 3 on API 36, none on 33 or 35.
**It is the same weakness the API 37 marker names.** `FailsOnEmulatorApi37`'s stated reason is that
Media3 transcodes "fail inside the emulator's own `c2.goldfish.h264.decoder`". That is this HAL.
One weakness, deterministic on the android-37 images and 0.5% below them.
**What is not settled:** *why* it dereferences null. `MediaCodec::reclaim` is logged 8 ms earlier,
and a reclaim is the resource manager taking a codec instance away — so "a reclaim races
`C2GoldfishAvcDec::process` and the block pool goes out under it" is the obvious hypothesis and is
**untested**. Recorded as a hypothesis, not as a cause.
**Two failures of that test are excluded and it matters that they are.** `32545625459` a1 (API 37)
had 37 tests fail together with the gralloc assertion present — that is mode 5, and this test was
collateral. `32669190757` a1 (API 35) predates #111's shape report and carries a bare `FAILED`
marker with no message at all; it is **unclassifiable, and is not classified**.
## Mode 6 — the back press that finished `MainActivity`
Traced on the API 35 gating leg of run `34161043035` **attempt 1**, whose head is #269's own
commit:
```
20:59:35.689 UiObject2: Clicking on (927, 2274) <- iteration 2's dismissal, on button1
20:59:36.033 MainActivity RESUMED <- the picker is gone, by the test's own hand
20:59:36.350 VRI[PickActivity]: visibilityChanged ... newVisibility=false
20:59:37.068 UiDevice: Pressing back button. <- iteration 2 presses anyway
20:59:41.094 UiDevice: Retrieving node ... [RES='android:id/aerr_wait']
20:59:41.169 Input channel 'Application Not Responding: ...nexuslauncher' was disposed
20:59:41.713 UiDevice: Pressing back button. <- iteration 3
20:59:41.754 TopTaskTracker: onTaskMovedToFront: ... NexusLauncherActivity
20:59:42.278 MainActivity DESTROYED
```
`dismissThePicker` guarded its back presses on `Activity.hasWindowFocus`. A system app-error dialog
is a fullscreen `system_server` window, so **it makes that false too** — the guard could not tell
"the picker is still up" from "a dialog is on top of an app that is already in front".
**And the first two lines are the part to read carefully, because the obvious reading is wrong.**
The picker did not close on its own: `dismissASystemErrorDialog` closed it on iteration 2, by
falling through to `android:id/button1` and clicking DocumentsUI's own positive button (#271). From
`20:59:36.033` there was nothing to back out of — and the loop pressed back on iteration 2 anyway,
then dismissed the launcher's ANR dialog on iteration 3 and pressed again on the reading taken
before doing so. That press finished `MainActivity`. **So this mode and #271 are one incident**, and
the fix stops it at iteration 2, where the re-read now returns.
Fixed by re-reading the focus after a dialog is actually dismissed, and only then —
`SafPickerRoundTripTest.dismissThePicker` carries the trace. That **removes** a press sent on a
stale reading rather than retrying one, and a picker genuinely in front still fails there.
**It cannot be demonstrated by re-running.** The launcher ANR is ambient and not reproducible on
demand, so a green sweep is not evidence for this fix; the trace is.
### The other six failures of those save tests are four different things
Filed as one mode, they are not one: seven occurrences, **five distinct messages** counting the
destroy above. Three of the six below are on heads that predate their own follow-up fix, and one is
on a head that **contains** the fix meant for it. This is the worked example for "split by message
before diagnosing". **The two rows still open are #270**; the `button1` finding below is **#271**.
| run / attempt | head | message | what it is |
|---|---|---|---|
| `34041593697` a1 (35) | `fa10d94` — the commit that **added** the test | `No compose hierarchies found`, thrown directly | pre-`b23ff0f` |
| `34056545386` a1 (35) | `cbbaf74` | `ComposeTimeoutException ... after 120000 ms` | the `CONVERSION_TIMEOUT_MS` case `19e3539` fixed. **Not a system-service failure at all** — API 35's software encode measured 134.8 s against a 120 s bound |
| `34057706195` a1 (34) | `19e3539` | `No compose hierarchies found` ×2 | **contains `b23ff0f`**, so that fix did not close this shape. Open |
| `34067653670` a1 (35), `34146936252` a1 (35) | `d45abe7`, `73482520` | `waited 300000ms for a node tagged action.saveFile`, **no** composition error | `awaitNode` appends the composition error only when `fetchSemanticsNodes` threw, so the composition was readable throughout. `34067653670`'s per-test logcat has `MainActivity` `RESUMED` for the whole 300 s and **no conversion running at all**. Open |
| `34146936252` a2 (35) | `73482520` | `waited 300000ms ...; last composition error: No compose hierarchies` | app `PAUSED` and never resumed; the back press at `17:38:32.479` follows `Waiting 5000ms for ... permissioncontroller`, the permission-dialog helper #269 replaced with `pm grant` |
### And the dismissal can click a dialog that is not a system dialog (#271)
In the same trace, at `20:59:35.689`, `aerr_wait` and `aerr_close` both missed and
`android:id/button1` — the framework's generic `AlertDialog` positive button, present on every
`AlertDialog` on the device — was found and clicked. `MainActivity` came back 339 ms later and
`PickActivity`'s window went away with it, so what was clicked was a button inside **DocumentsUI's
own create-document flow**. It did no harm on that run. Filed rather than fixed here, because
narrowing the selector is a decision about what `dismissASystemErrorDialog` may reach.
## Mode 2 — the wedge, and why this says "consistent with" rather than "fixed"
Nine occurrences, **all on API 33/34**, 9 in 497 leg-attempts before 2026-09-06T02:00Z — **1.8%**
— and **0 in the 106 since**. The last one, `34001741668` (2026-09-06T00:36), is
`thePickedInputSurvivesARealRotation` again, and `git merge-base --is-ancestor 32ab54d <head>` says
that head **does not contain** #219's fix, so no wedge has ever been recorded against the fix.
At the prior rate, P(0 in 106) ≈ 0.15. **That is suggestive and it is not evidence.** Re-count
before writing "fixed" here.
Its diagnostics say the framework is fine, which is what separates it from every other mode in this
document: `e2e-wedge-api34` of `34001741668` has `started:` the rotation test with no `finished:`,
and `input`, `window`, `activity` and `media.player` all `found`.
## Reading the evidence, when the leg is already gone
Four things that are not obvious and each cost a wrong answer:
- **A re-run destroys the log.** `gh run view --job <id> --log` resolves by *run* and serves the
latest attempt, so after a re-run to green it hands back a green log for a red attempt. Use
`gh api --allow-escape-sequences /repos/{owner}/{repo}/actions/jobs/{job_id}/logs`, with the job
id from `/actions/runs/{run}/attempts/{n}/jobs`. Without `--allow-escape-sequences`, `gh` writes
nothing and exits 0.
- **A re-run does *not* destroy the artifacts, but the convenient command hides them.**
`/actions/runs/{run}/artifacts` returns every attempt's upload under the same name with different
ids and `created_at`; `gh run download` takes the newest, which after a re-run-to-green is the
green one. Match `created_at` to the attempt's window and fetch
`/actions/artifacts/{id}/zip`.
- **The per-test logcat is the evidence, not the job log.** `e2e-report-apiNN` carries
`outputs/androidTest-results/connected/debug/<device>/logcat-<class>-<method>.txt` — one file per
test, scoped to that test's window — plus the JUnit XML with the untruncated stack. The job log
truncates a stack to its first frame, which is why the `ActivityScenario` frames in mode 6 are
invisible there.
- **Grep the fault, not the thread.** #102 once split one bug into two by grepping
`TaskSnapshotPer` — a thread name from a ticket title — instead of `hasReadColorBufferDma`, the
assertion. The assertion is the invariant; the thread is only which caller tripped it.
## What this does not cover
The advisory `E2E API 37 Media3 hardware transcode (advisory)` job is **red on every PR by design**
and is not a signal. `docs/api-37-emulator-crash.md` has API 37's own story;
`.github/scripts/e2e-report-shape.sh` explains the shape table every leg prints.
+82 -7
View File
@@ -1,9 +1,11 @@
# Coverage-read findings
**Status:** ten findings, none fixed, none urgent. F1-F4 came from the 2026-08-26 read; F5 was added
on 2026-08-27 while decomposing #132; **F6-F10 were added on 2026-09-02 from the wave-4 read**. Every
entry here is a *code* observation — something a test would document rather than repair. The test
gaps found in the same reads are tickets, not entries here; see [Not covered here](#not-covered-here).
**Status:** ten findings; **F1 is closed — by #254 on 2026-09-06, which found it was a defect rather
than the dead arm it was filed as** — and the other nine stand, none urgent. F1-F4 came from the
2026-08-26 read; F5 was added on 2026-08-27 while decomposing #132; **F6-F10 were added on
2026-09-02 from the wave-4 read**. Every entry here is a *code* observation — something a test
would document rather than repair. The test gaps found in the same reads are tickets, not entries
here; see [Not covered here](#not-covered-here).
**Scope:** what a JaCoCo read turned up that writing a test would not fix. This is a survey, not a
work order. Acting on any entry is a separate decision and would be its own commit.
**Last verified:** `main` at `54ca2dd`, 2026-09-02. Coverage measured that day with
@@ -40,7 +42,9 @@ Same vocabulary as `defect-audit.md`, deliberately, so the two read alike:
- **No action** — recorded because it looks like a finding and is not.
Nothing below was observed on a device, and nothing below needs to be: every entry is a claim about
what the code says, checkable by reading it.
what the code says, checkable by reading it. **F1's resolution is the exception, and it had to be**:
what that entry turned on — whether the encoder it named exists in the shipped binary — is not
readable from the source at all.
---
@@ -110,6 +114,56 @@ files agree and to say so in one place.
2. Correct the `ContainerCapabilities.kt:84` comment, which is false as written, and give the
`FFmpegCommandBuilder` arm the treatment `Media3Engine.kt:221-233` already models.
### Resolved 2026-09-06 (#254) — and the arm was not merely unreached, it was unrunnable
Vorbis is now in `ENCODABLE_AUDIO`, `OutputFormat.OGG_VORBIS` is a one-tap preset beside `OPUS`,
and `FFmpegEngineTest.encodesOggVorbisThroughAnEncoderTheBundledBinaryActuallyHas` asserts the
produced track's MIME and its channel count. The false comment is gone.
**The finding this entry did not have is that `-c:a libvorbis` could never have worked.** Three
independent sources agree and none of them is the coverage report:
| source | says |
|---|---|
| `bin/README.md`'s configure line, read back out of the shipped `libavutil.so` | `--enable-libopus`, `--enable-libmp3lame`, `--enable-libvpx`, `--enable-libx264/5`, `--enable-libdav1d`, `--enable-libsvtav1`, `--enable-libjxl` — **no `--enable-libvorbis`** |
| `tools/ffmpeg/build-ffmpeg.sh` | neither `COMMON_LIBS` nor `EXTRA_LIBS` names it |
| `strings` on `jni/x86_64/libavcodec.so` | the `lib*` encoder names present are `libdav1d libjxl libmp3lame libopus libsvtav1 libvpx libx264 libx265`. `libvorbis` is absent; `libavcodec/vorbisenc.c` is present |
So the first user to pick Ogg Vorbis would have got `Unknown encoder 'libvorbis'`. The arm was
*wrong*, not just dead — and **nothing short of building the command and running it could have
found that**, which is why the e2e half of this ticket is the load-bearing half. It is #238's shape
again: two covered facts (a builder arm, a configure line) that no test put together.
**The AAR was rebuilt rather than the arm rewritten, and the measurements are why.** A first pass
at this ticket implemented Vorbis on FFmpeg's in-tree `vorbisenc.c`, which the binary already had.
It works, and it is not good enough to sit in a picker beside MP3, FLAC and Opus:
| | `libvorbis` | in-tree `vorbis` |
|---|---|---|
| experimental gate | none | **needs `-strict experimental`** |
| channels | mono, stereo, surround | **stereo only** |
| `-q:a 0..10`, one 3 s clip | 10931 -> 64166 bytes | 7549 -> 14645 bytes |
`AV_CODEC_CAP_EXPERIMENTAL` is upstream FFmpeg saying *do not ship this by accident*. The
stereo limit forces `-ac 2`, so a mono source is silently upmixed — and **this repo's own fixture,
`sample_h264.mp4`, is mono**, so the compromise was not hypothetical. And a quality knob spanning
2x its floor against libvorbis's 6x has nowhere to go: libvorbis at `-q:a 5` writes 16429 bytes of
that clip, more than the in-tree encoder produces at q10.
So #254 added `--enable-libvorbis` to `tools/ffmpeg/build-ffmpeg.sh` and rebuilt: a new ~35 MB blob
in git history permanently, a new configure line and SHA-256 in `bin/README.md`. What that bought
is the arm as originally written — `-c:a libvorbis -q:a 5`, no experimental gate, no forced
channel count — and mono that stays mono, which the e2e test asserts alongside the track MIME.
Two things about the flag are worth keeping, because both are ways to get this wrong quietly.
ffmpeg-kit's `--enable-*` names come from its own `get_library_name()` and are not FFmpeg's — it is
`--enable-lame` for libmp3lame and `--enable-opus` for libopus — so `--enable-vorbis` is the
plausible guess and it is **wrong**; id 9 is literally `libvorbis`, so `--enable-libvorbis` is
right, and it pulls libogg in with it. And ffmpeg-kit does **not** error on an unrecognised
`--enable-*`, so a rebuild that quietly omitted the library looks exactly like one that worked.
`strings jni/*/libavcodec.so | grep -x libvorbis` and the e2e test are the only two things that
tell those apart.
---
## F2 — `ConversionRequest.hardwareEncodeAvailable` is written, read by nothing, and its KDoc describes behaviour that was removed
@@ -403,6 +457,27 @@ They are still correct to keep: `ForegroundInfo` is required by the `CoroutineWo
named — a `getForegroundInfo` that starts branching — plus one more: the day anything calls
`setExpedited`.
**Updated 2026-09-06 (#252, and the sentence above is half wrong).** "WorkManager calls
`getForegroundInfoAsync()` only for expedited work" is true and *not sufficient*, and the missing
half is what made the reopening trigger wrong. `WorkForeground.kt:38` in work-runtime 2.11.2 opens
the library's only caller with
```kotlin
if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return
```
and `minSdk` is 33. So calling `setExpedited` reopens nothing: on **every** device this app
supports, WorkManager does not consult `getForegroundInfo()` whether the work is expedited or not.
#252 was filed on the trigger as this entry stated it, and its acceptance criterion — "a request
now carries `setExpedited` and the existing worker tests drive them" — cannot be met that way.
What made the lines live instead was that each worker held **two** definitions of one notification:
the override, and an identical `ForegroundInfo` built inline in `doWork`. `doWork` now posts the
override's, so the duplicate is gone and what remains runs on every job. The general lesson is the
one E1 states from the other side: *check that the mechanism you are relying on actually fires on
the machine that runs it* — here the mechanism was a library early-return two source lines long,
and four waves of reading had taken the API summary's word for it.
---
## F10 — Three arms that are reachable, uncovered, and cannot be made to bite
@@ -444,7 +519,7 @@ the cheaper order.
| ID | Finding | Severity | Evidence | Action |
|---|---|---|---|---|
| F1 | `FFmpegCommandBuilder` emits a Vorbis encoder `ContainerCapabilities` says does not exist | low | confirmed by inspection; unreachability traced through four call sites | **decide**: feature or dead arm — the comment is false either way |
| F1 | `FFmpegCommandBuilder` emits a Vorbis encoder `ContainerCapabilities` says does not exist | low → **the severity was wrong** | confirmed by inspection; unreachability traced through four call sites | **closed #254 as a feature** — and the encoder it named is not in the shipped binary, so the arm could never have run |
| F2 | `hardwareEncodeAvailable` written, never read; KDoc describes removed behaviour | low | confirmed by inspection; `FFmpegCommandBuilderTest:132` corroborates | **decide**: delete or mark vestigial |
| F3 | `ConversionRequest.videoCodec` / `.audioCodec` have no callers | low | confirmed by inspection | delete, or keep for symmetry — **not** a test gap |
| F4 | Two private guards reachable only by direct call | n/a | confirmed by inspection | **no action** — named exemption, per #88 |
@@ -452,7 +527,7 @@ the cheaper order.
| F6 | Four more unreachable arms; `ConversionRouter:214-217`'s KDoc is false | low | confirmed by inspection; each traced to its upstream guard | **no action**, except the one-line KDoc fix |
| F7 | `probeWithExtractor`'s catch is unreachable, as `probeForConcat`'s is | n/a | measured across four URI shapes (recorded in `CLAUDE.md`) | **no action** — device-only, now written down for both sites |
| F8 | Three more dead members and six unused defaults | low | confirmed by inspection; grep per member | delete or keep knowingly — **not** a test gap |
| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **no action** — sharpens #88's close |
| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **closed 2026-09-06 by #252** — and its stated reopening trigger was wrong; see the update on the entry |
| F10 | Three reachable arms where no mutation bites | n/a | confirmed by inspection; each mutation traced to its masking guard | **no action** — recorded to stop the next read re-picking them |
Order, if these are acted on: **F1 and F5 first, separately.** They are the two with a possible
+105 -2
View File
@@ -495,12 +495,12 @@ Every one was read. **None of them is an e2e test gap**, which is the result:
| lines | where | classification |
|---|---|---|
| 9 | `Transcoders` ×3, `ConversionViewModel`, `ConverterScreen`, `JoinViewModel`, `JoinScreen`, `MainActivity`, `Reattachment` | **compiler-generated** — default-arg `$default` bridges, coroutine completion, the synthetic `NoWhenBranchMatchedException` arm of a `when` over `Destination` |
| 10 | `ConversionWorker:342-346`, `ConcatWorker:132-136` | `getForegroundInfo()` — WorkManager's **expedited-work** hook, and nothing here enqueues expedited work. The live path is `setForeground(foregroundInfo(...))`, which is covered. **#252** |
| 10 | `ConversionWorker:342-346`, `ConcatWorker:132-136` | `getForegroundInfo()` — WorkManager's **expedited-work** hook, and nothing here enqueues expedited work. The live path is `setForeground(foregroundInfo(...))`, which is covered. **#252 — closed 2026-09-06, and not the way this row expects.** Expedited work is now enqueued, but that is *not* what covers these lines: `WorkForeground.kt:38` returns before the hook whenever `SDK_INT >= 31`, and `minSdk` is 33. What covers them is `doWork` posting the override instead of a second copy of the same notification. See `coverage-read-findings.md` F9's update |
| 3 | `ConversionNotifications:60-62` | **F5** — `areEnabled()` has no callers. Already on record |
| 3 | `CopyPlanner:28`, `OutputFormat:222-223` | public members with no callers. **#253**, with F5 |
| 3 | `MediaProbe:210-212` | `probeWithFFprobe`'s `catch` — **F7's sibling, and now measured**. See below |
| 2 | `FFmpegCommandBuilder:167-168` | `COPY`/`NONE -> error(...)` — F4-shaped, deliberately exempt |
| 1 | `FFmpegCommandBuilder:188` | the `VORBIS` encode arm. No `OutputFormat` produces it, but `ContainerCapabilities` lists it for WEBM and OGG. **#254** |
| 1 | `FFmpegCommandBuilder:188` | the `VORBIS` encode arm. No `OutputFormat` produced it, but `ContainerCapabilities` listed it for WEBM and OGG. **#254 — closed, and it was the row that turned out to be a defect**: the arm named `libvorbis`, which was not compiled into the shipped AAR at all, so it could never have run. Closing it meant rebuilding the AAR with `--enable-libvorbis`, not editing the arm. See F1 in `coverage-read-findings.md` |
| 1 | `ConversionWorker:231` | `?: error("Could not open the input file.")`. `UnopenableUriTest` fails the job *downstream* of it, so the elvis is unprovoked — F4-shaped, same as the two above |
**`MediaProbe:210-212` is the one that gained a measurement.** F7 ruled `probeWithExtractor`'s catch
@@ -515,3 +515,106 @@ assumed.
**The reusable part**: a union report is what separates "no test calls this" from "only a device
calls it", and neither report alone can. Six of the eight rows above were indistinguishable from
real gaps in the JVM-only number.
---
## E9 — the branch tier of the same union, classified
**Severity: n/a · Measured 2026-09-07 at `c2cc9e2` · the half E8 stopped short of**
E8 classified the 32 lines neither suite executes and stopped there. It never asked the other
question a union can answer: which *arms* does neither suite take, on lines both suites run? That
tier had never been read, and it is where what is left actually lives.
**Line numbers below are as of `c2cc9e2`**, and #261 rewrites four of these files. Every row names
the expression beside the number for that reason — E8's own refs shifted under #252 within a day.
### How this was measured, and why it is not E8's report
E8's union was built once and kept as an artifact; no Gradle task produces one, because a connected
run only emits an `.ec` with `enableAndroidTestCoverage` set by hand. This read rebuilt it: a fresh
`:app:jacocoTestReport` merged with **E8's own API 34 `.ec`**, against one set of current class
files, through a scratchpad init script. Union: **99.0% line (2352/2375), 90.1% branch
(1206/1338)**; JVM alone 94.6% / 87.6%.
Controls, because a silently-rejected `.ec` looks exactly like a well-covered codebase: the device
half contributes 32 lines in `FFmpegEngine`, 24 in `Media3Engine`, 15 in `ConcatEngine` and 9 in
`MainActivity` that the JVM suite never reaches. It applied.
**Two classes are the exception, and the bound matters more than the exception.** #252 changed
`ConversionWorker` and `ConcatWorker`, so JaCoCo rejected E8's `.ec` for exactly those two — a class
is matched by a hash of its bytecode. E8's measurement says the device contributed **1** unique line
in `ConversionWorker` and **0** in `ConcatWorker`, so the blind spot is one line wide. It is
`ConversionWorker:252`, `getSafParameterForRead` — old line 228, and the one device-only line in
that file. It shows as never-executed here and **is not a gap**; #252's own commit message records
an instrumented API 34 pass on the new bytecode.
### The filter, named once
**`mi == 0 && mb > 0`** — a *fully* executed line carrying an arm nothing takes.
`CLAUDE.md` describes its second filter as `ci > 0 && mb > 0` at method level and reports **18**
lines from wave 4. That figure reproduces exactly under `mi == 0` (19 branches on 18 lines) and not
under `ci > 0`, which admits partially-executed signature lines and gives 139. The two are different
metrics, not a stale number and a correction — worth stating because the difference looks like drift
and is not.
### The artefact E8 flagged is retired
E8 warned that the union's branch denominator ran 16 ahead of the JVM's, "entirely inside
`MediaProbe`", and told readers not to quote a MediaProbe branch figure raw. Rebuilt, both
denominators are **1338**, and `MediaProbe:321` (`matroskaOrWebm`) reads `mb=0 cb=4` — fully
covered, against `mb=11` of 20 before.
**Stated as measured, because this entry is about a number that was quoted past its evidence.** That
one line accounts for a 16-branch difference, and the two denominators now agree. The remaining
lines were *not* enumerated in both reports, so read that as consistent with the whole gap sitting
at `:321` rather than as proof that nothing moved elsewhere. Either way the difference is an
artefact of how the report was constructed and not a property of the code, so E8's caveat is
withdrawn rather than carried forward: `matroskaOrWebm` is not, and never was, a gap.
### The result: 23 arms on 22 lines, and 12 of the 22 are decided here
Tier 1 is now **22** lines, down from 32: #252 closed the ten `getForegroundInfo` lines and added no
new one. Tier 2 did not move.
**Decided — no ticket.** Recorded here rather than as new F-entries, following E8's precedent and
because `coverage-read-findings.md` is being rewritten by #261. **A JVM-only read that flags any of
these should look here before re-filing them.**
| site | expression | why it is decided |
|---|---|---|
| `FFmpegCommandBuilder:113` | `when (codec)` in `encodeVideo` | the missed arm is the `COPY`/`NONE` pair whose body at `:167-168` is already Tier 1 and F4-exempt. One arm counted in two tiers |
| `FFmpegCommandBuilder:183` | `when (audio.codec)` | the `VORBIS` arm — **in flight**, see below |
| `ContainerCapabilities:277` | `?.let(::add)` | F6-shaped. `a?.let{b}?.let(::add)` reaches this branch only when the *lambda* returned null — `firstContainerHolding` finding no carrier. `VIDEO_ALIASES` targets are exactly H264, H265, VP8, VP9, AV1, and MKV carries all five, so it never does. A null at `:275` jumps past this line entirely |
| `ConversionViewModel:405` | `!is Idle \|\| activeWorkId != null` | F10-shaped, and the line's own comment says so: `ScreenOwnership`'s token is what holds the line. Delete the second half and the suite stays green, correctly |
| `ConverterScreen:362`, `JoinScreen:245` | `is Failed -> {` | the last arm of its `when` over a sealed state, so the missed branch is the synthetic `NoWhenBranchMatchedException` — compiler-generated |
| `ConverterScreen:91` | `) { viewModel.convert() }` | the `rememberLauncherForActivityResult` callback; Compose codegen, the shape `CLAUDE.md` already names at `JoinScreen:222` |
| `AndroidDeviceCodecs:52` | `cached ?: synchronized(this) { cached ?: … }` | double-checked locking's **inner** re-check. Reaching it needs two threads racing the same first call; a seam does not create one |
| `ConversionWorker:319` | `e is CancellationException \|\| isStopped` | the `isStopped` half — WorkManager stopping a worker mid-run. Device-only |
| `Media3Engine:83`, `:153`, `:157` | `if (cont.isActive)` ×3 | cancellation racing completion inside the Transformer listener. Device-only and inherently racy; a test that pinned it would be pinning a scheduler |
**Filed — 10 sites, 5 tickets.** Each names the mutation that must go red, or says the read *is* the
ticket where it cannot yet:
| ticket | sites | what |
|---|---|---|
| **#262** | `MediaProbe:303, :305, :306, :309` | `containerFrom`'s alias arms. `names` is `getFormat().split(',')` and ffprobe reports a demuxer *group* (`"mov,mp4,m4a,3gp,3g2,mj2"`), so the second half of each `\|\|` may be dead by construction. Per-site read. Carries `:312` (`aac`/`adts`) as the one that looks like a real fixture gap: the only AAC fixture is `sample_aac.m4a`, which matches `:305` and never reaches it |
| **#263** | `MediaProbe:386` | the `audio != null` arm — no fixture has two audio tracks, so the guard that makes "first track wins" true is unasserted. E1's shape |
| **#264** | `ConcatStrategy:57`, `ContainerCapabilities:122`, `OutputFormat:103` | three pure `model`-layer decision arms a test can call directly: the dimension check's height half, an image spec carrying a codec, and `isPureRemux`'s all-`NONE` case |
| **#265** | `FFmpegEngine:67` | `if (durationMs > 0)`'s false arm. `MediaProbe` returns `0` when it cannot read a duration, so this is a real input, not a second line of defence |
| **#266** | `FFmpegConcatCommand:95` | the non-MP4 concat output. Reachability depends on what the join UI offers — read that first; F4-shaped if it offers only MP4 |
### One row is being closed while this was written
`FFmpegCommandBuilder:183`'s missed arm is `VORBIS`, which is #254 — open as **#261**, which rebuilds
the AAR with `--enable-libvorbis` and makes the arm reachable. **The set is 21 arms on 21 lines the
day that merges**, and both tiers want re-deriving then rather than editing this sentence.
### The reusable part
E8's lesson was that a union separates "no test calls this" from "only a device calls it". This
tier's is narrower and less comfortable: **once the never-executed lines are gone, what is left is
mostly not a test gap at all** — 12 of 22 sites are compiler codegen, a documented exemption, or a
race, and they are indistinguishable from real gaps in any report. The five tickets are what
survived reading all 22, and three of them are reads rather than tests.
+19 -4
View File
@@ -60,11 +60,22 @@ Only `arm64-v8a` and `x86_64` are built, matching the app's `abiFilters`. Droppi
## Library selection
Flag names come from `get_library_name()` in the upstream `scripts/function.sh`. Two
Flag names come from `get_library_name()` in the upstream `scripts/function.sh`. Three
that are easy to get wrong:
- It is **`--enable-lame`**, not `--enable-libmp3lame`.
- It is **`--enable-libsvtav1`** for SVT-AV1.
- It *is* **`--enable-libvorbis`** — the rule above makes `--enable-vorbis` the natural
guess and it is wrong. Read the function rather than extrapolating from the first two;
library 9 is named `libvorbis` there. Enabling it also enables libogg, which ffmpeg-kit
pulls in as its dependency without being asked.
**An unrecognised `--enable-*` is ignored silently.** ffmpeg-kit does not error on one, so a
build that quietly dropped a library looks exactly like one that worked, and forty minutes
later there is an AAR that is wrong in a way nothing in the log says. #254 is where that
was learned, from the other end: the builder carried `-c:a libvorbis` for months against a
binary with no libvorbis in it — unreachable, so no user ever hit it, and no build log ever
mentioned it. Check the artifact, not the log — `strings jni/*/libavcodec.so | grep -x <name>`.
MP3 deserves a note: **Android has no MP3 encoder at any API level**. That is a platform
gap, not a Media3 limitation, so `--enable-lame` is the only way the app can output MP3.
@@ -100,11 +111,15 @@ and `x86_64`. Confirmed against the artifact rather than assumed:
`--enable-gpl --enable-version3 --enable-libx264 --enable-libx265 --enable-libsvtav1
--enable-libvpx --enable-libmp3lame --enable-libopus --enable-libdav1d --enable-libass
--enable-libfontconfig --enable-libfreetype --enable-libfribidi --enable-libharfbuzz
--enable-mediacodec --enable-jni --enable-shared --enable-small --enable-lto`
--enable-mediacodec --enable-jni --enable-shared --enable-small --enable-lto`.
**Since 2026-09-06 it also carries `--enable-libvorbis`** (#254), which is the only
difference between that build and the one in `bin/` today — same tag, same FFmpeg
version, same 10 shared libraries per ABI, all still `LOAD align 0x4000`.
- Present and verified: `libx264` (with an x264 core banner, so genuinely linked),
`libx265`, `libsvtav1`, `libmp3lame`, `h264_mediacodec`, `hevc_mediacodec`, `libopus`,
`libdav1d`, the GIF encoder and muxer, libass internals (`ass_shaper_new`), and the
`subtitles`, `scale`, `palettegen`, `paletteuse` and `concat` filters.
`libdav1d`, `libvorbis` (from 2026-09-06), the GIF encoder and muxer, libass internals
(`ass_shaper_new`), and the `subtitles`, `scale`, `palettegen`, `paletteuse` and
`concat` filters.
Note `--enable-version3`: combined with `--enable-gpl` this makes the binary **GPL-3.0**,
which is what `LICENSES/README.md` states.
+9 -1
View File
@@ -28,7 +28,10 @@ OUT=/work/out
# Library selection
# ---------------------------------------------------------------------------
# Flag names come from get_library_name() in scripts/function.sh — note it is
# --enable-lame, NOT --enable-libmp3lame.
# --enable-lame, NOT --enable-libmp3lame. Read that function before adding one: the
# names are ffmpeg-kit's, not FFmpeg's, and they agree only sometimes. libvorbis is
# one that does agree (id 9 is literally "libvorbis"), so --enable-libvorbis is right
# and the --enable-vorbis this rule would predict is not.
#
# android-media-codec gives FFmpeg the h264_mediacodec / hevc_mediacodec wrappers.
# Those are the fallback-within-the-fallback: hardware encode from the FFmpeg side
@@ -41,6 +44,11 @@ COMMON_LIBS=(
--enable-lame # MP3 encode. Android has NO MP3 encoder at any API level,
# so this is the only way the app can output MP3 at all.
--enable-opus
--enable-libvorbis # Ogg Vorbis encode. Android has no Vorbis ENCODER at any API
# level either, and FFmpeg's own in-tree vorbis encoder is
# experimental, stereo-only and barely responds to -q:a, so
# this is the only usable route. Pulls libogg in as its
# dependency (ffmpeg-kit sets LIBRARY_LIBOGG with it).
--enable-dav1d # fast AV1 decode
)
+249
View File
@@ -0,0 +1,249 @@
#!/usr/bin/env bash
#
# The local gate: what has to be green before a commit is made or a branch is pushed.
#
# THE RULE THIS ENFORCES (2026-09-06). Source changes must have the unit tests AND the
# instrumented tests passing at every supported API level before they are committed or
# pushed; test changes must have the whole suite passing at every API level. CI is not the
# place to find out. Four legs of this repo's history were spent discovering on CI what a
# local sweep would have said in twenty minutes -- and worse, the failing leg MOVED between
# runs (API 35 red then green, API 34 green then red), which is exactly the signal that gets
# misread as "someone else's flake" when it is read one leg at a time.
#
# WHY BOTH HOOKS RUN THE SAME GATE. A pre-commit-only gate is bypassed by amending; a
# pre-push-only gate lets a broken commit exist locally and get rebased into something else.
# Running both is not redundant in practice because of the cache below.
#
# THE CACHE IS KEYED ON CONTENT, NOT ON TIME, AND ON THE RIGHT CONTENT. The sweep is recorded
# under the hash of the `app/src` SUBTREE it verified, not the whole repo tree. Keying it on the
# whole tree was the first cut and it was wrong in a way that would have trained people to hate
# this hook: editing a comment in CLAUDE.md, or in this script, invalidated a sweep of identical
# application code and re-ran forty minutes of emulators to prove nothing. What the sweep is
# evidence about is `app/src`; that is what it is filed under. Any change to a single byte under
# `app/src` still invalidates it. The JVM gate is cheap and runs unconditionally.
#
# WHAT COUNTS AS "EVERY SUPPORTED API LEVEL", AND WHY 37 IS NOT AN EMULATOR HERE. 33, 34, 35
# and 36 run the whole suite on emulators. **API 37 cannot be run on an emulator on this host at
# all** -- not "is red", cannot run: measured 2026-09-06, the image logs
# `3 new surfaceflinger aborts in 45 s (want 0)` and then the APK install itself fails with
# `Can't find service: package`, because the framework is already gone before Gradle gets to
# install anything. `Starting 0 tests`. That is the same gralloc abort docs/api-37-emulator-crash.md
# measures, hit earlier in the sequence than the suite.
#
# So API 37 is covered here by the physical Pixel 10 Pro XL when it is attached, and by CI's
# gating leg otherwise. The hook says loudly which of the two happened rather than quietly
# claiming five levels when it ran four.
#
# THERE IS DELIBERATELY NO SKIP VARIABLE. An `LMC_SKIP_E2E=1` would be `--no-verify` wearing
# a different hat, and `--no-verify` needs the repo owner's say-so each time. If this gate is
# wrong, fix the gate.
set -uo pipefail
REPO_ROOT="$(git rev-parse --show-toplevel)"
cd "$REPO_ROOT" || exit 1
MODE="$(basename "$0")"
ZERO="0000000000000000000000000000000000000000"
# `git rev-parse --git-common-dir`, not a literal ".git" (#258). In a linked worktree `.git` is a
# FILE containing `gitdir: ...`, so `mkdir -p .git/lmc-verify` fails with "Not a directory" -- and
# because the write is the last thing this script does, it failed while the gate still printed
# green and exited 0. Every push from a worktree then re-swept 33-36 for nothing, silently, which
# is the worst shape a cache can fail in: invisible and expensive.
#
# --git-common-dir rather than --git-dir so the cache is SHARED across worktrees. The key is the
# app/src tree hash, and identical content is identical content whichever worktree produced it.
CACHE_DIR="$(git rev-parse --git-common-dir)/lmc-verify"
GRADLE_GATE=(:app:assembleDebug :app:testDebugUnitTest :app:compileDebugAndroidTestKotlin
:app:ktlintCheck :app:detekt :app:lintDebug)
# Says so when it cannot record, rather than leaving a cache that silently never fills (#258).
record_sweep() {
[ -n "$tree" ] || return 0
if mkdir -p "$CACHE_DIR" 2>/dev/null && : > "$CACHE_DIR/$tree" 2>/dev/null; then
return 0
fi
printf '\n\033[1m[local-gate]\033[0m could not record the sweep under %s -- it will re-run next
time. Not fatal, but it means every commit and push pays for it again.\n' "$CACHE_DIR"
}
say() { printf '\n\033[1m[local-gate]\033[0m %s\n' "$*"; }
die() {
printf '\n\033[1;31m[local-gate] BLOCKED\033[0m %s\n' "$*"
printf ' The rule: source work needs unit + e2e green at every API level before commit/push;\n'
printf ' test work needs the whole suite green at every level. Fix it, or ask before using\n'
printf ' --no-verify -- that flag is not yours to reach for unprompted.\n\n'
exit 1
}
# --- what changed, and what tree is being verified ---------------------------------------
changed_files=""
tree=""
case "$MODE" in
pre-commit)
changed_files="$(git diff --cached --name-only --diff-filter=ACMR)"
tree="$(git rev-parse "$(git write-tree):app/src" 2>/dev/null || echo "")"
;;
pre-push)
# stdin is `<local ref> <local sha> <remote ref> <remote sha>`, one line per ref pushed.
while read -r _ local_sha _ remote_sha; do
[ "$local_sha" = "$ZERO" ] && continue # branch deletion carries no content
base="$remote_sha"
if [ "$remote_sha" = "$ZERO" ]; then
# A new branch: compare against main rather than against every commit ever made.
base="$(git merge-base origin/main "$local_sha" 2>/dev/null || echo "")"
fi
if [ -n "$base" ]; then
changed_files="$changed_files$(git diff --name-only --diff-filter=ACMR "$base" "$local_sha")"$'\n'
else
changed_files="$changed_files$(git show --pretty=format: --name-only "$local_sha")"$'\n'
fi
tree="$(git rev-parse "$local_sha:app/src" 2>/dev/null || echo "")"
done
;;
*)
say "unknown hook name '$MODE'; nothing to do"
exit 0
;;
esac
if [ -z "${changed_files//[[:space:]]/}" ]; then
say "no added/modified files; nothing to verify"
exit 0
fi
touches_source=0
touches_tests=0
while IFS= read -r f; do
case "$f" in
app/src/main/*) touches_source=1 ;;
app/src/test/*|app/src/androidTest/*) touches_tests=1 ;;
esac
done <<< "$changed_files"
# --- the cheap gate always runs -----------------------------------------------------------
# --- shellcheck, at CI's exact pin ---------------------------------------------------------
# WHY THIS IS HERE. The gate ran ktlint, detekt and Android lint but not shellcheck, so a new or
# edited `.sh` file was precisely the case where this hook passed and CI's Static analysis leg
# still went red. That is not hypothetical: this script is itself a new `.sh` file, and the first
# thing it could not check was itself. It was caught by hand twice before it was caught here.
#
# THE DIGEST IS READ OUT OF status_check.yml, NOT COPIED INTO THIS FILE. shellcheck 0.9.0 and
# 0.11.0 disagree about how to report a trap handler -- SC2317 on seven body lines versus SC2329
# once on the declaration, same script, same directive, one red and one green. That disagreement
# is why CI pins by digest, and a second copy of the digest here would drift from it silently.
# When it drifts, the symptom is this gate passing and CI failing: the exact thing this section
# exists to prevent. So there is one digest in the repo and this reads it.
#
# ALL TRACKED FILES, not just changed ones, because that is what CI does -- `git ls-files '*.sh'`.
# The point is to predict that leg, not to audit the diff.
shellcheck_pin="$(grep -oE 'koalaman/shellcheck@sha256:[0-9a-f]{64}' \
.github/workflows/status_check.yml | head -1)"
# :z is podman's SELinux relabel and is what this host needs; docker on CI does without it.
runtime=""
mount=":z"
for candidate in podman docker; do
if command -v "$candidate" >/dev/null 2>&1; then
runtime="$candidate"
[ "$candidate" = "docker" ] && mount=""
break
fi
done
if [ -z "$shellcheck_pin" ]; then
say "NOT COVERED: shellcheck. Could not read the pinned digest out of
.github/workflows/status_check.yml -- if that pin moved or was reformatted, fix this grep
rather than leaving the check silently absent."
elif [ -z "$runtime" ]; then
say "NOT COVERED: shellcheck. Neither podman nor docker is on PATH, and there is no shellcheck
system package on this host. CI's Static analysis leg is what answers for .sh files then."
else
say "shellcheck ($runtime, $shellcheck_pin)"
if ! git ls-files -z '*.sh' |
xargs -0 -r "$runtime" run --rm -v "$PWD:/mnt$mount" "docker.io/$shellcheck_pin"; then
die "shellcheck failed. CI runs the same digest over the same files, so this is a red
Static analysis leg waiting to happen."
fi
fi
# --- actionlint, the half shellcheck cannot see ---------------------------------------------
# A good deal of this repo's bash lives in workflow `run:` blocks, which `git ls-files '*.sh'`
# does not match at all -- so without this a workflow edit is the same hole the section above
# just closed: green here, red on Static analysis. Pinned by digest for the reason in that
# section, and for actionlint's own: its documented install is `curl | bash` off a moving branch,
# which does not belong in a repo that pins every action by SHA.
actionlint_pin="$(grep -oE 'rhysd/actionlint@sha256:[0-9a-f]{64}' \
.github/workflows/status_check.yml | head -1)"
if [ -z "$actionlint_pin" ]; then
say "NOT COVERED: actionlint. Could not read the pinned digest out of
.github/workflows/status_check.yml -- fix this grep rather than leaving the check absent."
elif [ -z "$runtime" ]; then
say "NOT COVERED: actionlint. Neither podman nor docker is on PATH; CI's Static analysis leg
is what answers for the workflows then."
else
say "actionlint ($runtime, $actionlint_pin)"
if ! "$runtime" run --rm -v "$PWD:/repo$mount" -w /repo "docker.io/$actionlint_pin" -color; then
die "actionlint failed. CI runs the same digest over the same workflows."
fi
fi
say "$MODE: running the JVM gate"
if ! ./gradlew "${GRADLE_GATE[@]}" --continue; then
die "the JVM gate failed (assemble, unit tests, androidTest compile, ktlint, detekt, lint)."
fi
# --- the sweep, when code is involved ------------------------------------------------------
if [ "$touches_source" -eq 0 ] && [ "$touches_tests" -eq 0 ]; then
say "no app/src changes; the instrumented sweep is not required for this one"
record_sweep
exit 0
fi
if [ -n "$tree" ] && [ -f "$CACHE_DIR/$tree" ]; then
say "app/src ($tree) already swept and green; nothing under app/src has changed since"
exit 0
fi
say "app/src changed -- sweeping API 33, 34, 35, 36 (this takes tens of minutes, by design)"
if ! tools/local-emulator/run-e2e.sh 33 34 35 36; then
die "the instrumented suite is not green on 33-36."
fi
# API 37: the physical device if it is here, and an honest statement if it is not. run-e2e.sh is
# emulator-only (and overwrites E2E_EXTRA_GRADLE_ARGS with --rerun, so extra args cannot be passed
# through it), so this drives Gradle directly with the serial pinned -- the phone must never be
# picked up by accident, which is the hazard run-e2e.sh's header calls out.
export ANDROID_HOME="${ANDROID_HOME:-$HOME/Android/Sdk}"
export PATH="$ANDROID_HOME/platform-tools:$PATH"
device=""
while read -r serial state; do
[ "$state" = "device" ] || continue
case "$serial" in emulator-*) continue ;; esac
[ "$(adb -s "$serial" shell getprop ro.build.version.sdk 2>/dev/null | tr -d '\r')" = "37" ] || continue
device="$serial"
break
done < <(adb devices 2>/dev/null | tail -n +2)
levels="33, 34, 35, 36"
if [ -n "$device" ]; then
say "API 37 on the attached device $device"
if ! ANDROID_SERIAL="$device" ./gradlew :app:connectedDebugAndroidTest -PabiFilters=arm64-v8a; then
die "the instrumented suite is not green on API 37 (device $device)."
fi
levels="$levels, 37"
else
say "NOT COVERED LOCALLY: API 37. No API 37 device is attached, and the API 37 emulator cannot
install the APK on this host (see this script's header). CI's gating leg is what answers for it;
attach the Pixel 10 Pro XL to have this hook cover it too."
fi
record_sweep
# Name the levels rather than claiming "every supported level". The first cut said the latter on
# both paths, including the one that had just printed NOT COVERED two lines above -- a false claim
# printed by the tool whose whole job is to stop false claims reaching CI.
say "green on API $levels; $MODE allowed"
exit 0
+1
View File
@@ -0,0 +1 @@
local-gate.sh
+1
View File
@@ -0,0 +1 @@
local-gate.sh