Compare commits

...
Author SHA1 Message Date
JMR-dev 83b557409e Merge remote-tracking branch 'origin/main' into merge-116-tmp 2026-08-25 21:36:57 -05:00
Jason Ross 0eb00d2003 Merge pull request #113 from JMR-dev/fix/empty-composition-crash
Refuse a spec that would produce an empty file, and guard the Media3 export that used to die building one
2026-08-25 21:17:10 -05:00
JMR-devandClaude Opus 5 c887af0d83 Offer the file again after a failed save, rather than only offering to delete it
save()'s onFailure keeps the staged file on purpose -- it can be the only copy
of an hour of transcoding, and the destination did not receive it -- and then
handed the screen a Failed carrying a message and nothing else. That branch
rendered exactly one control: "Start over", wired to reset(), which discards
precisely the file the comment above it goes out of its way to keep. The intent
was already written down in main; the UI did not honour it, and the only rescue
was process death followed by reattach -- unadvertised, and bounded by a sweep
that collects anything a day old.

Failed now carries a PendingSave, and only where the failure came from save().
A transcode that died staged nothing and must not sprout a save button, so the
handle is nullable and the observe() arm leaves it null; so does a save that
found the file already gone. The branch renders "Try saving again" above "Start
over", opening the same CreateDocument flow with the same name and type the
first attempt used. A retry that fails again lands back on a carrying Failed
rather than a bare one, so the second failure cannot eat what the first kept.

Start over still deletes from there, and that is a decision rather than an
inheritance: deletion is the user's choice only once the alternative has been
offered. pendingStaged remains the single owner of the delete, so the carried
handle is a view of it rather than a second owner and no path out of the state
can drop a file the old shape could not.

pendingSave() exists so save() and each screen's CreateDocument registration
answer "what would a save target" once instead of twice -- the entry points
cast to Converted/Joined, which answered null for a Failed and fell back to the
current pickers, wrong for any spec edited since the job ran and for every
reattached job.

Both tabs, since JoinViewModel and JoinScreen have the same shape.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 21:16:02 -05:00
JMR-dev 2e0c6737d6 Merge remote-tracking branch 'origin/main' into merge-113-tmp 2026-08-25 21:09:13 -05:00
Jason Ross b53f326f9e Merge pull request #111 from JMR-dev/ci/advisory-failure-report
Say what the advisory API 37 job actually found, so a new failure is not invisible
2026-08-25 21:00:58 -05:00
JMR-devandClaude Opus 5 85461943d6 Keep the instrumented test counts in step with the suite
The API 37 entry names how many instrumented tests there are and how
many the gating leg runs, and this PR adds one. Nothing asserts those
figures, which is exactly why they rot quietly: 59/56 becomes 60/57.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 20:59:20 -05:00
JMR-devandClaude Opus 5 238142d9cc Guard the whole Media3 export instead of only its two ends
transcode() posts its work to a HandlerThread, and everything on that
thread has no caller to throw back to: an escaping exception reaches the
thread's uncaught handler and takes the process down, while the
continuation is never resumed. Both halves of that are bad, and the
second is arguably worse — a worker left suspended forever holds a
foreground service.

The guarding was two narrow runCatching blocks, one around
buildTransformer and one around transformer.start, with the two Media3
builders sitting unguarded between them. That gap was not theoretical.
EditedMediaItem.Builder rejects a composition with both tracks removed,
which is exactly what a plan of (Drop, Drop) asks for, and it does so
with a plain IllegalStateException from the constructor.

Validation now refuses the spec that produces such a plan, so neither
the picker nor ConversionWorker will start one. Routing is a separate
question and still answers Media3 for it — a dropped track makes nothing
un-hardware-able — so a request that skips validation still arrives
here: a job queued before the settings changed, or one made through
ConversionWorker.request directly. CopyPlanner's own KDoc already names
that path as the reason it re-checks what validation has checked; this
is the same belt for the same braces.

One guard around the whole body costs nothing on success and turns any
such refusal into a failed job with a reason attached. The export body
moves into startExport, whose contract is the thing that makes one guard
enough: returning normally means the export is running and the listener
owns the continuation, throwing means it never started and the caller
does. Cancellation is still registered before start.

Covered twice on purpose. Robolectric runs the real HandlerThread and
the real Media3 builders, so the JVM test exercises the whole sequence
and can be run anywhere; the instrumented one repeats it against the
real framework. Neither asserts only that the failure is an
IllegalStateException, because withTimeout raises
TimeoutCancellationException and java.util.concurrent.CancellationException
extends IllegalStateException — so that assertion alone calls an
unresumed continuation a pass. Both were written that way first, and
reverting the guard is what exposed it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 20:57:41 -05:00
JMR-devandClaude Opus 5 9c809d4e16 Refuse a spec that would leave the output with no tracks at all
Validation already refused two ways of asking for an empty file: None on
both codec axes, and Copy for a video track the input does not have. It
missed the third, because it read only the spec. Name H.265 with the
audio off, hand it an MP3, and the spec looks fine — it names a video
codec — while CopyPlanner drops that track anyway, because the *input*
has no video to encode. The plan is (Drop, Drop), the router still says
Media3, and EditedMediaItem.Builder refuses to build a composition with
both tracks removed. It refuses it on Transformer's own HandlerThread,
where the user sees the app die rather than a reason.

Asking the probe as well as the spec catches all three faces with one
guard, and the equivalence is exact rather than approximate: CopyPlanner
drops video for None or for an input with none, and audio for None, so
"(Drop, Drop)" and this condition are the same set. A sweep over every
non-image container by codec by codec against both probes asserts that,
so a new container or codec cannot reopen the gap on an axis nobody
wrote a case for.

This newly refuses a combination the Advanced picker accepts today, and
that is the point: today it crashes. What it must not do is refuse
without a way out. The Copy face had one only nominally — its single
hand-built suggestion was None + None, which validation rejects in the
next breath, so the one-tap fix fixed nothing. All three faces now go
through the shared repair-and-filter path, which for an MP3 into MP4
offers "copy the audio across" and nothing that has to be re-refused.

Repair is also stopped from naming a video codec for a file with no
video track. It used to fall through to the first codec the container
could encode, so the fix offered for an MP3 was "H.264" — a codec
CopyPlanner then drops, making the offer a fiction that happened to
validate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 20:49:16 -05:00
JMR-dev bf2214a549 Merge remote-tracking branch 'origin/main' into merge-111-tmp 2026-08-25 20:41:26 -05:00
Jason Ross 989069207e Merge pull request #110 from JMR-dev/docs/seven-run-counts
Stop counting the run this page calls inconclusive
2026-08-25 20:32:37 -05:00
JMR-dev 72ff7adfcc Merge branch 'main' into docs/seven-run-counts 2026-08-25 20:25:06 -05:00
JMR-dev 994ea8a3dd Say in the step log that the summary was written, since nothing else can
The job summary is the deliverable #83 asked for -- "readable without opening a
log" -- and GitHub exposes no API that reads a job summary back: the check-run
output for the advisory job returns summary: null, so a write that silently did
not happen would be invisible to everything except a human on the run page. The
step log can be read, so it now carries one line saying which of the two
happened, including the case where GITHUB_STEP_SUMMARY is unset entirely, which
is what running the script by hand looks like.
2026-08-25 20:23:05 -05:00
JMR-dev 3e9528454c Announce a baseline it cannot read, rather than falling quiet
"A comparison was asked for" and "a number was found to compare against" were one
variable, and collapsing them put the report one refactor away from being the
thing #83 filed. The sed that reads FAILS_ON_EMULATOR_API37_BASELINE is anchored
at the line start, so indenting the const into an object -- or renaming it, or
moving it -- empties it, and the old code then skipped the whole comparison while
the table kept printing exactly as before. Silent, and indistinguishable from a
run that matched.

Now an unreadable baseline is itself a deviation, with the notice naming the
const so the fix is obvious. Verified against the real captured log of run
32865281555 three ways: baseline file absent, const indented into an object, and
the committed file unchanged -- the first two announce, the third stays silent.
2026-08-25 20:20:59 -05:00
JMR-dev 0702916229 Say what the advisory API 37 job actually found, so a new failure is not invisible
That job is continue-on-error and red on every PR by design, which CLAUDE.md
states plainly -- and that instruction is exactly why nobody reads it. Nothing in
a red X separates "the known three" from "the known three plus yours".

A bare failure count would not have fixed it, and this is measured rather than
assumed. The run is usually truncated: seven of eight advisory runs read on
2026-08-25 ended in `Test run failed to complete. Expected 3 tests, received 2.`
with INSTRUMENTATION_ABORTED, and one did not. A count taken from a truncated run
misleads in both directions -- a fourth marked test can still yield the same
number if the abort lands earlier, and the known set getting worse can lower it.

The test XML does not rescue it either, which was the thing worth checking before
building on it: it IS written for an aborted run, and it reports a tidy
tests="3" failures="3" for a run the runner had just described as truncated. So
the XML is the authority on how many results landed, the runner's own output is
the only authority on whether the run finished, and the report reads both and
says which number came from where.

The baseline is one number beside the marker, because the marker means "cannot
pass on this image": the count is both how many tests the advisory leg runs and
how many should fail. A smaller failure count is the interesting direction -- it
means one now passes, which is the documented trigger for deleting the
annotation.

Nothing about the job's status changes. It stays continue-on-error, stays red,
stays out of the required contexts; a deviation is a ::notice::, never an
::error::. The report is a separate script so it can be run against a real log
saved from a real CI run, which is how the comparison was shown to fire.

The gating legs get the shape without the comparison: they run the whole suite,
so comparing there would announce a deviation five times a run -- but a truncated
run reporting fewer results than it ran is what #108 looks like, and "completed
cleanly" is the field that would show it.

Closes #83
2026-08-25 16:04:13 -05:00
Jason Ross 3fd34c24a6 Merge pull request #109 from JMR-dev/test/release-permission-guard
Notice if the release job loses the permission that lets it publish
2026-08-25 15:59:04 -05:00
JMR-dev d01a46a708 Stop counting the run this page calls inconclusive
R29 found the discriminator claimed "exact across all seven" while r07 is recorded lower
down as "inconclusive rather than ruled out, because no evidence came back from it". A row
this page calls inconclusive cannot also be counted as evidence for the conclusion.

Checking it turned up a second instance of the same over-count, which R29 did not name. The
abort-cadence section said "Measured across the seven runs above" -- but the table records
r07's aborts as **not readable**, because adb wedged before a crash buffer could be taken.
Six runs contributed gaps, not seven.

Both now say six, and both say why. The discriminator paragraph also says what excluding r07
costs, which is nothing: it is a `host` row, so the discriminator predicts it would not boot,
and confirming a prediction with the one run whose evidence did not come back adds no
information in either direction. That is the point R29 made -- claiming six does not weaken
the conclusion -- and it is worth stating in the document rather than only in the ticket,
because the next reader will otherwise wonder whether a run was quietly dropped.

Deliberately left: "four of the seven runs show the directory creation itself is broken
during the loop". That is a count of how many runs showed something, not a claim that all
seven were readable for it, so it survives. Checked rather than assumed, and named here so
the next pass does not re-audit it.

R29's other half -- "state how r07's boot outcome was read" -- is not taken, because I do
not know and inventing a source would be worse than narrowing the claim. Narrowing is the
option R29 offered and the one that can be honest.

Closes #38.
2026-08-25 15:54:48 -05:00
JMR-dev 3806641cb2 Merge branch 'main' into test/release-permission-guard 2026-08-25 15:50:20 -05:00
Jason Ross 5f9498150c Merge pull request #106 from JMR-dev/ci/build-workflow-permissions
Declare build.yml's token reach in build.yml
2026-08-25 15:49:19 -05:00
JMR-dev 8d8703ab49 Merge branch 'main' into ci/build-workflow-permissions 2026-08-25 15:41:39 -05:00
Jason Ross 27b7654418 Merge pull request #105 from JMR-dev/docs/api37-point-release
Say 37.0 is the choice, not the only api-level that exists
2026-08-25 15:41:11 -05:00
JMR-dev 4a8e30099e Notice if the release job loses the permission that lets it publish
build.yml's `release` job declares `contents: write`, and nothing checked it. Deleting those
lines leaves actionlint clean and CodeQL silent -- a narrower permission is not an alert --
and the job is `if: startsWith(github.ref, 'refs/tags/v')`, so no pull request and no merge
can exercise it. Measured with the declaration removed: every gating check still passed. The
first thing that would notice is a release failing to publish, at the moment someone is
trying to cut one.

