fix(feature): refuse a second float_adm debug instance and file the twins' debug key unsuffixed

CHANGELOG.md

@@ -3886,6 +3886,13 @@ make `core/AGENTS.md` a generated index over `AGENTS.d/` topic pages ([ADR-1454]
   unchanged.
 
 
+- A second `float_adm` instance with `debug=true` is refused when it is registered,
+  with a message naming the key `adm` both would write, instead of failing at the
+  first frame with "problem reading pictures". The unsuffixed `adm` key stays (the
+  Netflix tests read it); the CUDA, SYCL and HIP `float_adm` twins now file it
+  unsuffixed like the CPU, where they added the option suffix.
+
+
 - **`float_adm` refuses frames smaller than 17x17 instead of reading outside
   its buffers.** The float ADM extractor decomposes each frame into four
   wavelet levels. Below 17 pixels in width or height the coarsest level has a

changelog.d/fixed/float-adm-debug-key-refusal.md

@@ -0,0 +1,5 @@
+- A second `float_adm` instance with `debug=true` is refused when it is registered,
+  with a message naming the key `adm` both would write, instead of failing at the
+  first frame with "problem reading pictures". The unsuffixed `adm` key stays (the
+  Netflix tests read it); the CUDA, SYCL and HIP `float_adm` twins now file it
+  unsuffixed like the CPU, where they added the option suffix.

core/src/feature/AGENTS.d/float-adm-debug-key.md

@@ -0,0 +1,24 @@
+---
+paths:
+  - core/src/feature/float_adm.c
+  - core/src/feature/cuda/float_adm_cuda.c
+  - core/src/feature/sycl/float_adm_sycl.cpp
+  - core/src/feature/hip/float_adm_hip.c
+  - core/src/feature/metal/float_adm_metal.mm
+  - core/src/fex_ctx_vector.cpp
+  - core/test/float_adm_twin_parity.h
+  - core/test/test_float_adm_debug_key_refusal.c
+invariant: float_adm's debug ratio `adm` is never suffixed, on the CPU and every twin; a second debug instance is refused.
+---
+<!-- markdownlint-disable MD013 MD032 MD060 -->
+# `float_adm` debug key (ADR-2056)
+
+`float_adm` with `debug=true` files its ratio under the unsuffixed key `adm`; the Netflix
+golden tests read it under every option set, so it is a contract. The CPU extractor and the
+CUDA, SYCL, HIP and Metal twins list `adm_scale0` in `provided_features` (not `adm`, which
+would make the feature-name dictionary suffix it) and declare `.unsuffixed_debug_key = "adm"`.
+`feature_extractor_vector_append()` refuses a second context with the same declared key and
+`debug` set. A sync must not put `adm` back into a twin's `provided_features`, and must not
+rename the key. `test_float_adm_debug_key_refusal` and the option cases of
+`test_cuda_float_adm_parity` / `float_adm_twin_parity.h` (the unsuffixed `adm` closes every
+key list) guard it.

core/src/feature/AGENTS.md

@@ -41,6 +41,7 @@ PSNR, SSIM, MS-SSIM, LPIPS, …). Parent: [../../AGENTS.md](../../AGENTS.md).
 | `fastdvdnet_pre.c` | [fastdvdnet](AGENTS.d/fastdvdnet.md) | FastDVDnet 5-frame-window buffering and temporal pre-filter lifecycle contracts. |
 | `feature_collector.cpp`, `feature_collector.h` | [feature-collector](AGENTS.d/feature-collector.md) | feature_collector.cpp mount/unmount traversal and single-authority lifecycle contracts. |
 | `feature_extractor.cpp`, `feature_extractor.h` | [feature-registration](AGENTS.d/feature-registration.md) | feature_extractor_list[] is exactly-once and all extractors register in feature_extractor.cpp. |
+| `float_adm.c`, `cuda/float_adm_cuda.c`, `sycl/float_adm_sycl.cpp`, `hip/float_adm_hip.c`, `metal/float_adm_metal.mm`, `/core/src/fex_ctx_vector.cpp`, `/core/test/float_adm_twin_parity.h`, `/core/test/test_float_adm_debug_key_refusal.c` | [float-adm-debug-key](AGENTS.d/float-adm-debug-key.md) | float_adm's debug ratio `adm` is never suffixed, on the CPU and every twin; a second debug instance is refused. |
 | `float_adm.c`, `adm_tools.h` | [float-adm](AGENTS.d/float-adm.md) | Float ADM GPU exports, strict division (no reciprocal estimate), min dim 17x17, and signature stability. |
 | `feature_name.cpp`, `brisque_math.h` | [float-equality](AGENTS.d/float-equality.md) | Explicit floating-point comparison contracts replace direct equality tests. |
 | `float_moment_sum.h`, `float_moment_sum_gpu.h` | [float-moment-sum](AGENTS.d/float-moment-sum.md) | float_moment twins form the CPU's rounded 2nd-moment sum past 2^53 units; integers only, checked walk. |

