fix(cli): make the gpumask test use a positive mask instead of upstream's -1

CHANGELOG.md

@@ -19730,6 +19730,20 @@ Golden-safe: CLI plumbing only; the testdata 576x324 pair still scores pooled
 mean VMAF 94.32301 on CPU.
 
 
+- **`test_vmaf_cuda_gpumask` failed on every host that actually has an
+  NVIDIA GPU.** The script — inherited verbatim from upstream — documented
+  `--gpumask -1` as "use cpu" and invoked it twice under `set -e`, but the
+  fork's CLI rejects negative values for `--gpumask`. It looked green in CI
+  only because the script skips (exit 77) when `nvidia-smi -L` finds no
+  device. Upstream's `-1` works by accident: POSIX `strtoul` silently turns
+  `"-1"` into `ULONG_MAX`, and this fork deliberately refuses a leading `-`
+  rather than wrap a value the caller did not mean. Fixed by using
+  `--gpumask 1`, the documented "any non-zero value disables the GPU feature
+  extractors" spelling — measured byte-identical to `--no_cuda --no_sycl`.
+  The `--gpumask` entry in `docs/usage/cli.md` is corrected at the same time:
+  it described a per-op bitmask, which the option has never been.
+
+
 - Fix `vmaf --backend metal` / `--metal_device` / `--no_metal` on macOS.
   `core/tools/meson.build` previously defined `-DHAVE_CUDA=1`, `-DHAVE_SYCL=1`,
   and `-DHAVE_HIP=1` for `vmaf_tool_cflags` but omitted Metal entirely, leaving

changelog.d/fixed/cli-gpumask-negative-contract.md

@@ -0,0 +1,12 @@
+- **`test_vmaf_cuda_gpumask` failed on every host that actually has an
+  NVIDIA GPU.** The script — inherited verbatim from upstream — documented
+  `--gpumask -1` as "use cpu" and invoked it twice under `set -e`, but the
+  fork's CLI rejects negative values for `--gpumask`. It looked green in CI
+  only because the script skips (exit 77) when `nvidia-smi -L` finds no
+  device. Upstream's `-1` works by accident: POSIX `strtoul` silently turns
+  `"-1"` into `ULONG_MAX`, and this fork deliberately refuses a leading `-`
+  rather than wrap a value the caller did not mean. Fixed by using
+  `--gpumask 1`, the documented "any non-zero value disables the GPU feature
+  extractors" spelling — measured byte-identical to `--no_cuda --no_sycl`.
+  The `--gpumask` entry in `docs/usage/cli.md` is corrected at the same time:
+  it described a per-op bitmask, which the option has never been.

core/tools/AGENTS.md

@@ -240,3 +240,17 @@ Keep it that way — the golden-gate CLI invocations parse this stream.
 `goto cleanup` target**, which is what makes the restore run on the error
 paths. Moving its declaration below a jump target is ill-formed C++ and would
 silently leave the user's console in UTF-8 + VT mode after an error exit.
+
+## `parse_unsigned` rejects negatives on purpose (ADR-1209)
+
+`parse_unsigned` refuses a leading `'-'` before calling `strtoul`, because
+POSIX `strtoul` silently converts `"-1"` to `ULONG_MAX` without setting
+`errno`. Upstream relies on that wraparound — its own
+`test_vmaf_cuda_gpumask.sh` passes `--gpumask -1` and expects it to mean "all
+bits set". Do not loosen the check to make an inherited script pass; fix the
+caller instead. `--gpumask 1` means the same thing and says so.
+
+More generally, `--gpumask` is not a per-op bitmask despite the `$bitmask`
+placeholder: passing the flag opts into GPU backend selection, and any non-zero
+value then disables the GPU feature extractors, so the run falls back to CPU.
+`--gpumask 0` = use the GPU, `--gpumask 1` = use the CPU.

core/tools/test/test_vmaf_cuda_gpumask.sh

@@ -27,13 +27,21 @@ nvidia-smi -L >/dev/null 2>&1 || exit 77
   --frame_cnt 2 \
   --gpumask 0
 
-# gpumask: use cpu
+# gpumask: use cpu.
+#
+# Any NON-ZERO mask disables GPU feature-extractor selection and falls back to
+# the CPU implementation (see the `gpumask` docs in libvmaf.h). Upstream writes
+# `-1` here, which only ever worked because POSIX strtoul() silently converts
+# "-1" to ULONG_MAX; this fork rejects a leading '-' before calling strtoul
+# (core/tools/cli_parse.cpp::parse_unsigned) rather than accept a value the
+# caller did not mean. `1` expresses the same intent without relying on
+# unsigned wraparound. See ADR-1209.
 ./tools/vmaf \
   --reference /dev/zero \
   --distorted /dev/zero \
   --width 1920 --height 1080 --pixel_format 420 --bitdepth 8 \
   --frame_cnt 2 \
