fix(core): harvest and fix nine stale upstream Netflix/vmaf reports (ADR-1166)

.github/workflows/libvmaf-build-matrix.yml

@@ -525,6 +525,35 @@ jobs:
           # Static libs must include -lpthread and -lm
           pkg-config --static --libs libvmaf | grep -q '\-lpthread\|\-pthread'
           pkg-config --static --libs libvmaf | grep -q '\-lm'
+          # ... and the C++ runtime, because libvmaf contains C++ translation
+          # units (feature_extractor.cpp, luminance_tools.cpp, vendored libsvm,
+          # ...). Netflix/vmaf#1178.
+          pkg-config --static --libs libvmaf | grep -q '\-lstdc++\|\-lc++'
+          # Grepping the flag list is not the test that actually reproduces the
+          # downstream FFmpeg failure — LINKING is. Build a C consumer with the
+          # C driver, using exactly what pkg-config reports.
+          echo "=== static link smoke ==="
+          cat > /tmp/pc_static_smoke.c <<'SMOKE'
+          #include <libvmaf/libvmaf.h>
+          int main(void)
+          {
+              VmafContext *ctx = 0;
+              VmafConfiguration cfg = {0};
+              return vmaf_init(&ctx, cfg) == 0 ? 0 : 1;
+          }
+          SMOKE
+          # Use the leg's OWN compiler, not bare `cc`. The matrix builds with
+          # `ccache gcc-14` / `ccache clang-22`, while `cc` is the image
+          # default (a different gcc), so a bare `cc` link tests a toolchain
+          # the archive was not produced with. $CC is intentionally unquoted:
+          # its value is two words ("ccache gcc-14") and must word-split.
+          # (b_lto is meson-default false here, and explicitly false on the
+          # SYCL/CUDA legs, so the archive holds plain objects rather than LTO
+          # IR — a plugin mismatch is not in play, but matching the compiler
+          # still costs nothing.)
+          ${CC:-cc} /tmp/pc_static_smoke.c $(pkg-config --cflags libvmaf) \
+             $(pkg-config --static --libs libvmaf) -o /tmp/pc_static_smoke
+          echo "static link OK"
 
       # DNN legs: run the dedicated dnn suite first so its logs are easy
       # to find if the build regresses ORT compatibility. The full test

core/include/libvmaf/feature.h