core/src/feature/cuda/float_adm_cuda.c

@@ -967,7 +967,7 @@ static const char *provided_features[] = {
     "VMAF_feature_adm2_score", "VMAF_feature_adm_scale0_score", "VMAF_feature_adm_scale1_score",
     "VMAF_feature_adm_scale2_score", "VMAF_feature_adm_scale3_score",
     /* ADR-0574: AIM and ADM3 sub-features. */
-    "VMAF_feature_aim_score", "VMAF_feature_adm3_score", "adm", "adm_num", "adm_den",
+    "VMAF_feature_aim_score", "VMAF_feature_adm3_score", "adm_scale0", "adm_num", "adm_den",
     "adm_num_scale0", "adm_den_scale0", "adm_num_scale1", "adm_den_scale1", "adm_num_scale2",
     "adm_den_scale2", "adm_num_scale3", "adm_den_scale3", NULL};
 
@@ -981,6 +981,7 @@ VmafFeatureExtractor vmaf_fex_float_adm_cuda = {
     .options = options,
     .priv_size = sizeof(FloatAdmStateCuda),
     .provided_features = provided_features,
+    .unsuffixed_debug_key = "adm",
     .flags = VMAF_FEATURE_EXTRACTOR_CUDA,
 };
 

core/src/feature/feature_extractor.cpp

@@ -802,6 +802,23 @@ bool vmaf_feature_extractor_reads_shared_luma_only(const VmafFeatureExtractor *f
     return fex->reads_shared_luma_only(fex);
 }
 
+const char *vmaf_feature_extractor_context_debug_key(const VmafFeatureExtractorContext *fex_ctx)
+{
+    if (!fex_ctx || !fex_ctx->fex || !fex_ctx->fex->unsuffixed_debug_key)
+        return nullptr;
+    const VmafFeatureExtractor *fex = fex_ctx->fex;
+    if (!fex->priv || !fex->options)
+        return nullptr;
+    for (const VmafOption *opt = fex->options; opt->name; ++opt) {
+        if (strcmp(opt->name, "debug") != 0 || opt->type != VMAF_OPT_TYPE_BOOL)
+            continue;
+        bool debug = false;
+        memcpy(&debug, static_cast<const uint8_t *>(fex->priv) + opt->offset, sizeof(debug));
+        return debug ? fex->unsuffixed_debug_key : nullptr;
+    }
+    return nullptr;
+}
+
 int vmaf_feature_extractor_context_create(VmafFeatureExtractorContext **fex_ctx,
                                           const VmafFeatureExtractor *fex,
                                           VmafDictionary *opts_dict)

core/src/feature/feature_extractor.h

@@ -154,6 +154,13 @@ typedef struct VmafFeatureExtractor {
     size_t priv_size;               ///< sizeof private data.
     uint64_t flags;                 ///< Feauture extraction flags, binary or'd.
     const char **provided_features; ///< Provided feature list, NULL terminated.
+    /**
+     * Name of a score the extractor files without the option suffix when its
+     * `debug` option is set, or NULL. Two contexts that claim the same name
+     * would write one key twice, so vmaf_use_feature() refuses the second
+     * (ADR-2056). The unsuffixed name is a contract the Netflix tests read.
+     */
+    const char *unsuffixed_debug_key;
 
 #ifdef HAVE_CUDA
     VmafCudaState *cu_state; ///< VmafCudaState, set by framework
@@ -357,6 +364,13 @@ int vmaf_feature_extractor_context_create(VmafFeatureExtractorContext **fex_ctx,
                                           const VmafFeatureExtractor *fex,
                                           VmafDictionary *opts_dict);
 
+/**
+ * The unsuffixed debug key @p fex_ctx will write (VmafFeatureExtractor::
+ * unsuffixed_debug_key), or NULL when the extractor declares none or its
+ * `debug` option is off. Reads the options the context parsed at creation.
+ */
+const char *vmaf_feature_extractor_context_debug_key(const VmafFeatureExtractorContext *fex_ctx);
+
 int vmaf_feature_extractor_context_init(VmafFeatureExtractorContext *fex_ctx,
                                         enum VmafPixelFormat pix_fmt, unsigned bpc, unsigned w,
                                         unsigned h);

core/src/feature/float_adm.c

@@ -486,6 +486,7 @@ VmafFeatureExtractor vmaf_fex_float_adm = {
     .close = close_fex,
     .priv_size = sizeof(AdmState),
     .provided_features = provided_features,
+    .unsuffixed_debug_key = "adm",
 };
 
 /* NOLINTEND(modernize-use-nullptr) */

core/src/feature/hip/float_adm_hip.c

@@ -799,7 +799,7 @@ static const char *provided_features[] = {"VMAF_feature_adm2_score",
                                           "VMAF_feature_adm_scale3_score",
                                           "VMAF_feature_aim_score",
                                           "VMAF_feature_adm3_score",
-                                          "adm",
+                                          "adm_scale0",
                                           "adm_num",
                                           "adm_den",
                                           "adm_num_scale0",
@@ -826,6 +826,7 @@ VmafFeatureExtractor vmaf_fex_float_adm_hip = {
     .options = options,
     .priv_size = sizeof(FloatAdmStateHip),
     .provided_features = provided_features,
+    .unsuffixed_debug_key = "adm",
     .flags = VMAF_FEATURE_EXTRACTOR_HIP,
     .chars =
         {

core/src/feature/metal/float_adm_metal.mm

@@ -1020,6 +1020,7 @@ int close_fex_metal(VmafFeatureExtractor *fex)
     .options           = options,
     .priv_size         = sizeof(FloatAdmStateMetal),
     .provided_features = provided_features,
+    .unsuffixed_debug_key = "adm",
     .flags             = VMAF_FEATURE_EXTRACTOR_METAL,
     .chars = {
         .n_dispatches_per_frame = 5 * FADM_NUM_SCALES,

core/src/feature/sycl/float_adm_sycl.cpp

@@ -1093,7 +1093,7 @@ static const char *provided_features_float_adm_sycl[] = {"VMAF_feature_adm2_scor
                                                          "VMAF_feature_adm_scale3_score",
                                                          "VMAF_feature_aim_score",
                                                          "VMAF_feature_adm3_score",
-                                                         "adm",
+                                                         "adm_scale0",
                                                          "adm_num",
                                                          "adm_den",
                                                          "adm_num_scale0",
@@ -1120,6 +1120,7 @@ extern "C" VmafFeatureExtractor vmaf_fex_float_adm_sycl = {
     .priv_size = sizeof(FloatAdmStateSycl),
     .flags = VMAF_FEATURE_EXTRACTOR_SYCL,
     .provided_features = provided_features_float_adm_sycl,
+    .unsuffixed_debug_key = "adm",
 };
 
 } /* extern "C" */

core/src/fex_ctx_vector.cpp

@@ -88,6 +88,30 @@ int grow_context_vector(RegisteredFeatureExtractors *rfe)
     return 0;
 }
 
+/* ADR-2056: two contexts with `debug` set that claim the same unsuffixed key
+ * would write it twice ("cannot be overwritten" at the first frame). Refuse the
+ * second at registration, naming the key. */
+int refuse_debug_key_collision(const RegisteredFeatureExtractors *rfe,
+                               const VmafFeatureExtractorContext *fex_ctx)
+{
+    const char *key = vmaf_feature_extractor_context_debug_key(fex_ctx);
+    if (!key)
+        return 0;
+    for (unsigned i = 0; i < rfe->cnt; i++) {
+        const char *other = vmaf_feature_extractor_context_debug_key(rfe->fex_ctx[i]);
+        if (!other || strcmp(other, key) != 0)
+            continue;
+        vmaf_log(VMAF_LOG_LEVEL_ERROR,
+                 "feature extractor \"%s\": a second instance with debug=true would file its "
+                 "ratio under the key \"%s\", which instance \"%s\" already uses. The key is "
+                 "never suffixed with the options (the Netflix tests read it), so only one "
+                 "debug instance can run; set debug=false on the other.\n",
+                 fex_ctx->fex->name, key, rfe->fex_ctx[i]->fex->name);
+        return -EINVAL;
+    }
+    return 0;
+}
+
 void log_registered_context(const VmafFeatureExtractorContext *fex_ctx)
 {
     const unsigned cnt = fex_ctx->opts_dict ? fex_ctx->opts_dict->cnt : 0u;
@@ -137,6 +161,10 @@ int feature_extractor_vector_append(RegisteredFeatureExtractors *rfe,
         }
     }
 
+    const int collision = refuse_debug_key_collision(rfe, fex_ctx);
+    if (collision)
+        return collision;
+
     if (rfe->cnt >= rfe->capacity) {
         const int err = grow_context_vector(rfe);
         if (err)

core/test/float_adm_twin_parity.h

@@ -286,8 +286,8 @@ static inline mu_message_t adm_twin_check(const AdmTwin *twin, const AdmTwinCase
 }
 
 /* The seven scores under an option's feature-name suffix, and the sums
- * `debug=true` adds. The CPU files the debug ratio `adm` without the suffix,
- * so option cases leave it out. */
+ * `debug=true` adds. The debug ratio `adm` is filed without the suffix on the
+ * CPU and on every twin (ADR-2056), so option cases end with it unsuffixed. */
 #define ADM_TWIN_SCORE_KEYS(suffix)                                                                \
     "adm2" suffix, "aim" suffix, "adm3" suffix, "adm_scale0" suffix, "adm_scale1" suffix,          \
         "adm_scale2" suffix, "adm_scale3" suffix
@@ -304,8 +304,8 @@ static const char *const ADM_TWIN_DEBUG_KEYS[] = {
     ADM_TWIN_SUM_KEYS(""),
 };
 #define ADM_TWIN_NUM_DEBUG_KEYS (sizeof(ADM_TWIN_DEBUG_KEYS) / sizeof(ADM_TWIN_DEBUG_KEYS[0]))
-/* The seven scores and the ten sums of an option case. */
-#define ADM_TWIN_NUM_OPTION_KEYS 17u
+/* The seven scores and the ten sums of an option case, then the unsuffixed `adm`. */
+#define ADM_TWIN_NUM_OPTION_KEYS 18u
 #define ADM_TWIN_NUM_SCORE_KEYS 7u
 
 /* Default options with `debug=true`: every output, the per-scale sums
@@ -409,21 +409,22 @@ static inline mu_message_t adm_twin_1080p_exact(const AdmTwin *twin)
 static inline mu_message_t adm_twin_gain_limit_exact(const AdmTwin *twin)
 {
     static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_egl_1.2"),
-                                       ADM_TWIN_SUM_KEYS("_egl_1.2")};
+                                       ADM_TWIN_SUM_KEYS("_egl_1.2"), "adm"};
     return adm_twin_option(twin, "float_adm egl=1.2", ADM_TWIN_CONTRAST, "adm_enhn_gain_limit",
                            "1.2", keys);
 }
 
 static inline mu_message_t adm_twin_bypass_cm_exact(const AdmTwin *twin)
 {
-    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_bcm_1"), ADM_TWIN_SUM_KEYS("_bcm_1")};
+    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_bcm_1"), ADM_TWIN_SUM_KEYS("_bcm_1"),
+                                       "adm"};
     return adm_twin_option(twin, "float_adm bcm=1", ADM_TWIN_NOISE, "adm_bypass_cm", "1", keys);
 }
 
 static inline mu_message_t adm_twin_skip_aim_scale_exact(const AdmTwin *twin)
 {
-    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_sasc_1"),
-                                       ADM_TWIN_SUM_KEYS("_sasc_1")};
+    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_sasc_1"), ADM_TWIN_SUM_KEYS("_sasc_1"),
+                                       "adm"};
     return adm_twin_option(twin, "float_adm sasc=1", ADM_TWIN_NOISE, "adm_skip_aim_scale", "1",
                            keys);
 }
@@ -432,7 +433,8 @@ static inline mu_message_t adm_twin_skip_aim_scale_exact(const AdmTwin *twin)
  * 1e-10 denominator, and its score is reported as 0. */
 static inline mu_message_t adm_twin_skip_scale0_exact(const AdmTwin *twin)
 {
-    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_ssz"), ADM_TWIN_SUM_KEYS("_ssz")};
+    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_ssz"), ADM_TWIN_SUM_KEYS("_ssz"),
+                                       "adm"};
     const AdmTwinCase c = {.what = "float_adm ssz",
                            .w = FIXTURE_W,
                            .h = FIXTURE_H,
@@ -455,7 +457,7 @@ static inline mu_message_t adm_twin_skip_scale0_exact(const AdmTwin *twin)
 static inline mu_message_t adm_twin_view_dist_exact(const AdmTwin *twin)
 {
     static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_nvd_1.5"),
-                                       ADM_TWIN_SUM_KEYS("_nvd_1.5")};
+                                       ADM_TWIN_SUM_KEYS("_nvd_1.5"), "adm"};
     return adm_twin_option(twin, "float_adm nvd=1.5", ADM_TWIN_NOISE, "adm_norm_view_dist", "1.5",
                            keys);
 }
@@ -465,9 +467,9 @@ static inline mu_message_t adm_twin_view_dist_exact(const AdmTwin *twin)
 static inline mu_message_t adm_twin_weight_overrides_exact(const AdmTwin *twin)
 {
     static const char *const f1[] = {ADM_TWIN_SCORE_KEYS("_f1s0_0.5"),
-                                     ADM_TWIN_SUM_KEYS("_f1s0_0.5")};
+                                     ADM_TWIN_SUM_KEYS("_f1s0_0.5"), "adm"};
     static const char *const f2[] = {ADM_TWIN_SCORE_KEYS("_f2s2_1.75"),
-                                     ADM_TWIN_SUM_KEYS("_f2s2_1.75")};
+                                     ADM_TWIN_SUM_KEYS("_f2s2_1.75"), "adm"};
     mu_message_t msg =
         adm_twin_option(twin, "float_adm f1s0=0.5", ADM_TWIN_NOISE, "adm_f1s0", "0.5", f1);
     if (msg)
@@ -480,14 +482,16 @@ static inline mu_message_t adm_twin_weight_overrides_exact(const AdmTwin *twin)
  * only in Barten mode), and it files the scores under the CPU's `scf` alias. */
 static inline mu_message_t adm_twin_csf_scale_is_a_watson_mode_noop(const AdmTwin *twin)
 {
-    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_scf_2"), ADM_TWIN_SUM_KEYS("_scf_2")};
+    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_scf_2"), ADM_TWIN_SUM_KEYS("_scf_2"),
+                                       "adm"};
     return adm_twin_option(twin, "float_adm scf=2", ADM_TWIN_NOISE, "adm_csf_scale", "2.0", keys);
 }
 
 /* adm_p_norm = 1: the terms are the samples themselves, as powf(x, 1) is. */
 static inline mu_message_t adm_twin_p_norm_one_exact(const AdmTwin *twin)
 {
-    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_apn_1"), ADM_TWIN_SUM_KEYS("_apn_1")};
+    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_apn_1"), ADM_TWIN_SUM_KEYS("_apn_1"),
+                                       "adm"};
     return adm_twin_option(twin, "float_adm apn=1", ADM_TWIN_NOISE, "adm_p_norm", "1.0", keys);
 }
 
