e7c3e5688fcd2b384f1e33ca703eb52ed744f372
53
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
07f7ed4259 |
Merge #221, and make the two counts derived rather than remembered
#221 landed while this was in review, adding one instrumented test: 68 -> 69, and 64 on the gating leg. Its own commit was "Move CLAUDE.md's instrumented counts with the test that changes them", and it still arrived stale -- it says three markers and 61 tests, both true of the main it was branched from and neither true of the main it merged into. That is the third time these two numbers have gone stale in a day, so the paragraph now says where they come from: a grep for @Test over app/src/androidTest minus the marker count, cross-checkable against any run's shape, since a leg below 37 reports the first as `expected` and the API 37 gating leg reports the second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
7fd95ddede |
Merge main, and re-derive every count it moved
main landed 25 commits while this branch was open, including a third @FailsOnEmulatorApi37 on Media3EngineTest.cancellingARunningExportStopsIt and a batch of new instrumented tests. Every number this branch touches moved with them. Re-derived rather than adjusted, and cross-checked against run 34020234606: the API 34 leg (no filter) reports 68 tests and the API 37 gating leg 64, which is 68 minus main's four markers. With the picker test marked that is five markers, baseline 5, and 63 on the gating leg. The conflict in FailsOnEmulatorApi37.kt is resolved main's way: it had replaced the hardcoded "grows by two" with a reference to the constant, which is the same drift this file exists to prevent and a better fix than the number I put there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
350b179c9e | Merge branch 'main' into test/join-failure-message-on-device | ||
|
JMR-devandClaude Opus 5
|
31f249ae04 |
Cancel a running Media3 export, completing #224's third engine
The two FFmpeg engines were done in |
||
|
JMR-devandClaude Opus 5
|
6992f0e783 |
Reattach to a conversion that is still running (#230)
Reattachment.rank gives RUNNING the highest rank of all -- "live work outranks a finished result because a running job is holding a foreground service" -- and no test on either source set had ever produced one. ReattachOnLaunchTest covers a job that finished, one whose staged file is gone, an ambiguous pair, one still queued, and one the user cancelled. ReattachmentTest exercises the ranking as a pure function over fabricated snapshots. What was missing is a ViewModel meeting a real running job, which is also the likeliest reattachment there is: the user starts a conversion, leaves, and comes back while it is still going. The engine is a fake, deliberately. The job has to still be running when the ViewModel is built, and every real conversion in this suite finishes in about a second -- racing that is what made the cancellation tests flaky enough to need retries. A SoftwareTranscoder that blocks until released removes the race outright. Nothing about reattachment depends on which engine is transcoding: the tag query, Reattachment.choose over live WorkManager state, and observe's mapping to Converting all run identically whatever is doing the work. This is what #230 can actually deliver, and the ticket asked for the answer either way. Process death itself stays device-manual. D3/D13 already record that am kill refuses a process holding a foreground service, and there is a more basic obstacle underneath it: instrumentation runs in the app's own process, so any route that really killed it would take the test runner with it and leave nothing to assert with. Observing a relaunch needs two instrumentation runs, which the runner does not provide. So the closest observable analogue is a fresh ViewModel, with no memory of the work, meeting a job that is genuinely mid-flight. The teardown now resets ConversionDependencies. The suite runs without Android Test Orchestrator, so a BlockingTranscoder left in place would hang the next class that converts anything. Verified on a local API 34 emulator: 67 tests, 0 failures, 3 skipped; and making RUNNING unreattachable in Reattachment.rank fails this test and nothing else -- which is also the evidence that the JVM ranking test was not already covering it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
802997439d |
Let a join read the files the user actually picked (#238, #225)
Joining files picked through the system picker failed outright whenever the strategy was stream copy -- the matched-files case the UI advertises as "joined without re-encoding, no quality loss". [ffkitsaf @ ...] Protocol 'ffkitsaf' not on whitelist 'file,crypto,data'! Error opening input file .../joined_from_content.concat_list.txt JoinScreen picks with OpenMultipleDocuments, so real inputs are always content://. ConcatEngine maps each through getSafParameterForRead and FFmpegConcatCommand writes the resulting ffkitsaf: paths into the concat list file. The demuxer applies its own protocol whitelist, defaulting to file,crypto,data, and -safe 0 does not touch it: that permits absolute paths, this permits the scheme they carry. Two separate gates, and only one was open. Nothing caught it because the two halves of the bug never met. Only STREAM_COPY feeds the demuxer a list file -- REENCODE passes each input with its own -i, where the whitelist does not apply -- so joining over SAF worked for mismatched clips. And every join test passed Uri.fromFile, which takes ConcatEngine's uri.path arm instead of the bridge, so matchingClipsAreJoinedByStreamCopy exercised stream copy with a file: path and passed. The one broken combination was the one no test produced and the only one a user can reach. That is #225's gap: FFmpegKitConfig.getSafParameterForRead is on every real conversion and join, and was on no passing test -- only on UnopenableUriTest's failure side, which proves the error message rather than the bridge. ContentUriInputTest now drives both the convert and join paths from a real content:// URI. It uses a plain ContentProvider, because the documents provider cannot be reached. Measured three ways: a DOCUMENTS_PROVIDER without MANAGE_DOCUMENTS is refused at install, instrumentation runs in the target app's process so Instrumentation.getContext() still carries the app's uid and is denied, and adoptShellPermissionIdentity(MANAGE_DOCUMENTS) is denied identically -- the denial naming ACTION_OPEN_DOCUMENT as the only way in. The bridge needs no documents provider: it opens a descriptor through the resolver, so any readable content:// URI exercises it, and an ordinary provider may be exported unprotected. The whole class stays headless. Recorded on #226, which that also settles: its cheap half does not exist. Verified on a local API 34 emulator: 66 tests, 0 failures, 3 skipped; and removing the -protocol_whitelist pair reproduces the production failure verbatim in the join test and nothing else. FFmpegConcatCommandTest pins the flag on the JVM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
163ce54b77 |
Stop the cancel tests losing their race on a loaded runner
Both cancellation tests I added in |
||
|
JMR-devandClaude Opus 5
|
5416788274 |
Cancel a running join session too (#224)
The FFmpegEngine half landed in ad2a75d; this is the same gap in ConcatEngine. Between them, a real native session being asked to stop is now covered on both FFmpeg paths. The assertion is the session's return code again, and here that is not merely the better choice but close to the only one: ConcatEngine does not delete its output on cancellation at all. Its invokeOnCancellation is FFmpegKit.cancel and nothing else, where FFmpegEngine's also deletes the partial. Whether that asymmetry is deliberate is a separate question, so this asserts what is true of both engines rather than depending on it. The cancel triggers on SessionState.RUNNING rather than on progress. ConcatWorker publishes no progress at all, so there is no callback to hang it on even in principle -- and the conversion side already measured the deeper reason, that the committed clips outrun a callback-triggered cancel. The inputs are the mismatched pair on purpose, so ConcatPlanner chooses REENCODE. A stream copy of two 2 s clips is close to instantaneous and would leave nothing to interrupt; re-encoding is also the case where a user would actually reach for Cancel. Verified on a local API 34 emulator: three runs green at 64/0/0/3, and replacing invokeOnCancellation's body with an empty block fails this test and nothing else, with state=COMPLETED rc=0. Media3Engine's transformer.cancel() is still uncovered and #224 stays open for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Jason Ross
|
ad2a75d9a0 |
Merge pull request #236 from JMR-dev/test/cancelling-a-running-session
Cancel a running FFmpeg session, which nothing had ever done |
||
|
Jason Ross
|
2b921fafe4 |
Merge pull request #235 from JMR-dev/test/notification-cancel-action
Press the Cancel button in the notification |
||
|
JMR-devandClaude Opus 5
|
98c0e4dba2 |
Cancel a running FFmpeg session, which nothing had ever done (#224)
Every cancel in app/src/androidTest is WorkManager.cancelWorkById against work that is queued or already finished: ReattachOnLaunchTest cancels a job carrying a one-hour initial delay, and another immediately after enqueue. On the JVM, WorkerCancellationTest and HardwareFallbackTest's cancellation case drive a SoftwareTranscoder double that records the call. No test on any source set had asked a real native session to stop. That is docs/defect-audit.md D10's forcing condition. It is the one path where cancelling wrong is silently expensive rather than loudly broken: a missed FFmpegKit.cancel leaves the native process encoding to completion while the UI says the job is cancelled. Two things were measured rather than assumed, and both changed the test. The output file cannot be the assertion. invokeOnCancellation deletes the path, and on POSIX unlinking a file ffmpeg still holds open leaves ffmpeg writing to the unlinked inode -- so the path stays gone whether or not the cancel reached the session, and removing FFmpegKit.cancel passes that check every time. The session's own verdict is what separates them: a cancelled session ends with the cancel return code, a completed one does not. Cancelling from the first progress callback loses the race. It was tried first and failed with state=COMPLETED rc=0: every committed fixture is 2-3 s at 320x240, and the encode finishes before the first statistics callback is delivered and acted on. FFmpegKit.listSessions shows the session RUNNING far earlier, so that is what the test waits for. QualityTier.BEST is deliberate for the same reason -- preset medium leaves more of the encode ahead of the cancel. Verified on a local API 34 emulator. Four consecutive runs green at 62/0/0/3, and removing FFmpegKit.cancel while keeping output.delete fails this test and nothing else, with state=COMPLETED rc=1. ConcatEngine and Media3Engine carry the same shape and are not covered here; #224 stays open for them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
557b3edab4 |
Compare the advisory failure count only on a run that finished
The verification dispatch of the reworked harness (34011072884) came back 4 expected / 3 received / 3 failed, where the one before it (34008889182) had been 4/4/4 on the identical configuration. Nothing about the test list changed between them: the abort landed one test earlier and the picker test never started. The baseline check would have called that "one now passes", which is the wrong reading and the kind of notice #120 is about -- a deviation that is wrong often enough to teach everyone to skim past deviation notices. So `failed` is compared only when `completed cleanly` is yes, and `expected` is compared always, because `Starting N tests` is printed before anything can abort and is what actually answers "is the marked set the size the baseline says". Two cases in e2e-report-shape-test.sh, as a pair: a truncated run short by one is not a deviation, and a CLEAN run short by one still is -- so the first cannot have bought its quiet by disabling the check. This is a consequence of adding the fourth marker rather than a pre-existing bug worth its own ticket: with three, the advisory leg had been completing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
e0412329ff |
Press the Cancel button in the notification (#227)
ConversionNotifications.build attaches one action, wired to WorkManager.createCancelPendingIntent(id). Until now createCancelPendingIntent had no references anywhere outside its own declaration -- no JVM test, no instrumented test. That is worth more than an ordinary uncovered line. A conversion runs in a foreground service and the user is invited to leave the app; once they do, this action is the only way to stop it. If the PendingIntent carries the wrong id the button does nothing, the notification stays, and the job runs to completion, with no error, no log and no screen to look at. The obvious version of this test reads NotificationManager's active notifications for id 1001 and taps what it finds. Rejected: the instrumented suite grants no runtime permissions, so POST_NOTIFICATIONS is denied throughout, and whether a suppressed foreground-service notification is returned there is a platform detail that varies. The test would be asserting something about notification visibility rather than about cancellation. The PendingIntent is the subject and where it is read from is incidental, so this builds the notification for a real live work id and fires its action -- a real dispatch reaching real WorkManager, the same way on every API level. The job carries an initial delay so it stays ENQUEUED. A conversion of the 3 s fixture finishes in well under a second on an emulator, so racing a cancel against a running job would be flaky in the direction that fails, and cancelWorkById acts on ENQUEUED identically. What is under test is whether firing the action reaches WorkManager with the right id. Verified both ways on a local API 34 emulator: 61 tests, 0 failures, 3 skipped as written; and building the PendingIntent from a random UUID instead of the request's id fails the new test and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
948d53b67e |
Read the progress percentage FFmpeg has always been computing (#229)
FFmpegEngine derives progress as stats.time / durationMs * 100, and the statistics callback runs on every conversion in FFmpegEngineTest -- they all pass durationMs = 3_000. But every call site omits onProgress, so nothing on any source set had ever looked at the number. Replacing percent with a constant reddened nothing. What already existed covers the plumbing downstream and not this: #196 covered the worker's progress lambda with a fake engine that reports whatever the test tells it to, and ProgressNotificationTest covers the throttling the same way. The arithmetic was the one part with no reader. The new test passes 30 s as the duration for a fixture that is exactly 3.000 s, so the conversion still encodes the whole clip and the reported percentage tops out around 10 rather than 100. That is what makes it bite. A range check alone is worthless: a constant 0 satisfies both "every value is in 0..100" and "the values never go backwards", and so does a list of [0, 100]. Pinning the band rejects every constant, and because the band sits a tenth of the way up it also rejects an implementation that ignores durationMs, which would report ~100 for the same run. The bound is loose -- 5..25 for an expected 10 -- because the last statistics callback can land slightly before the final frame. Verified on a local API 34 emulator: 61 tests, 0 failures, 3 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
ba16f5a89b |
Make the fallback test say when it cannot test the fallback (#223)
HardwareFallbackTest is the only automated check of the hardware->software fallback against a real codec failure, and it passed on every CI leg without ever attempting the hardware path. Measured on run 34004304566: the API 33, 34, 35 and 37 legs each log Routing sample_h264_444.mp4 -> ... via FFMPEG (NO_HARDWARE_ENCODER) Emulators expose no hardware encoder, so the router never chooses Media3 and runMedia3OrFallBack's catch is never entered. The test's assertions -- succeeded, output non-empty -- are true of that conversion too. It finished in 448 ms, which is not long enough to fail an export and then software-encode a three-second clip. Deleting the catch reddened nothing. The ticket offered two fixes and left the choice open. Trying the first one answered it, and not the way the ticket expected. Pinning deviceCodecs to PERMISSIVE, as ForcedFailureTest does, makes the router choose Media3 -- and the export then SUCCEEDS. On a local API 34 emulator, MediaCodecInfo logs NoSupport [codec.profileLevel, avc1.F4000C, video/avc] for both c2.goldfish.h264.decoder and c2.android.avc.decoder, and ExoPlayer allocates the goldfish decoder anyway, which decodes the High 4:4:4 fixture regardless of the profile it declares. c2.android.hevc.encoder then encodes it and the job reports MEDIA3. So the class KDoc's "Media3 fails partway through the export on every device" is not true of the emulator images, and no routing pressure makes this fixture force a fallback there. Pinning would also swap in software codecs, which is not the path a real device takes -- it is what made the forced run succeed. That leaves assumeTrue on the production premise as the honest answer, now with a measurement behind it rather than a coin flip. The test skips where it cannot mean anything and runs on the Pixel, where it always could. When it does run the assertion is a pair, because KEY_ENGINE_USED is FFMPEG whether the fallback fired or the router went straight there: the router chose MEDIA3 for this request on this device, AND the worker reported FFMPEG. Together, and only together, that is the fallback. Verified on a local API 34 emulator: the test reports SKIPPED and the level reports skipped=3. ForcedFailureTest still covers the fallback wiring on every leg with a double; what needs a real encoder is two real engines disagreeing about a real file. The third permanent skip is recorded in docs/local-emulator.md and beside SafPickerRoundTripTest's run-shape note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
e2f8ef2918 |
Cite the two measurements the last two commits asserted
The advisory-leg count (4 expected, 4 received, 4 failed with the new marker) is api37-debug run 34008889182, dispatched with the annotation as its filter. The force-stop recovery was made to go red before it was believed: on a local API 36 emulator, with the picker left open and the back presses removed, the test passes with forceStopThePicker() and fails with exactly the API 37 message without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
9f06eb9988 |
Check that FLAC and Opus are FLAC and Opus (#228)
encodesFlacLosslessAudio and encodesOpus asserted only that a non-empty file appeared. Five siblings in the same class check what is in it -- encodesWav reads RIFF two lines away, and GIF, Matroska, MP3, H.264 and H.265 all assert a container marker or a track MIME. convert() throws on a non-zero return code, so these two did prove the command ran. What they could not distinguish is the command running and producing the wrong thing, which is a failure mode this codebase has already had once: F1 in docs/coverage-read-findings.md records a live Vorbis arm in FFmpegCommandBuilder that ContainerCapabilities says cannot exist. Pointing OutputFormat.FLAC's arm at pcm_s16le left both tests green. Four bytes each, in the idiom the class already uses. OutputFormat.FLAC is Container.FLAC, whose ffmpeg format is "flac", so the file opens with the native stream marker fLaC. OutputFormat.OPUS is Container.OGG -- "ogg" -- so it is an Ogg stream and opens with OggS. Verified against both muxers directly rather than assumed from the codec name; the marker belongs to the container, so it does not vary with the ffmpeg build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
d759ef32f1 |
Take the picker test off the API 37 gating leg, and give the SystemUI disable root
The gating `E2E API 37` leg failed three of the last ten status_check runs. Four runs were read logcat-first -- 34006456986, 34001744574, 34001377499 and the green 34002313300 -- and each carries exactly two `hasReadColorBufferDma` aborts before the suite (surfaceflinger, during boot and the SystemUI disable) and exactly one during it: `system_server`, thread `TaskSnapshotPer`, always inside `pickingAFileThroughTheSystemPickerFillsInTheFileCard`'s window. Nothing else in the gating set reaches the mapper. So that test kills the framework on this image whether it passes or not, and whether the leg goes red is luck: 34001377499 passed it and lost the leg anyway with `failed: 0`, 34002313300 passed it 0.6 s after the abort and went green. That is #108. The test now carries `@FailsOnEmulatorApi37` and the baseline goes 3 -> 4; the marker's own wording widens from "does not pass on this image" to "cannot be run on this image", because this carrier passes about half the time. `docs/api-37-emulator-crash.md` had counted those aborts on 2026-08-24, put them in its table, and then read the pass/fail column alone. The correction is recorded beside the original rather than replacing it. Probed on the same image and recorded there too: there is no shell knob for task snapshots -- not in `getprop`, `settings`, `device_config` or `cmd window` -- so the marker is the available answer rather than the lazy one. Two separate defects came out of the same logcats. `disable_region_sampling` has never restarted the framework on CI. `adb shell stop` and `start` are root-only and every API 37 leg has printed `Must be root` for both, so SystemUI stayed up for the whole run -- visible directly as `WindowManagerShell ... app=com.android.systemui` minutes after "final state: SystemUI disabled". Both waits also printed their own exhaustion as an elapsed time, so "system_server down after ~40 s" is what a stop that did nothing looks like. Measured on the local android-37.0 AVD, same fingerprint as CI: `adb root` makes `stop` return 0 with `pidof system_server` empty. Root is dropped again before Gradle runs, and both waits now say whether they observed anything. And when the picker test does fail, the abort is the coda rather than the cause: `InputDispatcher: No new touched window at (539.0, 525.0)` is in both reds and absent from the green, so the tap on the root is discarded, the picker is never navigated, and all four back presses land on an activity WindowManager says has not added a window yet. `forceStopThePicker` goes around input entirely so `pickTheFixture`'s whole-picker retry -- which exists for exactly this -- becomes reachable. That one is a fix on every API level, not just 37. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
39327beea7 |
Assert the join failure message against a real FFmpeg session
#217 closed by noting the join legs had not been run locally. Running them would not have answered it: nothing on either source set drove a real join *failure*, so the message unified in #203 was asserted only against values a JVM test hands to sessionOutcome directly. CI had already run those legs green on the merged commit, so the outstanding item was a gap in coverage rather than a gap in execution. The new case forces a failure with an input that does not exist -- rejected the same way by every FFmpeg build, unlike malformed media -- and asserts the message names the operation, carries the return code, and has detail after it. That last clause is the device-only half. getReturnCode, getFailStackTrace and getAllLogsAsString are native reads; if the log tail came back empty on a device the user would see "Joining failed (1): " with nothing after the colon, and every JVM test would still pass. It deliberately does not pin which detail source wins. On an ordinary non-zero return code FFmpegKit reports no fail stack trace, so stack-trace-first and log-tail-first produce identical text and no assertion here could tell them apart. SessionOutcomeTest pins that, where both sources can be non-blank at once. Claiming it here would be a KDoc asserting more than the test checks. Measured on the local API 33 emulator: 61/61 green with the test, and with both detail sources nulled in ConcatEngine it fails on the intended assertion -- "the message stopped at the return code and told the user nothing, was: 'Joining failed (1): '" -- so the prefix and return code survive the mutation and only the device-only claim goes red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
32ab54da3c |
Wait for the rotation to rebuild the Activity, not for the composition to idle
thePickedInputSurvivesARealRotation synchronised a rotation with waitForIdle(), which waits for the compose hierarchy to settle. Right after a rotation the window manager has accepted but not yet delivered as a configuration change, the old Activity's composition is already idle -- so it returns, composeRule.activity still resolves to the old instance, and the guard reads an unchanged identity hash. Nothing waited for MainActivity to be rebuilt. That is the clean AssertionError on #217's API 33 gating leg, run 33698846104: it failed the SECOND guard, so the first had passed and the display really had rotated. The wedges this ticket opened with are the same race taken the other way -- land while the composition is being torn down and waitForIdle has nothing coherent to settle on. awaitRecreation() waits on a counter fed by the runner's lifecycle monitor, bounded at 15 s. Deliberately not polling composeRule.activity: that resolves through scenario.onActivity, which blocks on the main thread, so polling it across a recreation is a plausible reading of the very wedge being fixed. Both guards stay. The identity-hash one is now a backstop rather than the primary detector -- a configChanges attribute trips the barrier's timeout first, with a message saying what was waited for. Measured on the local API 33 emulator. With the fix, 60/60 green. With the watcher mutated so the counter never increments, the test fails in 15 s naming itself and its condition, and the run still reports received 60/60 completed cleanly -- where the same missing recreation used to cost the leg 20 minutes and name nothing. The pre-fix flake does not reproduce on this host, so that is a demonstration of the timeout path, not a before/after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
2e0c6737d6 | Merge remote-tracking branch 'origin/main' into merge-113-tmp | ||
|
JMR-devandClaude Opus 5
|
238142d9cc |
Guard the whole Media3 export instead of only its two ends
transcode() posts its work to a HandlerThread, and everything on that thread has no caller to throw back to: an escaping exception reaches the thread's uncaught handler and takes the process down, while the continuation is never resumed. Both halves of that are bad, and the second is arguably worse — a worker left suspended forever holds a foreground service. The guarding was two narrow runCatching blocks, one around buildTransformer and one around transformer.start, with the two Media3 builders sitting unguarded between them. That gap was not theoretical. EditedMediaItem.Builder rejects a composition with both tracks removed, which is exactly what a plan of (Drop, Drop) asks for, and it does so with a plain IllegalStateException from the constructor. Validation now refuses the spec that produces such a plan, so neither the picker nor ConversionWorker will start one. Routing is a separate question and still answers Media3 for it — a dropped track makes nothing un-hardware-able — so a request that skips validation still arrives here: a job queued before the settings changed, or one made through ConversionWorker.request directly. CopyPlanner's own KDoc already names that path as the reason it re-checks what validation has checked; this is the same belt for the same braces. One guard around the whole body costs nothing on success and turns any such refusal into a failed job with a reason attached. The export body moves into startExport, whose contract is the thing that makes one guard enough: returning normally means the export is running and the listener owns the continuation, throwing means it never started and the caller does. Cancellation is still registered before start. Covered twice on purpose. Robolectric runs the real HandlerThread and the real Media3 builders, so the JVM test exercises the whole sequence and can be run anywhere; the instrumented one repeats it against the real framework. Neither asserts only that the failure is an IllegalStateException, because withTimeout raises TimeoutCancellationException and java.util.concurrent.CancellationException extends IllegalStateException — so that assertion alone calls an unresumed continuation a pass. Both were written that way first, and reverting the guard is what exposed it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
3e9528454c |
Announce a baseline it cannot read, rather than falling quiet
"A comparison was asked for" and "a number was found to compare against" were one variable, and collapsing them put the report one refactor away from being the thing #83 filed. The sed that reads FAILS_ON_EMULATOR_API37_BASELINE is anchored at the line start, so indenting the const into an object -- or renaming it, or moving it -- empties it, and the old code then skipped the whole comparison while the table kept printing exactly as before. Silent, and indistinguishable from a run that matched. Now an unreadable baseline is itself a deviation, with the notice naming the const so the fix is obvious. Verified against the real captured log of run 32865281555 three ways: baseline file absent, const indented into an object, and the committed file unchanged -- the first two announce, the third stays silent. |
||
|
JMR-dev
|
0702916229 |
Say what the advisory API 37 job actually found, so a new failure is not invisible
That job is continue-on-error and red on every PR by design, which CLAUDE.md states plainly -- and that instruction is exactly why nobody reads it. Nothing in a red X separates "the known three" from "the known three plus yours". A bare failure count would not have fixed it, and this is measured rather than assumed. The run is usually truncated: seven of eight advisory runs read on 2026-08-25 ended in `Test run failed to complete. Expected 3 tests, received 2.` with INSTRUMENTATION_ABORTED, and one did not. A count taken from a truncated run misleads in both directions -- a fourth marked test can still yield the same number if the abort lands earlier, and the known set getting worse can lower it. The test XML does not rescue it either, which was the thing worth checking before building on it: it IS written for an aborted run, and it reports a tidy tests="3" failures="3" for a run the runner had just described as truncated. So the XML is the authority on how many results landed, the runner's own output is the only authority on whether the run finished, and the report reads both and says which number came from where. The baseline is one number beside the marker, because the marker means "cannot pass on this image": the count is both how many tests the advisory leg runs and how many should fail. A smaller failure count is the interesting direction -- it means one now passes, which is the documented trigger for deleting the annotation. Nothing about the job's status changes. It stays continue-on-error, stays red, stays out of the required contexts; a deviation is a ::notice::, never an ::error::. The report is a separate script so it can be run against a real log saved from a real CI run, which is how the comparison was shown to fire. The gating legs get the shape without the comparison: they run the whole suite, so comparing there would announce a deviation five times a run -- but a truncated run reporting fewer results than it ran is what #108 looks like, and "completed cleanly" is the field that would show it. Closes #83 |
||
|
JMR-dev
|
d37c391c60 |
Stop telling people to stage the benchmark the one way it cannot be staged
RealMediaBenchmark's class KDoc said:
Populate with:
adb push <file>.mp4 /sdcard/Android/data/org.libremediaconverter/files/
Twelve lines below, the `samples` property KDoc -- on `get() = context.filesDir` -- says:
Internal storage, not the external files dir. Files placed in the external dir by
`adb push` or `adb shell cp` stay owned by the shell user, and the app then gets
EACCES trying to read them -- which presents as an unparseable input rather than a
permission problem.
Different directories, and the second exists specifically to explain why the first fails.
Anyone following the class KDoc stages files the benchmark cannot read, gets a skip, and
reads the skip as "not staged yet" -- the failure mode the property KDoc warns about, walked
into by the instruction in the same file.
The fix is not a corrected command. Restating the mechanism in a second place is what let
these drift, and a replacement command I have not executed would be the same defect with a
fresher date. The class KDoc now names [samples] as the single place that answers it.
Two things added that are checkable rather than remembered: the exact filenames the tests
look for, via [H264_SAMPLE] and [AV1_SAMPLE] -- the old text said `<file>.mp4`, so even the
right directory left you guessing -- and a note that the two skips every green E2E leg
reports are these.
Not claimed: that the benchmark misbehaves on CI. An earlier version of the ticket said so;
it was wrong, and measuring settled it -- both tests report SKIPPED on the gating legs, the
guards work, and "harmless in CI" is accurate. The failure that prompted the look is
Media3EngineTest, tracked as #102.
Closes #101.
|
||
|
JMR-devandClaude Opus 5
|
25f162923c |
Close the ANR dialog that was hiding every window from UiAutomator
SafPickerRoundTripTest began failing on gating legs at API 33, 34, 35 and 37 ninety minutes after it landed, on diffs that cannot cause it -- two KDoc comments, a MIME lookup table, a README paragraph. Every failure named the fixture root, so #93 was filed as a root-discovery race. It was not one, and finding out what it was took making the test say something else first. DocumentsUI was fine throughout: its own `ProvidersAccess: Matched roots` names the fixture authority five times inside the sixty seconds the test spent failing. What failed was reading any window at all -- 1095 `Retrieving node with selector` against 1095 `Node not found` on that leg, against 7 and 2 on the green one. So this now asks whether the app's OWN window is readable before it opens a picker, and prints the accessibility window list when it is not. That list named the culprit on the next occurrence: What it could see: com.android.systemui[type=3], android[type=3] No TYPE_APPLICATION window at all, on a device that had just logged `Displayed org.libremediaconverter/.MainActivity`. `android[type=3]` is system_server, and the same logcat says what it was holding, minutes before this class ran: ANR in com.google.android.apps.nexuslauncher Reason: Input dispatching timed out (Application does not have a focused window) Window{4ed8414 u0 Application Not Responding: com.google.android.apps.nexuslauncher} The launcher ANRs on a loaded runner emulator and the dialog it leaves behind never goes away. It is opaque and fullscreen, so AccessibilityWindowManager drops every application window beneath it -- which is how the app can be Displayed and unreadable at once, the contradiction that made this look like a SAF bug for six PRs. Present on both legs examined, API 33 and 34, at the failure timestamp. So the dialog is dismissed, by resource id rather than by localised button text, `aerr_wait` first so the app under it is left alone. Waking the device and rebuilding the UiAutomation connection are kept behind it and are recorded as measured non-causes rather than as fixes. A second PickActivity is not a remedy for this either, and that was measured: the failing leg opened one for the second test, in the same DocumentsUI process, and read as little from it. The whole pick is still retried, but for a smaller and separate claim -- a picker whose lists were built before their data arrived, which #80's node-level re-find cannot reach because it re-acquires a handle inside the one picker. One API 37 run failed a step deeper, on the file rather than the root. That shape has not been reproduced or diagnosed; the reopen covers it because a fresh pick re-walks from Recent, and the KDoc says that rather than claiming more. Two things the retry must not become. It must not tolerate an absent root, or #64's MIME mutation goes vacuous -- so a missing node is reported rather than retried away, and the mutation was re-run: both tests still fail, still with "the system picker never showed BySelector [TEXT='\QLMC R38 fixtures\E']", in 126 s and 127 s against the 1200 s wrapper timeout. And it must not decide the picker has closed by asking the same accessibility window list that is broken -- so the back presses are counted against Activity.hasWindowFocus, which comes from the framework. Each new path was forced on and measured rather than trusted: the injected-failure run showed the reopen recovering, with four OPEN_DOCUMENT starts for two tests; the rebuild was forced unconditionally and the suite stayed green, ruling out a connection that comes back without FLAG_RETRIEVE_INTERACTIVE_WINDOWS; the dialog dismissal was forced with no dialog present, ruling out a blind click breaking a healthy run. Dismissing a real ANR dialog has not been observed, because the fault has never reproduced locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
3925f1aa9f |
Re-find the picker node when it goes stale, and re-measure API 37
CI found a flake this workstation could not, and fixing it overturned half of what
the previous commit recorded about API 37.
THE FLAKE. UiObject2 caches the AccessibilityNodeInfo it was found with, and
DocumentsUI is still settling when a node first appears -- its list rebinds, the
roots strip lays out, a window animates. If the node is replaced in that gap,
click() throws against the handle rather than missing the target:
androidx.test.uiautomator.StaleObjectException
at androidx.test.uiautomator.UiObject2.getAccessibilityNodeInfo(UiObject2.java:1042)
at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526)
at SafPickerRoundTripTest.pickTheFixture(SafPickerRoundTripTest.kt:223)
It is not intermittent on a COLD emulator -- CI hit it on API 33, 34 and 35, every
one of them, on the first run. It never appeared here because the local emulator had
been warm for an hour. tapPickerNode now re-finds the node and taps again, three
attempts. That retries acquiring a handle to a node that has to be there anyway:
every attempt still goes through awaitPickerNode, which fails outright if it is
absent, so the MIME mutation's bite is untouched. Verified with `pm clear
com.google.android.documentsui` between runs, five for five green on API 34.
AND THE CORRECTION IT FORCED. The previous commit marked the whole class
@FailsOnEmulatorApi37 on the strength of two measured failures. One of them was
this bug. Re-measured with the fix, one method per fresh android-37.0 emulator:
thePickedInputSurvivesARealRotation INSTRUMENTATION_ABORTED:
System has crashed.
pickingAFileThroughTheSystemPickerFillsInTheFileCard PASSED
So a rotation, which rebuilds every surface at once, is what the gralloc mapper does
not survive; starting another app's activity is not. The marker moves to the one
method that earned it, and the picker test runs on the gating API 37 leg like
anything else. The workflow comment, run-e2e.sh and the doc all say that now.
The lesson is worth more than the measurement, and the doc keeps it: an annotation
is a claim about an IMAGE, and a broken test makes every image look broken. Both a
framework abort and a stale node read as "the run fell over". Re-measure after
fixing a test before deciding what the platform did.
Also measured rather than assumed, since it is what keeps the gating leg green: the
runner's annotation filter honours a class-level marker, expanding it to every
method. On API 34, `annotation=` selected exactly 4 tests (2 Media3EngineTest + 2
here) and `notAnnotation=` selected 55 with neither of these in it. CI's own gating
API 37 leg then reported 55 / 0 on the previous push. That is why moving the marker
to a single method is a narrowing rather than a repair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
a3c835b7c9 |
Keep the picker test off the API 37 gating leg, having measured why
The API 37 emulator images abort surfaceflinger inside the guest's Gralloc5 mapper,
init SIGKILLs zygote with it, and the framework restarts under the run. run-e2e.sh
and the CI leg disable SystemUI to remove the trigger -- but that removes the IDLE
one, RegionSamplingThread's nav-bar luma sampling. Driving DocumentsUI and rotating
the display are not idle. They are the first things in this suite that generate
surface traffic of their own.
Both tests were measured on android-37.0 under swangle_indirect with SystemUI
disabled and verified quiet, and measured SEPARATELY -- inferring the second from
the first is the mistake docs/api-37-emulator-crash.md opens by correcting. They
fail in the two shapes a framework restart produces:
thePickedInputSurvivesARealRotation
INSTRUMENTATION_ABORTED: System has crashed.
Expected 59 tests, received 50
(5 hasReadColorBufferDma aborts; the framework dies DURING the test, so six
later tests never run and the XML carries a failure with no text at all)
pickingAFileThroughTheSystemPickerFillsInTheFileCard
androidx.test.uiautomator.StaleObjectException
at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526)
(3 aborts; the picker's root node was rebuilt between finding it and tapping it)
Both pass on API 33 and API 36 locally -- whole suite, 59/0/0/2 on each -- which is
the same evidence pattern that made the Media3EngineTest pair the image rather than
the app.
So the class carries @FailsOnEmulatorApi37 and runs on the advisory leg.
THREE PLACES SAID "nothing in this suite touches system UI", and that is what makes
the SystemUI-disable deviation defensible. It is no longer true of the suite, and all
three are corrected rather than left to rot -- the workflow comment, run-e2e.sh's
header, and the doc. The rule they state is being APPLIED, not broken: the thing that
depends on system UI is excluded from the leg that cannot be trusted for it.
Two consequences stated rather than left to be discovered:
- run-e2e.sh applies no annotation filter, unlike CI, so a local `run-e2e.sh 37`
reports these two on top of the Media3 pair AND DOES NOT FINISH. Its totals come
back short and which later tests ran is arbitrary. The summary row now says so;
it previously promised "exactly two failures", which would have read as a
regression in someone else's diff.
- The advisory job is still named "E2E API 37 Media3 hardware transcode", and half
of what it now runs is neither. Renaming a check touches branch protection, so it
is deliberately not done here; the doc records the staleness and the revisit
trigger now says the marker covers two unrelated bugs that can go green apart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
650ca8fca3 |
Pick a file the way a user does, then rotate the phone
Two things nothing in this repo asserted, and they are one test class because
separately the second one asserts nothing new.
THE PICKER. ConverterScreen opens SAF with a MIME filter, and a filter is a thing
that can hide the user's file. Narrow it and the app still builds, still renders,
and still passes every JVM test -- the user taps "Choose file" and gets an empty
picker. The round trip now runs for real: DocumentsUI is driven with UiAutomator to
a fixture root, and the app is asserted to come back with the file.
The file card's name is not the only assertion, because a name proves less than it
looks: it comes from a metadata query, which a URI with no read grant answers just
as well. The "Container: MP4" detail row only appears once something has opened the
file and read its header, so it is what says the picker handed back a URI the app
can USE.
THE ROTATION. MainActivity declares no configChanges, and ConversionViewModel holds
the picked file in a plain MutableStateFlow with NO SavedStateHandle behind it.
Nothing persists it. The only thing that carries it across a rotation is the
ViewModelStore the Activity retains -- which no test anywhere asserted.
Two guards run before that assertion, because both ways it could pass while proving
nothing are silent: the display rotation really changed, and MainActivity really was
a different instance afterwards. Without the second one this is a recomposition test
wearing a rotation's name.
MUTATIONS, RUN RATHER THAN ASSERTED, on a local API 34 emulator.
Narrowing the filter to arrayOf("application/x-lmc-no-such-type") takes the fixture
root out of the picker entirely -- DocumentsUI matches the request against
Root.COLUMN_MIME_TYPES and drops roots that cannot answer -- and both tests fail:
java.lang.IllegalArgumentException: the system picker never showed
BySelector [TEXT='\QLMC R38 fixtures\E']
Making the ViewModel composition-scoped fails ONLY the rotation test:
androidx.compose.ui.test.ComposeTimeoutException: Condition (a node tagged
converter.fileCard.name exists) still not satisfied after 30000 ms
and :app:testDebugUnitTest stays BUILD SUCCESSFUL under it. That divergence is what
#64 exists to establish and what its own comment doubted; the PR body has the
verdict and why the doubt was reasonable.
THE PROVIDER HAD TO BE JAVA. It is the only Java file in the module. A
manifest-declared provider is a component of the instrumentation PACKAGE, so the
system starts a plain org.libremediaconverter.test process for it with only the test
APK on its dex path -- and the test APK is built without the Kotlin stdlib, because
the app APK has it and duplicating it is what checkDebugAndroidTestDuplicateClasses
prevents. The Kotlin draft died on its first query:
java.lang.NoClassDefFoundError: Failed resolution of: Lkotlin/jvm/internal/Intrinsics;
at org.libremediaconverter.saf.FixtureDocumentsProvider.queryDocument
The compiler emits that reference for the null checks on nearly every function, so
no Kotlin dialect avoids it. Same reason nothing in that file imports androidx.
No new test tags: CHOOSE_FILE, FILE_CARD_NAME and detailRow already named both ends.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
b18f45def7 |
Give the system file picker something to pick
Nothing in either source set drives SAF as a picker. The only SAF coverage is the publish side, in OutputPublisherPublishTest, against hand-written ContentProvider fakes -- so the launcher wiring in ConverterScreen, the MIME filter it passes, and the grant that comes back have never been executed by a test. Driving the real picker needs three things this repo did not have. UiAutomator, because DocumentsUI is another process. Compose's matchers stop at this process's composition and Espresso's stop at its view hierarchy; neither can see or tap a window belonging to another package. It FLOATS, at "2.+", which is the same argument the catalog already makes for work and lifecycle rather than a new one: androidx.test.uiautomator is inside floatedGroupPrefixes, so the componentSelection guard makes "+" mean "newest RELEASED", and that is load-bearing here -- this library publishes 2.4.0-alphas above its stable, so without the guard the float would be a pin to a prerelease. Resolved to 2.4.0 (released) on debugAndroidTestRuntimeClasspath, checked rather than assumed. It is deliberately NOT pinned alongside ktlint/detekt/JaCoCo/Robolectric: those are pinned because a new rule or a new runtime changes the verdict on files nobody touched. UiAutomator has no verdict -- it taps what a selector names, and a selector that stops matching is this repo's test to fix, in a diff that explains itself. The "2." rather than a bare "+" is the one thing held back: a major is where the selector API would be free to change under exactly that assumption. A DocumentsProvider, because DocumentsUI does not browse a filesystem -- it lists what providers offer it. Writing a file into Downloads would have worked and tested less: the fixture root declares Root.COLUMN_MIME_TYPES, and DocumentsUI filters the drawer by it, which is what gives the MIME filter a mutation with a shape rather than "one file among the hundreds in Downloads was not listed". Its contents are also exactly one file, where a shared directory accumulates whatever earlier runs left behind. And the first AndroidManifest.xml this source set has ever had, to declare it -- a ContentProvider is instantiated by the system and cannot be registered from test code. In androidTest rather than src/debug so it is installed by the instrumentation APK only, and never appears in a developer's own file picker. Two things worth knowing before editing either file. XML comments cannot contain "--", which the manifest's first draft failed the build on; and "*/" inside a KDoc closes the comment, which the provider's did. Both are silent in review and loud in the build. No test yet, and no new test tag: TestTags.Converter.CHOOSE_FILE and FILE_CARD_NAME already name both ends of the round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
225ecdd7e6 |
Split the API 37 leg so the part that works can gate
CI has never run the API level this app targets. The reason it did not was never "API 37 is untestable" -- it was that two tests fail on the emulator image, so one row would be permanently red or permanently allow-listed. This splits that row instead of choosing between those two. E2E API 37 gates. It runs 55 of the suite's 57 instrumented tests and must be green. E2E API 37 Media3 hardware transcode runs the other two, reports, and never blocks (continue-on-error). Both are driven off ONE marker, @FailsOnEmulatorApi37: the gating job passes notAnnotation, the advisory job passes annotation. Two lists would drift, and drift is silent in both directions -- a test that ends up in neither job reads as green. Excluding by class was not an option either: Media3EngineTest has four tests and two of them pass here, so notClass would have thrown away real coverage. The advisory job is named for what it runs, not for what we think is wrong. Both its tests drive a full H.264 -> H.265 hardware transcode, which is what distinguishes them from the two Media3EngineTest cases that pass -- those never decode video. The goldfish-decoder theory sits in a comment inside the job, where it can be corrected without renaming a check people have learned to look for; docs/api-37-emulator-crash.md keeps measurement and inference apart. The SystemUI disable moves into .github/scripts/e2e-run.sh behind E2E_DISABLE_SYSTEM_UI, unset everywhere but the two API 37 jobs, so the other four legs run byte-identical commands -- the same shape as E2E_EXTRA_GRADLE_ARGS. It runs BEFORE the streamed logcat starts, deliberately: `adb shell stop` would end that logcat and nothing restarts it, so a disable placed after it would cost the leg its diagnostics for the part of the run that matters. The body is probe v2 from api37-debug.yml -- the version measured 4/4 -- not the older one-round form: three rounds, waits for system_server to actually be gone, verifies against `pm list packages -d`, and requires a 45 s window with zero new aborts. The weaker probe reported success on a run that then started SystemUI eight more times. The caveat is written next to the row rather than left implicit: this leg runs with SystemUI disabled and the framework restarted under it, a device configuration no other leg and no Pixel run uses. Anything that touches system UI must not trust it, and the Pixel check before each release is still the only API 37 run with SystemUI intact. docs/api-37-emulator-crash.md's "So should CI take API 37?" said no on three reasons. Two were claims about CI that had never been measured; the section now carries the eight runs that measured them, and the third reason is what the split answers. docs/local-emulator.md and api37-debug.yml's header carried the same "the matrix stops at 36" claim and are corrected with it. CLAUDE.md is left alone deliberately -- its "CI's matrix therefore stops at API 36" clause is now false, and that correction is parked in the doc's existing "Correction owed to CLAUDE.md" section, where two others are already waiting. Making E2E API 37 an actually-required check is a repository-settings change and must come after this is on main: adding a required context that does not exist on the default branch blocks every PR. 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
|
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
|
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
|
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
|
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> |
||
|
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> |
||
|
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
|
a172cb137f |
Pin the fallback test's device profile so it is not machine-dependent
Running the suite across API 33, 34, 35 and 36 emulators failed the same test on all four, while it passed on a Pixel 10 Pro XL. The assertion was "the hardware path should have been attempted", and the routing was correct in both cases: those emulators expose no hardware H.264 encoder, so the router sent the job straight to FFmpeg and Media3 was never called. That is the same mistake made earlier with routesAFastMp4JobToMedia3 -- baking an assumption about the host's encoders into a test. A result that flips with the machine says nothing about the code. This test is about the fallback mechanism rather than about routing, so it now pins DeviceCodecs.PERMISSIVE through the existing seam and the router's choice becomes deterministic. Routing itself is covered separately, by tests that derive their expectation from the device. Verified on emulators for API 33, 34, 35 and 36: 40 instrumented tests each, 0 failures, 2 skipped, with the only skips being the opt-in benchmark. That also exercises all three foreground-service regimes for the first time -- no type below 34, dataSync at 34, mediaProcessing from 35 -- which had previously only ever run at API 37. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
99bd961022 |
Force the last five failure branches
The previous commit called five branches unforceable. That was wrong on all five, and the reasoning behind it was lazy rather than investigated. Three were described as needing a SAF grant revoked mid-job. They do not: a content URI whose authority does not exist reaches exactly the same null branch as one whose permission was withdrawn, and constructing one is a single line. That now covers the conversion input, the join inputs, and the publish destination. One was described as unreachable through ConcatWorker.request(). True, but irrelevant -- a worker is just input Data, and WorkManager will run one built without the input array. That is also what a version-skewed queue entry would look like after an app update, so it is worth covering rather than dismissing. One was described as Media3-internal. An output path whose parent directory does not exist makes Transformer.start() fail synchronously, and the engine has to surface that as a rejected suspension. The test asserts specifically that it is not a timeout: hanging would be far worse than throwing, because the worker would sit holding a foreground service indefinitely. The assertions check outcomes rather than exception types. Whether a resolver returns null or throws is a provider implementation detail; what matters is that the job fails cleanly and carries a message, instead of taking down the worker. Every failure branch in the conversion and join paths is now forced. 40 instrumented tests on a Pixel 10 Pro XL and 66 unit tests, 0 failures, with the only skips being the opt-in benchmark. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
83fb55515b |
Add a seam for forcing failure paths, and force thirteen of them
Error handling was the least-tested code in the app. It only runs when something goes wrong, which is exactly what a healthy test run avoids, so the branches a user meets on a bad day were the ones that had never executed. An audit put it at 4 of 18 fallback branches actually forced by a test. There was also no mechanism to do better: ConversionWorker constructed Media3Engine and FFmpegEngine directly, so no test could make either of them fail. Media3Engine and FFmpegEngine now implement HardwareTranscoder and SoftwareTranscoder, and ConversionDependencies holds the factories. Workers are built by WorkManager and the app deliberately carries no DI framework, so a small settable holder is the least machinery that does the job. The foreground-service timeout needed different treatment. Its trigger is the six-hour-per-day budget expiring, which no test can reach, so the decision moved out of the worker into FailureOutcome. The rule is now verified on the JVM across every WorkInfo stop reason, including that only the timeout earns a retry -- retrying a user cancellation would ignore the user, and retrying a constraint failure would spin. Thirteen branches are now forced, including both free-space prechecks, both engines failing, a missing input, and the router bypassing hardware entirely for MP3. The dynamic fallback is covered twice over: once by injection, and once by HardwareFallbackTest driving it with genuinely undecodable 4:4:4 footage. Injection alone would prove the plumbing without proving the condition ever arises in reality. Five remain unforced and are listed in ConversionDependencies' documentation. They need a SAF grant to be revoked mid-job, which is not something a test can arrange. Media3EngineTest now asserts against the muxed file rather than the engine's own ExportResult, which is a stronger check anyway: a result object can report success for a file that will not play. 35 instrumented tests on a Pixel 10 Pro XL and 66 unit tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
356c04d137 |
Make the 4:4:4 fallback regression run without manual setup
The test covering the runtime fallback read its input from the app's internal storage, which only ever contained a file because it had been piped in by hand with run-as. On a fresh checkout it hit assumeTrue and skipped -- silently, while still counting toward the suite total. A regression test that skips is worse than no test, because the number reads as coverage. It now ships its own fixture: three seconds of H.264 High 4:4:4 Predictive, 76 KB. Producing it needed x264, which the host toolchain cannot supply -- Fedora's ffmpeg carries openh264, which is Constrained Baseline only and cannot even decode 4:4:4 -- so it was generated with ffmpeg-full inside the existing FFmpeg build container. The command is recorded in the test's own documentation so the fixture can be regenerated rather than trusted blindly. Verified by deleting the hand-staged files first and running the suite clean: the fallback test executes, Media3 fails to decode as expected, and the worker completes the conversion through FFmpeg. It is no longer among the skips. The benchmark stays opt-in and is now documented as such. It needs real long-form media that does not belong in the repository, and its numbers should not be mistaken for something the suite verifies. 29 instrumented tests on a Pixel 10 Pro XL: 0 failures, 2 skipped, and both skips are the benchmark by design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
ec6e37ad08 |
Stop selecting FFmpeg's MediaCodec encoders, and pin the pixel format
Testing on a Pixel 10 Pro XL with real footage found two defects that neither the emulator nor any unit test could surface. The sample is H.264 High 4:4:4 Predictive (avc1.F4001F). Media3 cannot decode it on any device: hardware AVC decoders implement High 4:2:0, and Android's software c2.google.avc.decoder does not cover 4:4:4 either, so the export dies with a codec exception. The static routing rules cannot predict this -- the container is MP4, the codec is "h264", and the device reports AVC decode and encode -- so it is exactly the case the runtime fallback exists for. Until a file like this was tried on hardware, that fallback had never actually fired. Following it through exposed the two bugs behind it. First, only one video encode path named a pixel format. FFmpeg decodes 4:4:4 to yuv444p and hands those frames to encoders that cannot accept them; naming yuv420p makes it insert the conversion instead. Every path now does. Second, and not fixed by that: hevc_mediacodec still failed with "Error submitting video frame to the encoder". That is the second distinct failure from FFmpeg's MediaCodec wrappers in one session -- the first being that on a device with no hardware encoder they silently bind to the platform software codec and crawl while presenting as the fast path. They are undocumented, per-device flaky, and duplicate badly something Media3 already does properly. A job only reaches FFmpeg because Media3 could not handle it, which is itself evidence hardware encode is unlikely to work for that input. So FFmpeg now always encodes in software, and Fast means a fast preset rather than a different encoder: libx264/libx265 at veryfast against medium, both with CRF. With that, the fallback completes and the file converts. Measured on the Pixel: AV1 1080p through the hardware path runs at 7.9x realtime, confirming the figure the two-engine design was based on; software x264 CRF at 720p runs at 4.4x. Hardware encoders present are av01, avc and hevc -- AV1 encode included, which is still rare. 29 instrumented tests pass on device, 63 unit tests on the JVM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
64e146ce5d |
Cover the join path with device tests
The join flow was implemented but had never run end to end: unit tests covered the planner and the argument shapes, neither of which can tell you what FFmpeg actually does with real files. Three fixtures make the strategy decision testable. clip_a and clip_b match in codec, resolution and frame rate; clip_c deliberately differs in both resolution and frame rate. Without a genuinely mismatched input there is no way to prove the re-encode branch is ever taken. The tests assert which strategy ran, not merely that output appeared. That distinction is the whole point here: the concat demuxer does not reliably reject mismatched inputs, so a naive implementation produces a file whose later segments are garbled while still exiting successfully. Each test also checks the output is long enough to contain both inputs, since a truncated join is exactly what a wrong stream copy looks like. Also covered: the list file is cleaned up, fewer than two inputs is refused, the probe distinguishes the clips the planner depends on, and the chosen strategy reaches the UI through WorkManager -- it is what tells the user whether their files were copied losslessly or re-encoded. Measured on an API 37 emulator, the two paths differ by roughly thirty times on the same pair of clips: 0.026s to stream copy against 0.829s to re-encode. That gap is itself evidence the planner is not quietly re-encoding everything. 25 instrumented tests now pass, up from 17. 65 unit tests unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2fcacd199c |
Use a real software encoder when the device has none in hardware
Running the device suite for the first time surfaced a defect the unit tests could not: FFmpeg's *_mediacodec wrappers do not fail on a device without a hardware encoder. They quietly bind to the platform's software codec (c2.android.hevc.encoder on the test emulator) and encode far slower than libx264 or libx265 would, while still presenting as the fast path. A three second 320x240 clip did not finish inside a three minute timeout. The Fast tier now checks whether a hardware encoder actually exists for the target codec, and falls back to libx264/libx265 on -preset veryfast when one does not. That is both quicker and honest about what it is doing, and the distinction between Fast and Best survives the fallback: veryfast against medium, rather than both collapsing to the same slow path. The routing itself was already correct. The capability probe rightly rejects c2.android.* as software-only, so the job was correctly sent to FFmpeg -- the test asserting Media3 was encoding an assumption about the machine it ran on. It now derives its expectation from the device, so it means the same thing on hardware with a real encoder and on an emulator without one. Verified on an API 37 emulator: 17 of 17 instrumented tests pass in 4.2s, against six minutes of timeouts before. 65 unit tests still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
438f21783f |
Make the foreground-service test read the running API level
The three foreground-service-type regimes (none at 33, dataSync at 34, mediaProcessing at 35+) are the reason ConversionForegroundType exists, so the test derives its expectation from Build.VERSION rather than pinning one value. The same test then means something on any device in the supported range instead of only on the one it was written against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
0c352cdcfb |
Route conversions between hardware and software engines
Adds the second engine and the rules that choose between them, turning a
video transcoder into a converter.
The router encodes capability boundaries, not preferences. Media3 handles
what it genuinely can and FFmpeg takes the rest:
MKV, and any container Media3 cannot mux
MP3, which Android cannot encode at any API level -- a platform gap
rather than a Media3 limitation
GIF and PNG frame sequences, which have no Media3 muxer
VP9 and AV1 targets: Transformer.setVideoMimeType accepts only
H.263/H.264/H.265/MP4V, so the WebM muxer has no encoder behind it
inputs with no platform decoder, since Transformer ignores ExoPlayer's
bundled software decoders and the dav1d extension does not rescue it
the Best quality tier, because CRF and two-pass come from x264/x265 and
no Android hardware encoder exposes either
Rule order matters and is deliberate: specific reasons are checked before
general ones because the reason is shown to the user. "Android has no
encoder for this format" is actionable for MP3; "this container needs
FFmpeg" is not. A test caught the original ordering getting this backwards.
Hardware support is vendor-declared and, per the platform's own docs,
"cannot be tested for correctness", so the static rules are backed by a
dynamic fallback: a Media3 export that fails is retried on FFmpeg rather
than surfaced as a failed conversion.
Joining files chooses between a stream copy and a re-encode by inspecting
the inputs. The concat demuxer requires matching codec, resolution and
timebase, and does not reliably fail when they differ -- it can emit a file
whose later segments are garbled. Unknown properties count as a mismatch,
because two nulls are not evidence of agreement.
The UI surfaces the routing decision rather than hiding it, so a slow job
explains itself, and offers a per-job engine override.
Navigation is adaptive: a bottom bar on phones, a side rail on wider
screens. Not cosmetic -- from targetSdk 37 Android ignores screenOrientation
and resizableActivity on displays at least 600dp wide, with no opt-out, so
the app is resized whether or not it is ready.
62 unit tests cover the routing matrix, the FFmpeg argument builder and the
concat planner on the JVM, against fabricated device profiles so branches
like "this device cannot encode HEVC" are reachable without that hardware.
Device-level tests for the FFmpeg formats are written but not yet run; the
emulator in this environment will not stay up.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
e84ab6a89d |
Run conversions as durable foreground work
Conversions now go through WorkManager instead of a viewModelScope coroutine,
so a job outlives the ViewModel, survives process death, and keeps running
when the user leaves the app.
The foreground service type needs three branches across the supported range,
which is why ConversionForegroundType exists rather than a constant:
API 33 no type is required at all
API 34 a type is mandatory, but mediaProcessing does not exist yet, so
dataSync is the only sensible fit
API 35+ mediaProcessing, whose own documentation describes it as
"converting media to different formats"
The manifest declares both types on WorkManager's SystemForegroundService
via tools:node="merge" -- setForeground runs *that* service, not one of
ours, so declaring the type on an app-owned service would have no effect.
The manifest cannot branch on API level, so the runtime picks which type is
actually passed.
Both types share a budget of six hours per twenty-four across the whole app.
When it runs out WorkManager reports STOP_REASON_FOREGROUND_SERVICE_TIMEOUT,
which the worker translates into Result.retry() rather than a failure: the
work is still valid, there is simply no budget right now. The UI surfaces
that as a distinct Waiting state that explains the pause instead of showing
an error.
Expedited work is deliberately not used. It maps to JobScheduler expedited
jobs with a short quota, which is the wrong shape for a multi-minute
transcode.
POST_NOTIFICATIONS is requested when the user taps Convert, not on first
launch, so the ask arrives with visible justification. The conversion starts
either way -- without the permission the foreground service still runs, but
its progress notification is confined to the Task Manager rather than the
shade. Notification updates are throttled to roughly one per second because
progress updates arrive far faster than the system UI can absorb.
Tests run against the real WorkManager rather than a test double,
specifically so setForeground and the declared service type are exercised on
a device that enforces them. Verified on an API 37 emulator: the service
starts, and logcat shows no type or permission exceptions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|