@@ -34,13 +34,35 @@ extern "C" {
  * are detected at set-time and stored in a normalised form so that
  * `"1"` / `"1.0"` / `" 1 "` compare equal downstream.
  *
- * Ownership transfer rules:
- *   - On success of @ref vmaf_use_feature / @ref vmaf_model_feature_overload,
- *     ownership of the dictionary passes to the VmafContext / VmafModel and
- *     the caller MUST NOT free it.
- *   - On failure of those calls (non-zero return), the caller still owns the
- *     dictionary and is responsible for releasing it with
+ * Ownership transfer rules (identical for @ref vmaf_use_feature,
+ * @ref vmaf_model_feature_overload and
+ * @ref vmaf_model_collection_feature_overload; see Netflix/vmaf#1242, which
+ * reported the divergence these three headers used to carry):
+ *
+ *   - **NULL-argument failures do not consume the dictionary.** If the call
+ *     returns `-EINVAL` because a required argument was NULL, nothing was
+ *     taken: the caller still owns the dictionary and must release it with
  *     @ref vmaf_feature_dictionary_free.
+ *   - **@ref vmaf_use_feature additionally does not consume when
+ *     @p feature_name names no registered feature.** It looks the extractor up
+ *     first and returns `-EINVAL` before touching the dictionary.
+ *   - **Every other path consumes it.** Once those guards have passed, the call
+ *     releases the dictionary internally — on success and on failure alike,
+ *     including `-ENOMEM` from the merge/copy step — and the caller MUST NOT
+ *     free it.
+ *
+ * Note the asymmetry in the third bullet, which is deliberate and was the
+ * divergence Netflix/vmaf#1242 reported. @ref vmaf_model_feature_overload and
+ * @ref vmaf_model_collection_feature_overload match @p feature_name against the
+ * features of a *particular model*. A name that matches nothing there is not an
+ * error — it is a successful no-op that returns `0` — and the dictionary is
+ * still consumed. Only @ref vmaf_use_feature, which resolves against the global
+ * extractor registry, can report an unknown name as `-EINVAL` and hand the
+ * dictionary back.
+ *
+ * In practice: free the dictionary yourself only when the call returned
+ * `-EINVAL` *and* you either passed a NULL argument or called
+ * @ref vmaf_use_feature. Otherwise never.
  */
 typedef struct VmafFeatureDictionary VmafFeatureDictionary;
 

core/include/libvmaf/libvmaf.h

@@ -270,9 +270,15 @@ VMAF_EXPORT int vmaf_use_features_from_model_collection(VmafContext *vmaf,
  * Register specific feature extractor.
  * Useful when a specific/additional feature is required, usually one which
  * is not already provided by a model via `vmaf_use_features_from_model()`.
- * This may be called multiple times. `VmafContext` will take ownership of the
- * `VmafFeatureDictionary` (`opts_dict`). Use `vmaf_feature_dictionary_free()`
- * only in the case of failure.
+ * This may be called multiple times. `VmafContext` takes ownership of the
+ * `VmafFeatureDictionary` (`opts_dict`) on every path EXCEPT the
+ * argument-validation guards: if this returns `-EINVAL` because `vmaf` or
+ * `feature_name` was NULL, or because `feature_name` names no registered
+ * feature, nothing was consumed and the caller must release the dictionary
+ * with `vmaf_feature_dictionary_free()`. On any other return — success or
+ * failure — the dictionary has already been released internally and the
+ * caller MUST NOT free it. See <libvmaf/feature.h> for the shared contract
+ * (Netflix/vmaf#1242).
  *
  * @param vmaf         The VMAF context allocated with `vmaf_init()`.
  *

core/include/libvmaf/model.h

@@ -173,10 +173,23 @@ VMAF_EXPORT int vmaf_model_load_from_path(VmafModel **model, VmafModelConfig *cf
  * existing options. Useful when the caller wants to tweak (for example) the
  * VIF enhancement gain limit without editing the model JSON on disk.
  *
- * On both success and failure, ownership of @p opts_dict transfers to this
- * call — the function releases the dictionary internally before returning.
- * The caller MUST NOT call @ref vmaf_feature_dictionary_free on @p opts_dict
- * after invoking this function, even on a non-zero return.
+ * Ownership of @p opts_dict transfers to this call on every path EXCEPT the
+ * argument-validation guards: if the function returns `-EINVAL` because
+ * @p model, @p feature_name or @p opts_dict was NULL, nothing was consumed and
+ * the caller still owns the dictionary.  On any other return — success, or
+ * `-ENOMEM` from the merge step — the function has released the dictionary
+ * internally and the caller MUST NOT call
+ * @ref vmaf_feature_dictionary_free on it.
+ *
+ * This is NOT quite the same rule as @ref vmaf_use_feature, and the difference
+ * is deliberate.  That function resolves @p feature_name against the global
+ * extractor registry, so it can reject an unknown name with `-EINVAL` before
+ * touching the dictionary and hand it back.  This one matches @p feature_name
+ * against the features of a *particular model*: a name that matches nothing is
+ * not an error but a successful no-op returning `0`, and the dictionary is
+ * still consumed.  See <libvmaf/feature.h> for the contract covering all three
+ * entry points (Netflix/vmaf#1242 reported the headers contradicting each
+ * other; the `-ENOMEM` leak it described is fixed).
  *
  * @param model        Loaded model from @ref vmaf_model_load or
  *                     @ref vmaf_model_load_from_path. Must not be NULL.
@@ -342,10 +355,15 @@ VMAF_EXPORT int vmaf_model_collection_load_from_path(VmafModel **model,
  * the collection plus the lead model @p model, so a single override
  * propagates to the ensemble.
  *
- * Ownership of @p opts_dict transfers to this call — the function deep-copies
- * the dictionary onto each sub-model and releases the original (and every
- * temporary copy on failure) before returning. The caller MUST NOT call
- * @ref vmaf_feature_dictionary_free on @p opts_dict afterwards.
+ * Ownership of @p opts_dict transfers to this call on every path EXCEPT the
+ * argument-validation guards: if the function returns `-EINVAL` because
+ * @p model, @p model_collection, @p feature_name or @p opts_dict was NULL,
+ * nothing was consumed and the caller still owns the dictionary.  Otherwise
+ * the function deep-copies the dictionary onto each sub-model and releases the
+ * original (and every temporary copy, including a partially-built one on an
+ * allocation failure) before returning, and the caller MUST NOT call
+ * @ref vmaf_feature_dictionary_free on @p opts_dict afterwards.  Same rule as
+ * @ref vmaf_model_feature_overload; see <libvmaf/feature.h>.
  *
  * @param model            Lead model returned by
  *                         @ref vmaf_model_collection_load /

core/src/feature/AGENTS.md

@@ -1198,3 +1198,109 @@ after a port-upstream of any of these files.
      (`a=1/256`, `b_Y=-5.4715e-3`, `c_Y=1.91`) regenerate the official
      lookup table to 8 dp. Oracle values in `core/test/test_y_funque_plus.c`
      were re-derived against a `pywt` + OpenCV reference at places=4.
+
+## Reflect-101 mirror padding — invariants (ADR-1166)
+
+The separable float convolution in `common/convolution_internal.h` uses
+**reflect-101** mirror padding, and the fold is deliberately **iterative**:
+
+```c
+FORCE_INLINE int convolution_reflect101(int idx, int size)
+{
+    if (size <= 1) return 0;
+    while (idx < 0 || idx >= size)
+        idx = (idx < 0) ? -idx : (2 * size - idx - 2);
+    return idx;
+}
+```
+
+Load-bearing details a rebase or a "simplification" must not break:
+
+1. **The loop is not decoration.** Upstream (and this fork, before ADR-1166)
+   bounced once. One bounce only lands in range when `size >= radius + 1`;
+   at `size == 2` a tap of `-2` folds to `+2` and a tap of `+3` folds to `-1`,
+   and the caller dereferences out of bounds. Two live CPU paths reached those
+   sizes — `float_vif` on 9..15 px frames and `float_motion` with
+   `motion_add_uv` on 4x4 4:2:0 chroma. Do not collapse it back to an
+   `if/else if`.
+2. **The `size <= 1` short circuit is required for termination**, not just for
+   correctness: at `size == 1` the fold alternates between `-2` and `+2`
+   forever.
+3. **The fold is bit-identical to the single bounce for every in-contract
+   size** (the loop exits on the first iteration), which is what lets this be a
+   pure safety fix with no score movement.
+   `core/test/test_convolution_edge_small.c::test_large_plane_bit_identical`
+   pins that against an explicit single-bounce reference; if you change the
+   fold, that test must still pass unmodified.
+4. **`convolution.c`'s `convolution_clamp_borders()` is load-bearing too.**
+   `borders_right` / `borders_bottom` are derived as
+   `dim - (filter_width - radius)` and go **negative** for a plane narrower
+   than the filter, which makes the trailing border loop start at a negative
+   index and write before the destination. The clamp is a no-op for every
+   `dim >= filter_width`.
+
+The motion extractors' own `mirror()` bodies (`integer_motion.c`,
+`integer_motion_v2.c`, `x86/motion_avx2.c`, `x86/motion_avx512.c`,
+`arm64/motion_v2_neon.c`, and the CUDA / HIP / Metal twins) are **still
+single-bounce on purpose**: they sit behind an `init()` guard that rejects
+`w < 3 || h < 3`, so the defective sizes are unreachable. That is a deliberate
+divergence from Netflix/vmaf#1581, which instead fixes `mirror()` so tiny
+frames can be scored. Changing it is a behaviour decision, not a cleanup —
+see `docs/rebase-notes.md`.
+
+## Minimum-dimension guards cover every plane, not just luma (ADR-1166)
+
+`float_motion.c::motion_check_min_dim_all_planes` validates the **chroma**
+dimensions too when `motion_add_uv` is set, because `motion_blur_plane` is
+called per plane with `ref_pic->w[c]` / `ref_pic->h[c]`. The chroma geometry
+must stay in step with `core/src/picture.c` (`(dim + ss) >> ss`); a luma-only
+guard is exactly the bug Netflix/vmaf#1582 describes.
+
+`float_vif.c`'s guard is derived from `vif_get_min_dim(kernelscale)` — the
+largest `((filter_width_s / 2) + 1) << s` over the four-scale ladder, 16 at the
+default kernelscale — not from the scale-0 filter alone. Do not replace it with
+a constant.
+
+## `compat_builtin.h`: never `__lzcnt` (ADR-1166)
+
+The MSVC `__builtin_clz` / `__builtin_clzll` shim must use `_BitScanReverse` /
+`_BitScanReverse64`. `__lzcnt` emits the LZCNT instruction unconditionally with
+no runtime feature gate; on an x86-64 without ABM/LZCNT the `F3` prefix is
+ignored and it retires as BSR, returning the MSB index instead of the
+leading-zero count — silently wrong VIF and ADM shifts, with no fault and no CI
+signal (every hosted Windows runner has LZCNT). Netflix/vmaf#1422 proposes the
+`__lzcnt` form; Netflix/vmaf#1551 is upstream's own retraction of it.
+`scripts/ci/check-msvc-clz-shim.sh` fails the `fast` suite if it comes back.
+
+## `convolution_f32_c_s` dispatches to SIMD — fix the twins, not just the scalar
+
+`core/src/feature/common/convolution.c::convolution_f32_c_s` returns straight
+into `convolution_f32_avx_s` whenever `VMAF_X86_CPU_FLAG_AVX2` is set. That is
+every CI runner and the dev workstation. **A fix applied only to the scalar
+body in `convolution.c` is dead code on x86.**
+
+The AVX2 (`convolution_avx.c`) and AVX-512 (`convolution_avx512.c`) twins each
+derive the same vertical border split — `radius` and `height - radius` — at
+three sites apiece, once per kernel variant (`_s`, `_sq_s`, `_xy_s`). Six sites
+total. All of them must stay clamped via `convolution_clamp_borders` in
+`convolution_internal.h`: for a plane shorter than the radius, `height - radius`
+is negative, so the trailing border loop starts at a negative row and the
+leading one runs past the end. Both are heap **writes**, not reads.
+
+**Testing the scalar kernel does not test this.**
+`core/test/test_convolution_edge_small.c` calls `convolution_y_c_s` /
+`convolution_x_c_s` directly and so never reaches the dispatch;
+`test_motion_min_dim.c` only calls `init()`. Anything asserting the convolution
+is safe at small sizes must go through the public API — see
+`core/test/test_motion_convolution_oob.c`.
+
+**A guard must mirror the kernel it protects, not the option that named it.**
+`motion_blur_plane` keeps `filter_size = 5` for `motion_filter_size == 1` and
+merely swaps in `FILTER_5_NO_OP_s`, so the radius is 2 regardless. A guard that
+reads the option value instead of the filter width the kernel actually uses
+will let the defective sizes through.
+
+**Chroma plane geometry is the ceiling, `(dim + ss) >> ss`, matching
+`picture.c`.** Using `h / 2` under-allocates by one row for every odd luma
+height, and even-height fixtures — including both Netflix golden resolutions —
+never catch it.

core/src/feature/common/convolution.c

@@ -22,15 +22,29 @@
 #include "convolution_internal.h"
 #include "cpu.h"
 
-extern int vmaf_floorn(int, int);
-extern int vmaf_ceiln(int, int);
-
+/*
+ * Clamp the border/interior split into [0, dim].
+ *
+ * `borders_hi` is derived as `dim - (filter_width - radius)`, which goes
+ * NEGATIVE as soon as the plane is narrower/shorter than the filter.  The
+ * trailing border loop then starts at a negative index and writes
+ * `dst[i * dst_stride - 1]` / `dst[-dst_stride + j]` — a heap underflow
+ * WRITE, reported upstream as Netflix/vmaf#1582.  `borders_lo` can likewise
+ * exceed the dimension (ceil(radius) > dim), which walks the leading border
+ * loop past the end of the row.
+ *
+ * Clamping leaves every in-contract size untouched (for dim >= filter_width
+ * neither bound is out of range) and additionally removes the duplicate
+ * recomputation that happens when the two border bands would otherwise
+ * overlap.
+ */
 void convolution_x_c_s(const float *filter, int filter_width, const float *src, float *dst,
                        int width, int height, int src_stride, int dst_stride, int step)
 {
     int radius = filter_width / 2;
     int borders_left = vmaf_ceiln(radius, step);
     int borders_right = vmaf_floorn(width - (filter_width - radius), step);
+    convolution_clamp_borders(width, &borders_left, &borders_right);
 
     for (int i = 0; i < height; ++i) {
         for (int j = 0; j < borders_left; j += step) {
@@ -59,6 +73,7 @@ void convolution_y_c_s(const float *filter, int filter_width, const float *src,
     int radius = filter_width / 2;
     int borders_top = vmaf_ceiln(radius, step);
     int borders_bottom = vmaf_floorn(height - (filter_width - radius), step);
+    convolution_clamp_borders(height, &borders_top, &borders_bottom);
 
     for (int i = 0; i < borders_top; i += step) {
         for (int j = 0; j < width; ++j) {

core/src/feature/common/convolution.h

@@ -43,6 +43,16 @@ Filter widths above this one will not use the AVX path for convolutions.
 void convolution_f32_c_s(const float *filter, int filter_width, const float *src, float *dst,
                          float *tmp, int width, int height, int src_stride, int dst_stride);
 
+/* Scalar single-axis passes.  Exposed so the border/mirror regression test can
+ * drive them directly without going through the runtime SIMD dispatch in
+ * convolution_f32_c_s; they carry external linkage either way, so declaring
+ * them here also silences -Wmissing-prototypes. */
+void convolution_x_c_s(const float *filter, int filter_width, const float *src, float *dst,
+                       int width, int height, int src_stride, int dst_stride, int step);
+
+void convolution_y_c_s(const float *filter, int filter_width, const float *src, float *dst,
+                       int width, int height, int src_stride, int dst_stride, int step);
+
 /* AVX2 paths (256-bit, 8 floats per FMA). */
 void convolution_f32_avx_s(const float *filter, int filter_width, const float *src, float *dst,
                            float *tmp, int width, int height, int src_stride, int dst_stride);