The deletion also looks like tidying. #106 has just put a top-level `permissions: contents:
read` directly above it, so a reader could reasonably take the job-level block for a
duplicate. It is an override, and a comment saying so is not a check.

BackupExclusionsTest is the precedent: configuration rather than code, load bearing, and
unguarded because nothing compiles it.

The part worth reading twice is the second commit-worth of work in here. The test passed,
and then the mutation that is supposed to redden it did not:

  BUILD SUCCESSFUL in 614ms

Gradle cannot infer that a test depends on a file outside the source set, so the task stayed
UP-TO-DATE and the test never ran. Under --rerun-tasks the same mutation failed it properly,
which is the tell: the assertion was right and the wiring was not. A guard that does not
re-run when its subject changes is not a guard -- it is a test that will be green on the day
it matters, which is worse than no test because it reads as cover.

Fixed by declaring the workflow as a task input. Verified the whole way round afterwards,
without --rerun-tasks: mutate the file and the task re-runs and fails; restore it and the
task re-runs and passes.

What this pins and what it does not: it asserts the declaration exists in the release job's
block. It cannot assert a release actually publishes -- that needs a tag push, which is the
thing no PR can do. A tripwire against silent removal, not proof the path works, and the
KDoc says so.

Closes #107.
2026-08-25 15:38:14 -05:00
JMR-dev e7d84cc69f Merge branch 'main' into docs/api37-point-release 2026-08-25 15:33:27 -05:00
Jason Ross a8b494b846 Merge pull request #104 from JMR-dev/docs/benchmark-populate-path
Stop telling people to stage the benchmark the one way it cannot be staged
2026-08-25 15:33:02 -05:00
JMR-dev 865a4a7c8e Merge branch 'main' into docs/benchmark-populate-path 2026-08-25 10:21:23 -05:00
Jason Ross e856679395 Merge pull request #103 from JMR-dev/fix/dead-assertion-probe-test
Delete an assertion that could never fail, and say what guards instead
2026-08-25 10:21:18 -05:00
JMR-dev 49c483d877 Declare build.yml's token reach in build.yml
CodeQL alert #1, the only open one on this repository:

  actions/missing-workflow-permissions, warning / medium, build.yml:23
  Actions job or workflow does not limit the permissions of the GITHUB_TOKEN.

Alerts 2, 3 and 4 were the same rule against status_check.yml and are fixed -- that file
has a top-level block. build.yml declares permissions in exactly one place, the release
job's `contents: write`, and has no top-level default, so the `test` job inherits the
repository setting.

**Nothing is over-privileged today.** The repository default is already `read`
(default_workflow_permissions: read, can_approve_pull_request_reviews: false, read from the
API rather than assumed), so the test job holds a read token now. Saying so matters: this
is hygiene, and a commit that implied it was closing a live hole would be overstating it.

What it buys is that the default CANNOT widen these jobs later without someone editing this
file. That is not invented for the occasion -- it is the argument status_check.yml already
makes, which even names this file:

  the token's reach should be readable here, and a default that widens later should not
  silently widen these jobs with it. build.yml's release job makes the opposite
  declaration for the same reason.

So the principle was decided, applied in two workflows and in one job of this one, and the
top level of build.yml was the gap.

Verified the thing that would actually break: the release job's `contents: write` still
wins. Top level is a default, not a ceiling -- parsed and printed both, test inherits
`contents: read`, release keeps `contents: write`.

Also ran the ticket's mutation, and it found something. Deleting the release job's
`contents: write` leaves actionlint green and CodeQL quiet -- a narrower permission is not
an alert -- so nothing would catch it until a tagged release failed to publish. That is a
separate gap and is filed rather than fixed here.

actionlint clean at the pinned digest. Comment and permissions only; no step, job or
trigger changes.

Closes #100.
2026-08-25 10:16:08 -05:00
JMR-dev 1b220856ab Say 37.0 is the choice, not the only api-level that exists
R19 raised two things about this comment. One resolved itself: it used to explain why the
matrix had no API 37 row at all, and #56 added the gating row, so that half is gone.

The other survived, and this is it. The comment read

  api-level must be "37.0". A bare 37 is not an SDK package and fails during setup

The second sentence is true and was measured -- it cost a run to find. The first overstates
it. What must be true is that the api-level is a POINT release; 37.0 is one of several.
api37-debug.yml's own input descriptions already say so:

  API level, as the SDK spells it. 37.0, 37.1, 37.2-beta3, 36 ...
  System image target. android-37.1 and 37.2-beta* ship ONLY as google_apis_ps16k

and docs/api-37-emulator-crash.md measures android-37.0 rev 6 and android-37.1 rev 8 side
by side, both aborting. So the repo already knows 37.1 exists and behaves the same; only
this comment implied otherwise.

That matters for the reader it is written for. Someone debugging this row and wondering
whether a newer image helps reads "must be 37.0" as a constraint and stops. The measured
answer is that it does not help, which is a better thing to learn than a rule that is not
one -- and the ps16k-only wrinkle above 37.0 is the detail that would actually bite them.

Comment only. No job, matrix, filter or gating behaviour changes. actionlint clean at the
pinned digest.

Closes #28.
2026-08-25 10:14:34 -05:00
JMR-dev 40ae524388 Merge branch 'main' into fix/dead-assertion-probe-test 2026-08-25 10:13:12 -05:00
Jason Ross 58a29ab093 Merge pull request #99 from JMR-dev/ci/actionlint
Lint the bash inside the workflows, not only the bash in files
2026-08-25 10:13:00 -05:00
JMR-dev d37c391c60 Stop telling people to stage the benchmark the one way it cannot be staged
RealMediaBenchmark's class KDoc said:

  Populate with:
    adb push <file>.mp4 /sdcard/Android/data/org.libremediaconverter/files/

Twelve lines below, the `samples` property KDoc -- on `get() = context.filesDir` -- says:

  Internal storage, not the external files dir. Files placed in the external dir by
  `adb push` or `adb shell cp` stay owned by the shell user, and the app then gets
  EACCES trying to read them -- which presents as an unparseable input rather than a
  permission problem.

Different directories, and the second exists specifically to explain why the first fails.
Anyone following the class KDoc stages files the benchmark cannot read, gets a skip, and
reads the skip as "not staged yet" -- the failure mode the property KDoc warns about, walked
into by the instruction in the same file.

The fix is not a corrected command. Restating the mechanism in a second place is what let
these drift, and a replacement command I have not executed would be the same defect with a
fresher date. The class KDoc now names [samples] as the single place that answers it.

Two things added that are checkable rather than remembered: the exact filenames the tests
look for, via [H264_SAMPLE] and [AV1_SAMPLE] -- the old text said `<file>.mp4`, so even the
right directory left you guessing -- and a note that the two skips every green E2E leg
reports are these.

Not claimed: that the benchmark misbehaves on CI. An earlier version of the ticket said so;
it was wrong, and measuring settled it -- both tests report SKIPPED on the gating legs, the
guards work, and "harmless in CI" is accurate. The failure that prompted the look is
Media3EngineTest, tracked as #102.

Closes #101.
2026-08-25 09:45:00 -05:00
JMR-dev 95902a7889 Delete an assertion that could never fail, and say what guards instead
ConversionViewModelProbeFailureTest's pickedProbe() helper held:

  val ready = awaitState(viewModel.state, "Ready with a probe") {
      it is ConversionState.Ready && it.input.probe != null
  }
  assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed))

The predicate requires `Ready`. `Ready` and `Failed` are sibling subtypes of one sealed
interface, so `ready as? Failed` is always null and the assertNull could never fire. R26
filed this PLAUSIBLE on types read; it is measured now.

Flipping the line to assertNotNull failed 3 of the 4 tests in the class -- three, because
pickedProbe() has three callers, which is also why a dead line here was worth removing
rather than shrugging at: it read as coverage in a helper the whole class depends on.

Deleted rather than replaced. There is nothing for a live assertion to add: a pick that
ended in Failed never satisfies the predicate, so awaitState fails on its timeout naming
what it was waiting for -- "Ready with a probe" -- which is a better failure message than
the assertion would have produced. The comment now says that, so the next reader does not
re-add the guard the predicate already is.

This is the ninth vacuous assertion this line of work has turned up, and the pattern is
consistent: they hide in helpers, they pass, and they look like care. The suite is green
before and after, which is exactly the point -- deleting a dead assertion cannot change a
result, and if it had, the line was not dead.

Closes #35.
2026-08-25 09:36:36 -05:00
29 changed files with 1723 additions and 105 deletions
+294
View File
@@ -0,0 +1,294 @@
#!/usr/bin/env bash
#
# Reports the SHAPE of an instrumented run -- how many tests were expected, how many
# reported, how many failed, and whether the run completed at all -- to the step log and to
# the job summary. In advisory mode it also compares that shape against a committed baseline
# and says plainly whether it matches.
#
# WHY THIS EXISTS (#83): the advisory API 37 leg is red on every PR by design, so a NEW failure
# joining the known ones is invisible -- nothing in a red X distinguishes "the known ones" from
# "the known ones plus yours". CLAUDE.md tells everyone not to read that job's red as their
# change breaking something, which is correct, and which also means nobody looks.
#
# WHY NOT A BARE FAILURE COUNT, measured rather than assumed. On this image the run is usually
# truncated: `Test run failed to complete. Expected 3 tests, received 2.` with
# `INSTRUMENTATION_ABORTED: System has crashed.` A count taken from a truncated run misleads in
# both directions -- a fourth marked test can still yield the same number if the abort lands
# earlier, and the known set getting worse can LOWER it. So all four fields are recorded, and
# the one saying the run was truncated is recorded with them.
#
# WHY IT IS A SEPARATE SCRIPT rather than a function inside e2e-run.sh: it is a pure seam. It
# reads a captured log plus the test XML and writes a report, so it can be run against a REAL
# log saved from a REAL CI run -- which is how the baseline comparison was shown to fire
# without waiting on an emulator. `git ls-files '*.sh'` also picks it up for shellcheck for
# free.
#
# THIS SCRIPT NEVER FAILS A RUN. It is a diagnostic, and e2e-run.sh's header explains why that
# rule is absolute here. Every field defaults to `unknown` and every comparison is guarded,
# because an unset variable under `set -u`, or a `[ "" -eq 3 ]`, is exactly how a diagnostic
# becomes the thing that turns a leg red. It exits 0 unconditionally.
#
# Usage:
# e2e-report-shape.sh <label> <gradle-log> [<baseline-file>]
#
# With a third argument the run is compared against the baseline in that file (advisory mode)
# and a `::notice::` is emitted per deviation. NEVER `::error::`: the advisory job is
# `continue-on-error: true` and stays that way, and an error annotation would be a new way for
# a diagnostic to change a conclusion.
set -uo pipefail
LABEL="${1:-unknown}"
LOG="${2:-}"
BASELINE_FILE="${3:-}"
SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)"
REPO_ROOT="$(cd -- "$SCRIPT_DIR/../.." && pwd)"
XML_DIR="$REPO_ROOT/app/build/outputs/androidTest-results/connected/debug"
# Gradle colours its output even when it is piped, so `FAILED` arrives wrapped in escape codes.
# The numeric lines parsed below are not coloured, but stripping is cheap insurance against a
# pattern that would otherwise silently match nothing.
ESC="$(printf '\033')"
scan() { [ -s "$LOG" ] && sed -e "s/${ESC}\[[0-9;]*[a-zA-Z]//g" -- "$LOG"; }
first_number() { grep -oE '[0-9]+' | head -1; }
# ---------------------------------------------------------------------------
# Source 1: the runner's own output. This is the ONLY place a truncation is visible. The test
# XML read below is written even for an aborted run and says nothing whatever about the abort
# -- measured on run 32865281555, where the XML reports a tidy tests="3" failures="3" for a run
# the runner had just described as truncated. That is the reason this parses stdout at all.
# ---------------------------------------------------------------------------
starting_line="$(scan | grep -aoE 'Starting [0-9]+ tests on .*' | tail -1)"
abort_line="$(scan | grep -aoE 'Test run failed to complete\. Expected [0-9]+ tests, received [0-9]+\.' | tail -1)"
aborted_hits="$(scan | grep -ac 'INSTRUMENTATION_ABORTED' || true)"
failure_line="$(scan | grep -aoE 'There was [0-9]+ failure\(s\)\.' | tail -1)"
failed_names="$(scan | grep -aoE 'Execute [A-Za-z0-9_.$]+: FAILED' | sed -e 's/^Execute //' -e 's/: FAILED$//' | sort -u)"
expected="$(printf '%s' "$starting_line" | first_number)"
expected_src="\`$starting_line\`"
abort_expected="$(printf '%s' "$abort_line" | grep -oE 'Expected [0-9]+' | first_number)"
abort_received="$(printf '%s' "$abort_line" | grep -oE 'received [0-9]+' | first_number)"
log_failed="$(printf '%s' "$failure_line" | first_number)"
# `Starting N tests` is missing when the framework restarted under the run and Gradle never got
# a test list. The truncation line still carries the number it was told to expect.
if [ -z "$expected" ] && [ -n "$abort_expected" ]; then
expected="$abort_expected"
expected_src="\`$abort_line\`"
fi
# ---------------------------------------------------------------------------
# Source 2: the JUnit XML. Measured on both a truncated advisory run and a green gating leg:
# `<testsuites tests="N" failures="M">` is present in both, and aggregates every suite. It is
# the authority on how many results landed and how many were failures. It is NOT an authority
# on whether the run finished, which is what source 1 is for.
# ---------------------------------------------------------------------------
#
# Read ONLY when the runner said a test run happened. `app/build` survives between runs on a
# developer machine -- tools/local-emulator/run-e2e.sh drives several API levels against one
# checkout -- so a leg that never got as far as starting tests would otherwise be reported from
# the previous leg's XML, which is a wrong answer rather than a missing one.
xml_head=""
xml_count=0
if [ -n "$starting_line$abort_line" ] && [ -d "$XML_DIR" ]; then
while IFS= read -r f; do
xml_count=$((xml_count + 1))
[ -z "$xml_head" ] && xml_head="$(grep -ao '<testsuites[^>]*>' "$f" | head -1)"
done < <(find "$XML_DIR" -maxdepth 1 -name 'TEST-*.xml' -print 2> /dev/null | sort)
fi
xml_tests="$(printf '%s' "$xml_head" | grep -oE ' tests="[0-9]+"' | first_number)"
xml_failed="$(printf '%s' "$xml_head" | grep -oE ' failures="[0-9]+"' | first_number)"
# ---------------------------------------------------------------------------
# Derive the four fields, each with where its number came from. Everything stays a string, so a
# missing source reads `unknown` rather than becoming 0 -- a report claiming 0 tests when it
# merely could not see them would announce a deviation on every cancelled run.
# ---------------------------------------------------------------------------
received="unknown"
received_src="no source"
if [ -n "$xml_tests" ]; then
received="$xml_tests"
received_src="test XML \`<testsuites tests=\"$xml_tests\">\`"
elif [ -n "$abort_received" ]; then
received="$abort_received"
received_src="\`$abort_line\`"
elif [ -n "$expected" ] && [ -z "$abort_line" ]; then
received="$expected"
received_src="the run was not truncated, so every expected test reported"
fi
failed="unknown"
failed_src="no source"
if [ -n "$xml_failed" ]; then
failed="$xml_failed"
failed_src="test XML \`<testsuites failures=\"$xml_failed\">\`"
elif [ -n "$log_failed" ]; then
failed="$log_failed"
failed_src="\`$failure_line\`"
fi
if [ -z "$expected" ]; then
expected="unknown"
expected_src="no \`Starting N tests\` line"
fi
# A run whose start nobody can see is not a run of zero tests. Cancellation (this workflow sets
# cancel-in-progress) and the `Starting 0 tests` shape a framework restart produces both land
# here, and both have to say so rather than compare a number that does not exist.
no_run="none"
if [ "$expected" = "unknown" ] && [ "$received" = "unknown" ]; then
no_run="nothing"
elif [ "$expected" = "0" ]; then
no_run="zero"
fi
if [ -n "$abort_line" ]; then
completed="**no**"
completed_src="\`$abort_line\` with \`INSTRUMENTATION_ABORTED\`"
elif [ "${aborted_hits:-0}" -gt 0 ]; then
completed="**no**"
completed_src="\`INSTRUMENTATION_ABORTED\` in the runner output"
elif [ "$no_run" = "nothing" ]; then
# "cleanly" would be a lie about a run that left no evidence it happened.
completed="unknown"
completed_src="no runner output to read"
else
completed="yes"
completed_src="no truncation line and no \`INSTRUMENTATION_ABORTED\`"
fi
# ---------------------------------------------------------------------------
# Advisory mode: compare against the committed baseline.
#
# ONE number covers both compared fields, and that is deliberate rather than a shortcut. The
# marker means "cannot pass on this image", so the number of tests carrying it is both how many
# the advisory leg should run and how many should fail. A smaller `failed` means one now passes
# -- which is the trigger to delete the annotation, written down in FailsOnEmulatorApi37.kt.
# ---------------------------------------------------------------------------
#
# `advisory` and `baseline` are two variables on purpose. "A comparison was asked for" and "a
# number was found to compare against" are different facts, and collapsing them is how this
# report would go quietly back to being the thing #83 filed: the `sed` below is anchored, so
# indenting the const into an object -- or renaming it, or moving it to another file -- empties
# `baseline`, and a single flag would take the whole comparison down with it while the table
# kept printing. An unreadable baseline is itself a deviation, and is announced as one.
advisory="no"
baseline=""
marked=""
deviations=()
if [ -n "$BASELINE_FILE" ]; then
advisory="yes"
[ -f "$BASELINE_FILE" ] \
&& baseline="$(sed -nE 's/^const val FAILS_ON_EMULATOR_API37_BASELINE = ([0-9]+).*/\1/p' "$BASELINE_FILE" | head -1)"
# The #81 check, verbatim: what the tree actually carries. Reported next to the baseline so a
# stale baseline shows up here rather than only once the emulator disagrees with it.
if [ -d "$REPO_ROOT/app/src/androidTest" ]; then
marked="$(grep -rn "@FailsOnEmulatorApi37" "$REPO_ROOT/app/src/androidTest" --include='*.kt' \
| grep -v import | grep -c FailsOn || true)"
fi
fi
if [ "$advisory" = "yes" ] && [ -z "$baseline" ]; then
deviations+=("the committed baseline could not be read from \`$(basename -- "$BASELINE_FILE")\` — has \`FAILS_ON_EMULATOR_API37_BASELINE\` been renamed, indented into a class, or moved? Nothing was compared")
fi
if [ -n "$baseline" ]; then
if [ "$no_run" = "nothing" ]; then
deviations+=("no test run observed — the runner never reported starting one, where the baseline expects $baseline tests carrying \`@FailsOnEmulatorApi37\`")
elif [ "$no_run" = "zero" ]; then
deviations+=("the runner started 0 tests, where the baseline expects $baseline — on this image that is the framework having restarted under the run, not an empty test list")
else
if [ "$expected" != "unknown" ] && [ "$expected" != "$baseline" ]; then
deviations+=("the runner started $expected tests, the baseline is $baseline")
fi
if [ "$failed" != "unknown" ] && [ "$failed" != "$baseline" ]; then
deviations+=("$failed tests failed, the baseline is $baseline — every test carrying the marker is expected to fail on this image, so fewer means one now passes and more means a new one joined")
fi
fi
if [ -n "$marked" ] && [ "$marked" != "$baseline" ]; then
deviations+=("the tree carries $marked tests marked \`@FailsOnEmulatorApi37\` but the baseline says $baseline — update FAILS_ON_EMULATOR_API37_BASELINE")
fi
fi
# ---------------------------------------------------------------------------
# Emit. Step log first, so the common case needs neither the summary page nor an artifact.
# ---------------------------------------------------------------------------
echo "----- RUN SHAPE (api${LABEL}) -----"
echo " expected: $expected"
echo " received: $received"
echo " failed: $failed"
echo " completed cleanly: ${completed//\*/}"
if [ -n "$abort_received" ]; then
echo " received before the abort: $abort_received"
fi
if [ -n "$failed_names" ]; then
echo " failed tests:"
printf '%s\n' "$failed_names" | sed -e 's/^/ /'
fi
if [ "$advisory" = "yes" ]; then
if [ "${#deviations[@]}" -eq 0 ]; then
echo " baseline: matches ($baseline expected, $baseline failed)"
else
printf ' baseline DEVIATION: %s\n' "${deviations[@]}"
fi
fi
# A notice, never an error. See the header.
if [ "${#deviations[@]}" -gt 0 ]; then
for d in "${deviations[@]}"; do
echo "::notice::E2E api${LABEL}: $d"
done
fi
if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then
{
echo "### E2E api${LABEL} — run shape"
echo
echo "| field | value | where it came from |"
echo "| --- | --- | --- |"
echo "| expected | $expected | $expected_src |"
echo "| received | $received | $received_src |"
echo "| failed | $failed | $failed_src |"
echo "| completed cleanly | $completed | $completed_src |"
if [ -n "$abort_received" ]; then
echo "| received before the abort | $abort_received | the same line — the XML above counts the truncated test as a failure, this number does not |"
fi
echo
if [ -n "$failed_names" ]; then
echo "Failed:"
echo
printf '%s\n' "$failed_names" | sed -e 's/^/- `/' -e 's/$/`/'
echo
fi
if [ "$xml_count" -gt 1 ]; then
echo "> $xml_count test XML files were present; the counts above come from the first."
echo
fi
if [ "$advisory" = "yes" ]; then
if [ "${#deviations[@]}" -eq 0 ]; then
echo "**Matches the committed baseline of $baseline** — $baseline tests carry \`@FailsOnEmulatorApi37\` and all $baseline failed, which is what this job is for."
elif [ -z "$baseline" ]; then
echo "**The committed baseline could not be read, so nothing was compared.** Announced as a notice, not an error: this job is advisory and its conclusion is unchanged by anything here."
echo
printf -- '- %s\n' "${deviations[@]}"
else
echo "**DEVIATION from the committed baseline of $baseline.** Announced as a notice, not an error: this job is advisory and its conclusion is unchanged by anything here."
echo
printf -- '- %s\n' "${deviations[@]}"
fi
echo
echo "<sub>The baseline lives beside the marker, in \`FailsOnEmulatorApi37.kt\`. \`completed cleanly\` is recorded rather than compared: the truncation is intermittent — of eight advisory runs read on 2026-08-25, seven aborted and one did not — so comparing it would announce a deviation on a run that is fine.</sub>"
else
echo "<sub>No baseline comparison: that is the advisory API 37 leg only. The shape is recorded here anyway because a truncated run reports fewer results than it ran, which is what issue #108 looks like on a gating leg.</sub>"
fi
echo
} >> "$GITHUB_STEP_SUMMARY"
# The summary page is the deliverable -- "readable without opening a log" is what #83 asked
# for -- and GitHub exposes no API for reading a job summary back, so a write that silently
# did not happen would be invisible. This line is in the step log, which can be read.
echo " (the table above is also on the job summary page)"
else
echo " (GITHUB_STEP_SUMMARY is unset -- step log only)"
fi
exit 0
+36 -1
View File
@@ -34,6 +34,13 @@ TMP="${RUNNER_TEMP:-/tmp}"
LOGCAT_LOG="$TMP/logcat-api${LABEL}.txt"
DIAG_LOG="$TMP/diagnostics-api${LABEL}.txt"
WEDGE_LOG="$TMP/wedge-diagnostics-api${LABEL}.txt"
# Gradle's own output, captured to a file as well as the step log, because the run-shape report
# below has to parse it. Uploaded with the diagnostics, so a report that reads wrong can be
# checked against what it read.
GRADLE_LOG="$TMP/gradle-api${LABEL}.txt"
SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)"
REPO_ROOT="$(cd -- "$SCRIPT_DIR/../.." && pwd)"
# ~5 min is a healthy leg (measured across API 33-36), and this wraps only the gradle client,
# a subset of that. 20 min is generous enough never to trip on a slow-but-working run, and far
@@ -214,12 +221,40 @@ status=0
# script rather than forking it: that runs several API levels back to back against one checkout
# and passes `--rerun`, so a level cannot be skipped as up-to-date and report the previous
# level's results as its own. CI gets a fresh runner per level and does not need it.
#
# `2>&1 | tee`, and the `2>&1` is the load-bearing half. The step log merges both streams, so
# reading one cannot tell you which stream a line came from -- and the single line the report
# below needs most, `Test run failed to complete. ... INSTRUMENTATION_ABORTED`, is not on
# stdout. Capturing stdout alone would leave the report saying "completed cleanly: yes" forever,
# which is precisely the comparison that cannot fire. pipefail is already set and tee exits 0,
# so the pipeline's status is still gradle's -- including the 124 that means the wrapper fired.
#
# `tee` and not `tee -a`, unlike the logcat above: CI gets a fresh runner per leg, but
# tools/local-emulator/run-e2e.sh reuses one machine, and an appended log would have the report
# reading the PREVIOUS run of the same API level. The console goes plain rather than showing
# gradle's live progress bar, which is what it already did in CI.
# shellcheck disable=SC2086
timeout -k 30s "$WEDGE_TIMEOUT" \
./gradlew :app:connectedDebugAndroidTest -PabiFilters=x86_64 --stacktrace \
${E2E_EXTRA_GRADLE_ARGS:-} || status=$?
${E2E_EXTRA_GRADLE_ARGS:-} 2>&1 | tee "$GRADLE_LOG" || status=$?
echo "::endgroup::"
# The run-shape report: expected/received/failed and whether the run finished, every time,
# green or red. It never changes `status` -- it is a diagnostic, and the header's rule about
# diagnostics applies to it as much as to every probe below.
#
# The baseline argument, and only it, turns on the comparison, and only the advisory API 37 job
# passes E2E_ADVISORY=1. Comparing on the gating legs would announce a deviation on all five of
# them every run, since they run the whole suite rather than the marked three. They still get
# the report: a truncated run reporting fewer results than it ran is what #108 looks like, and
# `completed cleanly` is the field that shows it.
if [ "${E2E_ADVISORY:-}" = "1" ]; then
bash "$SCRIPT_DIR/e2e-report-shape.sh" "$LABEL" "$GRADLE_LOG" \
"$REPO_ROOT/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt" || true
else
bash "$SCRIPT_DIR/e2e-report-shape.sh" "$LABEL" "$GRADLE_LOG" || true
fi
if [ "$status" -eq 0 ]; then
kill "$LOGCAT_PID" 2>/dev/null || true
exit 0
+11
View File
@@ -13,6 +13,17 @@ on:
# reference amounts to running whatever that repository contains tomorrow. This matters
# more here than on pull requests: these jobs sign nothing today, but they do publish
# the artifacts people install.
# Declared here rather than inherited, for the reason status_check.yml gives for its own
# block: the token's reach should be readable in the file that uses it, and a repository
# default that widens later should not silently widen these jobs with it. The repository
# default is `read` today, so this changes nothing about what runs -- it fixes what a
# reader can know without leaving the file, and it is what CodeQL alert #1 asked for.
#
# The `release` job below overrides this with `contents: write`, which is how job-level
# permissions work: this is a default, not a ceiling.
permissions:
contents: read
env:
GRADLE_CACHE_PATHS: |
~/.gradle/caches
+15 -2
View File
@@ -280,8 +280,13 @@ jobs:
# docs/api-37-emulator-crash.md has the per-method measurements, and the
# correction that produced them.
#
# api-level must be "37.0". A bare 37 is not an SDK package and fails
# during setup, which cost a run to discover.
# api-level must be a POINT release. A bare 37 is not an SDK package and
# fails during setup, which cost a run to discover. `37.0` is the choice
# here rather than the only option: `37.1` and `37.2-beta*` exist and
# abort the same way, and api37-debug.yml's inputs document both, with
# the wrinkle that above 37.0 they ship only as google_apis_ps16k.
# 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 three tests that do not pass on this image; they
# run in the advisory job below, off the same marker so they cannot end up
@@ -372,6 +377,7 @@ jobs:
path: |
${{ runner.temp }}/logcat-api${{ matrix.label }}.txt
${{ runner.temp }}/diagnostics-api${{ matrix.label }}.txt
${{ runner.temp }}/gradle-api${{ matrix.label }}.txt
if-no-files-found: warn
# Only exists when the wrapper timeout tripped, so `ignore` keeps healthy runs quiet
@@ -462,6 +468,12 @@ jobs:
# The complement of the gating row's notAnnotation, off the same marker,
# so a test can never be excluded from both jobs or run in both.
E2E_EXTRA_GRADLE_ARGS: "-Pandroid.testInstrumentationRunnerArguments.annotation=org.libremediaconverter.FailsOnEmulatorApi37"
# Turns on the baseline comparison in the run-shape report, and only here. Every leg
# prints the shape; this is the one that also says whether it matches
# FAILS_ON_EMULATOR_API37_BASELINE, because this is the one whose test list is the
# marker. A deviation is a `::notice::` -- this job stays continue-on-error and stays
# out of the required contexts, so nothing the report finds can change a conclusion.
E2E_ADVISORY: "1"
with:
# Every device pin below matches the gating row exactly, so a difference
# between the two jobs is the test selection and nothing else.
@@ -494,6 +506,7 @@ jobs:
path: |
${{ runner.temp }}/logcat-api${{ env.E2E_LABEL }}.txt
${{ runner.temp }}/diagnostics-api${{ env.E2E_LABEL }}.txt
${{ runner.temp }}/gradle-api${{ env.E2E_LABEL }}.txt
if-no-files-found: warn
- uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
+11 -2
View File
@@ -76,11 +76,11 @@ 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. **Three** of the 59 instrumented
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Three** of the 60 instrumented
tests cannot pass on that image, for two unrelated reasons: two Media3 hardware transcodes fail
inside the emulator's own `c2.goldfish.h264.decoder`, and one SAF test takes the framework down
when it rotates the display. All three carry `@FailsOnEmulatorApi37` and run in a separate
`continue-on-error` job; the gating leg runs the other 56.
`continue-on-error` job; the gating leg runs the other 57.
That job is still called `E2E API 37 Media3 hardware transcode (advisory)`, which no longer
describes everything in it. The name is kept deliberately — it is not a required context and
@@ -89,6 +89,15 @@ days. Read it as the current answer, and see the git history if you need the old
not read a green run as evidence those three 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,
received, failed, and whether the run completed — to the job summary, and compares it against
`FAILS_ON_EMULATOR_API37_BASELINE`, committed beside the marker. A deviation is a `::notice::`;
the job stays advisory and its conclusion is untouched. **Add or remove a `@FailsOnEmulatorApi37`
and that number changes in the same diff**, or the next run says so. A bare failure count would
not have worked: the run is usually truncated by an `INSTRUMENTATION_ABORTED`, and the test XML
is written anyway and says nothing about it — `.github/scripts/e2e-report-shape.sh` is where that
is measured and explained.
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 three tests are the one thing CI cannot answer
for.
+11
View File
@@ -1,3 +1,4 @@
import org.gradle.api.tasks.PathSensitivity
import org.gradle.testing.jacoco.tasks.JacocoReport
plugins {
@@ -206,6 +207,16 @@ detekt {
// `excludes` is not optional. Without it JaCoCo walks JDK-internal classes that Robolectric has
// no location for either, and the test JVM dies rather than reporting a number.
tasks.withType<Test>().configureEach {
// ReleasePermissionTest reads .github/workflows/build.yml, and Gradle cannot infer that a
// test depends on a file outside the source set. Without this the task stays UP-TO-DATE
// when the workflow changes, so the guard goes stale exactly when it matters. Measured:
// deleting the release job's `contents: write` and re-running gave "BUILD SUCCESSFUL in
// 614ms" with the test never executing; the same mutation under --rerun-tasks failed it.
// A guard that does not re-run when its subject changes is not a guard.
inputs.file(rootProject.file(".github/workflows/build.yml"))
.withPropertyName("releaseWorkflow")
.withPathSensitivity(PathSensitivity.RELATIVE)
extensions.configure<JacocoTaskExtension> {
isIncludeNoLocationClasses = true
excludes = listOf("jdk.internal.*")
@@ -18,7 +18,38 @@ package org.libremediaconverter
* Removing it is the goal, and the trigger is written down: a new API 37.x system image, or an
* ATD image for 37. Delete the annotation from the tests, and the advisory job goes empty and
* the gating one grows by two.
*
* **How many tests carry it is committed below**, as [FAILS_ON_EMULATOR_API37_BASELINE], and the
* advisory job checks the run against it. Adding or removing a marker means changing that number
* in the same diff.
*/
@Retention(AnnotationRetention.RUNTIME)
@Target(AnnotationTarget.CLASS, AnnotationTarget.FUNCTION)
annotation class FailsOnEmulatorApi37
/**
* How many tests carry [FailsOnEmulatorApi37] — the advisory API 37 job's committed baseline.
*
* **No Kotlin reads this, and it is not stray config.** `.github/scripts/e2e-report-shape.sh`
* parses it out of this file by name, with a line-anchored pattern, and the advisory job compares
* the run it just did against it: this many tests should start, and all of them should fail.
* Deleting it, renaming it, or indenting it into a class stops the comparison — the report would
* keep printing with nothing to compare to, so it announces that it could not read the baseline
* rather than falling quiet. If you see that notice, this line is what it means.
*
* **One number, both checks, and that is what the marker means.** A test carrying it cannot pass
* on this image, so the count is 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.
*
* So: adding or removing a [FailsOnEmulatorApi37] means changing this number, in this file, in
* the same diff. The report says so on the run itself if you forget — it prints the tree's own
* `grep` count beside this one.
*
* Why a baseline at all (#83): that job is `continue-on-error` and red on every PR by design, so
* a red X cannot distinguish the known failures from the known failures plus a new one. Counting
* failures alone does not fix it either — the run is usually truncated by an
* `INSTRUMENTATION_ABORTED`, so the count is a number taken from a partial run. The report
* records the truncation next to the counts for that reason.
*/
const val FAILS_ON_EMULATOR_API37_BASELINE = 3
@@ -36,8 +36,20 @@ import java.io.File
* 1. that the hardware path is worth having a second engine for at all, and
* 2. that x264's CRF is worth the GPL licence the app carries for it.
*
* Skips itself when the sample files are absent, so it is harmless in CI. Populate with:
* adb push <file>.mp4 /sdcard/Android/data/org.libremediaconverter/files/
* Skips itself when the sample files are absent, so it is harmless in CI — every green E2E
* leg reports two skips, and these are they.
*
* The two files it looks for, by exact name:
*
* - [H264_SAMPLE] for [hardwareVersusSoftwareOnRealVideo]
* - [AV1_SAMPLE] for [av1InputRoutesAccordingToDeviceDecodeSupport]
*
* **Where they go, and how, is on [samples] — read it before staging anything.** This used to
* carry an `adb push` line naming the external files dir, which [samples] then explains cannot
* work: a pushed file stays owned by the shell user and the app reads EACCES, surfacing as an
* unparseable input rather than a permission error. The instruction and its own refutation sat
* twelve lines apart. It is named in one place now rather than restated here, because restating
* it is what let the two drift.
*/
@UnstableApi
@RunWith(AndroidJUnit4::class)
@@ -11,15 +11,26 @@ import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.FailsOnEmulatorApi37
import org.libremediaconverter.model.AudioCodec
import org.libremediaconverter.model.AudioPlan
import org.libremediaconverter.model.Container
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.CopyPlanner
import org.libremediaconverter.model.InputKind
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.model.OutputSpec
import org.libremediaconverter.model.VideoCodec
import org.libremediaconverter.model.VideoPlan
import java.io.File
import java.util.concurrent.CancellationException
import java.util.concurrent.Executors
import java.util.concurrent.TimeUnit
@@ -170,6 +181,63 @@ class Media3EngineTest {
)
}
/**
* The builders that used to throw where nothing could catch them.
*
* `EditedMediaItem.Builder` rejects a composition with both tracks removed —
* checkState("Audio and video cannot both be removed") — and the engine builds it on its own
* HandlerThread. That build sat *between* two narrow `runCatching` blocks, one around
* `buildTransformer` and one around `start`, so the exception reached the thread's uncaught
* handler and took the process with it while the continuation was never resumed.
*
* `ContainerCapabilities.validate` now refuses the spec that gets here from the picker; this
* is the other half — the engine surviving a request that arrives without being validated.
* Deliberately not `@FailsOnEmulatorApi37`: nothing here decodes or encodes, so no emulator
* codec is involved. The builder refuses the input before any media is touched.
*/
@Test
fun aPlanThatRemovesBothTracksFailsInsteadOfKillingTheProcess() {
val request = ConversionRequest(
spec = OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE),
probe = InputProbe(
videoCodec = null,
audioCodec = "mp3",
hasVideo = false,
container = Container.MP3,
kind = InputKind.AUDIO_ONLY,
),
)
// Asserted rather than assumed: ConversionRequest's default probe says hasVideo = true,
// and with it this same spec plans to (Encode, Drop) and nothing throws at all — which
// would make the whole test vacuous without a word of warning.
val plan = CopyPlanner.plan(request.spec, request.probe)
assertEquals(VideoPlan.Drop, plan.video)
assertEquals(AudioPlan.Drop, plan.audio)
val failure = runCatching {
runBlocking {
withTimeout(BUILDER_TIMEOUT_MS) {
engine.transcode(Uri.fromFile(input), output, request) {}
}
}
}.exceptionOrNull()
// Two assertions, and the second is not pedantry. withTimeout raises
// TimeoutCancellationException, and `java.util.concurrent.CancellationException` *extends*
// IllegalStateException — so testing only the type below would call an unresumed
// continuation a pass. A hang is the other half of this defect and every bit as bad as the
// crash: the worker would sit holding a foreground service forever.
assertFalse(
"the continuation was never resumed — the failure escaped instead of being reported: " +
"$failure",
failure is CancellationException,
)
assertTrue(
"the builder's refusal must surface as a failed job, not a dead process; got $failure",
failure is IllegalStateException,
)
}
private fun durationMsOf(file: File): Long {
val extractor = MediaExtractor()
return try {
@@ -211,5 +279,12 @@ class Media3EngineTest {
private companion object {
const val TIMEOUT_SECONDS = 120L
/**
* Short on purpose. Nothing is decoded or encoded on this path — the builder refuses the
* input outright — so anything approaching this is a hang, which is what the test is
* looking for.
*/
const val BUILDER_TIMEOUT_MS = 30_000L
}
}
@@ -101,7 +101,42 @@ sealed interface ConversionState {
val mimeType: String = "",
) : ConversionState
data class Saved(val displayName: String) : ConversionState
data class Failed(val message: String) : ConversionState
/**
* The job, or the save that followed it, could not be finished.
*
* [retry] is non-null for exactly one cause: a [ConversionViewModel.save] whose copy to the
* user's destination threw. That save deliberately keeps the staged file -- it can be the only
* copy of an hour of transcoding -- and this is what lets the screen offer it again. Every
* other failure leaves it null, because there is nothing staged to offer: a transcode that
* died produced no output, and a save that found the file gone has nothing left to save.
*
* Nullable rather than a `SaveFailed` state of its own. What the screen does with the message
* is identical either way, so a second variant would make every exhaustive `when` grow an arm
* that duplicates this one.
*
* A view of the file, not a second owner of it -- see [PendingSave].
*/
data class Failed(val message: String, val retry: PendingSave? = null) : ConversionState
}
/**
* The staged output a save would target from this state, or null when there is nothing to save.
*
* One function for two callers that have to agree. [ConversionViewModel.save] picks the file to
* copy with it, and `ConverterScreen` registers its `CreateDocument` contract with the MIME type
* it returns; when those two read the state separately, a retry offered after a failed save opened
* the dialog with the *picker's* current type instead of the finished job's -- wrong for any job
* whose spec has been edited since, and for every reattached job, whose spec was never in these
* settings at all.
*
* Top-level and `internal` rather than a member of the ViewModel, so the screen can call it
* without one -- which is also what makes the derivation testable on the JVM.
*/
internal fun ConversionState.pendingSave(): PendingSave? = when (this) {
is ConversionState.Converted -> PendingSave(staged, suggestedName, mimeType)
is ConversionState.Failed -> retry
else -> null
}
@UnstableApi
@@ -161,10 +196,12 @@ class ConversionViewModel @JvmOverloads constructor(
/**
* The staged output this ViewModel is responsible for deleting.
*
* A field rather than something read back out of [_state], because the state machine
* cannot answer the question on the path that needs it most: a failed [save] lands on
* [ConversionState.Failed], which carries a message and no file at all. By then the
* only remaining reference would have been lost.
* A field rather than something read back out of [_state], and still one now that
* [ConversionState.Failed] carries a [PendingSave] after a failed [save]. That handle is a
* view for the screen to offer a retry through; this one is the single reference [reset]
* deletes through, and keeping the two apart is what stops a second owner appearing. Reading
* the file back out of the state machine instead would mean trusting every state that has no
* file -- `Idle`, `Saved`, a transcode failure -- to say so.
*/
private var pendingStaged: File? = null
@@ -426,35 +463,50 @@ class ConversionViewModel @JvmOverloads constructor(
/**
* Copies the staged result out to the destination the user picked.
*
* Reached from [ConversionState.Converted] and again from a [ConversionState.Failed] that an
* earlier save left carrying its file. [pendingSave] is what makes those one call rather than
* two, so a retry cannot drift from the first attempt in what it copies or what it calls it.
*
* The existence check is not redundant with the one reattachment already made. That one ran
* inside a tag query which, for a result offered on launch, can be hours older than the tap —
* and `cacheDir` is exactly the directory the OS empties when it wants space, which is also
* what the sweep does to anything a day old. Without it the file's absence arrived as
* `staged.inputStream()` throwing, and `e.message` put a raw ENOENT path on screen.
* `staged.inputStream()` throwing, and `e.message` put a raw ENOENT path on screen. A retry
* meets that same check a second time, which is the point of reusing it here.
*/
fun save(destination: Uri) {
val converted = _state.value as? ConversionState.Converted ?: return
if (!converted.staged.isFile) {
val pending = _state.value.pendingSave() ?: return
if (!pending.staged.isFile) {
// No retry handle: the file such a state would offer again is exactly the one that
// has gone, so carrying it would put a button on screen that cannot do anything.
_state.value = ConversionState.Failed(STAGED_FILE_GONE_MESSAGE)
return
}
viewModelScope.launch {
runCatching {
withContext(Dispatchers.IO) {
publisher.publish(converted.staged, destination)
converted.staged.delete()
publisher.publish(pending.staged, destination)
pending.staged.delete()
}
}.onSuccess {
// publish() already deleted it; nothing left to clean up.
pendingStaged = null
_state.value = ConversionState.Saved(converted.suggestedName)
_state.value = ConversionState.Saved(pending.suggestedName)
}.onFailure { e ->
// Deliberately NOT cleared. A failed save may mean the staged file is the
// only copy of an hour of transcoding, and the user's destination did not
// receive it -- deleting here would destroy the work to tidy up a cache
// directory. It stays collectable: by a later reset(), or by the sweep once
// it is old enough to be certain nobody is coming back for it.
_state.value = ConversionState.Failed(e.message ?: "Could not save the file.")
//
// `pending` rides on the state so the screen can offer that file again. It used
// to live only in `pendingStaged`, where nothing on screen could reach it -- so
// the single button this branch rendered was "Start over", which deletes the very
// file the paragraph above goes out of its way to keep. It is `pending` rather
// than a fresh handle for the second failure's sake: a retry that fails again
// lands back here still carrying the file, not on a bare Failed that would take
// the offer away.
_state.value = ConversionState.Failed(e.message ?: "Could not save the file.", pending)
}
}
}
@@ -468,6 +520,12 @@ class ConversionViewModel @JvmOverloads constructor(
* cancelled with [viewModelScope] if the Activity finishes first, so it is a best
* effort rather than a guarantee. `OutputPublisher.sweepStaging` is the backstop for
* the times it does not run.
*
* **It still deletes from a [ConversionState.Failed] carrying a [PendingSave], and that is a
* decision rather than something inherited.** Deletion is acceptable there only because the
* alternative was offered first: the screen puts "Try saving again" directly above this
* button, so reaching it is the user saying the work is not worth keeping. Until that button
* existed, this delete was the only thing a failed save could lead to — which was the defect.
*/
fun reset() {
observer?.cancel()
@@ -73,8 +73,11 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode
// stands: some providers rewrite a document's extension to match it, so an MP3 offered as
// video/webm can arrive with the wrong one. Read straight off the collected state, so this
// recomposes because it depends on that rather than because an unrelated line happens to.
// Through pendingSave() rather than a cast to Converted, so a retry offered after a failed
// save opens the dialog with the type its first attempt used -- the cast answered null for a
// Failed, and the fallback below is the current picker, which a reattached job never set.
// Remembered against the type so the launcher re-registers only when it actually changes.
val destinationMime = (state as? ConversionState.Converted)?.mimeType ?: settings.spec.mimeType
val destinationMime = state.pendingSave()?.mimeType ?: settings.spec.mimeType
val chooseDestination = rememberLauncherForActivityResult(
remember(destinationMime) { ActivityResultContracts.CreateDocument(destinationMime) },
) { uri -> uri?.let(viewModel::save) }
@@ -325,13 +328,36 @@ internal fun ConverterScreenContent(
color = MaterialTheme.colorScheme.error,
style = MaterialTheme.typography.bodyMedium,
)
Button(
onClick = actions.onReset,
modifier = Modifier
.fillMaxWidth()
.height(PrimaryButtonHeight)
.testTag(TestTags.START_OVER),
) { Text("Start over") }
val retry = s.retry
if (retry == null) {
// Nothing was staged, so "Start over" is the whole of what is on
// offer and stays the primary button.
Button(
onClick = actions.onReset,
modifier = Modifier
.fillMaxWidth()
.height(PrimaryButtonHeight)
.testTag(TestTags.START_OVER),
) { Text("Start over") }
} else {
Button(
onClick = { actions.onSave(retry.suggestedName) },
modifier = Modifier
.fillMaxWidth()
.height(PrimaryButtonHeight)
.testTag(TestTags.RETRY_SAVE),
) { Text("Try saving again") }
// Start over still deletes the file this state is carrying, and that
// is deliberate: `reset()` is what stops a full-size output sitting in
// cache until the sweep. What makes the delete acceptable is the
// button above it. Deletion is the user's choice only once the
// alternative has been offered -- and until that button existed, this
// one was the only thing a failed save could lead to.
OutlinedButton(
onClick = actions.onReset,
modifier = Modifier.fillMaxWidth().testTag(TestTags.START_OVER),
) { Text("Start over") }
}
}
}
}
@@ -68,42 +68,64 @@ class Media3Engine(private val context: Context) : HardwareTranscoder {
): Unit = suspendCancellableCoroutine { cont ->
val plan = CopyPlanner.plan(request.spec, request.probe)
handler.post {
val transformer = runCatching { buildTransformer(plan, cont) }
.getOrElse {
cont.resumeWithException(it)
return@post
}
// Dropping the tracks the target does not have is what stops an audio-only export
// from carrying a re-encoded video track. Without setRemoveVideo, asking for M4A
// produced an HEVC stream in a file named .m4a.
val item = EditedMediaItem.Builder(MediaItem.fromUri(input))
.setRemoveVideo(plan.video == VideoPlan.Drop)
.setRemoveAudio(plan.audio == AudioPlan.Drop)
.build()
// A Composition is the only way to ask for transmuxing; the plain
// start(EditedMediaItem, path) overload always re-encodes. This is the remux path.
val composition = Composition.Builder(EditedMediaItemSequence.Builder(item).build())
.setTransmuxVideo(plan.video == VideoPlan.Copy)
.setTransmuxAudio(plan.audio == AudioPlan.Copy)
.build()
cont.invokeOnCancellation {
// cancel() has the same single-thread requirement as start().
handler.post { runCatching { transformer.cancel() } }
}
runCatching { transformer.start(composition, output.absolutePath) }
.onFailure {
cont.resumeWithException(it)
return@post
}
pollProgress(transformer, cont, onProgress)
// One guard around the whole body, deliberately.
//
// This used to be two narrow ones — around `buildTransformer` and around
// `transformer.start` — with the two Media3 builders sitting unguarded between them.
// On this thread that is not a small gap: nothing here has a caller to throw back to,
// so an escaping exception reaches the HandlerThread's uncaught handler and takes the
// process down, while [cont] is never resumed either way. `EditedMediaItem.Builder`
// does exactly that for a plan that drops both tracks
// ("Audio and video cannot both be removed"), which a queued job can still carry.
// Widening the guard costs nothing on success and turns every such refusal into a
// failed job with a reason.
runCatching { startExport(input, output, plan, cont, onProgress) }
.onFailure { if (cont.isActive) cont.resumeWithException(it) }
}
}
/**
* Builds the export and hands it to Transformer. Runs on the HandlerThread; may throw.
*
* Everything Transformer's single-thread contract covers lives here, so that the caller has
* exactly one place to catch. Returning normally means the export is running and [cont] belongs
* to the listener; throwing means it never started and the caller owns resuming.
*/
private fun startExport(
input: Uri,
output: File,
plan: ConversionPlan,
cont: CancellableContinuation<Unit>,
onProgress: (Int) -> Unit,
) {
val transformer = buildTransformer(plan, cont)
// Dropping the tracks the target does not have is what stops an audio-only export
// from carrying a re-encoded video track. Without setRemoveVideo, asking for M4A
// produced an HEVC stream in a file named .m4a.
val item = EditedMediaItem.Builder(MediaItem.fromUri(input))
.setRemoveVideo(plan.video == VideoPlan.Drop)
.setRemoveAudio(plan.audio == AudioPlan.Drop)
.build()
// A Composition is the only way to ask for transmuxing; the plain
// start(EditedMediaItem, path) overload always re-encodes. This is the remux path.
val composition = Composition.Builder(EditedMediaItemSequence.Builder(item).build())
.setTransmuxVideo(plan.video == VideoPlan.Copy)
.setTransmuxAudio(plan.audio == AudioPlan.Copy)
.build()
// Registered before start(), so a cancellation racing the export always finds a
// transformer to cancel.
cont.invokeOnCancellation {
// cancel() has the same single-thread requirement as start().
handler.post { runCatching { transformer.cancel() } }
}
transformer.start(composition, output.absolutePath)
pollProgress(transformer, cont, onProgress)
}
/**
* @throws IllegalArgumentException if [plan] names a container Media3 cannot mux. That is a
* routing bug rather than a runtime condition — [org.libremediaconverter.model.ConversionRouter]
@@ -23,6 +23,24 @@ const val STAGED_FILE_GONE_MESSAGE: String =
"The finished file is no longer in the cache, so there is nothing left to save. " +
"Start over to make it again."
/**
* A staged file that is still there to be saved, and everything the save dialog needs to offer it.
*
* The three travel together because a save cannot be repeated without all of them: the file to
* copy, the name to suggest, and the MIME type `CreateDocument` has to be registered with. None of
* them can be rederived from the pickers once the job is over -- they come from the job's own
* output `Data`, and a reattached job's spec was never in the current settings at all.
*
* Kept next to [STAGED_FILE_GONE_MESSAGE] for the same reason it is: both ViewModels need it and
* staging is what it is about.
*
* **A view of the staged file, never an owner of it.** The delete still runs through each
* ViewModel's own `pendingStaged` field, so a state carrying one of these can be dropped without
* losing the only reference -- which is what keeps "a `Failed` that carries a file" from being a
* new way to leak one.
*/
data class PendingSave(val staged: File, val suggestedName: String, val mimeType: String)
/**
* Staging and publication of conversion output.
*
@@ -46,9 +46,10 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod
// The contract's MIME type comes from the finished job rather than from a literal: some
// providers rewrite a document's extension to match it, so naming MP4 for a join that is not
// one can hand the user a file the extension lies about. Remembered against that type so the
// launcher re-registers only when it actually changes.
val destinationMime = (state as? JoinState.Joined)?.mimeType ?: ConcatWorker.DEFAULT_FORMAT.mimeType
// one can hand the user a file the extension lies about. Through pendingSave() rather than a
// cast to Joined, so a retry after a failed save opens with the type its first attempt used.
// Remembered against that type so the launcher re-registers only when it actually changes.
val destinationMime = state.pendingSave()?.mimeType ?: ConcatWorker.DEFAULT_FORMAT.mimeType
val chooseDestination = rememberLauncherForActivityResult(
remember(destinationMime) { ActivityResultContracts.CreateDocument(destinationMime) },
) { uri -> uri?.let(viewModel::save) }
@@ -226,13 +227,32 @@ internal fun JoinScreenContent(state: JoinState, actions: JoinActions, modifier:
color = MaterialTheme.colorScheme.error,
style = MaterialTheme.typography.bodyMedium,
)
Button(
onClick = actions.onReset,
modifier = Modifier
.fillMaxWidth()
.height(PrimaryButtonHeight)
.testTag(TestTags.START_OVER),
) { Text("Start over") }
val retry = s.retry
if (retry == null) {
// Nothing staged, so "Start over" is all there is and stays primary.
Button(
onClick = actions.onReset,
modifier = Modifier
.fillMaxWidth()
.height(PrimaryButtonHeight)
.testTag(TestTags.START_OVER),
) { Text("Start over") }
} else {
Button(
onClick = { actions.onSave(retry.suggestedName) },
modifier = Modifier
.fillMaxWidth()
.height(PrimaryButtonHeight)
.testTag(TestTags.RETRY_SAVE),
) { Text("Try saving again") }
// Start over still deletes the carried file, for the reason the
// converter screen writes out next to the same pair of buttons:
// the delete is a choice only once the alternative is on screen.
OutlinedButton(
onClick = actions.onReset,
modifier = Modifier.fillMaxWidth().testTag(TestTags.START_OVER),
) { Text("Start over") }
}
}
}
}
@@ -18,6 +18,7 @@ import kotlinx.coroutines.withContext
import org.libremediaconverter.convert.ConversionDependencies
import org.libremediaconverter.convert.InputFile
import org.libremediaconverter.convert.InputQuery
import org.libremediaconverter.convert.PendingSave
import org.libremediaconverter.convert.STAGED_FILE_GONE_MESSAGE
import org.libremediaconverter.model.ConcatStrategy
import org.libremediaconverter.work.ConcatWorker
@@ -44,7 +45,30 @@ sealed interface JoinState {
val mimeType: String,
) : JoinState
data class Saved(val displayName: String) : JoinState
data class Failed(val message: String) : JoinState
/**
* The join, or the save that followed it, could not be finished.
*
* [retry] is non-null for exactly one cause, and for the same reason as on
* `ConversionState.Failed`: a [JoinViewModel.save] whose copy to the user's destination threw
* keeps the staged file, and this is what lets the screen offer it again. Every other failure
* leaves it null — a join that died produced no output, and a save that found the file gone
* has nothing left to save.
*/
data class Failed(val message: String, val retry: PendingSave? = null) : JoinState
}
/**
* The staged output a save would target from this state, or null when there is nothing to save.
*
* The join tab's half of `ConversionState.pendingSave`, and it exists for the same reason: `save`
* and `JoinScreen`'s `CreateDocument` registration both have to answer this question, and answering
* it twice is how a retry ends up opening the dialog with a type the finished job never chose.
*/
internal fun JoinState.pendingSave(): PendingSave? = when (this) {
is JoinState.Joined -> PendingSave(staged, suggestedName, mimeType)
is JoinState.Failed -> retry
else -> null
}
@UnstableApi
@@ -70,9 +94,10 @@ class JoinViewModel @JvmOverloads constructor(
* The staged output this ViewModel is responsible for deleting.
*
* Held here rather than read back out of [_state] for the same reason as in
* `ConversionViewModel`: a failed [save] lands on [JoinState.Failed], which carries a
* message and no file, so the state machine cannot answer this on the one path that
* most needs it.
* `ConversionViewModel`, and still held here now that [JoinState.Failed] carries a
* [PendingSave] after a failed [save]: that handle is a view for the screen to offer a retry
* through, this one is the single reference [reset] deletes through, and keeping the two
* apart is what stops a second owner of the file appearing.
*/
private var pendingStaged: File? = null
@@ -229,29 +254,38 @@ class JoinViewModel @JvmOverloads constructor(
* join offered by reattachment was last seen during a tag query that may be hours old, and
* `cacheDir` is reclaimed by the OS and swept by this app. Without it the file's absence
* reached the screen as a raw ENOENT path.
*
* Reached from [JoinState.Joined] and again from a [JoinState.Failed] an earlier save left
* carrying its file; [pendingSave] is what makes those the same call.
*/
fun save(destination: Uri) {
val joined = _state.value as? JoinState.Joined ?: return
if (!joined.staged.isFile) {
val pending = _state.value.pendingSave() ?: return
if (!pending.staged.isFile) {
// No retry handle -- the file it would offer again is the one that has gone.
_state.value = JoinState.Failed(STAGED_FILE_GONE_MESSAGE)
return
}
viewModelScope.launch {
runCatching {
withContext(Dispatchers.IO) {
publisher.publish(joined.staged, destination)
joined.staged.delete()
publisher.publish(pending.staged, destination)
pending.staged.delete()
}
}.onSuccess {
// publish() already deleted it; nothing left to clean up.
pendingStaged = null
_state.value = JoinState.Saved(joined.suggestedName)
_state.value = JoinState.Saved(pending.suggestedName)
}.onFailure { e ->
// Deliberately NOT cleared -- see the same branch in ConversionViewModel.
// A failed save can leave the staged file as the only copy of the work, so
// it is left for a later reset() or for the sweep to collect once its age
// makes it certain nobody is coming back for it.
_state.value = JoinState.Failed(e.message ?: "Could not save the file.")
//
// `pending` travels on the state so the screen can offer the file again rather
// than leaving "Start over" -- which deletes it -- as the only thing on offer.
// Passing `pending` rather than rebuilding it is what keeps a retry that fails
// again on a carrying Failed instead of a bare one.
_state.value = JoinState.Failed(e.message ?: "Could not save the file.", pending)
}
}
}
@@ -261,6 +295,10 @@ class JoinViewModel @JvmOverloads constructor(
*
* Best effort, not a guarantee: the delete is cancelled with [viewModelScope] if the
* Activity finishes first. `OutputPublisher.sweepStaging` is the backstop.
*
* It deletes from a [JoinState.Failed] carrying a [PendingSave] too, deliberately and for the
* reason `ConversionViewModel.reset` writes out: the screen offers "Try saving again" above
* this button, so deletion is what the user chose rather than all this state could do.
*/
fun reset() {
observer?.cancel()
@@ -129,9 +129,25 @@ object ContainerCapabilities {
}
}
if (spec.videoCodec == VideoCodec.NONE && spec.audioCodec == AudioCodec.NONE) {
// Two faces of one rule: the output would carry no tracks at all.
//
// The first is visible in the spec alone — NONE on both axes. The second only emerges once
// the spec meets the probe, because [CopyPlanner] drops a video track the *input* does not
// have no matter which codec was named for it, so "H.265 + no audio" on an MP3 plans to
// (Drop, Drop) exactly as "None + None" does. Asking the spec alone answered the first and
// missed the second, and the miss was not cosmetic: `EditedMediaItem.Builder` refuses that
// composition with IllegalStateException("Audio and video cannot both be removed"), on
// Transformer's own thread, where the user would have seen a dead app rather than a reason.
if (spec.audioCodec == AudioCodec.NONE && (spec.videoCodec == VideoCodec.NONE || !probe.hasVideo)) {
return Validation.Invalid(
"This would produce an empty file — keep at least one track.",
if (spec.videoCodec == VideoCodec.NONE) {
"This would produce an empty file — keep at least one track."
} else {
// Names both halves. "No video track" alone reads as though the video setting
// were the only thing wrong, and the user would fix that and still be stuck.
"This file has no video track, so turning the audio off too would produce an " +
"empty file."
},
suggestions(
// Ask for both tracks back, then let repair settle what this container and
// this input can actually give.
@@ -284,8 +300,12 @@ object ContainerCapabilities {
private fun repairVideo(spec: OutputSpec, probe: InputProbe): VideoCodec {
val container = spec.container
if (spec.videoCodec == VideoCodec.NONE || !container.canHoldVideo) return VideoCodec.NONE
// There is no video track to make one out of, so naming a codec would be a suggestion
// [CopyPlanner] drops on the floor. It also read as a non-sequitur: before this line, the
// repair offered for an MP3 was "H.264", the first codec MP4 happens to encode.
if (!probe.hasVideo) return VideoCodec.NONE
val source = CodecNames.videoFromName(probe.videoCodec).takeIf { probe.hasVideo }
val source = CodecNames.videoFromName(probe.videoCodec)
val copyable = source != null && accepts(container, source, CodecMode.COPY)
return when {
@@ -45,6 +45,17 @@ object TestTags {
const val SAVE_FILE: String = "action.saveFile"
/**
* The retry a `Failed` offers after a save that threw, on both screens.
*
* Its own tag rather than [SAVE_FILE], because the two are different claims about the screen.
* [SAVE_FILE] is the first attempt from a finished job; this one may appear only where a staged
* file survived a failed save. Sharing a tag would collapse "a transcode failure offers nothing
* to save" and "a failed save offers the file again" into one query, and that first assertion
* is the one stopping a Save button from appearing where there is nothing to save.
*/
const val RETRY_SAVE: String = "action.retrySave"
/** `ConverterScreen`. */
object Converter {
const val CHOOSE_FILE: String = "converter.chooseFile"
@@ -0,0 +1,67 @@
package org.libremediaconverter.ci
import org.junit.Assert.assertTrue
import org.junit.Test
import java.io.File
/**
* That the release job still holds the one permission it needs to publish.
*
* `build.yml`'s `release` job declares `contents: write`, and nothing was checking it. Deleting
* those two lines leaves actionlint clean and CodeQL silent — a *narrower* permission is not an
* alert — and the job is `if: startsWith(github.ref, 'refs/tags/v')`, so no pull request and no
* merge to `main` can exercise it. Measured: with the declaration removed, every gating check
* still passes. The first thing that would notice is a release failing to publish, at the moment
* someone is trying to cut one.
*
* The deletion also looks like tidying. A top-level `permissions: contents: read` now sits
* directly above it, so a reader could reasonably take the job-level block for a duplicate. It is
* an override, not a duplicate, and a comment saying so is not a check.
*
* `BackupExclusionsTest` is the precedent: a file that is configuration rather than code, load
* bearing, and unguarded because nothing compiles it.
*
* **What this pins, and what it does not.** It asserts the declaration exists in the `release`
* job's block. It cannot assert that a release actually publishes — that needs a tag push, which
* is the thing no PR can do. So this is a tripwire against silent removal, not proof the release
* path works.
*/
class ReleasePermissionTest {
@Test
fun `the release job declares the write permission it needs to publish`() {
val release = jobBlock("release")
assertTrue(
"build.yml's `release` job no longer declares `contents: write`. It is the only " +
"permission that lets the job create a release, the top-level block above it is " +
"`contents: read`, and nothing else in CI would catch this until a tag failed to " +
"publish. If the release moved elsewhere, delete this test deliberately.",
release.any { it.trimStart().startsWith("contents: write") },
)
}
/**
* The lines of one top-level job, from its ` <name>:` header to the next job at that indent.
*
* Line-based rather than parsed: the module has no YAML dependency, and adding one to read two
* lines would be a worse trade than a scan that fails loudly when the shape changes.
*/
private fun jobBlock(name: String): List<String> {
val lines = workflow.readLines()
val start = lines.indexOfFirst { it == " $name:" }
check(start >= 0) { "no ` $name:` job in ${workflow.path} — has the file been restructured?" }
val rest = lines.drop(start + 1)
val end = rest.indexOfFirst { it.matches(Regex("^ {2}[A-Za-z0-9_-]+:.*")) }
return if (end < 0) rest else rest.take(end)
}
/**
* Found by walking up rather than by a fixed relative path: Gradle's working directory for the
* unit tests is the module, but that is a default rather than a promise.
*/
private val workflow: File
get() = generateSequence(File(".").absoluteFile) { it.parentFile }
.map { File(it, ".github/workflows/build.yml") }
.firstOrNull { it.isFile }
?: error("could not find .github/workflows/build.yml above ${File(".").absolutePath}")
}
@@ -93,8 +93,12 @@ class ConversionViewModelCleanupTest {
assertTrue("a failed save must not destroy the only copy", staged.exists())
assertEquals(emptyList<File>(), publisher.discarded)
// Failed carries no file reference at all, so this only works because the handle is
// a ViewModel field rather than something read back out of the state machine.
// The handle is a ViewModel field rather than something read back out of the state
// machine, and stays one now that a save-failed `Failed` also carries a `PendingSave`:
// that is a view for the screen to offer a retry through, never a second owner of the
// file. This delete goes through the field, which is what keeps a state that is dropped
// rather than read from taking the only reference with it. What the state carries, and
// what the screen then does with it, are `FailedSaveRetryTest`'s.
viewModel.reset()
assertEquals(listOf(staged), publisher.discarded)
@@ -140,10 +140,16 @@ class ConversionViewModelProbeFailureTest {
private fun pickedProbe(): InputProbe? {
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
viewModel.onInputPicked(INPUT)
// The predicate is the guard, and it is the only one needed. It requires `Ready`, so a
// pick that ended in `Failed` never satisfies it and `awaitState` fails on its timeout
// naming what it was waiting for -- "Ready with a probe" -- which says more than a
// separate assertion could. A `ready as? ConversionState.Failed` check used to sit here
// and was dead: `Ready` and `Failed` are sibling subtypes of one sealed interface, so
// the cast was always null and the assertNull could never fire. Measured, not assumed --
// flipping it to assertNotNull failed all three callers of this helper.
val ready = awaitState(viewModel.state, "Ready with a probe") {
it is ConversionState.Ready && it.input.probe != null
}
assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed))
return (ready as ConversionState.Ready).input.probe
}
@@ -53,10 +53,11 @@ import java.io.File
* it -- so it is unobservable from a JVM test, the same limit `FileCardTest` records for
* `HorizontalDivider`. The message text itself is asserted; the colour would need a screenshot.
* - **The three `assertDoesNotExist` checks on [TestTags.Converter.FILE_CARD] are compile-guarded,
* not guarded by this file.** `Idle` is a `data object`, and `Saved` and `Failed` carry only a
* `displayName` and a `message`; none of the three has an `input`, so `FileCard(s.input)` does not
* compile in those arms. The lines stay because they state the intent cheaply, but they are not
* what stops a `FileCard` appearing there and this file does not claim they are.
* not guarded by this file.** `Idle` is a `data object`, `Saved` carries a `displayName`, and
* `Failed` carries a message and -- after a failed save only -- the staged file it left behind;
* none of the three has an `input`, so `FileCard(s.input)` does not compile in those arms. The
* lines stay because they state the intent cheaply, but they are not what stops a `FileCard`
* appearing there and this file does not claim they are.
* - **Which constant each chip hands back** belongs to `ConverterPickerSelectionTest`, and **what
* the file card says about an unknown size** to `FileCardTest`. This file asserts that `Ready`
* puts those leaves on screen at all, not what they then do.
@@ -326,6 +327,22 @@ class ConverterStateAffordancesTest {
composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD).assertDoesNotExist()
}
/**
* **The assertion that bounds #30's whole change**, and the one worth breaking things to keep.
*
* A transcode that died staged nothing, so its `Failed` carries no [PendingSave] and there is
* nothing for a save dialog to be handed. Making the retry unconditional -- or making the
* `WorkInfo.State.FAILED` arm of `ConversionViewModel.observe` carry a handle it has no file
* for -- puts a button on screen that can only fail, and this is what notices.
*/
@Test
fun `a transcode failure offers no way to save`() {
setContent(ConversionState.Failed(message = "Ran out of space while writing the output."))
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertDoesNotExist()
composeRule.onNodeWithTag(TestTags.SAVE_FILE).assertDoesNotExist()
}
@Test
fun `tapping start over after a failure resets and does nothing else`() {
setContent(ConversionState.Failed(message = "Ran out of space while writing the output."))
@@ -335,6 +352,50 @@ class ConverterStateAffordancesTest {
assertEquals(listOf("reset"), fired)
}
// ----------------------------------------------------- Failed, carrying a file
/**
* The defect in #30, stated as what the branch must render.
*
* `save()` keeps the staged file on a failure deliberately -- it can be the only copy of an
* hour of transcoding -- and before this the only control here was "Start over", wired to
* `reset()`, which deletes exactly that file. Both buttons, not one: the restart has to stay
* reachable, because leaving a full-size file in cache is the outcome it exists to avoid.
*/
@Test
fun `a failed save offers the file again as well as a restart`() {
setContent(failedSave())
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertExists()
composeRule.onNodeWithTag(TestTags.START_OVER).assertExists()
}
/**
* The name, not just that something fired: it comes from the finished job, and a retry wired to
* a literal or to the picker's current guess would hand the dialog a name the job never chose.
*/
@Test
fun `tapping try saving again hands back the name the job chose`() {
setContent(failedSave())
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).performScrollTo().performClick()
assertEquals(listOf("save:holiday.mp4"), fired)
}
/**
* Start over from here still resets, and resetting still deletes -- see `reset()`'s KDoc for
* why that is acceptable now and was not before. What it must not do is save on the way past.
*/
@Test
fun `tapping start over after a failed save resets and does not save`() {
setContent(failedSave())
composeRule.onNodeWithTag(TestTags.START_OVER).performScrollTo().performClick()
assertEquals(listOf("reset"), fired)
}
// ------------------------------------------------------------------ Harness
private fun input() = InputFile(
@@ -348,6 +409,21 @@ class ConverterStateAffordancesTest {
* missing file rather than throwing, so the size line reads `0 B` and no temporary folder is
* needed to render the arm.
*/
/**
* A `Failed` an earlier save left carrying its file, which is the only way [retry] is non-null.
*
* The same missing `staged` path as [converted], and for the same reason: this arm renders no
* size line at all, so nothing here ever touches the filesystem.
*/
private fun failedSave() = ConversionState.Failed(
message = "There was not enough room on the destination.",
retry = PendingSave(
staged = File("no-such-staged-output.mp4"),
suggestedName = "holiday.mp4",
mimeType = "video/mp4",
),
)
private fun converted(routeReason: String = "") = ConversionState.Converted(
input = input(),
staged = File("no-such-staged-output.mp4"),
@@ -0,0 +1,370 @@
package org.libremediaconverter.convert
import android.app.Application
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import androidx.work.workDataOf
import kotlinx.coroutines.Dispatchers
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.join.JoinState
import org.libremediaconverter.join.JoinViewModel
import org.libremediaconverter.join.pendingSave
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputFormat
import org.libremediaconverter.work.ConcatWorker
import org.libremediaconverter.work.ConversionWorker
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
/**
* A failed save has to leave the file *offerable*, not merely undeleted.
*
* The defect is #30, and both halves of it were already written down in `main`. `save()`'s
* `onFailure` kept the staged file on purpose -- "deleting here would destroy the work to tidy up
* a cache directory" -- and then handed the screen a `Failed` carrying a message and nothing else,
* so the single control that branch rendered was "Start over", wired to `reset()`, which deletes
* exactly that file. The intent and the affordance disagreed, and the affordance won.
*
* `ConversionViewModelCleanupTest` already pins the *keeping*: after a failed save the file is
* still on disk and nothing has been discarded. It stays green with the state carrying nothing,
* because it reads the filesystem rather than the state. This file asserts the other half -- that
* the handle reaches the state a screen can read -- and the negative that bounds it: a failure
* with nothing staged behind it must not sprout a save button.
*
* Both ViewModels in one class, following `MissingStagedFileTest`. They are separate state
* machines that can each hold a staged file at once, but this defect and its fix are the same
* shape in both, and splitting them would put the two halves of one invariant in two files.
*
* ### Not asserted here, so each is a decision rather than an omission
*
* - **That the destination received the bytes.** [RecordingPublisher.publish] is a stub, which is
* the only way to make a save fail deterministically -- and making it fail is what every case
* here needs. `OutputPublisherPublishTest` owns what a real publish writes.
* - **The screen's two buttons.** `ConverterStateAffordancesTest` and `JoinStateAffordancesTest`
* own what each state renders; this file owns what each state carries.
* - **`ConverterScreen`'s `destinationMime` line itself.** It lives in the entry point, above the
* `ScreenContent` seam, and reaching it needs a real ViewModel inside a composition. What it
* reads -- `pendingSave()?.mimeType` -- is asserted directly instead, which is why that
* derivation was moved out of the entry point in the first place.
* - **Picking a new input while a `Failed` carries a file.** `onInputPicked` overwrites the state
* without discarding, from `Converted` exactly as much as from a carrying `Failed`, and neither
* branch renders a picker. It is a pre-existing path this change neither opens nor widens: the
* carried handle is a view of `pendingStaged`, never a second owner of the file.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class FailedSaveRetryTest {
private lateinit var app: Application
private lateinit var publisher: RecordingPublisher
private lateinit var staged: File
@Before
fun setUp() {
app = RuntimeEnvironment.getApplication()
publisher = RecordingPublisher(app)
ConversionDependencies.publisher = { publisher }
// MediaProbe spawns FFprobe, whose loader throws with no native library present.
ConversionDependencies.probe = { _, _ -> InputProbe() }
staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) }
}
@After
fun tearDown() {
ConversionDependencies.reset()
}
// ------------------------------------------------------------------ Convert
/**
* The bite named in #30's fix. Emitting a plain `Failed` from `save()`'s `onFailure` -- which
* is what `main` did -- reddens this case on the null handle, and nothing else in the suite.
*/
@Test
fun `a failed save leaves the staged file offerable, not merely undeleted`() {
val viewModel = failedSaveViewModel()
val failed = viewModel.state.value as ConversionState.Failed
val retry = failed.retry
assertNotNull("a failed save must leave the staged file offerable, not just on disk", retry)
assertEquals("the retry must name the file the conversion actually produced", staged, retry?.staged)
// Both from the job's own output Data rather than from the pickers, so a retry opens the
// same dialog the first attempt did.
assertEquals(SUGGESTED_NAME, retry?.suggestedName)
assertEquals(JOB_MIME_TYPE, retry?.mimeType)
assertTrue("a failed save must not destroy the only copy", staged.exists())
}
/**
* The whole point of carrying the handle: the second attempt is a real save, not a new job.
*
* `publishFailure` is cleared between the two calls, so one `RecordingPublisher` plays both a
* full destination and an empty one -- which is exactly the user's situation.
*/
@Test
fun `retrying a failed save publishes the file and leaves nothing staged`() {
val viewModel = failedSaveViewModel()
publisher.publishFailure = null
viewModel.save(DESTINATION)
val saved = awaitState(viewModel.state, "Saved") { it is ConversionState.Saved }
assertEquals(SUGGESTED_NAME, (saved as ConversionState.Saved).displayName)
assertFalse("a successful retry should have removed the staged file", staged.exists())
// Nothing to collect afterwards: the retry published it, so reset() has no work left.
viewModel.reset()
assertEquals(emptyList<File>(), publisher.discarded)
}
/**
* The second failure must not eat the file the first one kept.
*
* A `Failed` built fresh from `e.message` alone would drop the handle here while every other
* assertion in this file stayed green -- the file is still on disk, and the first failure
* already proved the state can carry it.
*/
@Test
fun `a retry that fails again still carries the file rather than dropping it`() {
val viewModel = failedSaveViewModel()
publisher.publishFailure = IllegalStateException("destination volume still full")
viewModel.save(DESTINATION)
// Waited for by the *second* message rather than by `is Failed`: the state was already
// Failed when the retry started, so the type alone would be satisfied before it ran.
val failed = awaitState(viewModel.state, "the second failure") {
it is ConversionState.Failed && it.message == "destination volume still full"
} as ConversionState.Failed
assertEquals("the second failure must offer the same file the first one did", staged, failed.retry?.staged)
assertTrue(staged.exists())
assertEquals(emptyList<File>(), publisher.discarded)
}
/**
* "Start over" still deletes, and that is the decision `reset()`'s KDoc records: acceptable
* only because "Try saving again" is on screen beside it. Exactly once, through the publisher.
*/
@Test
fun `start over from a failed save discards the carried file exactly once`() {
val viewModel = failedSaveViewModel()
viewModel.reset()
assertEquals(ConversionState.Idle, viewModel.state.value)
assertEquals(listOf(staged), publisher.discarded)
assertFalse(staged.exists())
}
/**
* A retry meets the same existence check the first attempt did, so a file collected by the
* sweep or by the OS in between is reported as a sentence rather than as a raw ENOENT path.
* And the state that reports it carries nothing: there is no file left to offer.
*/
@Test
fun `a retry whose staged file has gone says so and offers nothing further`() {
val viewModel = failedSaveViewModel()
assertTrue("the fixture must start with a real staged file", staged.delete())
publisher.publishFailure = null
viewModel.save(DESTINATION)
val failed = viewModel.state.value as ConversionState.Failed
assertEquals(STAGED_FILE_GONE_MESSAGE, failed.message)
assertNull("a file that has gone cannot be offered again", failed.retry)
}
/**
* The negative that bounds the whole change, and the reason `retry` is nullable.
*
* A transcode that died staged nothing, so there is no file to hand back -- and a `Failed`
* that carried one anyway would put a save button on a screen with nothing to save. Driven
* through a worker that really fails rather than by constructing the state, because the line
* under test is the `WorkInfo.State.FAILED` arm of `observe`.
*/
@Test
fun `a transcode failure carries nothing to save`() {
installFailingTestWorkManager(app, workDataOf(ConversionWorker.KEY_ERROR to "The encoder gave up."))
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
viewModel.onInputPicked(Uri.parse("content://test/holiday.mp4"))
awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
viewModel.convert()
val failed = awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } as ConversionState.Failed
assertEquals("The encoder gave up.", failed.message)
assertNull("a transcode failure has nothing staged, so it must offer no save", failed.retry)
assertNull("and nothing for the save dialog to open with either", failed.pendingSave())
}
/**
* What the save dialog reopens with, which is the entry point's only reader of this state.
*
* The pickers are moved *after* the job finishes, which is what makes this bite: a retry that
* asked the current settings would offer `audio/mpeg` for a file the job wrote as MP4. The
* same gap is permanent for a reattached job, whose spec was never in these settings at all.
*/
@Test
fun `a retry offers the type the job chose, not the one the pickers now show`() {
val viewModel = failedSaveViewModel()
viewModel.setPreset(OutputFormat.MP3)
assertEquals(
"the fixture needs the pickers to disagree with the job",
"audio/mpeg",
viewModel.settings.value.spec.mimeType,
)
assertEquals(JOB_MIME_TYPE, viewModel.state.value.pendingSave()?.mimeType)
}
// --------------------------------------------------------------------- Join
@Test
fun `a failed join save leaves the staged file offerable, not merely undeleted`() {
val viewModel = failedJoinSaveViewModel()
val failed = viewModel.state.value as JoinState.Failed
val retry = failed.retry
assertNotNull("a failed save must leave the staged file offerable, not just on disk", retry)
assertEquals(staged, retry?.staged)
assertEquals(SUGGESTED_NAME, retry?.suggestedName)
assertEquals(JOB_MIME_TYPE, retry?.mimeType)
assertTrue(staged.exists())
}
@Test
fun `retrying a failed join save publishes the file and leaves nothing staged`() {
val viewModel = failedJoinSaveViewModel()
publisher.publishFailure = null
viewModel.save(DESTINATION)
val saved = awaitState(viewModel.state, "Saved") { it is JoinState.Saved }
assertEquals(SUGGESTED_NAME, (saved as JoinState.Saved).displayName)
assertFalse(staged.exists())
viewModel.reset()
assertEquals(emptyList<File>(), publisher.discarded)
}
@Test
fun `a join retry that fails again still carries the file rather than dropping it`() {
val viewModel = failedJoinSaveViewModel()
publisher.publishFailure = IllegalStateException("destination volume still full")
viewModel.save(DESTINATION)
// By the second message, not by `is Failed` -- see the converter case above.
val failed = awaitState(viewModel.state, "the second failure") {
it is JoinState.Failed && it.message == "destination volume still full"
} as JoinState.Failed
assertEquals(staged, failed.retry?.staged)
assertTrue(staged.exists())
assertEquals(emptyList<File>(), publisher.discarded)
}
@Test
fun `start over from a failed join save discards the carried file exactly once`() {
val viewModel = failedJoinSaveViewModel()
viewModel.reset()
assertEquals(JoinState.Idle, viewModel.state.value)
assertEquals(listOf(staged), publisher.discarded)
assertFalse(staged.exists())
}
@Test
fun `a join failure carries nothing to save`() {
installFailingTestWorkManager(app, workDataOf(ConcatWorker.KEY_ERROR to "The files could not be joined."))
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4")))
awaitState(viewModel.state, "Ready") { it is JoinState.Ready }
viewModel.join()
val failed = awaitState(viewModel.state, "Failed") { it is JoinState.Failed } as JoinState.Failed
assertEquals("The files could not be joined.", failed.message)
assertNull("a join failure has nothing staged, so it must offer no save", failed.retry)
assertNull(failed.pendingSave())
}
// ------------------------------------------------------------------ Harness
/**
* A ViewModel driven to `Converted` and then through a save that threw.
*
* The WorkManager is installed here rather than in `@Before`, because two cases in this class
* need one whose workers fail instead.
*/
private fun failedSaveViewModel(): ConversionViewModel {
installTestWorkManager(app, conversionOutput())
// Unconfined so reset()'s delete runs inline instead of on a real IO thread.
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv"))
awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
viewModel.convert()
awaitState(viewModel.state, "Converted") { it is ConversionState.Converted }
publisher.publishFailure = IllegalStateException("destination volume full")
viewModel.save(DESTINATION)
awaitState(viewModel.state, "Failed") { it is ConversionState.Failed }
return viewModel
}
/** The join tab's equivalent, driven to `Joined` and then through a save that threw. */
private fun failedJoinSaveViewModel(): JoinViewModel {
installTestWorkManager(app, joinOutput())
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4")))
awaitState(viewModel.state, "Ready") { it is JoinState.Ready }
viewModel.join()
awaitState(viewModel.state, "Joined") { it is JoinState.Joined }
publisher.publishFailure = IllegalStateException("destination volume full")
viewModel.save(DESTINATION)
awaitState(viewModel.state, "Failed") { it is JoinState.Failed }
return viewModel
}
/**
* The output `Data` a finished conversion reports.
*
* The name and type are set rather than left out, so the assertions above are about what the
* *job* chose. Both ViewModels fall back to a derivation when they are missing, and a fixture
* that omitted them would be asserting the fallback while looking like it asserted the job.
*
* Spelled out per worker rather than shared with [joinOutput], even though the two constants
* hold the same strings today. A test that leaned on that would be asserting a coincidence.
*/
private fun conversionOutput() = workDataOf(
ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath,
ConversionWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME,
ConversionWorker.KEY_MIME_TYPE to JOB_MIME_TYPE,
)
/** The output `Data` a finished join reports. See [conversionOutput]. */
private fun joinOutput() = workDataOf(
ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath,
ConcatWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME,
ConcatWorker.KEY_MIME_TYPE to JOB_MIME_TYPE,
)
private companion object {
val DESTINATION: Uri = Uri.parse("content://test/destination.mp4")
const val SUGGESTED_NAME = "holiday.mp4"
/** What the job wrote. [OutputFormat.MP3]'s `audio/mpeg` is what the pickers move to. */
const val JOB_MIME_TYPE = "video/mp4"
}
}
@@ -0,0 +1,105 @@
package org.libremediaconverter.convert
import android.net.Uri
import androidx.media3.common.util.UnstableApi
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.model.AudioCodec
import org.libremediaconverter.model.AudioPlan
import org.libremediaconverter.model.Container
import org.libremediaconverter.model.ConversionRequest
import org.libremediaconverter.model.CopyPlanner
import org.libremediaconverter.model.InputKind
import org.libremediaconverter.model.InputProbe
import org.libremediaconverter.model.OutputSpec
import org.libremediaconverter.model.VideoCodec
import org.libremediaconverter.model.VideoPlan
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import java.io.File
import java.util.concurrent.CancellationException
/**
* What happens when Media3 refuses the export before it starts.
*
* `EditedMediaItem.Builder` rejects a composition with both tracks removed —
* checkState("Audio and video cannot both be removed") — and [Media3Engine] builds it on its own
* HandlerThread. That build used to sit *between* two narrow `runCatching` blocks, one around
* `buildTransformer` and one around `start`, so the exception escaped `handler.post`'s body: it
* reached the thread's uncaught handler, which on Android takes the process down, and the
* continuation was left unresumed either way.
*
* Robolectric runs the real [android.os.HandlerThread] and the real Media3 builders, so the whole
* sequence happens here — the engine really posts, really builds, and really throws. What it cannot
* reproduce is the *consequence* of an escaped throw: a JVM background thread dying is not process
* death. So the assertion is on the half that is observable everywhere and is the half that
* matters to the user — the suspension is resolved, with the reason, rather than left hanging.
* `Media3EngineTest.aPlanThatRemovesBothTracksFailsInsteadOfKillingTheProcess` is the same case on
* a device.
*/
@UnstableApi
@RunWith(RobolectricTestRunner::class)
class Media3EngineEmptyCompositionTest {
@Test
fun `a plan that removes both tracks fails the job instead of escaping the handler thread`() {
val context = RuntimeEnvironment.getApplication()
val engine = Media3Engine(context)
val request = ConversionRequest(
spec = OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE),
probe = InputProbe(
videoCodec = null,
audioCodec = "mp3",
hasVideo = false,
container = Container.MP3,
kind = InputKind.AUDIO_ONLY,
),
)
// Asserted rather than assumed: ConversionRequest's default probe says hasVideo = true, and
// with it this same spec plans to (Encode, Drop), nothing throws, and the test would pass
// over a code path it never entered.
val plan = CopyPlanner.plan(request.spec, request.probe)
assertEquals(VideoPlan.Drop, plan.video)
assertEquals(AudioPlan.Drop, plan.audio)
val failure = try {
runCatching {
runBlocking {
withTimeout(TIMEOUT_MS) {
engine.transcode(Uri.parse("file:///dev/null"), File(context.cacheDir, "empty.mp4"), request) {}
}
}
}.exceptionOrNull()
} finally {
engine.close()
}
// Both halves are load-bearing, and the second is not pedantry: withTimeout raises
// TimeoutCancellationException, and `java.util.concurrent.CancellationException` *extends*
// IllegalStateException — so testing only the first would call an unresumed continuation a
// pass. This assertion was written that way, and the mutation is what found it.
assertFalse(
"the continuation was never resumed — the failure escaped instead of being reported: $failure",
failure is CancellationException,
)
assertTrue(
"the builder's refusal must surface as a failed job; got $failure",
failure is IllegalStateException,
)
}
private companion object {
/**
* Short on purpose. Nothing is decoded, encoded or muxed on this path — the builder refuses
* the input outright — so anything approaching this is a hang, which is the failure mode
* this test is looking for.
*/
const val TIMEOUT_MS = 10_000L
}
}
@@ -82,6 +82,28 @@ class SucceedingWorkerFactory(private val outputData: Data) : WorkerFactory() {
}
}
/**
* Stands in for a worker that died, reporting [outputData] on the way out.
*
* The counterpart to [SucceedingWorkerFactory], and needed for the same reason: the real workers
* cannot run on the JVM, so the only way to ask what a ViewModel does with a `FAILED` `WorkInfo` is
* to produce one. A `Result.failure` carrying `KEY_ERROR` is exactly what both real workers report
* when their engine gives up, and it is the one path where nothing has ever been staged.
*
* `runAttemptCount` is irrelevant here: `Result.failure` is terminal, so WorkManager does not retry
* it and the state goes straight to `Failed` rather than through `Waiting`.
*/
class FailingWorkerFactory(private val outputData: Data) : WorkerFactory() {
override fun createWorker(
appContext: Context,
workerClassName: String,
workerParameters: WorkerParameters,
): ListenableWorker = object : Worker(appContext, workerParameters) {
override fun doWork(): Result = Result.failure(outputData)
}
}
/**
* Installs a synchronous test WorkManager whose workers succeed with [outputData].
*
@@ -89,6 +111,16 @@ class SucceedingWorkerFactory(private val outputData: Data) : WorkerFactory() {
*/
fun installTestWorkManager(context: Context, outputData: Data): SucceedingWorkerFactory {
val factory = SucceedingWorkerFactory(outputData)
installWorkManager(context, factory)
return factory
}
/** Installs a synchronous test WorkManager whose workers fail, reporting [outputData]. */
fun installFailingTestWorkManager(context: Context, outputData: Data) {
installWorkManager(context, FailingWorkerFactory(outputData))
}
private fun installWorkManager(context: Context, factory: WorkerFactory) {
WorkManagerTestInitHelper.initializeTestWorkManager(
context,
Configuration.Builder()
@@ -98,7 +130,6 @@ fun installTestWorkManager(context: Context, outputData: Data): SucceedingWorker
.setWorkerFactory(factory)
.build(),
)
return factory
}
/**
@@ -18,6 +18,7 @@ import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.libremediaconverter.convert.InputFile
import org.libremediaconverter.convert.PendingSave
import org.libremediaconverter.model.ConcatStrategy
import org.libremediaconverter.ui.TestTags
import org.robolectric.RobolectricTestRunner
@@ -219,6 +220,51 @@ class JoinStateAffordancesTest {
assertEquals(listOf("reset"), events)
}
/**
* The negative that bounds #30 on this screen. A join that died staged nothing, so its `Failed`
* carries no [PendingSave] and there is nothing a save dialog could be handed. A retry button
* rendered unconditionally here could only fail, and this is what notices.
*/
@Test
fun `a failed join offers no way to save`() {
setContent(JoinState.Failed(message = "The second file has no audio track, so joining stopped."))
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertDoesNotExist()
composeRule.onNodeWithTag(TestTags.SAVE_FILE).assertDoesNotExist()
}
/**
* #30 on this screen: `save()` keeps the staged file when the copy out throws, and until this
* branch grew a second button the only control it rendered was "Start over" -- `reset()`, which
* deletes exactly that file. Both, not one: the restart still has to be reachable.
*/
@Test
fun `a failed join save offers the file again as well as a restart`() {
setContent(failedSave())
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertExists()
composeRule.onNodeWithTag(TestTags.START_OVER).assertExists()
}
/** The name comes from the job, so a retry wired to a literal would hand back the wrong one. */
@Test
fun `tapping try saving again hands back the name the join chose`() {
setContent(failedSave())
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).performScrollTo().performClick()
assertEquals(listOf("save:joined.mp4"), events)
}
@Test
fun `tapping start over after a failed join save resets and does not save`() {
setContent(failedSave())
composeRule.onNodeWithTag(TestTags.START_OVER).performScrollTo().performClick()
assertEquals(listOf("reset"), events)
}
/** Anything `FileRow` tagged, whichever file it is showing. The prefix comes from the table. */
private val isFileRow = SemanticsMatcher("is a join file row") { node ->
node.config.getOrNull(SemanticsProperties.TestTag)?.startsWith(TestTags.Join.fileRow("")) == true
@@ -230,6 +276,19 @@ class JoinStateAffordancesTest {
sizeBytes = 4_000_000L,
)
/**
* A `Failed` an earlier save left carrying its file, which is the only way `retry` is non-null.
* `staged` names a missing file for the same reason [joined] does -- this arm reads no length.
*/
private fun failedSave() = JoinState.Failed(
message = "There was not enough room on the destination.",
retry = PendingSave(
staged = File("no-such-staged-output.mp4"),
suggestedName = "joined.mp4",
mimeType = "video/mp4",
),
)
/** `staged` names a missing file deliberately -- see the same helper in `JoinScreenContentTest`. */
private fun joined(strategy: ConcatStrategy) = JoinState.Joined(
staged = File("no-such-staged-output.mp4"),
@@ -20,6 +20,21 @@ class ContainerCapabilitiesTest {
container = Container.MP4,
)
/**
* An MP3, and the reason several rules below need a second probe.
*
* `hasVideo = false` is the load-bearing field. Every rule that reads only the spec answers the
* same for this input as for a video file, which is exactly how a spec naming a video codec was
* called valid for a file with no video track to put in it.
*/
private val mp3Source = InputProbe(
videoCodec = null,
audioCodec = "mp3",
hasVideo = false,
kind = InputKind.AUDIO_ONLY,
container = Container.MP3,
)
// --- copy and encode are different questions ----------------------------
/**
@@ -85,17 +100,27 @@ class ContainerCapabilitiesTest {
/** A suggestion that is itself invalid is worse than no suggestion. */
@Test
fun `every suggestion is itself valid`() {
val broken = OutputSpec(Container.WEBM, VideoCodec.H264, AudioCodec.AAC)
val result = ContainerCapabilities.validate(broken, h264Source)
val cases = listOf(
OutputSpec(Container.WEBM, VideoCodec.H264, AudioCodec.AAC) to h264Source,
// The audio-only input. Every rejection it can reach used to hand back `None + None`
// — a spec validation refuses in the next breath — because these branches built their
// suggestion by hand instead of going through the repair-and-filter path.
OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE) to mp3Source,
OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.NONE) to mp3Source,
OutputSpec(Container.MP4, VideoCodec.NONE, AudioCodec.NONE) to mp3Source,
OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.AAC) to mp3Source,
)
val invalid = result as? Validation.Invalid
?: throw AssertionError("expected H.264 in WebM to be rejected")
assertTrue("no alternatives offered", invalid.suggestions.isNotEmpty())
invalid.suggestions.forEach { suggestion ->
assertTrue(
"suggested $suggestion is itself invalid",
ContainerCapabilities.validate(suggestion, h264Source).isValid,
)
cases.forEach { (spec, probe) ->
val invalid = ContainerCapabilities.validate(spec, probe) as? Validation.Invalid
?: throw AssertionError("expected $spec to be rejected")
assertTrue("no alternatives offered for $spec", invalid.suggestions.isNotEmpty())
invalid.suggestions.forEach { suggestion ->
assertTrue(
"suggested $suggestion for $spec is itself invalid",
ContainerCapabilities.validate(suggestion, probe).isValid,
)
}
}
}
@@ -127,6 +152,117 @@ class ContainerCapabilitiesTest {
assertTrue((result as Validation.Invalid).suggestions.isNotEmpty())
}
/**
* The same rule, seen only against the probe.
*
* A video codec named for a file with no video track is dropped, not encoded — so
* MP4/H.265/None on an MP3 empties the output exactly as None/None does. Reading the spec
* alone answered "valid" because the spec names a video codec, and the job went to Media3,
* where `EditedMediaItem.Builder` refuses a composition with both tracks removed by throwing
* on Transformer's own HandlerThread.
*/
@Test
fun `a video codec named for a file with no video track and no audio is refused`() {
ContainerCapabilities.encodableVideo(Container.MP4).forEach { codec ->
val spec = OutputSpec(Container.MP4, codec, AudioCodec.NONE)
val result = ContainerCapabilities.validate(spec, mp3Source)
assertFalse(
"MP4/${codec.label}/None on an audio-only input plans to (Drop, Drop) and would " +
"produce an empty file; it must be refused. Got $result",
result.isValid,
)
}
}
/**
* The refusal is only worth having if it leads somewhere.
*
* The COPY form of this was already refused, but its one hand-built suggestion was
* `None + None` — which validation refuses in the next breath, so the Advanced picker offered
* a one-tap fix that fixed nothing. Every face of the rule now goes through the shared
* suggestion path, so the offer keeps the one track the input actually has.
*/
@Test
fun `refusing an empty output still offers a way to keep the audio`() {
listOf(VideoCodec.H265, VideoCodec.H264, VideoCodec.COPY, VideoCodec.NONE).forEach { codec ->
val spec = OutputSpec(Container.MP4, codec, AudioCodec.NONE)
val invalid = ContainerCapabilities.validate(spec, mp3Source) as? Validation.Invalid
?: throw AssertionError("expected MP4/${codec.label}/None to be rejected")
assertTrue(
"a refusal with no way out is a dead end in the Advanced picker",
invalid.suggestions.isNotEmpty(),
)
assertTrue(
"every suggestion must keep a track, got ${invalid.suggestions}",
invalid.suggestions.all { it.audioCodec != AudioCodec.NONE },
)
}
}
/**
* A repair must not name a track the input does not have.
*
* `repairVideo` used to fall through to "the first codec this container can encode" whenever
* nothing else fitted, and for an MP3 that produced the non-sequitur `MP4 · H.264 · Copy`.
* It validated, so nothing caught it — but [CopyPlanner] drops that video track anyway, which
* makes the codec in the offer a fiction.
*/
@Test
fun `a repair for a file with no video track never names a video codec`() {
listOf(
OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE),
OutputSpec(Container.MP4, VideoCodec.NONE, AudioCodec.NONE),
OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.NONE),
).forEach { spec ->
val invalid = ContainerCapabilities.validate(spec, mp3Source) as Validation.Invalid
invalid.suggestions.forEach {
assertEquals(
"offering ${it.videoCodec.label} for a file with no video track is a fiction; " +
"CopyPlanner drops it. Suggested $it for $spec",
VideoCodec.NONE,
it.videoCodec,
)
}
}
}
/**
* The rule stated as the property it is, over the whole matrix.
*
* A plan of (Drop, Drop) is precisely the composition `EditedMediaItem.Builder` refuses to
* build, so no non-image spec that reaches it may be called valid. Sweeping every container ×
* codec × codec against both probes is what stops the next container or codec from
* reintroducing the gap on an axis nobody thought to write a case for.
*
* Image outputs are exempt and deliberately so: GIF and PNG frames carry no codecs at all, and
* `None + None` is the only spec they accept — but they never reach Media3, because the router
* sends every image output to FFmpeg.
*/
@Test
fun `no valid non-image spec plans to remove both tracks`() {
val specs = Container.entries
.filterNot { it == Container.GIF || it == Container.IMAGE_SEQUENCE }
.flatMap { container -> VideoCodec.entries.map { container to it } }
.flatMap { (container, video) -> AudioCodec.entries.map { OutputSpec(container, video, it) } }
val cases = specs.flatMap { spec -> listOf(h264Source, mp3Source).map { spec to it } }
val empties = cases.filter { (spec, probe) ->
val plan = CopyPlanner.plan(spec, probe)
plan.video == VideoPlan.Drop && plan.audio == AudioPlan.Drop
}
assertTrue("the sweep found nothing to check — the filter has gone wrong", empties.isNotEmpty())
empties.forEach { (spec, probe) ->
assertFalse(
"$spec on $probe plans to (Drop, Drop) — an empty file, and the composition " +
"Media3 cannot build — so it must not validate",
ContainerCapabilities.validate(spec, probe).isValid,
)
}
}
@Test
fun `copying is offered as the fix when the codec is right but unencodable`() {
val av1Source = InputProbe(videoCodec = "av1", audioCodec = "aac", container = Container.MKV)
@@ -350,6 +350,34 @@ class ConversionRouterTest {
}
}
/**
* Why `Media3Engine` still needs a guard of its own.
*
* `ContainerCapabilities.validate` now refuses "a video codec with the audio off" for an input
* with no video track, so neither the picker nor `ConversionWorker` will start one. Routing is
* a separate question and still answers MEDIA3 — nothing about a dropped track makes the job
* un-hardware-able — so a request that skips validation, from a direct
* `ConversionWorker.request(...)` or a job queued before the settings changed, arrives at the
* engine with a plan Media3 cannot build. That has to fail the job, not the process.
*/
@Test
fun `a plan that drops both tracks still routes to media3`() {
val audioOnly = InputProbe(
videoCodec = null,
audioCodec = "mp3",
hasVideo = false,
container = Container.MP3,
kind = InputKind.AUDIO_ONLY,
)
val spec = OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE)
val plan = CopyPlanner.plan(spec, audioOnly)
assertEquals(VideoPlan.Drop, plan.video)
assertEquals(AudioPlan.Drop, plan.audio)
assertEquals(Engine.MEDIA3, route(spec, probe = audioOnly).engine)
}
@Test
fun `audio-only formats are flagged as such`() {
assertEquals(true, OutputFormat.MP3.isAudioOnly)
@@ -146,6 +146,33 @@ class CopyPlannerTest {
assertTrue("copying the only track is still a remux", plan.isPureRemux)
}
/**
* The one plan `Media3Engine` cannot be handed.
*
* `EditedMediaItem.Builder` refuses a composition with both tracks removed —
* checkState("Audio and video cannot both be removed") — and this is how an ordinary-looking
* spec reaches it: a video codec named for a file that has no video, with the audio switched
* off. Neither half is unusual on its own, which is why validation could read the spec, see a
* video codec, and call it fine.
*/
@Test
fun `an audio-only source with the audio dropped removes both tracks`() {
val audioOnly = InputProbe(
videoCodec = null,
audioCodec = "mp3",
hasVideo = false,
container = Container.MP3,
kind = InputKind.AUDIO_ONLY,
)
val plan = CopyPlanner.plan(
OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE),
audioOnly,
)
assertEquals(VideoPlan.Drop, plan.video)
assertEquals(AudioPlan.Drop, plan.audio)
assertTrue("an empty plan is not a remux", !plan.isPureRemux)
}
@Test
fun `copying one track and encoding the other is not a pure remux`() {
val plan = CopyPlanner.plan(
+9 -4
View File
@@ -32,8 +32,12 @@ reached* is not. Re-measured on 2026-08-22, seven runs, one variable at a time:
| r06 | `android-37.1` rev 8 | `swangle_indirect` | ANGLE | **yes, 285 s** | 23 |
| r07 | `android-37.0` rev 6 | `host` + `-feature -HostComposition` | host | **no**, wedged adb at 208 s | not readable |
The discriminator is exact across all seven: **a run boots if and only if the emulator log says
something other than `gles_mode_selected:host`.**
The discriminator is exact across the **six runs that reported**: a run boots if and only if the
emulator log says something other than `gles_mode_selected:host`. r07 is excluded on purpose — it
wedged adb at 208 s and is recorded below as inconclusive rather than ruled out, and a row this
page calls inconclusive cannot also be counted as evidence. Excluding it costs nothing: r07 is a
`host` row, so the discriminator predicts it would not boot, and confirming a prediction with the
one run whose evidence did not come back would add no information either way.
One caveat about how independent those rows are, because the table flatters itself. `-gpu
angle_indirect` (r05) and `-gpu swangle_indirect` (r03) both logged `gles_mode_selected:swangle`
@@ -509,8 +513,9 @@ API 37", not "is that codec broken".
`.github/workflows/api37-debug.yml` carried "roughly every 20 s" for the kill cycle in its own
comments. That number was the watchdog's **sampling** interval, not the cadence, and the two got
conflated. Measured across the seven runs above, gaps between successive `hasReadColorBufferDma`
aborts run **20 s to 90 s, median 60–70 s — three to five aborts in a four-minute window**.
conflated. Measured across the **six runs whose crash buffer could be read** — r07 wedged adb
before one could be taken, so it contributes no gaps — successive `hasReadColorBufferDma` aborts
run **20 s to 90 s, median 60–70 s — three to five aborts in a four-minute window**.
Slower than assumed, and still not slow enough: install, data-directory creation and
instrumentation start-up do not fit inside one gap.