Skip to content

refactor(core): pilot C++23 conversion (metadata_handler) + migration research digest - #19

Merged
lusoris merged 1 commit into
masterfrom
refactor/cpp23-pilot-metadata-handler
May 28, 2026
Merged

refactor(core): pilot C++23 conversion (metadata_handler) + migration research digest#19
lusoris merged 1 commit into
masterfrom
refactor/cpp23-pilot-metadata-handler

Conversation

@lusoris

@lusoris lusoris commented May 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Converts core/src/metadata_handler.c to metadata_handler.cpp via git mv as the Phase 4 C++ internals modernization pilot (ADR-0708).
  • vmaf_metadata_destroy now uses std::unique_ptr<VmafCallbackList> with a custom CallbackListDeleter that walks and frees the linked-list nodes — replaces the manual traversal loop and guarantees cleanup on any future early-return path.
  • Ships Research-0732: full ROI-ranked survey of all core/src/*.c candidates + per-file migration recipe for Wave 1–3 follow-up PRs.

Changed files

File Change
core/src/metadata_handler.cpp (was .c) C++20 conversion with RAII unique_ptr teardown
core/src/metadata_handler.h extern "C" guards added (C callers unchanged)
core/src/meson.build Isolated metadata_handler_cpp20_lib static lib with override_options: ['cpp_std=c++20']
core/test/meson.build 25 .c → .cpp references updated
docs/adr/0708-vmafx-cpp23-internals-pilot.md Policy ADR
docs/research/0732-vmafx-cpp23-internals-migration-plan.md ROI survey + migration recipe

Netflix golden gate (post-conversion)

Run inside the worktree against /tmp/cpp23-pilot-build:

Pair 1 (src01 576x324):   VMAF mean = 76.6678  ✓ places=4 PASS
Pair 2 (CB 1-px shift):   VMAF mean = 35.0687
Pair 3 (CB 10-px shift):  VMAF mean = 7.9859

Score unchanged from master — the conversion touches zero float arithmetic.

Top-5 ROI candidates (from Research-0732)

# File ROI Key C++20 idiom
1 metadata_handler.cpp (this PR) 4.0 unique_ptr linked-list RAII
2 log.c 3.0 constexpr arrays, future std::format
3 mem.c 2.0 std::align_val_t / aligned operator new
4 opt.c 1.5 std::variant, concepts
5 fex_ctx_vector.c 1.0 std::vector, concepts

Prerequisites

  • Phase 4 umbrella ADR (C-ABI-preserving C++ modernization policy) must land before merge.

Deliverables checklist (ADR-0108)

  • Research digest: docs/research/0732-vmafx-cpp23-internals-migration-plan.md
  • Decision matrix: ADR-0708 ## Alternatives considered
  • AGENTS.md invariant: core/src/AGENTS.md §9 (extern "C" guard invariant)
  • Reproducer / smoke-test: meson setup /tmp/cpp23-pilot-build core -Denable_cuda=false -Denable_sycl=false && ninja -C /tmp/cpp23-pilot-build && meson test -C /tmp/cpp23-pilot-build --suite=fast (50/50 pass)
  • Changelog fragment: changelog.d/changed/0708-cpp23-internals-pilot.md
  • Rebase notes: docs/rebase-notes.md

PR checklist

  • No Netflix golden assertions modified
  • extern "C" in metadata_handler.h — all C callers unaffected
  • meson.build uses isolated static lib (cpp_std=c++20 does not leak)
  • docs/state.md updated (T-CPP23-INTERNALS-PILOT-2026-05-28 row)
  • ADR-0708 in docs/adr/README.md index
  • ffmpeg-patches: no change to public C API — not applicable
  • No user-discoverable surface changed — doc bar waived (internal refactor)

🤖 Generated with Claude Code

@lusoris
lusoris marked this pull request as ready for review May 28, 2026 11:37
@lusoris @claude
refactor(core): pilot C++23 conversion (metadata_handler) + migration…
f89f82f 
… research digest

Convert core/src/metadata_handler.c to metadata_handler.cpp as the Phase 4 C++
internals modernization pilot (ADR-0708). Research-0732 surveys all core/src/*.c
candidates and ranks them by ROI.

Pilot changes:
- git mv metadata_handler.c -> metadata_handler.cpp (preserves blame)
- vmaf_metadata_destroy: manual linked-list traversal replaced by
  std::unique_ptr<VmafCallbackList> with CallbackListDeleter RAII;
  guaranteed teardown on any future early-return path
- metadata_handler.h: extern "C" guards added so C callers
  (feature_collector.c) continue to link unchanged
- meson.build: compiled as isolated metadata_handler_cpp20_lib static lib
  with override_options ['cpp_std=c++20'], matching the Vulkan VMA precedent;
  project-wide cpp_std=c++11 is not affected
- core/test/meson.build: 25 .c -> .cpp references updated

Research deliverables:
- docs/research/0732-vmafx-cpp23-internals-migration-plan.md: full survey
  with ROI table for all 14 candidates; top-5 targets for Wave 1-3 PRs
- docs/adr/0708-vmafx-cpp23-internals-pilot.md: policy ADR with
  alternatives considered table

Netflix golden gate (post-conversion, all three reference pairs):
  Pair 1 (src01 576x324): 76.6678  [places=4 PASS]
  Pair 2 (CB 1-px shift): 35.0687
  Pair 3 (CB 10-px shift): 7.9859

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lusoris
lusoris force-pushed the refactor/cpp23-pilot-metadata-handler branch from 5f84338 to f89f82f Compare May 28, 2026 11:52
@lusoris
lusoris merged commit 7fe8cc9 into master May 28, 2026
21 of 53 checks passed
@lusoris
lusoris deleted the refactor/cpp23-pilot-metadata-handler branch May 28, 2026 11:52
@lusoris lusoris mentioned this pull request May 28, 2026
Merged
6 tasks done
@lusoris lusoris mentioned this pull request Sep 2, 2026
Closed
1 task
@lusoris lusoris added this to the 1.0.0 — First release milestone Sep 4, 2026
lusoris added a commit that referenced this pull request Sep 22, 2026
@lusoris
refactor(tune): split the subcommand drivers and stop discarding thei…
b019e77 
…r errors

vmafx-tune held the last of this slice's debt: six cobra constructors
and ten run/emit functions over the 60-line bound, ten discarded
errors, and one unbounded model-path walk.

Flag registration moves out of each constructor into a register*Flags
helper, which is where the bulk of every constructor was. The run
functions split into validate, execute and render stages along the
seams their own comments already marked, so argv order, JSON key
order, error strings and the order validations run in are untouched --
a request with several bad values still reports the same first
complaint.

The remaining `_ = cmd.MarkFlagRequired(...)` sites now call
markCommandFlagsRequired, the package's existing helper that turns a
wiring defect into an exit-2 validation error; fast.go and corpus.go
already used it. The scratch-file bookkeeping in the per-shot driver
reports its failures instead of dropping them: a probe that will not
close, a reference that will not delete and a workdir that will not
pre-create are each a step toward the disk-pressure failure ADR-0577
and ADR-0598 exist to contain, and none of them was visible before.

The saliency model walk is bounded by the starting path's depth, since
filepath.Dir removes one element per step and is a fixed point only at
the root.

Also records the changelog fragment for the whole cmd/** batch and the
invariant the MCP tool-registration split creates, appended as #19 so
no existing invariant number moves: a register* helper that
registerTools does not call registers nothing, and the parity test only
asserts every Python tool is present, so it would not catch it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.0.0 — First release

Development

Successfully merging this pull request may close these issues.

1 participant

@lusoris