Skip to content

fix(test): gate test_speed on enable_float to match speed.c compile guard#263

Merged
lusoris merged 1 commit intomasterfrom
fix/test-speed-chroma-registration
May 2, 2026
Merged

fix(test): gate test_speed on enable_float to match speed.c compile guard#263
lusoris merged 1 commit intomasterfrom
fix/test-speed-chroma-registration

Conversation

@lusoris
Copy link
Copy Markdown
Owner

@lusoris lusoris commented May 2, 2026

Summary

speed_chroma + speed_temporal live in libvmaf/src/feature/speed.c,
which the build only compiles when -Denable_float=true. Their
registration entries in feature_extractor.c are guarded by
#if VMAF_FLOAT_FEATURES. The test_speed test, however, was
unconditionally registered with meson — so a default
meson setup build libvmaf (which has enable_float=false by
default) returned NULL from vmaf_get_feature_extractor_by_name("speed_chroma")
and failed its first assertion.

CI sets -Denable_float=true everywhere, so this never tripped on
hosted runners; locally each PR landed with the failure documented as
"pre-existing on master" (e.g. PR #241, #260).

Fix

Wrap both the executable() and test() registrations in
if get_option('enable_float'), matching the pattern test_vulkan_*
uses for its feature gate.

Build flavour Behaviour before Behaviour after
Default (enable_float=false) test_speed runs and fails (NULL extractor lookup) test_speed is not in the test list
CI shape (enable_float=true) test_speed runs and passes (5/5) unchanged — still 5/5

Six-deliverable checklist (ADR-0108)

  • Research digest — no digest needed: trivial — single-config gate.
  • Decision matrix — no alternatives: only-one-way fix (mirror existing enable_vulkan gating).
  • AGENTS.md invariant note — no rebase-sensitive invariants — meson gate.
  • Reproducer / smoke-test command — see Test plan.
  • CHANGELOG fragment — no changelog needed: test-only change, no user-visible delta.
  • Rebase notedocs/rebase-notes.md entry 0097.

Test plan

# Default (enable_float=false): test_speed must NOT run
meson setup build libvmaf -Denable_cuda=false -Denable_sycl=false --reconfigure
ninja -C build
meson test -C build   # expect: 47/47 pass, no test_speed in the list

# CI shape (enable_float=true): test_speed must run + pass
meson setup build libvmaf -Denable_float=true --reconfigure
ninja -C build
meson test -C build test_speed  # expect: 5/5 pass
  • Default build: 47/47 pass (was 47/50 before).
  • CI build: test_speed 5/5 pass.
  • make pr-check BODY=… clean (deliverables gate self-test).

State.md update (CLAUDE.md §12 r13)

  • No bug close / open / not-affected ruling — pre-existing
    default-build failure, not a Netflix upstream report.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings May 2, 2026 08:03
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR fixes a default-build failure by gating the test_speed Meson executable and test registration behind the same enable_float build option that controls compilation of the underlying speed.c float-only extractors.

Changes:

  • Wrap test_speed executable definition in if get_option('enable_float').
  • Wrap test('test_speed', ...) registration in if get_option('enable_float').
  • Add a rebase note documenting the rationale and local/CI test commands.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
libvmaf/test/meson.build Gates test_speed build + registration on enable_float to avoid running it when float-only extractors aren’t compiled.
docs/rebase-notes.md Documents the gating change and provides re-test commands for rebase workflows.
Comments suppressed due to low confidence (1)

libvmaf/test/meson.build:1

  • The same condition is duplicated in two separate places, which increases the chance of drift (e.g., one guard gets updated but the other doesn’t). Consider defining a single enable_float = get_option('enable_float') variable once and reusing it (or using a single consistent predicate like enable_float.enabled()), so the gating logic remains synchronized.
if not get_option('enable_tests')

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread libvmaf/test/meson.build
# wrapped in `#if VMAF_FLOAT_FEATURES`. Gate the test on the same option
# so a default `meson setup` (enable_float=false) doesn't fail on a
# config that legitimately omits these extractors.
if get_option('enable_float')
Comment thread libvmaf/test/meson.build
libsvm_static_lib.extract_all_objects(recursive: true),
]
)
endif
Comment thread libvmaf/test/meson.build
test('test_mobilesal', test_mobilesal)
test('test_transnet_v2', test_transnet_v2)
test('test_cambi', test_cambi)
if get_option('enable_float')
Comment thread libvmaf/test/meson.build
test('test_mobilesal', test_mobilesal)
test('test_transnet_v2', test_transnet_v2)
test('test_cambi', test_cambi)
if get_option('enable_float')
…uard

speed_chroma + speed_temporal live in libvmaf/src/feature/speed.c, which
the build only compiles when -Denable_float=true. Their entries in the
feature_extractor.c registration list are wrapped in
`#if VMAF_FLOAT_FEATURES`, so vmaf_get_feature_extractor_by_name returns
NULL on a default build (enable_float=false) — the test then fails its
first assertion.

CI passes -Denable_float=true everywhere, so this never tripped on the
hosted runners; a default `meson setup build libvmaf` locally has been
returning 49/50 since the SpEED port (Netflix d3647c7 / PR #197) and
landing each PR documented the failure as "pre-existing on master".

Wrap both the executable() and test() registrations in
`if get_option('enable_float')`, matching the pattern test_vulkan_*
already uses for its feature gate. The test still runs on every CI
build; local default builds now report 47/47 instead of 47/50.

Six-deliverable checklist (ADR-0108):
- Research digest: no digest needed: trivial — single-config gate
- Decision matrix: no alternatives: only-one-way fix (mirror existing
  enable_vulkan gating pattern)
- AGENTS.md invariant note: no rebase-sensitive invariants — meson gate
- Reproducer / smoke-test command: see PR body Test plan
- CHANGELOG fragment: no changelog needed — test-only change, no
  user-visible delta
- Rebase note: docs/rebase-notes.md entry 0097

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lusoris lusoris force-pushed the fix/test-speed-chroma-registration branch from f466a74 to 5cd2e99 Compare May 2, 2026 08:24
@lusoris lusoris merged commit cb1d49c into master May 2, 2026
54 checks passed
@lusoris lusoris deleted the fix/test-speed-chroma-registration branch May 2, 2026 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants