diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index fbc65f3..1b685f8 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -81,7 +81,11 @@ jobs: - name: Verify the released artifacts run: | - APK=$(ls app/build/outputs/apk/release/*.apk | head -1) + # A glob, not `ls | head`: the glob is already here, and parsing ls is what + # SC2012 is about. Gradle's names have no spaces today, which is exactly the + # kind of assumption that holds until it does not. + apks=(app/build/outputs/apk/release/*.apk) + APK="${apks[0]}" # A release that shipped one ABI, or lost 16 KB alignment, would install # fine on a test device and fail for users or at Play submission. Both are # cheap to check and expensive to discover later. diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index b6d540a..f3b3355 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -182,6 +182,23 @@ jobs: docker run --rm "$SHELLCHECK" --version git ls-files -z '*.sh' | xargs -0 -r docker run --rm -v "$PWD:/mnt" "$SHELLCHECK" + # actionlint closes the half shellcheck cannot see. The step above reads .sh files; + # a good deal of this repo's bash lives in inline `run:` blocks instead -- the release + # verification here, the emulator setup and teardown in this file and in + # api37-debug.yml. actionlint parses each workflow and runs shellcheck over every + # `run:`, on top of its own checks for expression syntax, `needs:` references, matrix + # keys and action input names. + # + # Pinned by digest for the same reason shellcheck is, and with a second reason of its + # own: actionlint's documented install is `bash <(curl -s .../download-actionlint.bash)` + # off a moving branch, which would sit badly in a repo that pins every action by SHA. + - name: actionlint + env: + ACTIONLINT: rhysd/actionlint@sha256:9d36088643581e728c969f35141f88139fec77280b2be23c1f66f8e40e1025e7 + run: | + docker run --rm "$ACTIONLINT" -version + docker run --rm -v "$PWD:/repo" -w /repo "$ACTIONLINT" -color + # `!cancelled()` rather than a plain sequence: a shellcheck failure above must not # cost the ktlint/detekt/lint lists. Same reason this step passes --continue -- one # round trip should produce every list, not stop at the first. diff --git a/CLAUDE.md b/CLAUDE.md index dd3a15f..4aa165a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -185,11 +185,12 @@ install for code that can never run — and on API 37 the full APK does not fit `podman run --rm -v "$PWD:/mnt:z" docker.io/koalaman/shellcheck@sha256:61862eba... ` (the digest is in `status_check.yml`; there is no shellcheck system package on this host). - **It does not cover inline `run:` blocks in the workflows**, and a good deal of this repo's bash - lives there. `actionlint` does cover them — it runs shellcheck over each `run:` — and reports one - pre-existing `info` finding in `build.yml`. It is not wired in because every action here is - pinned by SHA, and actionlint's usual installer is a `curl | bash` off a moving branch; doing it - properly means pinning a container digest. Tracked separately rather than bolted on. + **`actionlint` covers the half shellcheck cannot see** — the inline `run:` blocks, where a good + deal of this repo's bash lives. It runs shellcheck over each `run:` plus its own checks on + expression syntax, `needs:` references, matrix keys and action inputs. It sits in the same job, + **pinned by digest** for the reason above and one of its own: its documented installer is a + `curl | bash` off a moving branch, which does not belong in a repo that pins every action by SHA. + Locally: `podman run --rm -v "$PWD:/repo:z" -w /repo docker.io/rhysd/actionlint@sha256:9d360886... -color`. ## Dependency versions diff --git a/app/src/test/java/org/libremediaconverter/codec/CodecVocabularyTest.kt b/app/src/test/java/org/libremediaconverter/codec/CodecVocabularyTest.kt index bc4c045..7045b7f 100644 --- a/app/src/test/java/org/libremediaconverter/codec/CodecVocabularyTest.kt +++ b/app/src/test/java/org/libremediaconverter/codec/CodecVocabularyTest.kt @@ -140,4 +140,34 @@ class CodecVocabularyTest { assertFalse("this device has no AVC decoder, and x264 is AVC", hevcOnly.canDecode("x264")) assertTrue("a name nobody knows keeps the permissive answer", hevcOnly.canDecode("cinepak")) } + + /** + * The other half of the null policy, at the seam it exists for — #86. + * + * `mimeFor`'s `COPY, NONE -> null` arm carries its consequence in a comment: "Returning null + * makes canEncode answer true, which is the right answer: a copied or absent track places no + * demand on the hardware." That is a product decision, and until this test nothing held it. A + * MIME appearing in that arm would make a device with no matching encoder refuse a stream copy + * — a job that never encodes anything — and the router would send it to FFmpeg to re-mux what + * Media3 could have re-muxed. + * + * The `H264` line is what makes the other two mean something: without it, a `canEncode` that + * simply returned `true` would satisfy this test. `NONE` is asserted separately from `COPY` + * because they are one arm today and two answers, and splitting the arm must not silently + * halve the coverage. + */ + @Test + fun `a device with no video encoder at all still permits a copied or absent track`() { + val noEncoders = AndroidDeviceCodecs.forTesting(encoders = emptySet(), decoders = setOf("video/avc")) + assertTrue( + "a copied track is re-muxed, not encoded, so no encoder is required", + noEncoders.canEncode(VideoCodec.COPY), + ) + assertTrue("an absent track places no demand on the hardware", noEncoders.canEncode(VideoCodec.NONE)) + assertFalse( + "this device has no AVC encoder, so an H.264 target has to be refused — without this, " + + "a canEncode that always answered true would satisfy the two assertions above", + noEncoders.canEncode(VideoCodec.H264), + ) + } } diff --git a/app/src/test/java/org/libremediaconverter/codec/VideoCodecMimeAgreementTest.kt b/app/src/test/java/org/libremediaconverter/codec/VideoCodecMimeAgreementTest.kt new file mode 100644 index 0000000..e801657 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/codec/VideoCodecMimeAgreementTest.kt @@ -0,0 +1,162 @@ +package org.libremediaconverter.codec + +import androidx.media3.common.util.UnstableApi +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Test +import org.libremediaconverter.convert.Media3Engine +import org.libremediaconverter.model.VideoCodec + +/** + * Bites on #86: a fifth `VideoCodec -> MIME` table, and nothing checking it agrees with the fourth. + * + * [AndroidDeviceCodecs.mimeFor] and [Media3Engine.videoMimeTypeFor] take the same enum and return a + * MIME string, from opposite ends of one export. The first asks the device *"have you an encoder + * for this?"*; the second tells Transformer *"produce this."* If they name different MIME types for + * the same codec, the app checks for one encoder and then requests another — the check passes, the + * export succeeds, and the user's H.265 file contains H.264. Both were `private` until #85 and #87 + * widened them, so this assertion could not be written before; each table had per-arm tests that + * pinned its own answers and could not see the other side. + * + * **They do not agree everywhere, and must not be forced to.** Three buckets, all pinned below: + * + * - **H.264 and H.265** — both tables name a MIME, and it has to be the same one. This is the + * bucket the defect lives in. + * - **VP8, VP9 and AV1** — the device table names a real MIME, Transformer's returns null. That is + * correct, not drift: `Transformer.setVideoMimeType` will not accept them, so the router sends + * them to FFmpeg before Media3 is asked anything, while a device may still genuinely own a VP9 + * encoder and `canEncode` has to give a truthful answer about it. Flattening `mimeFor` to null + * here to "make the tables agree" would make `canEncode(VP9)` answer true on hardware that has + * no VP9 encoder. The routing half of that claim is proved in + * `Media3EngineMimeTypesTest.the router sends exactly H264 and H265 video encodes to Media3`, + * which drives the real router; it is not repeated here. + * - **COPY and NONE** — neither names a MIME, because neither is encoded at all. + * + * The fourth bucket is asserted empty: a codec Transformer names and the device check cannot ask + * about would mean `canEncode` waving through a target the app then really does encode. + * + * **Audio has no partner, and that is a gap rather than a decision.** [Media3Engine.audioMimeTypeFor] + * is the same shape one enum over — `AudioCodec -> MIME` — but [AndroidDeviceCodecs] enumerates + * `video/` MIME types only, so there is no device-side audio table to cross-check it against. An + * audio encoder this device lacks is therefore not caught up front the way a video one is; the job + * reaches Media3 and falls back after failing. Named here so the asymmetry reads as unfinished + * rather than intended. + */ +@UnstableApi +class VideoCodecMimeAgreementTest { + + /** Both tables name a MIME. The pair has to match; this is the whole point of the file. */ + private val bothNameAMime = setOf(VideoCodec.H264, VideoCodec.H265) + + /** Only the device table names one, because Transformer is never asked for these. */ + private val deviceOnly = setOf(VideoCodec.VP8, VideoCodec.VP9, VideoCodec.AV1) + + /** Neither names one: nothing is encoded, so there is no encoder to name. */ + private val neitherNamesOne = setOf(VideoCodec.COPY, VideoCodec.NONE) + + /** + * Sorts every [VideoCodec] by what the two tables actually answer, then compares the sorting + * with the buckets documented above. + * + * This is what makes the agreement test below non-vacuous, and it is deliberately an exact + * comparison in all four directions. A codec added to the enum lands in some bucket and fails + * here rather than arriving unclassified. A table that starts returning null for everything — + * the shape a filtered loop would pass on — empties two buckets and fails here. And a + * *convergence* fails too: giving `videoMimeTypeFor(VP9)` a real MIME moves VP9 out of + * `deviceOnly`, which is the point. The divergence should be deliberate and visible, so + * changing it should require saying so in this file. + */ + @Test + fun `each video codec is in the bucket the two tables actually put it in`() { + assertEquals( + "codecs both tables name a MIME for", + bothNameAMime, + VideoCodec.entries.filter { device(it) != null && transformer(it) != null }.toSet(), + ) + assertEquals( + "codecs only the device check names a MIME for, because Transformer will not encode them", + deviceOnly, + VideoCodec.entries.filter { device(it) != null && transformer(it) == null }.toSet(), + ) + assertEquals( + "codecs neither table names a MIME for, because nothing is encoded", + neitherNamesOne, + VideoCodec.entries.filter { device(it) == null && transformer(it) == null }.toSet(), + ) + assertEquals( + "codecs Transformer names a MIME for that the device check cannot ask about — canEncode " + + "would answer true without looking, for a codec Media3 really is told to produce", + emptySet(), + VideoCodec.entries.filter { device(it) == null && transformer(it) != null }.toSet(), + ) + } + + /** + * The cross-check itself. + * + * Per-arm tests in either file cannot catch this: each pins its own table's answers, so a pair + * changed in lockstep with its own expectations stays green on both sides while the two tables + * describe different codecs. + */ + @Test + fun `where both tables name a MIME they name the same one`() { + bothNameAMime.forEach { codec -> + val asked = device(codec) + val requested = transformer(codec) + assertNotNull("AndroidDeviceCodecs has no MIME to ask the device about for ${codec.label}", asked) + assertNotNull("Media3Engine has no MIME to give Transformer for ${codec.label}", requested) + assertEquals( + "${codec.label}: the device is asked about $asked and Transformer is then told to " + + "produce $requested, so the capability check answers about a codec that is not the output", + asked, + requested, + ) + } + } + + /** + * The documented divergence, asserted rather than described. + * + * Both halves matter. The null side is Media3's refusal; the non-null side is the device + * check's genuine question, and it is the half a reader "tidying up" the disagreement would + * delete. + */ + @Test + fun `the codecs Transformer will not encode are still codecs this device may or may not have`() { + deviceOnly.forEach { codec -> + assertNotNull( + "${codec.label} goes to FFmpeg, but canEncode still has to answer truthfully about " + + "this device's encoder — a null here makes it answer true without looking", + device(codec), + ) + assertNull( + "Transformer rejects ${codec.label}, so naming a MIME for it would request an export " + + "Media3 cannot perform", + transformer(codec), + ) + } + } + + /** + * Guards every comparison above against passing as `null == null`. + * + * `MediaFormat`'s MIME types are Java compile-time constants and are inlined, so the unit-test + * classpath's stubbed `android.jar` never supplies them; `MimeTypes`' come from a real + * `media3-common` jar. If either stopped holding, the buckets would collapse and this fails + * first, with the reason. Same guard, and the same reason, as + * `CodecVocabularyTest.the MIME constants are real strings rather than stubs`. + */ + @Test + fun `both tables return real MIME strings rather than stubs`() { + assertEquals("video/avc", AndroidDeviceCodecs.mimeFor(VideoCodec.H264)) + assertEquals("video/hevc", AndroidDeviceCodecs.mimeFor(VideoCodec.H265)) + assertEquals("video/x-vnd.on2.vp9", AndroidDeviceCodecs.mimeFor(VideoCodec.VP9)) + assertEquals("video/avc", Media3Engine.videoMimeTypeFor(VideoCodec.H264)) + assertEquals("video/hevc", Media3Engine.videoMimeTypeFor(VideoCodec.H265)) + } + + private fun device(codec: VideoCodec): String? = AndroidDeviceCodecs.mimeFor(codec) + + private fun transformer(codec: VideoCodec): String? = Media3Engine.videoMimeTypeFor(codec) +} diff --git a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt index 9bfe5b9..2aa69ac 100644 --- a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt @@ -140,10 +140,16 @@ class ConversionViewModelProbeFailureTest { private fun pickedProbe(): InputProbe? { val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) viewModel.onInputPicked(INPUT) + // The predicate is the guard, and it is the only one needed. It requires `Ready`, so a + // pick that ended in `Failed` never satisfies it and `awaitState` fails on its timeout + // naming what it was waiting for -- "Ready with a probe" -- which says more than a + // separate assertion could. A `ready as? ConversionState.Failed` check used to sit here + // and was dead: `Ready` and `Failed` are sibling subtypes of one sealed interface, so + // the cast was always null and the assertNull could never fire. Measured, not assumed -- + // flipping it to assertNotNull failed all three callers of this helper. val ready = awaitState(viewModel.state, "Ready with a probe") { it is ConversionState.Ready && it.input.probe != null } - assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed)) return (ready as ConversionState.Ready).input.probe }