Repository navigation
refactor(feature): bring adm.c to the lint and HISS standard (ADR-1142) - #1859
Merged
Merged
Conversation
…ard (ADR-1142) (#1853) * refactor(hip): bring the HIP runtime and host stubs to the lint standard (ADR-1142) Standards batch B5, HIP side: the clang-tidy debt the hip lane records for core/src/hip/ and core/src/feature/hip/, and the two findings of core/src/hip/stubs.c that every other lane records. - hip/kernel_template.c, hip/common.c: the stream and event handles are kept as uintptr_t (ADR-0241) and were converted back with integer-to-pointer casts (performance-no-int-to-ptr, 14 sites) and one const on a pointer typedef. They go through hip_handle.h now, the union the feature extractors already use. Same calls, same order, same streams and events. - hip/stubs.c: the ADR-1138 NOLINT bracket for NULL in a C translation unit (modernize-use-nullptr, 2). C keeps NULL: MSVC's C mode has no nullptr. - feature/hip/hip_hsaco_stubs.c: the stub macro's argument is a parenthesised declarator (bugprone-macro-parentheses). - feature/hip/speed_chroma_hip.c, speed_temporal_hip.c: braces. - feature/hip/float_adm_hip.c: a dead store in the build without hipcc, which the hip lane measures and its baseline did not record. Baselines, each by the ratchet's scoped write: hip 752 to 733, cpu 376 to 374, cuda 734 to 732, sycl 837 (stubs.c 2 to 0), arm64 606 to 604. No HISS row names these files. Behaviour: every HIP twin was swept before and after on a gfx1036 at --precision max (fourteen typical and stress fixtures and six small ones, every gate feature, float_adm with debug): 427 runs, 17 800 of 17 800 values identical to the run before. Suites unchanged: HIP build fast 256, gpu 72 and 1 skipped; CPU build fast 243; test_gpu_dispatch_runtime passes.
…int standard (ADR-1142) (#1854) * refactor(hip): bring the cambi and psnr_hvs device host code to the lint standard (ADR-1142) Standards batch B5, HIP side, part 2. The hip clang-tidy lane is configured without hipcc, so it analyses the -ENOSYS stubs of the HIP host files and not the bodies under HAVE_HIPCC that a device runs. A hipcc build measured 35 findings there, in two files, and the lane itself shows 11 in a device header that its baseline does not record. - integer_cambi_hip.c (24): the device arena was bound with sixteen casts through void * (bugprone-casting-through-void); one accessor, cambi_hip_arena_at(), returns the block and the assignment converts it. Kernel arguments are converted explicitly, a const leaves a pointer typedef, six statements get braces. - integer_psnr_hvs_hip.c (11): the kernel argument arrays convert their pointer-to-pointer elements explicitly. - integer_cambi/cambi_hip_device.h (11): the row and column offsets added to a pointer are widened before the multiplication (bugprone-implicit-widening-of-multiplication-result). Same offsets for every plane below 2^32 samples; the header's other indices are 32-bit as before. No baseline changes: the lane cannot see the first two, and the header was at 0 in the baseline. T-TIDY-RATCHET-GPU-LANES-UNREPRODUCIBLE-2026-09-22 records the blind spot. Behaviour: the sweep of every HIP twin recorded on the base was repeated on a gfx1036 at --precision max: 427 runs, 17 800 of 17 800 values identical. cambi takes 14.1 ms per 1920x1080 frame (15.0 before) and 84.6 ms per 3840x2160 frame (90.0 before); psnr_hvs is unchanged (51.3 and 51.4 ms at 3840x2160). HIP build: fast 256, gpu 72 and 1 skipped, as before.
…42) (#1856) * refactor(feature): bring the ADM headers to the lint standard (ADR-1142) adm_tools.h, adm_csf_tools.h, adm_options.h and integer_adm.h report no clang-tidy finding on the cpu, cuda, hip, sycl and arm64 lanes (145, 145, 145, 156 and 145 before). adm_tools.h had 142: 141 bugprone-macro-parentheses, all in upstream's nine ADM_CM_THRESH_S_* macros, and one portability-avoid-pragma-once. Nothing has expanded those macros since ADR-1141 moved the masking threshold into adm_tools.c::adm_cm_thresh3x3_s(), so they are removed rather than parenthesised: a kept macro is dead text that would absorb an upstream change without changing a score. The pragma goes; the include guard upstream also has stays. adm_csf_tools.h and adm_options.h lose the pragma. _USE_MATH_DEFINES is the name MSVC's and MinGW's <math.h> look for and keeps a cited suppression (ADR-1234). integer_adm.h is a C header a SYCL translation unit includes, so the C++-only checks it trips there (modernize-use- using, performance-enum-size, modernize-redundant-void-arg, modernize-use-designated-initializers) are suppressed in one cited block (ADR-1138), as its sibling headers do; one local becomes const. Not one bit moves. Every object file of an x86 GCC release build (155) and of an aarch64 GCC release build (138) is byte-identical before and after, as are the HIP host objects and the SYCL objects that include these headers. Every adm and float_adm output with debug=true and 21 option sets, and the model scores, at --precision max: 1695 of 1695 cases (271 962 values) identical on x86 for scalar, AVX2 and AVX-512, and 1066 of 1066 (127 704 values) on aarch64 under qemu for scalar and NEON. Netflix golden gate: 271 passed, 12 skipped on x86 GCC and on aarch64 GCC, before and after. The CUDA, HIP and SYCL twins pass their ADM tests and their adm and float_adm gate cells report 0. The header entries of the five tidy baselines are not tightened here: the ratchet's scoped write covers translation units only, and a full write on this host would record seven misc-static-assert false positives in core/tools/vmaf.cpp (T-TIDY-GLIBC-244-STATIC-ASSERT-FALSE-POSITIVE- 2026-10-02).
…ndard (ADR-1142) (#1858) * refactor(cuda): bring the CUDA runtime and host files to the lint standard (ADR-1142) Standards batch B5, CUDA side: every file under core/src/cuda/ and core/src/feature/cuda/ measures 0 in the cuda clang-tidy lane. - cuda/picture_cuda.c (5 recorded, 4 measured): the two CUDA_MEMCPY2D descriptors are initialised with their memory types as designators instead of {0}, which is no CUmemorytype enumerator (bugprone-invalid-enum-default-initialization); two braces. - cuda/picture_cuda.h, cuda/cuda_helper.cuh: include guards that are not reserved identifiers. - cuda/picture_cuda.c, picture_cuda.h, common.h, cuda_helper.cuh: the SPDX line of the licence their header states (BSD-2-Clause-Patent, Netflix-inherited, ADR-1250). - cuda/common.h (3), cuda/cuda_helper.cuh (1): clang-tidy reads these C headers under a C++ translation unit and proposes `using` and an anonymous namespace; C has neither. Cited NOLINT brackets (ADR-0141). - feature/cuda/integer_cambi_cuda.c (1): the extractor symbol is referenced by the registry; the tree's NOLINT for that (ADR-0278). - feature/cuda/integer_psnr_hvs_cuda.c (14, none recorded): the module load moves into psnr_hvs_load_module() (readability-function-size); the kernel argument arrays convert their pointer-to-pointer elements explicitly; the device address of the scratch buffer gets the cited NOLINT its neighbour has (ADR-0747); the plane loop is bounded by the size of the header's offset table, which the analyzer could not see. Baseline: cuda 734 to 728 by the scoped writer (picture_cuda.c 5 to 0, integer_cambi_cuda.c 1 to 0). The header entries (common.h 3, cuda_helper.cuh 2, picture_cuda.h 1, kernel_template.h 7) are 0 in a full run of the lane and stay in the file: the scoped writer tightens translation units only, and a full write is blocked by four files outside this batch that measure above their baseline. Behaviour: every CUDA twin was swept before and after on an RTX 4090 at --precision max (fourteen typical and stress fixtures and six small ones, every gate feature, float_adm with debug): 427 runs, 18 192 of 18 192 values identical to the run before. Suites unchanged: CUDA build fast 252, gpu 65 and 1 timeout (test_cuda_parity_gate_default_run, on the base too); CPU build fast 243; HIP build fast 256, gpu 72 and 1 skipped. Golden gate: 271 passed. T-GPU-LINT-SWEEP-HIP-CUDA-2026-09-16 carries the numbers of the whole batch.
…2) (#1859) * refactor(feature): bring adm.c to the lint and HISS standard (ADR-1142) compute_adm(), the float ADM driver, was one function of 260 lines with eight gotos and 31 clang-tidy findings. It keeps its name and signature and is a 59-line function over helpers now: - adm_frame_alloc() / adm_frame_free(): the band planes and the two index tables, in upstream's order, with upstream's stdout messages; every goto fail is a return and the frees happen once - adm_scale_dwt2(): the wavelet of one scale for both pictures - adm_scale_sums(): decouple, denominator, CSF, detail numerator, CSF, additive-impairment numerator, in upstream's order - adm_accumulate_scales(): the loop over the four scales and the four double accumulators Every statement that computes is carried over whole, with the type of every temporary: float per-scale sums added into double frame sums, aim_den before aim_num, the halving of w and h after the wavelet. The two ADM_OPT_DEBUG_DUMP blocks are removed: they call write_image() and PRINTF(), which nothing defines, so they could not compile. The casts through void in init_dwt_band*() are direct casts; NULL stays (ADR-1138); the discarded printf / fflush results are cast; adm.c includes its own header. clang-tidy: 31 to 0 on the cpu, cuda, hip, sycl and arm64 lanes, five baselines tightened by the ratchet's scoped write. float_adm.c already measured 0 on the hip, sycl and arm64 lanes, where its baseline still held 5, 5 and 4: tightened with it. HISS: nine rows of adm.c removed, 260 to 251. Not one bit moves. Every adm and float_adm output with debug=true and 21 option sets, and the model scores, at --precision max: 1695 of 1695 cases (271 962 values) identical on x86 for scalar, AVX2 and AVX-512, 1066 of 1066 (127 704 values) on aarch64 under qemu for scalar and NEON. One object file changes in each build, adm.c's own. Netflix golden gate: 271 passed, 12 skipped on x86 GCC and on aarch64 GCC, before and after. The GPU twins do not include this file. * docs: regenerate the indexes and the citation map after rebasing
Merged
12 of 26 tasks
lusoris
force-pushed
the
refactor/adm-c-standards
branch
from
October 2, 2026 14:55
94fcdf3 to
ae25063
Compare
This was referenced Oct 2, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second PR of standards batch B1 (the ADM family, plan in #1856):
core/src/feature/adm.cis at the lint and HISS standard, and no bit of any score moves.core/src/feature/adm.c, clang-tidycore/src/feature/float_adm.c, baseline entry (the file already measured 0)scripts/ci/tidy-baseline-<lane>.jsonHISS: the nine rows of
adm.care gone (compute_adm260 lines, and eightgoto):.standards-baseline.json260 → 251, recorded withpraetorctl baseline --record; the README count follows. UncitedNOLINTs: 0.What changed in
adm.ccompute_adm()keeps its name and its signature (adm.h,float_adm.cand the Cython harness see no difference) and is a 59-line function over helpers:adm_alloc_bands(),adm_alloc_indices(),adm_frame_alloc(),adm_frame_free()goto failbecame areturn, and the three frees happen once incompute_adm()adm_scale_dwt2()adm_dwt2_lowhen the scale is skipped)adm_scale_sums()adm_decouple,adm_csf_den_scale,adm_csf,adm_cm,adm_csf,adm_cm, in upstream's order with upstream's arguments (the options travel in one struct)adm_accumulate_scales()doubleaccumulatorscompute_adm()holds no kernel arithmetic, only call order and accumulation. Each statement that computes moved whole, with the type of every temporary:floatper-scale sums added intodoubleframe sums,aim_denbeforeaim_num,wandhhalved after the wavelet,numden_limitas written.The findings, by check:
bugprone-casting-through-voidinit_dwt_band*(); the planes are carved out of oneMAX_ALIGNbuffer inMAX_ALIGNstepsmodernize-use-nullptrNOLINTBEGIN/NOLINTEND(ADR-1138: a C translation unit keepsNULL)cert-err33-c(void)on theprintf/fflushof the allocation messages, asms_ssim.cdoesreadability-isolate-declarationbugprone-assignment-in-if-conditionbugprone-implicit-widening-of-multiplication-result(size_t)row_bytes * 4misc-use-internal-linkageadm.cincludesadm.hreadability-function-sizeOne suppression in the file, the ADR-1138 block.
Removed: the two
#ifdef ADM_OPT_DEBUG_DUMPblocks. They callwrite_image()andPRINTF(), which nothing in the tree defines, so they could not compile; the macro stays commented out inadm_options.h.core/src/feature/AGENTS.mdlistedcompute_admamong the functions that stay unsplit. That note is updated: the function has no arithmetic to split, and the rule it stood for (a helper boundary never cuts an expression) is kept.Not one bit moves
Scores. Every
admandfloat_admoutput withdebug=trueunder 21 option sets and the scores of the default model and ofvmaf_v0.6.1, at--precision max, against the record taken from master513d2a6fc:The option sets reach every argument
compute_adm()passes on:adm_enhn_gain_limit,adm_csf_mode1, 2, 5 and 9,adm_p_norm2.5 and 4,adm_bypass_cm, viewing distance and display height, noise weight and both CSF scales,adm_adm3_apply_hm/adm_dlm_weight/adm_min_val, four band weights,adm_skip_aim_scaleandadm_skip_scale0.Object code. One object file differs from the base build in each of the x86 (155 objects) and aarch64 (138) builds:
feature_adm.c.o.Netflix golden gate. x86-64 GCC: 271 passed, 12 skipped before and after. aarch64 GCC under qemu: 271 passed, 12 skipped before and after.
Unit tests.
--suite=faston the CPU build: 243 of 243.Cython harness.
compat/python-vmaf/core/adm_dwt2_cy.pyxtext-includesadm.cforinit_dwt_band_d(); that function keeps its signature and the three files it includes still compile together.GPU twins. None includes
adm.corfloat_adm.c; nothing to re-run. (refactor(feature): bring the ADM headers to the lint standard (ADR-1142) #1856 covers the shared headers.)scripts/dev/preflight.sh --stage msvcism: pass.Type
feat— new featurefix— bug fixperf— performance improvementrefactor— no behavior changedocs— documentation onlytest— test-onlybuild/ci— tooling / infraport— cherry-pick from upstream Netflix/vmafsycl/cuda/simd— backend-specificChecklist
make format && make lintis green locally. (clang-format and the commit hooks; clang-tidy 0 for the file on five lanes.)python3 scripts/ci/run_meson_test.py -- -C build. (--suite=faston a CPU build: 243 of 243.)/cross-backend-diffand the worst ULP is ≤ 2. (No SIMD or GPU file is touched; the dispatch paths were compared, see above.).c/.cpp/.cu/.h/.hpp, it has the appropriate license header (seeCONTRIBUTING.md). (None added; the SPDX line ofadm.cadded.)!orBREAKING CHANGE:and the migration path is documented below. (Not breaking.)docs/adr/_index_fragments/<NNNN-slug>.md. (No ADR: ADR-1142 is the rule.)Bug-status hygiene (ADR-0165)
docs/state.mdupdated in this PR. no state delta: refactor to the lint and HISS standard; no row names this file's debt.Netflix golden-data gate (ADR-0024)
assertAlmostEqual(...)score in the Netflix golden Python tests.Deep-dive deliverables (ADR-0108)
AGENTS.mdinvariant note —core/src/feature/AGENTS.md, "Split helpers must not split an expression":compute_admleaves the unsplit list, with its helper map and the orders that must not change.changelog.d/changed/adm-c-lint-hiss-standard.md.docs/rebase-notes.md, "compute_adm()is split into helpers; its debug-dump blocks are gone", with the map from upstream's statements to the helpers.Reproducer
Known follow-ups