-  --gpumask -1
+  --gpumask 1
 
 # no gpumask: use cuda for vmaf features, cpu for psnr
 ./tools/vmaf \
@@ -45,11 +53,11 @@ nvidia-smi -L >/dev/null 2>&1 || exit 77
   --feature psnr \
   --output /dev/stdout
 
-# gpumask: use cpu for vmaf features and psnr
+# gpumask: use cpu for vmaf features and psnr (non-zero mask; see above)
 ./tools/vmaf \
   --reference /dev/zero \
   --distorted /dev/zero \
   --width 1920 --height 1080 --pixel_format 420 --bitdepth 8 \
   --frame_cnt 2 \
-  --gpumask -1 \
+  --gpumask 1 \
   --feature psnr

docs/adr/1209-cli-gpumask-negative-contract.md

@@ -0,0 +1,87 @@
+<!-- markdownlint-disable MD013 MD041 MD060 -->
+
+# ADR-1209: `--gpumask` keeps rejecting negative values; the test script uses a positive mask
+
+- **Status**: Proposed
+- **Date**: 2026-09-06
+- **Deciders**: Lusoris
+- **Tags**: cli, testing, upstream-divergence, correctness
+
+## Context
+
+`core/tools/test/test_vmaf_cuda_gpumask.sh` is inherited verbatim from upstream
+and invokes `--gpumask -1` twice, commented "gpumask: use cpu". The fork's CLI
+rejects it:
+
+```text
+Invalid argument "-1" for option --gpumask; should be a non-negative integer
+```
+
+The script runs under `set -e`, so `test_vmaf_cuda_gpumask` fails on **any host
+that actually has an NVIDIA GPU**. It reports green in CI only because the
+script exits 77 (meson SKIP) when `nvidia-smi -L` finds no device.
+
+`-1` only ever "worked" upstream by accident. Upstream's `parse_unsigned` calls
+`strtoul` directly, and POSIX `strtoul` silently converts `"-1"` to `ULONG_MAX`
+without setting `errno`; that then truncates to `UINT_MAX`. The fork
+deliberately closed that hole (`core/tools/cli_parse.cpp::parse_unsigned`
+rejects a leading `'-'` before calling `strtoul`, with a comment saying exactly
+why). So the CLI is behaving as designed and the script is the stale side.
+
+The semantics make a positive mask the correct spelling anyway. `gpumask` is
+documented in `libvmaf.h` as: *any non-zero value disables the GPU
+feature-extractor selection for both the CUDA and SYCL backends (the runtime
+falls back to the CPU implementation)*. It is not a per-op bitmask despite the
+`<bitmask>` placeholder in the usage string.
+
+Measured on an RTX 4090 over the Netflix 576x324 pair, 4 frames:
+
+| invocation | pooled VMAF |
+|---|---|
+| `--gpumask 0` | 88.80022305138327 |
+| `--gpumask 1` | 88.8002154453433 |
+| `--no_cuda --no_sycl` | 88.8002154453433 |
+
+`--gpumask 1` is byte-identical to an explicit CPU run, which is exactly what
+the script's `-1` was reaching for.
+
+## Decision
+
+We will keep the CLI's rejection of negative `--gpumask` values and change the
+test script to use `--gpumask 1`, the documented "any non-zero" spelling. The
+`--gpumask` entry in `docs/usage/cli.md` is corrected at the same time: it
+described a per-op mask, which the option has never been.
+
+## Alternatives considered
+
+| Option | Pros | Cons | Why not chosen |
+|---|---|---|---|
+| Keep strict parsing, fix the script to `1` (chosen) | The CLI keeps failing loudly on input the caller did not mean; the script states its intent directly; matches the documented API semantics | Diverges from upstream's accidental acceptance of `-1` | — |
+| Special-case `--gpumask` to accept negatives as "all bits set" | Restores upstream-compatible spelling | Reintroduces exactly the silent unsigned wraparound the fork removed on purpose, for one option; CERT INT and the fork's own coding standards forbid the implicit conversion | Rejected |
+| Relax `parse_unsigned` globally | One change covers any future case | Would silently accept `-1` for `--width`, `--threads`, `--frame_cnt` and every other unsigned option — a much worse footgun | Rejected |
+| Delete the script's `-1` invocations | Trivially green | Loses coverage of the CPU-fallback path, which is the thing the script exists to test | Rejected — never remove a user surface's coverage to make a gate pass |
+
+## Consequences
+
+- **Positive**: `test_vmaf_cuda_gpumask` passes on a GPU host instead of only
+  skipping on a GPU-less one. Verified: `rc=0` on the RTX 4090 workstation,
+  where it failed before.
+- **Negative**: anyone who scripted `--gpumask -1` against upstream gets a hard
+  error on this fork. That is pre-existing — the fork has rejected it since
+  `parse_unsigned` was hardened — and the error message names the constraint.
+  `docs/usage/cli.md` now documents the divergence.
+- **Neutral / follow-ups**: the usage string still calls the argument
+  `$bitmask`. It is left alone here because changing the help text is a
+  user-visible string change with its own compatibility surface; the reference
+  table in `docs/usage/cli.md` carries the accurate description.
+
+## References
+
+- `core/tools/cli_parse.cpp::parse_unsigned` — the deliberate negative
+  rejection and its rationale comment.
+- `core/include/libvmaf/libvmaf.h` — the `gpumask` "any non-zero" contract.
+- Upstream ships the same script and the same `strtoul`-based parser:
+  `libvmaf/tools/test/test_vmaf_cuda_gpumask.sh`,
+  `libvmaf/tools/cli_parse.c`.
+- Source: `req` — user direction to fix the `--gpumask -1` regression found
+  while running the full suite for ADR-1204 / ADR-1205.

docs/adr/README.md

@@ -1018,3 +1018,4 @@ ADRs may exist there for local session continuity, but the tracked
 | [ADR-1204](1204-adm-cm-edge-clamp-gpu-twins.md) | GPU ADM contrast-masking twins clamp the far edge instead of mirroring it | Proposed | cuda, sycl, hip, metal, correctness, feature-extractor, testing |
 | [ADR-1205](1205-ssimulacra2-fma-unification-scalar-and-gpu.md) | The ssimulacra2 FMA unification extends to the scalar fallback and every GPU host copy | Proposed | cuda, sycl, hip, metal, simd, correctness, feature-extractor, reproducibility |
 | [ADR-1206](1206-gpu-parity-large-fixture-variants.md) | Every CUDA parity test also runs against a second, larger fixture | Proposed | testing, cuda, ci, correctness |
+| [ADR-1209](1209-cli-gpumask-negative-contract.md) | `--gpumask` keeps rejecting negative values; the test script uses a positive mask | Proposed | cli, testing, upstream-divergence, correctness |

docs/adr/_index_fragments/1209-cli-gpumask-negative-contract.md

@@ -0,0 +1 @@
+| [ADR-1209](1209-cli-gpumask-negative-contract.md) | `--gpumask` keeps rejecting negative values; the test script uses a positive mask | Proposed | cli, testing, upstream-divergence, correctness |

docs/adr/_index_fragments/_order.txt

@@ -927,3 +927,4 @@
 1204-adm-cm-edge-clamp-gpu-twins
 1205-ssimulacra2-fma-unification-scalar-and-gpu
 1206-gpu-parity-large-fixture-variants
+1209-cli-gpumask-negative-contract

docs/rebase-notes.md

@@ -48978,3 +48978,25 @@ Documentation only. One thing worth knowing:
    in item 2 passed its bit-exactness assertion for as long as it did. When
    touching the conversion, change the shipped copies and the test reference
    together, or the test will keep agreeing with itself.
+
+## ADR-1209 — `--gpumask` and upstream's negative-value accident
+
+1. **Do not "restore" `--gpumask -1` during an upstream sync.** Upstream's
+   `parse_unsigned` calls `strtoul` directly, and POSIX `strtoul` silently
+   converts `"-1"` to `ULONG_MAX` without setting `errno`. This fork rejects a
+   leading `'-'` before calling `strtoul`
+   (`core/tools/cli_parse.cpp::parse_unsigned`) precisely to stop that. A sync
+   that pulls upstream's parser back in re-opens the hole for *every* unsigned
+   option, not just this one.
+
+2. **`core/tools/test/test_vmaf_cuda_gpumask.sh` diverges from upstream on
+   purpose.** It uses `--gpumask 1` where upstream writes `--gpumask -1`. Both
+   mean "disable the GPU feature extractors"; only the fork's spelling survives
+   the fork's argument validation. If a sync reverts those two lines, the test
+   goes back to failing on any host with a GPU while still passing CI on
+   GPU-less runners.
+
+3. **`--gpumask` is not a per-op bitmask.** Any non-zero value disables GPU
+   feature-extractor selection wholesale for CUDA and SYCL. The `$bitmask`
+   placeholder in the usage string is inherited and inaccurate; the reference
+   table in `docs/usage/cli.md` carries the real contract.