@@ -535,7 +539,8 @@ static inline mu_message_t adm_twin_p_norm_reaches_kernel(const AdmTwin *twin)
  * 1e-10 of that, zeroes the denominator and reports adm2 = 1. */
 static inline mu_message_t adm_twin_small_sums_are_not_floored(const AdmTwin *twin)
 {
-    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_nw_0"), ADM_TWIN_SUM_KEYS("_nw_0")};
+    static const char *const keys[] = {ADM_TWIN_SCORE_KEYS("_nw_0"), ADM_TWIN_SUM_KEYS("_nw_0"),
+                                       "adm"};
     const AdmTwinCase c = {.what = "float_adm isolated",
                            .w = 576u,
                            .h = 324u,

core/test/meson.build

@@ -5764,6 +5764,16 @@ test_float_adm_coverage = executable('test_float_adm_coverage',
 )
 test('test_float_adm_coverage', test_float_adm_coverage, suite : ['fast'])
 
+# ADR-2056 — a second float_adm instance with debug=true is refused at
+# registration, naming the key both would file.
+test_float_adm_debug_key_refusal = executable('test_float_adm_debug_key_refusal',
+    ['test.c', 'test_float_adm_debug_key_refusal.c'],
+    include_directories : [libvmaf_inc, test_inc],
+    link_with : get_option('default_library') == 'both' ? libvmaf.get_static_lib() : libvmaf,
+    dependencies : [stdatomic_dependency, pthread_dependency, gpu_all_deps],
+)
+test('test_float_adm_debug_key_refusal', test_float_adm_debug_key_refusal, suite : ['fast'])
+
 # ADR-1420 — the arithmetic float_adm_cuda's and float_adm_hip's kernels run
 # (src/feature/float_adm_gpu_common.h, through the CUDA spelling header
 # src/feature/cuda/float_adm/float_adm_device.h), compiled for the host and