test/device-codecs-encode-consequence
168
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Jason Ross
|
5f1999ef6c |
Merge pull request #7 from JMR-dev/docs/defect-audit
Write down what is actually wrong with this app, and how we know |
||
|
Jason Ross
|
6d250e6c8e | Merge branch 'main' into docs/defect-audit | ||
|
Jason Ross
|
9279816a0d |
Merge pull request #6 from JMR-dev/chore/ignore-claude-dir
Keep Claude Code's agent worktrees out of the repository |
||
 JMR-devandClaude Opus 5
|
cf13e0225c |
Keep Claude Code's agent worktrees out of the repository
.claude/worktrees/ holds complete working copies -- during a parallel agent run there were six, each a full checkout with its own build output. None of it is tracked, so it sat in `git status` as untracked noise, and a `git add -A` at the wrong moment would have committed the repository into itself. scheduled_tasks.lock is equally machine-local. Both are named individually rather than ignoring .claude/ wholesale. That directory is also where shared project config lives -- settings.json, agents/, skills/ -- and ignoring the parent would have pre-emptively hidden files a project would normally commit, for no benefit today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
ef3d87e12d |
Write down what is actually wrong with this app, and how we know
detekt reports zero findings and there is no baseline, no @Suppress and no tools:ignore anywhere -- so the static-analysis gate is green and honest, and it is not where the defects are. They are in the Android-framework edge the linters cannot see into: OutputPublisher, both ViewModels, both Workers and MainActivity, which between them have no JVM unit tests at all and account for most of the ~31% coverage figure. Sixteen entries. Each records what is wrong, how confident we are that it is wrong, how to provoke it, and what a fix would have to decide. The confidence labels are load-bearing: four entries were driven on a physical Pixel 10 Pro XL running API 37, and they are marked differently from the ones that are still inspection only. The device pass earned its keep by contradicting us. D1 -- the one defect that was already known and deferred, the UsableSpace lint finding -- did not reproduce. getAllocatableBytes measured 500 MiB SMALLER than usableSpace, and writing 3 GB into the app's own cache moved both numbers identically, so no cache counted as reclaimable at 66% free. The entry keeps the falsified prediction next to the measurement that killed it, because that is the useful part. Two entries, D15 and D16, were found while fixing others and are recorded rather than folded in silently. D16 is the one worth reading: two individually correct fixes compose into a gap neither of them owns. Entry bodies describe each defect as found and are deliberately not rewritten as fixes land. This is the record of what was wrong, not a changelog; the summary table carries the fix status. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
49535998b5 | Merge branch 'fix/worker-durability-and-naming' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
159320dfa0 |
Name a finished file after the job that made it
Six places decided what a converted or joined file should be called, and not one of them asked
the job. `JoinViewModel` reported `JoinState.Saved("joined.mp4")` whatever the format;
`JoinScreen` opened `CreateDocument("video/mp4")` and launched it with `"joined.mp4"`. On the
convert side `save()` and `suggestedOutputName()` built the name from `_settings.value.spec`, and
the screen took the MIME type from the same place -- the picker as it stands *now*, which is not
the spec the job ran with.
All six are right today, and all six are right by accident. The join screen has no format picker,
so the three MP4 literals agree with `ConcatWorker.request`'s default. The conversion pickers are
drawn only in the `Ready` state, so the settings cannot move between enqueue and save. Neither
accident is load-bearing anywhere it is written down.
One of them has already stopped holding, quietly. A job picked up by `reattach()` ran with a spec
that was never in this ViewModel's settings, because those settings belong to a process that no
longer exists -- so a reattached MP3 conversion is offered `.mp4` and `video/mp4` today. The
previous commit made that path more reachable rather than less: `Reattachment` exists precisely
to find work this ViewModel did not start.
The fix is to ask the only thing that knows. The spec travels to the worker as input `Data` and
`WorkInfo` hands input `Data` back to nobody, so the worker is the single point at which the
input's name and the spec that ran are both in scope. Both workers now report the two derived
strings -- the name to suggest and the type to open the dialog with -- in their output `Data`,
and they ride on `ConversionState.Converted` and `JoinState.Joined` from there. `save()` reports
what it saved rather than recomputing it, and both screens read the name and the MIME type off
the state they already collect, remembering their `CreateDocument` contract against that type
instead of a literal. The MIME type is not cosmetic: some providers rewrite a document's
extension to match it, so an MP3 offered as `video/webm` can arrive with the wrong one.
`suggestedOutputName()` is deleted rather than repaired. With the answer on the state there is no
caller left for it, and an accessor recomputing the same string would only be a second place for
it to be wrong -- which is what it was.
`ConcatWorker` gains `DEFAULT_FORMAT` and `outputNameFor(format)`. Three copies of "MP4" is the
shape this entry is about, and the input-Data default, `request`'s parameter default and the
ViewModel's fallback for a job that predates this change are exactly three copies.
Work already in the queue carries neither string, and WorkManager keeps finished work for about a
week, so that is the ordinary case for a few days rather than a corner. Those fall back to the
old derivation, which is a guess -- but it is the same guess the app was already making, it is
confined to jobs enqueued before this commit, and for such a job there is genuinely nothing
better to hand. New work never reaches it. The join's fallback is not even a guess: the format
`ConcatWorker.request` has always defaulted to is the format such a job really used.
`reattach()`'s KDoc carried this as a known wart it was deliberately leaving alone. That
paragraph is now a description of the fix rather than of a defect.
Tested at both ends, since either alone would pass while the other was wrong.
`WorkerOutputNamingTest` runs the real worker on an MP3 job -- nothing like the default preset, so
a name built from the picker is visibly wrong rather than accidentally right -- and compares the
whole success `Result`, which is what makes a missing key fail rather than go unnoticed. The two
ViewModel tests drive a finished job and then move the picker, which is what a reattached job
amounts to from the ViewModel's point of view.
Restoring just the two name sources -- `save()`'s `outputNameFor(displayName, _settings.value.spec)`
and `JoinState.Saved("joined.mp4")` -- turns them red with
`expected:<holiday_converted.mp3> but was:<input_converted.webm>` and
`expected:<joined.mkv> but was:<joined.mp4>`. The convert side loses the extension *and* the name:
`_settings.value` had been moved to WebM, and the display name a reattached job cannot supply had
already fallen back to the placeholder.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
2a68f03134 |
Give every job a staging path of its own
`<cacheDir>/conversions/` is shared by the convert tab, the join tab and `ConcatEngine`, and
until now none of the three named a file that belonged to one job. A conversion derived its name
from the input's display name, so two `holiday.mp4` from different folders wrote the same file.
A join used the constant `joined.<ext>`, so any two joins of one format did. The list file was
the constant `concat_list.txt`, so any two joins at all did, and one of them would read the
other's input list.
The naming half is not a hypothesis. Two independent conversions on a Pixel each computed
`cache/conversions/input_converted.mp4`, the second silently overwrote the first, and a tag query
in a fresh process then returned **two SUCCEEDED `WorkInfo`s naming that one file** with one file
on disk. That is the collision reaching the point where it makes a *fix* ambiguous rather than
just a file: `Reattachment` can offer the bytes, because they are the user's either way, but it
cannot say which job produced them.
`StagingNames` keys the name on the WorkManager request id. That id is what stays still across a
retry -- `WorkerWrapper` builds `WorkerParameters` from the `WorkSpec` id and only increments
`runAttemptCount` -- which matters more here than uniqueness does, and matters more since the
previous commit made retries routine. A failed attempt deletes its staged file on the way out,
and that only collects the partial the *previous* attempt left when the name has not moved.
Opaque rather than sanitised, deliberately. The staged name is never shown to anyone: `save()`
recomputes a suggested name and the user picks the real one in the SAF dialog. So there was
nothing to lose by dropping the display name, and something to gain -- a provider-supplied
display name can contain a separator, be empty, or be four kilobytes long, and `File(stagingDir,
"../escape_converted.mp4")` resolves to a path outside staging. That was reachable before this
commit and is now unreachable by construction rather than by a sanitiser that has to be right
about every case. There is a test for exactly that name.
The extension stays, and is not decoration: `FFmpegConcatCommand` names no output muxer, so
FFmpeg infers it from the output path. A fully opaque name would quietly produce the wrong
container.
`ConcatEngine`'s list file is derived from the output it belongs to rather than taking another
parameter, so the two cannot drift apart, a directory listing shows which list belongs to which
join, and the sweep ages them together.
Three neighbouring comments claimed things that are no longer true, and are corrected rather than
left to mislead the next reader:
- `Reattachment.Ambiguous` said it "resolves on its own once each job stages under a name of
its own". It now does -- for work enqueued from here on. The case is **kept**, because the
queue outlives the change: WorkManager holds finished work for about a week, and the jobs
likeliest to be sitting in it when this code first runs are the ones named the old way.
Behaviour is unchanged and `ReattachmentTest` is untouched.
- `OutputPublisher.sweepStaging` justified its age rule partly on there being "no per-job
namespacing". There is now, and the rule still stands on its own: per-job names stop two jobs
from sharing a file, and say nothing about whether a file's job is still running, which is the
question a sweep actually asks. Same for `StagingSweep` and the note in
`LibreMediaConverterApp`.
- Both ViewModels' `reattach()` explained aliasing as something nothing prevented. Narrowed to
what is still true of work already in the queue.
`ConcatEngineTest` asks `StagingNames` for the list file's name instead of spelling out
`concat_list.txt`. That is the difference between a test and a tautology: a literal there would
have gone on passing after the rename while asserting that a file nothing creates does not exist.
The same trap was live in the two worker tests from the previous commits, whose staged-file
assertions computed a path of their own -- they now assert on the staging directory being empty,
which cannot go vacuous when a name moves.
`PerJobStagingTest` drives the real worker, because the naming function was never the part that
was wrong: what was wrong was which name the worker asked for. Two jobs converting one file must
leave two files; a second attempt at one job must not leave a second; and a display name that
climbs out of staging must not. The first and third fail before the change with "each job must
have staged its own file, found [input_converted.mp4] expected:<2> but was:<1>" and "the output
belongs in staging expected:<1> but was:<0>" -- the latter because the file had landed in
`cacheDir` instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
dcdcbfd3af |
Let a cancelled conversion stay cancelled
Both workers' outer `catch (e: Throwable)` caught `CancellationException` along with everything else and answered it with a `Result`. `runMedia3OrFallBack` goes out of its way to rethrow cancellation rather than fall back to software, and then the catch above it converted it anyway. A coroutine that reports completion inside a scope which has already been cancelled is structured concurrency's one rule broken, and it is the kind of break that stays quiet: nothing downstream complains, and the next thing to hold a resource across that boundary is the thing that finds out. Nothing on screen disagreed today, which is why this is a low-severity entry rather than a bug report. WorkManager cancels the worker's coroutine through `WorkerWrapper.interrupt`, which cancels `workerJob` with a `WorkerStoppedException`; the surrounding `withContext(workerJob)` then throws that whatever the worker returned, and `launch()` resolves it as `ResetWorkerStatus`. The returned `Result` is read only when nothing stopped the worker at all. So the change is about the shape of the code rather than about a symptom. One behaviour does move, and it is worth naming rather than discovering later. A cancellation that is *not* WorkManager stopping us -- FFmpegKit reporting `ReturnCode.isCancel`, which cancels the continuation -- now leaves `doWork` as a cancellation, and `WorkerWrapper` resolves a self-cancelled worker as `Resolution.Failed()` with no output data instead of the `Result.failure(KEY_ERROR ...)` it used to build. Both ViewModels already fall back on blank output data, deliberately and with a test, so the user sees "Conversion failed." either way. That is also the honest answer: the only route to `isCancel` is a cancellation someone asked for. The `staged.delete()` on that path is kept, and moved into the new branch rather than left to the one below it. A cancelled attempt leaves a partial in staging, the next attempt starts `doWork()` from the top rather than resuming it, and this catch holds the only handle to the file. Reaching any of this from a JVM test needed one more thing: `doWork` called `MediaProbe.probe` directly, and it was the last caller bypassing `ConversionDependencies.probe`. FFprobe's loader throws a bare `java.lang.Error` with no native library present, so no unit test could reach a single line below it. The seam's own KDoc says this is what it is for -- coverage of the branches that only run when something goes wrong -- and the default is the same real probe, so the app and the instrumented tests are unchanged. `WorkerCancellationTest` drives the real worker through a real `WorkManager` to the software engine, forced with `FORCE_SOFTWARE` because it is the one preference that decides without consulting the input, so the test does not depend on a routing rule it is not about. The engine stub writes bytes before it throws, which is what makes the delete assertion mean something: a stub that only threw would let a missing `delete()` pass. Three cases -- cancellation propagates, cancellation still deletes, and an ordinary failure is still answered with a `Result` carrying its message, which is the half that would break if the rethrow were widened past cancellation. Before the fix the first of those failed with "cancellation must leave doWork as cancellation, not as a Result; got null" -- `runCatching` had nothing to report, because `doWork` had returned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
5c27801c77 |
Retry work the system refused to start, instead of failing it terminally
`ConversionWorker`'s KDoc says the queue survives process death, and that is the app's stated
reason for choosing WorkManager at all. A Pixel 10 Pro XL says otherwise. An 18-minute
transcode was killed mid-write with `kill -9`; 119 seconds later, on a natural dispatch with
no `cmd jobscheduler run` anywhere in the session, WorkManager recovered it by itself and the
system refused it:
WM-ForceStopRunnable: Found unfinished work, scheduling it.
ActivityManager: Background started FGS: Disallowed [callingPackage:
org.libremediaconverter; uidState: CEM; BFGS denied: true; code:DENIED]
WM-WorkerWrapper: android.app.ForegroundServiceStartNotAllowedException
WM-WorkerWrapper: Worker result FAILURE
WM-Processor: Processor 3d1c9862 executed; reschedule = false
`Found unfinished work` is the clean process-death recovery path, not a force-stop. The job was
ordinary -- `Priority: 300 [DEFAULT]`, not expedited -- so there was no allowance it could have
carried. `HAS_FOREGROUND_EXEMPTION` was set on it and it was still denied; that flag governs the
runtime guarantee once a service is running, not permission to start one.
The cause is structural rather than subtle. `setForeground(...)` sat above `return try {`, so its
throw escaped `doWork()` without reaching `handleTimeoutIfNeeded`, `Result.retry()`, the
`workDataOf(KEY_ERROR ...)` or `staged.delete()`. Three separate costs, all measured on the
device: `reschedule = false`, so nothing ran again despite `run_attempt_count=2`; output `Data`
of `X'ABEF000100000000'`, a header with zero entries, so the screen said "Conversion failed."
with nothing to add; and 2 MB of a partial `long_input2_converted.mp4` left in staging.
`ConcatWorker` had the same shape.
So `setForeground` moves inside the `try` in both workers, and the staging handle is named just
above it -- `createStagingFile` only builds a path, nothing is written until an engine opens it,
so naming it early costs nothing and makes the catch total. `MediaProbe.probe` was outside the
`try` for the same reason and had the same problem; it is now inside too. On a retry the staged
name is unchanged, so the delete on the way out collects the partial the killed attempt left
behind rather than orphaning it.
What to return was the real decision. `Result.retry()`, because the denial is about *when* the
job ran and not about the job: the file is fine, the settings are fine, and the one thing that
grants an app permission to start a foreground service is being in the foreground, which the
user supplies by opening the app. But WorkManager never gives up on its own, so an unbounded
retry means a job nobody comes back for waking the device forever while the screen says "paused"
and never explains itself. `FailureOutcome` therefore bounds it at ten attempts, chosen against
the default backoff rather than as a round number: exponential from
`WorkRequest.DEFAULT_BACKOFF_DELAY_MILLIS` (30 s), doubling, clamped at `MAX_BACKOFF_MILLIS`
(5 h), which spans about 8 h 30 m before the eleventh attempt fails with a message the user can
act on. The counter is `runAttemptCount`, which counts every attempt and not only denied ones --
WorkManager exposes no other -- so a transcode already retried ten times by the six-hour budget
will fail on its first denial rather than getting ten of its own. That is accepted rather than
overlooked, and the timeout branch ignores the count entirely so the budget's own retries are
untouched.
The rule goes in `FailureOutcome` rather than beside it, which is what its KDoc asks for: isolate
a decision whose triggering condition cannot be provoked in a test. It now reads the stop reason
*and* the cause, with precedence stated rather than left to branch order -- once something has
stopped the worker, the exception it was holding describes that stop and not a reason of its own.
The match is on `ForegroundServiceStartNotAllowedException` exactly, never on its
`IllegalStateException` supertype: a muxer that was never started throws one of those too, and
matching the supertype would retry every ordinary failure for eight hours.
Expedited work is still not used, and this is not an argument for it. It maps to JobScheduler
expedited jobs with a short quota, which is the wrong shape for a multi-minute transcode; the
exemption it buys is for starting, and the quota it costs would be paid by every job.
Tested at both levels, because only one of them bites. `FailureOutcomeTest` gains six cases for
the rule -- retry at the bound, give up past it, an unrelated `IllegalStateException` that must
not be mistaken for a denial, a stop reason winning over the exception, and the budget timeout
ignoring the count. `DeniedForegroundStartTest` covers the wiring, which is where the defect
actually was: a `ForegroundUpdater` whose future completes exceptionally makes `setForeground`
throw exactly what the platform throws, because `WorkForegroundUpdater`'s own comment says it
propagates the exception to the caller and `ListenableFuture.await()` unwraps the
`ExecutionException` on the way. Moving `setForeground` back above the `try` turns all four of
those red with a bare `android.app.ForegroundServiceStartNotAllowedException`, which is the same
line the device logged.
Four comments claimed the old behaviour and are corrected with it. `ConversionState.Waiting`
said the budget had run out; `observe()`'s ENQUEUED branch said the budget was the likely cause,
where a denied restart is now the likelier of the two; and `Reattachment.choose` justified
excluding FAILED partly on interrupted workers coming back FAILED "rather than retried", which is
the sentence this commit falsifies. The exclusion stands on its own and says so now.
The bound's own KDoc says what giving up does *not* buy, too. `FOREGROUND_DENIED` reports through
a FAILED job and `Reattachment` excludes FAILED, so a user who was not watching at the eleventh
attempt meets an empty screen rather than the message. What the bound reliably buys is the end of
the retrying.
Both `Waiting` screens change their wording. The state is `ENQUEUED` with a run attempt behind
it, which cannot distinguish the two causes that now reach it, and the old copy named only the
six-hour budget -- which is now the less likely of the two. "Keeping the app open helps it along"
covers both, and for a denied start it is not filler but the actual remedy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
22c7914395 |
Find out why the emulators segfault, and make them run
CLAUDE.md has said "Emulators segfault on this host -- qemu dies on every AVD" since the E2E matrix landed, and the PR that introduced it called the failure "exit 139 across three AVDs and both GPU backends, environmental". That is accurate about the symptom and wrong about the cause, and the cost of being wrong was the whole instrumented suite being unrunnable here. SwiftShader's Reactor JIT writes generated GLES shader code onto the heap and mprotects it executable. Fedora's SELinux policy denies that -- execheap is not granted to unconfined_t and selinuxuser_execheap is off -- so the mprotect fails and the emulator takes SIGSEGV the moment it calls the routine it just generated. The AVC denial and the core are the same event, one second apart. The predictor is mechanical and held 7 for 7 across every -gpu mode: a run crashes if and only if it dlopens gles_swiftshader/libGLESv2.so. host, angle_indirect and swangle_indirect boot. auto, off, guest and swiftshader_indirect crash -- and auto is the default, which is why the failure looked universal rather than renderer-specific. tools/local-emulator/run-e2e.sh picks a renderer that works and refuses the ones that do not. It reuses .github/scripts/e2e-run.sh rather than forking it, so the local and CI diagnostics cannot drift; the one change there adds an optional E2E_EXTRA_GRADLE_ARGS that is unset in CI, so CI runs byte-identical commands. The API 33-36 sweep has now been run and is written down. All four levels are green on a local emulator and match the physical Pixel 10 Pro XL baseline exactly: 49 tests, 0 failures, 0 errors, 2 skipped, every level. Those counts come from the result XML, not the UTP console counter, which double-counts skips and reported "Finished 51 tests" on all four. No boot log dlopens SwiftShader GLES and the sweep window holds no AVC denial and no qemu core -- which is confirmation of the mode matrix's first row rather than new coverage, since every one of these runs is -gpu host. The table is still seven modes measured once each. Two things the sweep surfaced that the doc now records: pre-build before sweeping, or a fresh checkout spends API 33's 20-minute wrapper budget compiling and wedges before a test runs; and the device pinning is untested by this run, because the Pixel dropped off USB five seconds before it started. Still offered for review rather than applied: the CLAUDE.md correction the doc drafts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
07933c80c5 | Merge branch 'fix/native-boundary-guards' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
7db320018a |
Correct the JDK claim and decide the backup rules
D11's documentation and scaffold items, less the one row that belongs to another change stream. README's "Requires JDK 17+ (AGP 9 will not run on older)" was wrong twice over, and `f496291` already corrected the same claim in CLAUDE.md. The floor is not AGP's, and 17 is not what compiles anything: Gradle 9.7.1's own `SupportedJavaVersions` carries MINIMUM_CLIENT_JAVA_VERSION = 8 and MINIMUM_DAEMON_JAVA_VERSION = 17, and this repo then overrides the daemon upward to 25 in gradle-daemon-jvm.properties. So the honest statement is that the launcher floor is 8, the daemon is 25 whatever JAVA_HOME says, and the app's bytecode is 25 -- which is what `./gradlew --version` shows on this machine right now, launcher 21 against daemon 25. The data_extraction_rules TODO is filled in rather than deleted, because `android:allowBackup="true"` makes it a live question and the answer is not "nothing to say". The app stores nothing of its own -- no settings, no history -- so WorkManager's queue is the entire backup payload, and restoring it is wrong rather than merely useless: every row names a content:// grant and a cacheDir path that do not survive reaching another device, and cacheDir is not backed up at all. Since `ec969c4` the ViewModel queries WorkManager by tag on launch, so those rows would not sit inert either -- a fresh install would come up reattached to a job the user never ran on it. WorkManager declares no exclusion of its own, so nothing upstream prevents it. allowBackup stays true. The decision belongs in the rules file, where it is per-file and legible to whoever adds real user data later, rather than in an app-wide switch that would also turn off device-to-device transfer. Each file is named instead of excluding the "database" domain in one line. Lint's FullBackupContent detector skips an <exclude> that carries no path without checking it, so the one-line spelling could have silently protected nothing; the enumerated paths are ones the gate actually verifies, and they are present in the built APK's compiled resource. backup_rules.xml and android:fullBackupContent are deleted rather than filled in. That attribute is only read on Android 11 and lower and minSdk is 33, so it could never have applied here -- an equally empty template that, unlike the other one, had no live question behind it. Not touched: OutputPublisher's hasSpaceFor KDoc, which the audit lists under D11. That code belongs to a parked branch and another change stream. The stale com/example/androidmediaconverter package directory needs no commit: it is empty, and git has never tracked it because git cannot track an empty directory. Removed from the working copy directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
97fdc49f01 |
Guard the native boundary against what it actually throws
D14: picking a file died instead of reporting an unreadable one when FFmpegKit's native library could not load. `probeWithFFprobe` guarded its call with `catch (e: Exception)`, and `ConversionViewModel.onInputPicked` guarded nothing, so the failure escaped a `viewModelScope.launch` -- which has no handler, and on a device ends the process. All three of the obvious narrow guards catch nothing, which is why this needed reading the AAR rather than guessing. `NativeLoader.loadLibrary` catches the `UnsatisfiedLinkError` that `System.loadLibrary` raises and rethrows a *bare* `java.lang.Error` wrapping it, so `UnsatisfiedLinkError` never escapes and the escaping type carries no information at all. Every touch of the class after the first is a different type again -- `NoClassDefFoundError` -- so a guard written for the first shape lets the second pick onwards crash, which is the harder half to notice. Both are in the test output verbatim. `catch (Throwable)` was the wrong answer for the reason the audit gave: it would swallow a genuine `OutOfMemoryError` in a method that spawns a native process, turning "this device is out of memory" into "this file looks unreadable" and letting the app act on it. So the line is drawn by a named predicate, `isNativeLoadFailure`, rather than by the catch clause -- every class-loading shape is a `LinkageError`, and nothing that means the JVM is failing is one. That disjointness is what makes the guard narrow. This is consistent with the position `config/detekt/detekt.yml` already takes for `TooGenericExceptionCaught`: the boundary's failure types are undocumented, so guessing crashes the app on a file it could have reported. One level up the opposite mistake is available too, and the predicate is what lets both be avoided at once. `TooGenericExceptionThrown` is relaxed for the test source sets only. A test that reproduces a failed native load has to throw what the library throws, and a tidier subclass would leave it passing against a defect it no longer reproduces. Main source is untouched by that and throws nothing generic. `ConversionDependencies.probe`'s KDoc is rewritten rather than left. It said this hazard was "deliberately not fixed here ... its own commit, with its own test", which this is -- leaving it would have replaced one true comment with a false one, which is the same defect class as the D11 work. Both halves are covered independently: reverting the `MediaProbe` catch reds only the two `MediaProbeNativeLoadTest` cases, reverting the ViewModel guard reds only the two injected-seam cases, and widening the ViewModel guard to `Throwable` reds the OutOfMemoryError case -- so the narrowness is pinned, not just the catch. Audited the sibling boundaries named in the audit and left all three alone: `FFmpegEngine` and `ConcatEngine` both construct and run under `catch (e: Throwable)` in their workers, and `Media3Engine` has no native loader of this kind and already routes failures through `runCatching`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
a6baf41866 | Merge branch 'fix/rotation-and-partial-publish' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
dbfc463c6d |
Delete the half-written destination instead of leaving it under the user's name
publish() streamed the staged file into the SAF destination with copyTo and had no
answer for a copy that failed partway. The destination volume filling up is the obvious
way in; a provider giving out mid-write is the other. Either way the bytes it had
managed stayed at the name the user picked, while the UI said "Could not save the file".
The user was left holding a truncated file they had just been told was never written,
and nothing in the app would ever tidy it up -- staging cleanup reaches
<cacheDir>/conversions and nowhere else, by design.
So a failed copy now deletes the document. The interesting part is not the delete, it is
what stops it, because removing a file the user already had would be a far worse defect
than the one being fixed.
Only a document URI. DocumentsContract.deleteDocument is the only delete this code has
any right to attempt and it is defined on document URIs, so isDocumentUri() gates it.
That is not a formality: it asks the package manager whether anything answers
ACTION_DOCUMENTS_PROVIDER for the authority, so a file:// path, a MediaStore item or a
content URI from an ordinary provider all fall out here untouched.
Only a destination that was empty when we started. The size is read BEFORE the stream
is opened -- opening for write truncates, so afterwards the question can no longer be
asked -- and the delete runs only when the answer was positively zero. Every
destination that reaches publish() today comes from the SAF CreateDocument contract, so
in practice it is a document this app created seconds earlier; but publish() cannot
verify that from a Uri, and a provider that hands back an EXISTING document for a name
the user re-picked would otherwise have its file deleted rather than merely truncated.
Truncated is bad. Gone is worse, and it is the user's file either way.
That guard fails towards doing nothing. A provider that does not report _size, a query
that returns no row, a resolver call that throws -- all of them land in "not known to
be empty", so the fix is conservative rather than universal: it will not clean up
behind such a provider, and it will not delete anything of theirs either. The defect is
closed for providers that answer a size query; ExternalStorageProvider backs its
documents with real files, so a freshly created one reports 0, but that is reasoning
about it rather than a run against it. Stated here rather than implied, because
"fixed" would overclaim what was verified.
The original failure is what the caller sees. Cleanup runs in its own runCatching and a
throw from it is attached to the original exception as a suppressed one. deleteDocument
reports failure two different ways -- false, or a rethrown RuntimeException -- and
neither is worth failing the save over, because the save has already failed.
ConversionViewModel.save() reports e.message, and "could not delete the half-written
file" is not what to tell someone whose disk just filled up.
Two smaller decisions in the control flow. The whole `use` is guarded, not just copyTo:
a close() that throws while flushing IS the disk-full case and it arrives after copyTo
has returned successfully, so guarding only the copy would miss exactly the failure this
commit is about. The cost is that a file whose every byte reached the provider before a
failing flush is deleted too, which is the right way round -- a flush that failed means
the bytes are not durably there. And openOutputStream's own failure is deliberately
OUTSIDE the guard: nothing has been written at that point, so there is nothing of ours to
remove.
Nothing else in the class moves. hasSpaceFor, discardStaged and sweepStaging are
untouched, and so is save(): the staged file is still deliberately kept on a failed save,
for the reason its own comment gives -- it may be the only copy of an hour of
transcoding, and now the destination genuinely does not have it either.
Seven tests, JVM, Robolectric. The failure has to be injected, which is what shapes them:
Robolectric's ShadowContentResolver consults its registered-stream map before it reaches
any provider, so a test can hand out a stream that writes 512 bytes to a real file and
then throws "No space left on device" while a fake provider answers the size query and
the delete against that same file. The provider is registered twice under two authorities
and two component names, once with the ACTION_DOCUMENTS_PROVIDER intent filter and once
without, which is the only way to have a content URI that is not a document URI. The
delete is observed by the wire names DocumentsContract.deleteDocument actually sends --
"android:deleteDocument" and the "uri" extra -- because both constants are hidden from
the public SDK.
fails partway leaves nothing RED before the fix: expected the delete, got []
close that fails while flushing RED before: the file was still there
cleanup that fails RED before: suppressed was []
already held bytes is not deleted green before the fix -- see below
not a document is left alone green before the fix -- see below
cannot be opened at all green before, and must stay so
succeeds, every byte, no delete green before, and must stay so
The last four are green against the unfixed code for a reason worth writing down: the old
publish() never deleted anything, so every guard passes trivially. They pin the guards
rather than the defect, and pinning is only worth something if the pin is real, so both
were reverted on their own:
- dropping `if (destinationWasEmpty)` fails "a destination that already held bytes is
not deleted" with "a document this app did not create must survive"
- dropping the isDocumentUri() check fails "a destination that is not a document is
left alone" with "deleteDocument has no business on a URI that is not a document
expected:<[]> but was:<[content://...test.plain/document/holiday_plain.mp4]>"
Both were restored. 193 unit tests green.
UnopenableUriTest's unwritable-destination case (an authority with no provider behind it)
is unchanged and keeps passing by two independent routes: the size query on a dead
authority yields "not known to be empty", and the failure itself comes out of
openOutputStream, which sits outside the guard. It is an instrumented test and was
compile-verified here, not executed -- instrumented tests do not run on this host
(CLAUDE.md). Its JVM twin is in the list above.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
ce4d0ff7d4 |
Keep the selected tab through the recreation targetSdk 37 guarantees
AppRoot held the selected tab in `remember`, which survives recomposition and nothing
else. MainActivity declares no configChanges, so every rotation and every resize
destroys and recreates the Activity, and the tab went back to Convert each time.
The KDoc directly above that line is the argument for why it matters: from targetSdk 37
Android ignores screenOrientation, resizableActivity and the aspect-ratio limits on any
display at least 600dp wide, and the Android 16 opt-out is gone, so the app is resized
and rotated whether or not it is ready. The shell was written for that case and then
lost its own state to it. Both ViewModels are Activity-scoped and come back intact, so
a conversion in flight was never at risk -- only the tab, which is what makes this
visibly wrong rather than merely stale.
rememberSaveable, with a Saver that writes the constant's NAME. Three ways to make an
enum saveable and the reasons for this one:
- autoSaver already accepts it. An enum is Serializable, so plain
`rememberSaveable { mutableStateOf(Destination.CONVERT) }` compiles, works, and
passes the restoration test below unchanged. That is a reason to be explicit, not a
reason not to be: nothing in the declaration says Destination has to stay
Serializable, so the implicit route keeps working right up until someone makes it a
value class or a sealed interface -- and then stops, silently, on a path only a
rotation reaches.
- The ordinal is a position, not an identity. Inserting a tab between Convert and Join
would redefine every value already written down. A name only changes when someone
renames a constant, which is an edit that shows up in a diff. It also reads as
itself in a Bundle dump.
- An unknown name restores to null, which rememberSaveable treats as "nothing saved"
and falls back to Convert. That is exactly what a downgrade or a renamed constant
leaves behind, and Convert is the right answer for it.
The test runs on the JVM, which took two changes to reach.
AppRoot and Destination are `internal` rather than `private` -- the unit test source set
is a friend of main, so this stays invisible outside the module -- and AppRoot takes its
`content` as a defaulted parameter instead of calling Content() directly. Nothing in the
app passes it. It is there because both screens resolve a ViewModel, which builds a
WorkManager and a media probe, and none of that has anything to do with which tab is
selected; the test hands in a tagged Box and drives the shell alone. Content() stays
private and is still what the app gets.
compose-ui-test-junit4 joins the JVM test source set. It was already in the catalog for
androidTest, it is inside the prerelease guard via its androidx. group, and its version
comes from the BOM, so this adds no new pinning argument. It is there because
createComposeRule() runs under Robolectric: a red test in androidTest is one nobody on
this host can execute (CLAUDE.md), which is not a loop anyone can work in.
ui-test-manifest is NOT repeated on that source set. It supplies the ComponentActivity
the rule launches, and the existing debugImplementation entry already puts it in the
merged manifest the unit tests build against -- checked by removing the line and
watching AppRootRestorationTest stay green, rather than assumed.
What the test does and does not prove. StateRestorationTester's
emulateSavedInstanceStateRestore() disposes the composition and rebuilds it, so anything
held only by `remember` is gone -- that is what makes it bite. It saves into an in-memory
map rather than parcelling through a Bundle, so it cannot tell a name from an ordinal
from autoSaver. The saved representation is pinned separately by three pure-JVM tests
over the Saver itself, which is where the choice above is actually held down.
Verified by writing it red first, against the restructured AppRoot with `remember` still
in place: all three restoration tests failed at the post-restore assertion with
"Expected exactly '1' node but could not find any node that satisfies:
(TestTag = 'content:JOIN')", while every assertion before the restore -- including the
bar's own selected state -- passed. Six new tests, 186 green in all.
Not addressed here, and deliberately: MainActivity still declares no configChanges, and
should not. Handling the configuration change is not the same as keeping one enum, and
Compose's saved-state machinery is the mechanism the platform intends for it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
308333cb49 | Merge commit '8bc2a33' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
8bc2a337d0 |
Merge the staging-cleanup fix, and pin the seam the two changes share
The two changes meet at one line. `pendingStaged` is recorded in the `SUCCEEDED` branch of each ViewModel's `observe()` collector, and reattachment reaches `Converted`/`Joined` through that same collector rather than by building the state itself -- so a job picked up from a previous process arrives with its cleanup handle already set, and "Start over" on it deletes the staged file exactly as it does for a conversion run in this process. Nothing had to be added for that; it falls out of routing reattachment through `observe()`. Which is precisely why it needed a test. The claim is structural -- one assignment, in one function, that both changes assume -- and the conflict here was `JoinViewModel`, where the cleanup change edits a collector body that the reattachment change had moved out of `join()` into a private `observe()`. Resolving that by putting the assignment back in `join()` would compile, pass every test either branch brought, and silently leak a full-size file on the one path both changes were written for. `ReattachedCleanupTest` fails if it lands anywhere else. Verified the way the cleanup commit verified its own wiring: deleting `pendingStaged = staged` from `ConversionViewModel.observe()` fails "start over on a reattached conversion deletes the staged file" alongside the two `ConversionViewModelCleanupTest` cases that share the line. Restored, all 183 JVM tests pass. The reattachment path also gains JVM coverage it could not have had before this merge, since Robolectric and the `ConversionDependencies` seams arrived with it: a ViewModel constructed after a job has already finished, with nothing left that observed it, now demonstrably reaches `Converted` with the display name recovered from the job's tags -- on a machine where no instrumented test can run. Conflict resolution: both sides kept in `JoinViewModel`, with the cleanup handle declared alongside the other fields and the reattachment `init` after them. Nothing else conflicted; `ConversionViewModel` merged clean because the reattachment change touches the `CANCELLED` and `FAILED` branches while the cleanup change touches `SUCCEEDED`. No build file, manifest or `OutputPublisher` line in this merge is mine -- they arrive from the cleanup commit verbatim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
eb37f9ebea |
Merge branch 'fix/reattach-unfinished-work' into scratch/integrate-d2-d3
# Conflicts: # app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt |
||
|
JMR-devandClaude Opus 5
|
ec969c41dc |
Reattach to conversions and joins the ViewModel did not start
The queue surviving process death is the stated reason this app uses WorkManager, and
the ViewModel was where that protection stopped. `activeWorkId` and `observer` are
plain fields, so a process reclaimed after a conversion finished came back to Idle
while the output sat in `cacheDir` with nothing in the UI able to reach it. The
realistic window is not a crash mid-transcode -- it is the job finishing, the user not
saving yet, and the process being reclaimed hours later as an ordinary background one.
Both ViewModels now query their own worker's class name on init and pick up what they
find. Nothing is persisted for it, and nothing needed to be: `WorkRequest.Builder`
seeds every request's tag set with `workerClass.name` (`tags = mutableSetOf(
workerClass.name)`, work-runtime 2.11.2), and R8 keeps those names through
work-runtime's own consumer rule, `-keepnames class * extends
androidx.work.ListenableWorker`. A UUID in a `SavedStateHandle` would have been both
more machinery and less: it cannot find work enqueued by a previous install.
What the query cannot return is the job's input. `WorkInfo` hands back id, state, tags,
progress, output and run-attempt count -- never the `Data` a request was enqueued with
-- so a ViewModel could see that a conversion existed and where its output went, but
not which file it was converting. The display name and size therefore ride on tags too,
which is the whole of the production change to the request builders. The picked `Uri`
deliberately does not: nothing in a reattached state reads it, and a `content://` grant
taken by a picker in a process that no longer exists is not something to hand back as
though it still worked.
The decision is a pure function on the JVM test stack, following `FailureOutcome`:
`Reattachment.choose` takes what WorkManager reported and answers which job, if any.
Cancelled work is excluded -- the user already said no. Failed work is excluded, which
matters more than it looks now that a device has shown an interrupted worker coming
back FAILED rather than retried, its restart's `setForeground` refused as a background
foreground-service start: nothing marks a failure as seen, so it would otherwise
reappear on every launch. A success whose staged file is gone is excluded, because a
Save button that fails on tap is worse than no button. Live work outranks a finished
result, since a running job holds a foreground notification and someone opening the app
while that notification is in the shade is looking for that conversion.
Where several jobs qualify, the newest staged file wins, and that is not a detail.
Losing a tie is not the same as waiting for the next launch: the tag query has no
`ORDER BY`, so its order is unspecified but stable, and an arbitrary winner would keep
winning every launch while the other result stayed unreachable for as long as its file
existed. It is reachable today -- dismiss one result with "Start over", which leaves its
file behind, then convert something else and do not save it. `WorkInfo` carries no
timestamp of any kind, but the edge is already stat'ing the file, so the file's own
mtime is the ordering. A clock that moves backwards makes it a heuristic; an order that
is unspecified and repeats itself is worse.
Aliases are the tie that does not resolve, and that case is not hypothetical. A tag
query on a device returned two SUCCEEDED jobs whose output paths were both
`.../conversions/input_converted.mp4`, with one file on disk -- the staging name is
derived from the input's display name, so a later job overwrites an earlier one's output
and both go on reporting it. Checking the file does not separate them, and neither does
its mtime, since they share it. So the two halves are separated instead. The file is
offered, because it is the user's file either way and losing it is the defect being
fixed; the label is not, because saying which job produced it would be a guess.
`Reattachment.Ambiguous` says so and the card falls back to a neutral name rather than
borrowing the other job's. Aliases whose tags are identical -- the ordinary case, the
same file converted twice -- stay attributed, since nothing turns on which wrote it.
Age is not filtered on, and cannot be: WorkManager keeps finished work about a week and
prunes on its own schedule, so a query in a fresh process routinely returns jobs from
earlier sessions. Whether the staged file is still there is the only signal separating a
result still worth offering from one already dealt with, which is why that check carries
the weight.
Two smaller behaviours fall out of reattaching rather than starting:
- A reattached job that is cancelled lands on Idle rather than Ready. Ready would put
a Convert button over an input URI that belongs to a dead process. `observe()` takes
the cancelled destination as a defaulted parameter, so a job started here is
unchanged.
- A FAILED job's message falls back when blank, not only when absent. A worker killed
before it can report leaves no output data at all, and an exception's message can be
the empty string; both used to reach the screen as a failure with nothing said.
Deliberately not fixed here, each being its own change: the save dialog's suggested name
and MIME still come from the current picker rather than from the job that ran, so a
reattached job in a non-default format is offered the default extension; a result
dismissed with "Start over" still keeps its staged file, so it can be offered again next
launch -- the file check closes that for free once the file is deleted; and
`setForeground` still sits outside `doWork`'s try, so its throw bypasses the retry
decision entirely.
Tested where it can be. 28 JVM tests cover the choice and the tag round trip, including
the ordering, the aliasing rules and a display name that looks like another tag. The
reattachment itself is instrumented: a ViewModel constructed against the real
WorkManager is the next launch, with no memory of the work. `WorkManagerTestInitHelper`
is deliberately not used -- its `setDelegate` replaces the singleton for the whole
process, which would quietly turn `ConversionWorkerTest` into a synchronous test double
depending on class order.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
cfd705af05 |
Delete staged output the user never saved, instead of waiting for the OS
Every conversion writes a full-size file into <cacheDir>/conversions/. save()
published it and deleted it, but reset() -- what the "Start over" button on the
Converted and Joined states calls -- dropped the File reference and left the file
behind. Converting something and deciding not to save it is an ordinary path
through the UI, so it leaked a full-size copy every time. cacheDir is evictable,
so this was never unbounded growth; it was the app relying on the OS to clean up
after it, and on a device under no storage pressure "eventually" means never.
OutputPublisher.clearStaging() was written for exactly this and called from
nowhere. It is NOT wired up here -- it is deleted. It emptied the directory
unconditionally, and the convert tab, the join tab and ConcatEngine's
concat_list.txt all share that directory with no per-job namespacing (D8), so a
blanket delete could take a file out from under a running job. Two narrower
methods replace it:
discardStaged(file) one file, guarded. The handle reaches the ViewModel as a
path string in WorkInfo.outputData and becomes a File with
nothing checking where it points, so this compares the
CANONICAL parent against the staging dir -- the naive
string comparison accepts conversions/../elsewhere.
sweepStaging(now) age-based, for orphans no ViewModel is left to clean up.
The cleanup handle is a ViewModel field, not something read back out of the state
machine, because the state machine cannot answer it on the path that needs it
most: a failed save lands on Failed(message), which carries no file reference at
all. On that path the file is deliberately kept -- it may be the only copy of an
hour of transcoding and the destination did not receive it, so deleting to tidy a
cache directory would destroy the work. It stays collectable by a later reset()
or by the sweep.
The sweep runs once per process from a new Application subclass, off the main
thread. The reason it cannot race a live job is the grace period, not ordering:
WorkManager initialises through androidx.startup's InitializationProvider, a
ContentProvider, so it is already up before onCreate() and can be resuming a
worker in this same process while the sweep runs. StagingSweep only collects a
file nothing has written to for 24 hours. Outputs are written continuously and
keep their own mtime fresh; concat_list.txt is the one file written once and then
only read, and a WorkManager attempt is capped by the six-hour foreground-service
budget with retries restarting doWork() from the top, so no attempt can hold a
file still for a day. sweepStaging() also re-reads each timestamp immediately
before deleting, closing the window between listing the directory and acting on
the list -- unlinking an inode a running job still holds open would end with the
job reporting success for a path that no longer exists.
The rule itself is a pure function over (name, lastModifiedMs) pairs and a clock.
Timestamps are values rather than Files so the tests measure the arithmetic --
the grace boundary, and a clock that moved backwards -- rather than the
filesystem's mtime granularity.
Tests cover the tool AND the wiring, because the wiring is where the defect was.
A pure rule test and a Robolectric test of OutputPublisher both stay green when
the discardStaged call is deleted from reset(), which would have made the number
read as coverage of a bug that was still there. So both ViewModels are driven --
through a real WorkManager, to Converted/Joined -- and then asserted on the
filesystem: Start over deletes the staged file; a successful save leaves nothing
to delete twice; a failed save keeps the file and a later reset collects it, which
pins the argued decision above rather than leaving it as a comment. Verified by
deleting the discardStaged line from both reset() methods: 4 of the 6 fail with
"reset() should have discarded exactly the staged file expected:<[...]> but
was:<[]>", and the two save-path tests correctly stay green.
Three things made that reachable, all reusing what was already here:
- Both ViewModels now resolve their publisher through ConversionDependencies,
like the workers already did. They were the only place bypassing the seam.
- MediaProbe joins that seam too. It spawns FFprobe, and FFmpegKit's loader
throws a bare java.lang.Error when the native library is absent -- which its
own `catch (e: Exception)` cannot catch, so every JVM test died on the file
pick. Instrumented tests are unaffected and still get the real probe.
That error path is a LATENT PRODUCTION HAZARD, recorded in the KDoc and
deliberately not fixed here: onInputPicked does not catch it either, so a
missing .so would surface as an uncaught error rather than the "could not read
this file" the code was written to give. It cannot fire on a device that ships
the libraries, so widening MediaProbe's catch to Throwable would change the
pick path on the strength of a condition no user meets. Its own commit.
- reset()'s cleanup dispatcher is a constructor parameter defaulting to
Dispatchers.IO, which makes the delete assertable and states the ordering --
Idle is published synchronously, the delete is dispatched -- as a decision
rather than an accident. @JvmOverloads keeps the single-argument constructor
that viewModel()'s AndroidViewModelFactory looks up reflectively.
androidx-work-testing was already in the catalog and already inside the prerelease
guard via its androidx. group, so it needed no new pinning argument.
Robolectric is added for the one assertion no pure function can make: that the
file is really gone from a real cacheDir. It is PINNED at 4.16.1 and belongs with
ktlint/detekt/jacoco rather than the floating libraries. The prerelease guard in
app/build.gradle.kts only covers androidx., junit and com.arthenica, so
org.robolectric is unguarded and a "4.+" would resolve to 4.17-beta-3; beyond
that, a bump changes which android-all jar the tests execute against, which is the
same "a tool moved under a diff that cannot explain it" failure the linters are
pinned for.
Two things Robolectric needed. testOptions did not exist in this module at all;
it now sets isIncludeAndroidResources so the merged manifest and resource table
reach the JVM tests, and grants --enable-native-access, which Java 25 otherwise
warns about four times per run when Robolectric's native runtime calls
System.load(). And robolectric.properties pins sdk=36: Robolectric defaults to the
manifest's targetSdk of 37, there is no android-all jar for 37, and the class
fails to initialise before any test body runs. 36 is where CI's emulator matrix
already stops, so this does not widen the gap -- API 37 was already a manual check
on the Pixel 10 Pro XL before each release.
Known and left alone: a conversion that fails inside the worker never reaches
Converted, so pendingStaged is never set and any partial output relies on the
sweep alone. reset()'s delete is also fire-and-forget on viewModelScope, so it is
cancelled if the Activity finishes first. The sweep is the backstop for both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Jason Ross
|
903b43c30a |
Merge pull request #5 from JMR-dev/chore/lint-and-format-parity
Add lint, formatting and static analysis; move the toolchain to Java 25 |
||
|
JMR-devandClaude Opus 5
|
39e0900928 |
Adapt LibreMail's emulator instrumentation for the E2E matrix
The E2E legs could fail with almost nothing to show for it. The previous handler
was a single line of semicolons printing meminfo and 60 lines of crash logcat,
and it only ran when gradle RETURNED non-zero -- a hang left nothing at all, and
`adb logcat -d` at the end only holds whatever survived in the ring buffer, which
a chatty run evicts.
The two failure shapes want different evidence, so they are handled separately:
FAILED -- gradle returned non-zero. The test reports already say which test and
why, so this captures the surrounding state: guest memory and
storage, whether the app even installed, native crashes, and the
runner's own kvm/memory/disk.
WEDGED -- gradle never returned and the wrapper timeout killed it. There are no
reports, so the evidence has to come off the live device: which test
was in flight per the TestRunner logcat, whether the binder services
are published, and SIGQUIT thread dumps of both processes. That last
one is the point -- ART writes full stacks to logcat and /data/anr,
which is what separates a deadlocked test from a stuck MediaCodec
from a device that stopped answering. dumpsys media.player is in
there because both engines transcode through MediaCodec, so a hung
conversion shows up in it.
Logcat is now streamed to a file from the start of the step and uploaded whichever
way the leg goes, since the leg worth reading is usually the one that went red once
and green on re-run -- by which time the emulator is gone.
It is a script rather than inline YAML because it has to be. The action splits its
`script` input on newlines and runs each line as its own `sh -c`, so functions and
`if` blocks cannot survive there; that constraint is what produced the one-line
handler in the first place. One line calls the script now.
The wrapper timeout is 1200s against measured ~5-minute healthy legs, so it cannot
trip on a slow-but-working run, and sits far enough under the 60-minute cap to
leave room for the capture. It wraps only the foreground gradle client, never the
emulator the action owns, so it cannot hang the leg itself.
Not adopted from LibreMail: the hand-provisioned AVD boot, its SDK-integrity
installer and its focus gate. Those answer failures this repo has not had, and
replacing a boot path that works to fix problems we do not have is how a working
matrix breaks. Every emulator setting here -- ram-size, disk-size, the ABI filter,
swiftshader -- is untouched, along with the reasoning already written next to it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
3c7b4b9063 |
Stop the prerelease guard from rejecting AGP's own test platform
All four E2E legs failed to resolve :app:connectedDebugAndroidTest: Could not find com.google.testing.platform:android-device-provider-local:0.0.9-alpha04 That is AGP's Unified Test Platform, the thing that actually runs instrumented tests, and the guard added two commits ago was rejecting it. The guard was written as a blanket rule over every configuration, and AGP resolves its own tooling through this project's configurations. There is no stable version to move to, and there never has been: every module in com.google.testing.platform has only -dev and -alpha releases, going back to 0.0.1-dev. Nor is the version ours to choose -- AGP 9.3.1 pins it through com.android.tools.utp:android-test-plugin-host-additional-test-output:32.3.1. So an allowlist entry would have been the first of several, one per prerelease tool AGP happens to depend on, discovered one red matrix at a time. Scoping the guard to the groups this project actually floats fixes the class rather than the instance. It also retires the detekt exception: we do not float dev.detekt, so the guard now has no opinion about it, where before it passed only because "alpha.6" has a dot the pattern missed. Worth naming the shape of this bug, because it is the second time in this branch that a change looked complete locally and was not. Nothing that runs without a device resolves the UTP configurations -- unit tests, ktlint, detekt, lint, assembleDebug and assembleRelease were all green while connectedAndroidTest could not resolve at all. Verified now by resolving that exact configuration directly, and by confirming the guard still does its job: lifecycle 2.+ resolves to 2.11.0, not 2.12.0-alpha01. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
8727fccba1 |
Declare read-only permissions on the status-check workflow
CodeQL flagged the new static-analysis job for relying on the repository's default GITHUB_TOKEN scope. Fair, and the repo already holds the opposite opinion elsewhere: build.yml's release job spells out contents: write with a comment saying the token's reach should be visible at the point of use. Set at workflow level rather than on the one job that was flagged, because none of these four write anything -- they read the code, build it and attach reports. It also means a repository default that widens later cannot quietly widen these jobs with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
fd6e5325cf |
Raise Kotlin to 2.4.10 so the bytecode can join the toolchain on Java 25
The previous commit settled for Java 24 everywhere because Kotlin 2.2.10 refuses jvmTarget 25. That was the wrong constraint to accept, for two reasons. The first is that 24 turned out to be unbuyable. Adoptium's repository carries 8, 11, 17, 21, 25 and 26 -- no 24, because it is a non-LTS that went end of life in July 2025. The builds passed only because Gradle quietly auto-provisioned 24.0.2+12 through foojay, and .idea/misc.xml had been pointed at a temurin-24 that cannot be installed. A toolchain nobody can install is not pinned, it is lucky. The second is that the cap was never on the toolchain at all. Kotlin's ceiling applies to jvmTarget -- the bytecode -- and the JDK running the build is a separate axis. Conflating them is what steered this at 24 in the first place. So the fix is the one the sibling repo already uses: put KGP on the root buildscript classpath, where AGP's built-in Kotlin picks it up instead of the 2.2.10 it bundles. Kotlin 2.4.10 supports jvmTarget through 26, which lifts the ceiling above the toolchain rather than under it. The Compose compiler plugin is versioned in lockstep and reads the same catalog entry, so the two cannot drift, and the module now applies both by id() because they come from the classpath rather than from plugin resolution. Checked rather than assumed, since a silent downgrade would look identical to success: compiled classes report major version 69, which is Java 25. D8 dexes them, R8 minifies them, and ktlint, detekt, lint, the unit tests and the androidTest compile are all green on top. 25 is the right landing place independent of all this: it is LTS, it is in the Adoptium repository, and temurin-25-jdk is already installed here -- so the daemon runs on a real system JDK rather than a provisioned copy of an unpatched one. Two catalog plugin aliases went with it. android-application and kotlin-compose now resolve from the buildscript classpath, so leaving aliases behind would have left two entries that read like the source of truth and control nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
a7aa003020 |
Fix three ways the floating versions could have gone wrong quietly
All three shared a failure mode: the build stays green while doing something other than what the config says. composeBom was "2026.+". The Compose BOM numbers as YYYY.MM.PP, so the year is the major -- that float stops finding releases on 1 January 2027 and keeps building happily against a frozen BOM, with nothing in CI or the diff to say so. Bare "+" now, which is safe only because the prerelease guard is there. smart-exception was floating on "0.+". Under semver a 0.x minor may break, and this library is load-bearing precisely where breakage hides: the ffmpeg-kit wrapper reaches for smartexception.java.Exceptions only when a call FAILS, so a moved class shows up as an R8 missing-class error at release, or as a crash on the error path -- the least-exercised code in the app, by its own comment. Pinned, with that written down. It was noticed while the float was being written and shipped anyway, which is the actual mistake here. The prerelease guard permitted detekt's alpha by accident. The pattern wanted digits straight after the marker word, and detekt reads "2.0.0-alpha.6" with a dot -- so it passed on punctuation. Had it read "alpha6" the build would have broken with no way to see why from the config. There is now an explicit prereleasePermitted set, and the pattern tolerates both spellings, so the exemption is a decision instead of a coincidence. Also corrects a comment that was confidently wrong: componentSelection rejects STATIC prerelease versions too, not only floating ones. Naming "2.12.0-alpha01" in the catalog does not get you that alpha, it fails to resolve -- verified, not assumed, because the obvious guess is the opposite. Prereleases are taken by adding the group to prereleasePermitted. Two stale references to Gradle 9.5 updated to 9.7.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
e9542d2223 |
Let the libraries float on minor and patch
Library versions now read "1.+" instead of "1.19.0". Three groups stay pinned, and the reasons differ: agp/kotlin/ksp are version-locked to each other -- AGP 9.3.1's POM declares kotlin-gradle-plugin 2.2.10, so a float that picked up Kotlin 2.4.x would put the Compose compiler ahead of the Kotlin AGP actually compiles with. ktlint/detekt/jacoco because a linter is not a library. A library bump that misbehaves usually still compiles; a new lint rule makes files nobody touched stop passing, turning a PR red for something absent from its diff. Upgrading those is worth a commit that reads the new findings. The FFmpeg AAR is a committed file, not a coordinate. The componentSelection block is the part that makes this safe rather than the part that makes it work. Gradle resolves "+" to the highest version it can find and does not skip prereleases, and androidx routinely publishes alphas numbered above the current stable: lifecycle 2.12.0-alpha01, work 2.12.0-rc01, navigation 2.10.0-rc01, datastore 1.3.0-alpha10, annotation 1.11.0-alpha01 all outrank the releases this app uses. Without the guard, five dependencies would have moved onto unreleased code on the next build with nothing in the diff to say so. With it, every float resolves to exactly the version that was pinned before -- checked against :app:dependencies, not assumed. So this changes nothing today. Every library was already at its newest stable when the catalog was audited; floating is about what happens next month, not this commit. Trying a prerelease is still possible: name the exact version, which pins it rather than floating it. That is the right way round -- an alpha should be a deliberate act with a version number attached to it. Verified: ktlint, detekt, lint, unit tests, androidTest compile, assembleDebug all green, and the configuration cache still reuses across runs of the same task set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
3841f58c74 |
Put the whole toolchain on Java 24, and take Gradle to 9.7.1
Java was scattered across four numbers that nobody had chosen together: the
daemon ran on 25 (pinned in gradle-daemon-jvm.properties), CI installed 17, the
IDE was set to 25, and the app compiled to 17 bytecode. Now all four say 24.
24 rather than 25 because 25 is not reachable end to end. Kotlin 2.2.10 refuses
jvmTarget 25 outright -- "available targets are 1.8 ... 23, 24" -- so the app's
bytecode could never have joined a 25 toolchain, and "everything on the same
version" would have stayed false in the one place it is hardest to notice. 24 is
the highest number all four can actually hold. Checked, not assumed: D8 dexes
Java 24 class files, and R8 full mode minifies them, so the shipped artifact
builds on this too.
Floating where floating is native:
- java-version: '24' -- setup-java resolves the newest 24.x at run time.
- toolchainVersion=24 -- Gradle reports it as "Compatible with Java 24, any
vendor", and provisions whatever 24.x it finds or downloads.
The Gradle wrapper deliberately does NOT float, because it cannot: distributionUrl
names one archive and distributionSha256Sum is the checksum of that exact file.
That pairing is the wrapper's integrity check, and it is the same reasoning the
workflows already apply to action SHAs. Set via `./gradlew wrapper`, not by hand,
so the checksum matches the URL.
Dependencies were audited against Google Maven and Maven Central rather than
guessed at, and almost everything was already current: AGP, the Compose BOM,
core-ktx, activity, lifecycle, navigation, work, datastore, media3, room,
documentfile, annotation, espresso, androidx-junit, junit and ktlint are all at
their newest stable. Only two had moved -- detekt to 2.0.0-alpha.6 and JaCoCo to
0.8.15 -- and both are here.
Kotlin stays at 2.2.10 and that is now recorded as a verified fact rather than a
warning: the AGP 9.3.1 POM declares kotlin-gradle-plugin 2.2.10 at runtime scope,
which is what AGP's built-in Kotlin actually compiles with. Android lint suggests
2.4.10 and taking that suggestion breaks the build unless KGP is also forced onto
the root buildscript classpath. agp, kotlin and ksp move together or not at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
f4962913e1 |
Correct the JDK claim, and finish the @UnstableApi propagation
Two things the first pass got wrong. CLAUDE.md said "use a JDK 17-21, AGP 9 does not support 25+". That was carried over from the sibling repo and is not true here: gradle-daemon-jvm.properties pins toolchainVersion=25, so Gradle provisions and runs the daemon on Java 25 whatever JAVA_HOME says -- JAVA_HOME only picks the launcher. `gradlew --version` prints both, and shows them differing on this machine right now. It also means CI's java-version: '17' is not the JDK that compiles anything, and that the daemon JVM is the same on a laptop as on a runner, which is a better guarantee than the one the file claimed. Marking ConversionDependencies @UnstableApi propagates to its callers, and FakeFailures in androidTest calls it. That is a warning rather than an error in Kotlin, and lint does not read the androidTest source set, so the previous commit compiled clean while leaving one file inconsistent with the very pattern it described. Marked now. Left alone deliberately: gradlew.bat. The new `*.bat text eol=crlf` attribute governs how it is checked out from here on, which is the point of adding it, and the file already has CRLF in both the tree and the index. Rewriting the stored bytes of the wrapper script to prove the attribute works is not this branch's business. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
65a94b4ec1 |
Clear the 35 findings the new tools reported
detekt found 29 and Android lint 6, on a codebase neither had ever seen. Each
one was either fixed or relaxed with the reason written next to it; nothing was
suppressed to make the build quiet.
Fixed, because the tool was right:
- ConversionDependencies constructs Media3Engine, which is @UnstableApi, and
was not marked. Every other type here that touches Media3 propagates the
marker rather than swallowing it with @OptIn, so this one does too. Lint
was the only thing that had ever noticed.
- MediaProbe converted microseconds to milliseconds with a bare 1000, twice,
in a file that also handles a seconds-based duration from a different API.
US_PER_MS and MS_PER_SECOND now say which is which -- that confusion is a
real bug source in media code, not a style question.
- Foreground service types compared SDK_INT against 34 and 35 as raw ints
while the doc comment above spelled the version names out. VERSION_CODES
says it in the code.
- take(3) is a product decision about how many alternatives an error offers.
It means nothing until it is named; MAX_SUGGESTIONS does.
- setProgress(100, ...) is a percentage max, now PERCENT_MAX.
Relaxed, because the rule did not fit:
- The model package is excluded from ReturnCount and CyclomaticComplexMethod
ONLY. It is the decision layer: ConversionRouter.route scores 17 because
the app can give 17 distinct answers to "which engine, and why", each with
its own user-visible reason, and route's own comment records that their
ORDER decides which message is shown. Counting those as complexity measures
how many answers exist, not how hard the code is to follow. Everything else
-- LongMethod, NestedBlockDepth, ComplexCondition -- still applies there.
- A flat `when` used as a lookup table scores a point per entry, so
MediaProbe's demuxer-name to Container map read as complexity 21 with no
nesting and no state. ignoreSingleWhenExpression is the rule's own answer.
- TooGenericExceptionCaught off. MediaProbe, ConversionWorker and ConcatWorker
sit in front of native code that reports a malformed file as anything from
IllegalArgumentException to a bare RuntimeException, undocumented.
Enumerating that list means guessing, and a wrong guess crashes the app on a
file it could have reported as unreadable. SwallowedException stays on, so
these still have to log and handle.
- SI thresholds in the byte formatter, via ignoreNumbers. Each literal sits on
the line with the unit string it belongs to; BYTES_PER_MB would need a Long
and a Double and say nothing the line does not.
- allowedFunctionsPerObject, which the first pass simply missed.
Lint's three version-freshness nags are off. They do not describe this code --
they go red the day someone else publishes a release, which turns a PR red for
something its author cannot see in their diff, and they want the network at
lint time. Upgrades here are deliberate; Kotlin in particular is pinned to AGP's
bundled KGP and is not free to follow the newest release.
UsableSpace is informational rather than disabled, because it is a real finding
that this commit is choosing not to act on. hasSpaceFor reads File.usableSpace,
which ignores reclaimable cache, so the app can refuse a conversion it had room
for. StorageManager.getAllocatableBytes is the better answer, but it changes
when a job is rejected and can throw -- a behaviour change to a safety check,
which deserves its own commit and its own test rather than a drive-by here.
informational keeps it in every lint report instead of hiding it.
Also adds the CI gate and a CLAUDE.md. Coverage is reported and not gated: the
measured baseline is 31% of lines, which is exactly why LibreMail's 0.84 floor
was evidence about LibreMail and not a number to copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
496f1c7e73 |
Apply ktlintFormat
Tool output only, no hand edits, so this is safe to read with whitespace diffing off. It is its own commit for exactly that reason: a whole-repo reformat folded into the commit that configured the formatter would have made both unreviewable. What it did, mostly: trailing commas on wrapped argument lists, signatures collapsed onto one line where they fit inside 120 columns, import order, and four genuinely unused imports removed. Unit tests pass unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
93b1fbd5a9 |
Give this project the lint and formatting setup LibreMail already has
There was none: no .editorconfig, no static analysis, and CI ran only tests. Style was whatever the IDE happened to do, which is fine until two of them disagree. The split is LibreMail's, because it is the one that avoids arguments between tools: ktlint owns formatting, detekt owns static analysis with its formatting ruleset left off. Neither can contradict the other about the same line. Adapted rather than copied. LibreMail is Gradle 9.6 / JDK 21 with the configuration cache off; this is Gradle 9.5 / JDK 17 with it on, and Kotlin lives under src/main/java rather than src/main/kotlin -- so its plugin versions were evidence, not proof. Verified here before committing: both plugins resolve, ktlint reads src/main/java, and the run stores a configuration cache entry rather than tripping over it. detekt.yml carries only what applies. The Compose relaxations transfer intact -- a @Composable function is legitimately long, PascalCase, and full of dp literals no matter which app it is in. LibreMail's ForbiddenImport guard and its LargeClass exclusions do not: they name an AppLog facade and two test files that exist over there and nowhere here, and config that guards nothing is worse than no config, because the next reader has to work out that it is dead. detekt 2.0 is an alpha. That is not a preference: stable 1.23.x stops at Gradle 8.12 and this project is on 9.5, so there is no other line to be on. Coverage is reported, not gated. A floor needs a measured baseline, and the JVM test stack here is still junit-only -- a number picked before measuring would either fail on day one or mean nothing. Android lint gets warningsAsErrors because the other two tools fail on any finding, and a gate that stays green while its report fills up is not a gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
20a8049719 |
Merge pull request #4 from JMR-dev/feat/remux-and-codec-matrix
feat(media format) - Let the user pick a container and codecs independently and remux without re-encoding |
||
|
JMR-devandClaude Opus 5
|
b5d5fcfeff |
Merge Media3's MP4-only correction into the remux branch
CI on the parent branch proved that four of the five containers the router claimed for Media3 cannot be written by Transformer at all: WebmMuxer, OggMuxer, WavMuxer and AacMuxer each throw UnsupportedOperationException from addMetadataEntry, which MuxerWrapper calls for every metadata entry on the track format. Consequences here beyond the merge itself: - MEDIA3_MUXABLE_VIDEO and MEDIA3_MUXABLE_AUDIO drop to a single MP4 entry. Every other container is already on its way to FFmpeg before those maps are consulted. - Reason.WEBM_CODEC_UNSUPPORTED is removed. WebM now fails the container check first, so nothing could ever produce that reason, and a routing reason no code path can reach is worse than no reason at all. - Media3Muxers gains null branches for the six containers this branch adds. MOV is among them despite being MP4's own family: Mp4Muxer exposes no QuickTime file format. - The README no longer claims Media3 writes five containers. The remux behaviour this branch exists for is unaffected: MKV -> MP4 was always the hardware direction, because Media3 reads Matroska but has never been able to write it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
1294439602 |
Merge pull request #2 from JMR-dev/fix/media3-honours-output-format
Make Media3 write the container and codec it was asked for |
||
|
JMR-devandClaude Opus 5
|
92b7395ba9 |
Media3 can only write MP4, so stop claiming otherwise
CI proved the WAV and Ogg exports this branch added cannot work, and the reason
generalises further than those two.
media3-muxer 1.11.0 ships WebmMuxer, OggMuxer, WavMuxer and AacMuxer, which is
why MEDIA3_CONTAINERS listed the matching containers. But all four throw
UnsupportedOperationException from addMetadataEntry, and
MuxerWrapper.addTrackFormat calls it for every metadata entry on the track
format. Any real recording carries at least a creation timestamp, so the export
dies partway through:
Caused by: java.lang.UnsupportedOperationException
at androidx.media3.muxer.OggMuxer.addMetadataEntry(OggMuxer.java:123)
at androidx.media3.transformer.MuxerWrapper.addTrackFormat(MuxerWrapper.java:488)
They are standalone muxers, not Transformer-compatible ones. WAV fails a second
way before even reaching that: DefaultEncoderFactory has no PCM encoder, so
Transformer reports "No MIME type is supported by both encoder and muxer"
instead of passing raw samples through.
Both observed on an API 35 emulator in CI, not inferred. The tests that found
them were written on the assumption these containers worked.
So MEDIA3_CONTAINERS becomes {MP4}. That the set was wrong went unnoticed
because the engine ignored the container and wrote MP4 regardless — the set
being wrong and the engine being wrong cancelled out. WAV, Opus and raw AAC move
to FFmpeg, which already produces all three with instrumented coverage asserting
the produced files.
WEBM_VP9's routing reason changes from NO_PLATFORM_ENCODER to
CONTAINER_UNSUPPORTED. Both were always true; the container is the more
fundamental, since even given a VP9 encoder the file could not be written.
The audio-only regression guard this branch exists for passed on API 35: M4A
output now carries exactly one AAC track and no video.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
2e0cf2f5d6 |
Let the user pick a container and codecs independently, and remux without re-encoding
OutputFormat was a closed enum of twelve (container, videoCodec, audioCodec) triples, defended on the grounds that a closed set was what made routing decidable. Two things it could not express: changing the container while copying the streams, and choosing codecs per track. OutputSpec replaces it as the vocabulary; OutputFormat stays as presets over it. Decidability moves to ContainerCapabilities, which is explicit and unit-tested rather than implicit in whichever combinations somebody enumerated. The matrix is indexed by (container, codec, trackType, mode), not one boolean. "Can MP4 carry AV1" and "can this app encode AV1" have different answers, and copy is where the difference shows: a single flag would refuse a legitimate remux or promise an encode neither engine can deliver. COPY is a codec value rather than a flag, so every exhaustive `when` in the codebase had to say what it does about copying. CopyPlanner resolves it before anything else reads the request, and inherits ConcatPlanner's rule that an unproven match is never a copy — a needless re-encode costs time, a wrong stream copy costs a file that will not play. Container now drives -f, the extension and the SAF MIME type, so Matroska without video is .mka and MP4 without video is .m4a without a preset for each. FLAC was declared as Container.MKV with a .flac extension, inert only while nothing read the container; it now has its own. Six containers added: MOV, MKV audio, MPEG-TS, AVI, FLV and WMV/ASF. Routing asks the plan, never the request. COPY belongs to none of the capability sets, so testing the request directly sends every remux to FFmpeg on the first check — and nothing notices, because -c copy produces a correct file, just on the CPU. The router also learns what Media3 can *carry* as opposed to encode: its MP4 muxer takes AAC, Opus, Vorbis and PCM but neither MP3 nor FLAC. MediaProbe now separates "no video track" from "could not parse" and reports the source container, which MediaExtractor cannot supply at all. FFprobe runs on every pick for that reason, not as a fallback. The Advanced picker shows the whole matrix and lets an impossible combination be selected on purpose, then explains it and offers alternatives. Convert is what blocks the job. ConversionWorker validates too, so a stale queued spec fails with the reason rather than being coerced into something else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
00c422f317 |
Make Media3 write the container and codec it was asked for
Media3Engine never called setMuxerFactory or setAudioMimeType, and built a bare EditedMediaItem, so it always produced MP4 with an H.265 video track. The router meanwhile sends it WebM, Ogg, WAV and AAC-ADTS jobs, plus audio-only M4A, Opus and WAV — and ConversionWorker.media3MimeType() mapped VideoCodec.NONE through its else branch to VIDEO_H265. The visible result: "extract audio to M4A" transcoded the video to HEVC and named the file .m4a. Nothing failed, and nothing caught it, because Media3EngineTest had no audio-only case at all. media3-muxer already ships WebmMuxer, OggMuxer, WavMuxer and AacMuxer; only the MP4 ones come pre-wrapped as a Muxer.Factory. Media3Muxers supplies the rest. Their reported sample MIME types are read from each muxer's own support check rather than assumed, because Transformer uses those lists to decide whether a track needs re-encoding. HardwareTranscoder.transcode now takes the OutputFormat instead of a video MIME string, which is what gives the container, the audio codec and "this output has no video" somewhere to travel. MEDIA3_CONTAINERS stops being private so a test can assert it agrees with the factories. Those two drifted once already: the router's set was right the whole time the engine was ignoring it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
243e7ec03d |
Merge pull request #1 from JMR-dev/feat/media3-conversion-pipeline
Media3 + FFmpeg conversion pipeline |
||
|
JMR-devandClaude Opus 5
|
edd6385bf7 |
Record that the suite passes on real API 37 hardware
The doc reasoned that the WorkManager and lateinit failures in CI were downstream of the broken framework rather than real defects, but said so as inference and flagged that only a healthy API 37 device could settle it. One was available. The full instrumented suite runs green on a Pixel 10 Pro XL on Android 17 -- a release build, not a preview -- with 40 tests, 0 failures, 2 skipped, both skips being benchmarks that assume sample files present. ConversionWorkerTest and ConcatWorkerTest drive a real WorkManager round trip and are among the tests that failed that way in CI; they pass on hardware. So the bug is confined to the emulator image, and the gap left by the missing matrix row is automated coverage rather than confidence in the app. Noted that the suite should be run on a physical API 37 device before each release while the row is absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
4e6fe6b75a |
Drop API 37 from the E2E matrix and write down why
The android-37.0 emulator image crash-loops surfaceflinger inside its own gralloc mapper: RegionSamplingThread calls GraphicBuffer::lock, which reaches GoldfishMapper::readFromHost, which asserts that the host has not negotiated ReadColorBufferDma. It has, so surfaceflinger aborts, restarts, and aborts again. Nothing this app does can survive that, and it reproduces on a GitHub runner under swiftshader_indirect and on a workstation under -gpu host alike. There is no ATD image at android-37.0 to fall back to, and -feature -GLDMA is accepted by the emulator but does not prevent the assertion. Correcting the previous commit, which is already pushed so its message stands: ram-size was not the cause of that failure. Setting it did move the job from failing at install to failing during the test run, which is how the real crash became visible, but at 2560M the guest had 1.5 GB free when it died. The setting is kept because the emulator's own floor varies by API level -- 2048M at 33, 2560M at 34 to 36 -- and pinning it makes the matrix uniform. Also corrected: a comment claiming this could not be reproduced locally. It can, and the local crash was the same one all along. Dropped the dmesg probe. adb shell is not root, so klogctl is denied and it only ever printed a permission error -- which a later reader would reasonably misread as "no OOM kills". docs/api-37-emulator-crash.md carries the evidence, the ruled-out fixes, the reproduction, and how to file it upstream, so re-adding the row later starts from what is already known rather than from scratch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2fe1aa9f9c |
Stop the memory probe from being able to fail the run
The probe line runs before the tests and its exit status is grep's, so a run where adb returned nothing would have exited 1 on the first line and reded the job before Gradle started -- on all five levels, four of them currently green. The action passes no ignoreReturnCode, so exec throws straight into setFailed. A diagnostic must never be the thing that turns a run red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
a79ff62b61 |
Give the API 37 emulator the RAM every other level already gets
API 37 was the only red job in the matrix, and the last two fixes each corrected a real problem only to reveal the next one. This is the cause of the third failure. The emulator raises an undersized guest to 2560M on its own, but only for API levels it recognises, and it does not recognise "37.0". Comparing the two CI logs from the same emulator binary (37.1.11.0) shows the asymmetry directly: the API 36 job logs "Increasing RAM size to 2560MB" and the API 37 job has no such line. So four levels were quietly running at 2560M while API 37 ran at the pixel_6 default of 1536M, lost system_server partway through installing the 82 MB APK, and surfaced it as "Can't find service: package". 2560M is not a guess at a sufficient value -- it is the value the other four levels already pass at, so this makes the matrix uniform rather than introducing a fifth configuration. Verified that the setting actually lands: the action appends hw.ramSize to a config.ini that already has one from the profile, so the fix only works if the later key wins. Appending a distinctive 3072M to an API 36 AVD produced MemTotal 3047924 kB and suppressed the automatic bump, confirming it does. This failure cannot be reproduced locally -- API 37 will not boot on a workstation under either GPU mode, aborting surfaceflinger in the goldfish mapper under -gpu host and segfaulting the emulator under swiftshader_indirect -- so the job now reports guest memory on every run and dumps OOM kills and native crashes on failure. That makes the next run conclusive either way instead of producing another bare "Can't find service: package". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
97cddea973 |
Pin build.yml's actions and verify what it publishes
Brings the release workflow in line with the status check. It matters more here, not less: these jobs publish the artifacts people install, so running whatever a mutable tag points at on the day is a worse bargain than it is on a pull request. Every action is pinned to a commit with its release in a trailing comment, and each hash was checked to resolve to the tag it claims. gradle/actions is dropped for the same reason as before -- its v6 caching component is closed source and carries separate terms -- with Gradle running through the committed wrapper, which verifies its own distribution against distributionSha256Sum. The release job now checks what it is about to publish. A release that shipped a single ABI, or that lost 16 KB alignment in a rebuild, installs fine on a test device and then fails for users or at Play submission. Both are cheap to assert and expensive to discover afterwards. It deliberately does not pass -PabiFilters: that override exists so emulator jobs skip libraries they cannot execute, and a released artifact must carry every ABI. The contents permission is declared explicitly rather than inherited from the repository default, so the token's reach is visible in the file that uses it. The corresponding-source tarball now includes bin/README.md as PREBUILT.md, so the GPL source drop carries the shipped binary's SHA-256 and configure line rather than only the recipe that produces it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
5b47764c70 |
Build only the emulator's own ABI for instrumented tests
API 37 got as far as running the suite this time and then failed to install: 'package install-create ... -S 117817978' java.io.IOException: Requested internal only, but not enough space The 117 MB debug APK did not fit on the emulator's data partition. The DELETE_FAILED_INTERNAL_ERROR that followed was the same exhaustion, not a second problem. It surfaced on API 37 because that system image is the largest and leaves the least free userdata. The margin was thin at every level, so this was never really an API 37 bug -- the others were simply further from the edge and would have caught up as the APK grew. Roughly half that APK is arm64-v8a FFmpeg libraries that an x86_64 emulator can never load. abiFilters is now overridable, so a test run builds only what it will execute: 114 MB becomes 80 MB. Release builds ignore the property and still ship both ABIs, so nothing about what gets distributed changes. disk-size is raised to 8G for every level rather than only the one that failed, since fixing just API 37 would leave the rest waiting their turn. Verified locally on an API 36 emulator with an x86_64-only APK: 40 instrumented tests, 0 failures, and the installed APK contains lib/x86_64 only. 66 unit tests still pass, and a release build still carries both ABIs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2d2687aa3c |
Pin CI actions to commit hashes and fix the API 37 emulator run
The API 37 job failed after 23 seconds, before an emulator ever started. The CI log names the cause exactly: sdkmanager --install 'build-tools;37.0.0' platform-tools 'platforms;android-37' Warning: Failed to find package 'platforms;android-37' There is no platforms;android-37. The release is published as android-37.0, alongside 37.1 and the 37.2 betas. The earlier attempt to fix this with system-image-api-level was aimed at the wrong package: that input only names the system image, while the platform is installed from api-level directly. Setting api-level to 37.0 resolves all three packages, and build-tools is a hardcoded constant in the action rather than derived from api-level, so it is unaffected. A separate label field keeps the job name reading "API 37". Every action is now pinned to a commit hash with its release in a trailing comment. A tag is mutable: the owner can repoint v4 at new code whenever they like, so a tag reference amounts to running whatever that repository contains tomorrow. Each hash was verified to resolve to the tag its comment claims, because a wrong hash is worse than a tag -- it looks deliberate. Versions moved a long way in the process: checkout v4 -> v7.0.1, setup-java v4 -> v5.7.0, upload-artifact v4 -> v7.0.1. gradle/actions is gone rather than upgraded. Its v6 release moved the caching component closed-source and states that upgrading accepts Gradle's Terms of Use for it. That has no bearing on the project's own licence -- a CI tool is never combined with or distributed alongside the app, unlike the FFmpeg libraries that make the APK GPL -- but it is a component in the build path that cannot be audited or forked. Gradle now runs through the committed wrapper, which verifies its own distribution against distributionSha256Sum, and caching is a handful of lines of actions/cache. The rest of the matrix passed on this run: API 33, 34, 35 and 36 all green, along with the unit tests and the FFmpeg archive check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
dd2fcf0a19 |
Rename the project to LibreMediaConverter
Done now rather than later: the application ID is permanent once published -- Play treats a change as an entirely different app -- so this is the last cheap moment to choose it. applicationId / namespace dev.jasonmross.mediaconverter -> org.libremediaconverter source tree java/dev/jasonmross/mediaconverter -> java/org/libremediaconverter gradle project AndroidMediaConverter -> LibreMediaConverter theme Theme.MediaConverter -> Theme.LibreMediaConverter compose theme MediaConverterTheme -> LibreMediaConverterTheme display name "Media Converter" -> "LibreMediaConverter" org.* rather than dev.jasonmross.* because "Libre" signals a project rather than a personal app, and a project-owned namespace lets maintainership move later without the identifier contradicting reality. The source trees moved with git mv so history follows the files instead of showing 42 deletions beside 42 additions. Verified after the rename: 66 unit tests, and 40 instrumented tests on an API 36 emulator, 0 failures. The built APK reports org.libremediaconverter, and no stale jasonmross, AndroidMediaConverter or MediaConverterTheme identifiers remain anywhere in the tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
128763e99c |
Commit the FFmpeg binary so test runs stop depending on a rebuild
CI rebuilt FFmpeg on every cold cache, which made results ambiguous: a red run could mean the code was broken or that a forty-minute cross-compile of FFmpeg, x264, x265 and SVT-AV1 had hiccuped. Those are not the same signal, and only one of them is worth a developer's attention. The archive is now checked in under bin/, so a failing run points at code. It also removes roughly forty minutes from a cold run and lets a fresh clone build without a container toolchain. bin/README.md records provenance -- upstream tag, FFmpeg version, NDK, ABIs, SHA-256 and the full configure line read back out of the shipped libavutil -- so the binary is auditable rather than opaque. The recipe in tools/ffmpeg remains the authority: this archive is its output, and is also what satisfies the GPL corresponding-source obligation. The status check is now seven independent runners: one validating the archive, one for the JVM tests, and one per API level from 33 to 37. The FFmpeg job verifies rather than builds. It asserts native libraries are present for both ABIs and that every one is 16 KB aligned, which is a Play requirement that is easy to lose in a rebuild and expensive to discover at submission. Checking for file existence alone would not do: a Git LFS pointer checked out without LFS passes that and then surfaces as an obscure linker error much later. It is a separate job rather than a step in each emulator run so a bad archive reports once, clearly, instead of five confusing emulator failures. build.yml no longer builds FFmpeg either, and keeps only its post-merge and release duties. Two costs, deliberately accepted. The repository goes from about 1 MB to 35 MB, and every future rebuild adds another 35 MB blob to history permanently, so bin/README.md says to regenerate only when the FFmpeg version or the configure flags actually change. And F-Droid's scanner flags checked-in native libraries, so submitting there needs a scandelete entry for bin/ -- noted in bin/README.md, and nothing prevents a from-source build. Verified against the relocated archive: 66 unit tests, and 40 instrumented tests on an API 36 emulator, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |