Merge branch 'main' into fix/dead-assertion-probe-test

.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.

.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.

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... <files>`
   (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
 

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),
+        )
+    }
 }

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>(),
+            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)
+}

tools/local-emulator/run-e2e.sh

@@ -236,10 +236,24 @@ ensure_avd() {
   else
     if [ ! -d "$img_dir" ]; then
       echo "  installing $pkg"
-      yes | sdkmanager --install "$pkg" > /dev/null 2>&1 || {
+      # Read sdkmanager's own status, not the pipeline's. `yes` never ends, so the moment
+      # sdkmanager exits and closes the pipe, `yes` dies of SIGPIPE with 141 -- and this
+      # script runs under `pipefail`, which takes the rightmost NON-ZERO status. A package
+      # that installed perfectly therefore reported "FAILED to install".
+      #
+      # Measured rather than reasoned: under `set -o pipefail`, `yes | true` exits 141 on
+      # every run, and `yes | sh -c 'exit 3'` exits 3 -- so the pipeline status cannot tell
+      # a clean install from a broken one, while ${PIPESTATUS[1]} reports 0 and 3.
+      #
+      # The `echo no | avdmanager` below is deliberately NOT changed. One line fits the pipe
+      # buffer, so echo has already exited before the close and there is no signal to
+      # receive; `echo no | true` measured 0 on every run. Only an unbounded producer is
+      # exposed to this.
+      yes | sdkmanager --install "$pkg" > /dev/null 2>&1
+      if [ "${PIPESTATUS[1]}" -ne 0 ]; then
         echo "  FAILED to install $pkg"
         return 1
-      }
+      fi
     fi
     echo "  creating AVD $avd from $pkg"
     echo no | avdmanager create avd -n "$avd" -k "$pkg" -d pixel_6 --force > /dev/null 2>&1 || {