ViewBridge Phase 3: standalone, AU v2 (+ dual-Processor fix), AU v3, cross-format tests - #146
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bd9a2e919
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
danielraffel
force-pushed
the
viewbridge-phase2-phase3
branch
from
April 13, 2026 00:49
1086b03 to
796928e
Compare
danielraffel
added a commit
that referenced
this pull request
Apr 13, 2026
…(Codex P1 on PR #146) Codex flagged a use-after-free: the window close_callback called stop() which reset processor_, then run_event_loop() returned and we called bridge->close() on the now-dead Processor, dispatching on_view_closed(*processor_) through a dangling reference. Fix: move bridge->close() into the close_callback itself, before stop(), so on_view_closed fires while processor_ is still alive. run_event_loop() still returns on window close; the subsequent stop() after it is unchanged. bridge->close() is idempotent so the second invocation is a no-op.
Collaborator
Author
|
Codex P1 (use-after-free on |
danielraffel
added a commit
that referenced
this pull request
Apr 13, 2026
…(Codex P1 on PR #146) Codex flagged a use-after-free: the window close_callback called stop() which reset processor_, then run_event_loop() returned and we called bridge->close() on the now-dead Processor, dispatching on_view_closed(*processor_) through a dangling reference. Fix: move bridge->close() into the close_callback itself, before stop(), so on_view_closed fires while processor_ is still alive. run_event_loop() still returns on window close; the subsequent stop() after it is unchanged. bridge->close() is idempotent so the second invocation is a no-op.
danielraffel
added a commit
that referenced
this pull request
Apr 13, 2026
CI's 'Enforce version & skill sync' gate failed on PR #146 because the new .agents/skills/view-bridge/ directory had no corresponding entry in tools/scripts/skill_path_map.json — the check asserts every skill subdir is mapped. Also adds the existing mpe/ skill which the gate flagged at the same time. The view-bridge entry covers every source file the skill documents (processor hooks, all four adapter files + CLAP/VST3 headers, standalone, AU v2/v3, examples/view-bridge-demo). The mpe entry covers core/midi MPE + UMP sources and examples/mpe-*.
danielraffel
force-pushed
the
viewbridge-phase2-phase3
branch
from
April 13, 2026 04:17
f9357aa to
c989689
Compare
danielraffel
added a commit
that referenced
this pull request
Apr 13, 2026
…(Codex P1 on PR #146) Codex flagged a use-after-free: the window close_callback called stop() which reset processor_, then run_event_loop() returned and we called bridge->close() on the now-dead Processor, dispatching on_view_closed(*processor_) through a dangling reference. Fix: move bridge->close() into the close_callback itself, before stop(), so on_view_closed fires while processor_ is still alive. run_event_loop() still returns on window close; the subsequent stop() after it is unchanged. bridge->close() is idempotent so the second invocation is a no-op.
danielraffel
added a commit
that referenced
this pull request
Apr 13, 2026
CI's 'Enforce version & skill sync' gate failed on PR #146 because the new .agents/skills/view-bridge/ directory had no corresponding entry in tools/scripts/skill_path_map.json — the check asserts every skill subdir is mapped. Also adds the existing mpe/ skill which the gate flagged at the same time. The view-bridge entry covers every source file the skill documents (processor hooks, all four adapter files + CLAP/VST3 headers, standalone, AU v2/v3, examples/view-bridge-demo). The mpe entry covers core/midi MPE + UMP sources and examples/mpe-*.
danielraffel
force-pushed
the
viewbridge-phase2-phase3
branch
from
April 13, 2026 06:57
c989689 to
8bcf957
Compare
danielraffel
added a commit
that referenced
this pull request
Apr 13, 2026
…idge The version-bump gate flagged minor-bump-required for both surfaces because PR #146 ships: - SDK: new public API surface (ViewBridge, ViewSize, ViewRole, Processor::create_view / view_size / on_view_*, AU v2 kPulpEditorContextProperty, AU v3 -pulpProcessor / -pulpStore, ViewBridge::release_view / notify_attached / scripted_ui). - Plugin: new .agents/skills/view-bridge skill + CLI template hint for create_view(). Applied via tools/scripts/version_bump_check.py --mode=apply.
danielraffel
added a commit
that referenced
this pull request
Apr 13, 2026
… gate) PR #146's macOS ARM github-hosted CI failed because the new au_v2_adapter.hpp declarations brought <AudioUnitSDK/AUUtility.h> (which #includes <expected>) into pulp-format's TUs and downstream consumers. The ausdk target already requires PUBLIC cxx_std_23, but pulp-format only forwarded the link, not the compile-features requirement, so consumers compiled with the project default of C++20 and Apple clang 17 refused to expose std::expected. Adding 'target_compile_features(pulp-format PUBLIC cxx_std_23)' inside the PULP_HAS_AUSDK guard makes the C++23 requirement transitive: every TU that links pulp::format now compiles with -std=c++23 on Apple, matching what the AU headers need. Local: build clean, 2148/2148 ctest pass. Should unblock the macOS-ARM github-hosted gate.
…lease_view() Phase 3 standalone item from Feature 1 plan. - ViewBridge::release_view(): transfers unique_ptr<View> ownership to the caller while keeping a raw view_raw_ so the bridge continues to dispatch Processor::on_view_opened/closed/resized on the same instance. Required because TabPanel::add_tab needs a unique_ptr. - ViewBridge::view_raw_ + view_released_ state, view_count/view_at use the raw pointer, close() drops the unique_ptr (no-op if released), destructor path stays safe if caller respects the lifetime contract documented on release_view(). - standalone.cpp: construct ViewBridge with enable_hot_reload=true, open() → release_view() into TabPanel, notify_attached() after WindowHost creation, close() after event loop returns (fires on_view_closed while the TabPanel-owned view is still alive), use bridge->scripted_ui() / bridge->uses_script_ui() in place of the previous editor_ui.* fields. ctest: 2082/2082 pass (now includes the deferred-dispatch and failed-attach tests added in the P2 fix plus the standalone-through- bridge chain exercised transitively via existing standalone tests).
The AU v2 Cocoa view factory used to call registered_factory() to spin
up a second Processor for the UI. Parameter state only synced at view
construction time, so any audio-thread-side change drifted from the
UI's private Processor. Fixed by exposing the host's Processor +
StateStore via a new private AU property and reading that in the
Cocoa factory.
- core/format/include/pulp/format/au_v2_adapter.hpp: add
kPulpEditorContextProperty ('PuEd'), PulpEditorContext struct, and
GetPropertyInfo / GetProperty overrides.
- core/format/src/au_v2_adapter.cpp: implement the property handlers
(Global-scope, read-only) returning raw pointers to processor_ and
store_. Fall through to AUEffectBase for all other property IDs.
- core/format/src/au_v2_cocoa_view.mm: ownership struct shrinks to
{ ViewBridge, PluginViewHost } — no more duplicate Processor /
StateStore fields. uiViewForAudioUnit fetches the context via
AudioUnitGetProperty, constructs ViewBridge against the host
Processor, calls bridge->open → PluginViewHost::create →
bridge->notify_attached. The NSView dealloc path closes the bridge
so Processor::on_view_closed fires with the same view pointer that
on_view_opened saw.
ctest: 2082/2082 pass.
Phase 3 AU v3 item from Feature 1 plan. - core/format/src/au_adapter.mm: add Objective-C accessors -(pulp::format::Processor *)pulpProcessor and -(pulp::state::StateStore *)pulpStore on PulpAudioUnit so the view controller can pick up the same Processor + StateStore the audio callback uses (same pattern as the AU v2 kPulpEditorContextProperty fix; avoids a second Processor in the view path). - core/format/src/au_view_controller_ios.mm: rewrite to build a ViewBridge against the host Processor, open → PluginViewHost::create → bridge->notify_attached inside viewDidLoad; viewDidLayoutSubviews forwards to bridge->resize so on_view_resized fires; dealloc calls bridge->close so on_view_closed fires exactly once. Falls back to an empty View when self.audioUnit hasn't been set yet (Xcode preview case). ctest: 2082/2082 pass (iOS target builds but runs no tests on macOS — manual validation will land with the cross-format lifecycle test suite).
Adds a parameterised test that replays each adapter's call sequence against a single ViewBridge + StubProcessor and asserts the expected on_view_opened / on_view_resized / on_view_closed counts: - VST3-style: open → notify_attached → resize → close - CLAP-style: open → notify_attached (on gui_set_parent) → resize → close - AU v2-style: open → notify_attached → close (no resize in this path) - AU v3-style: open → notify_attached → resize × 2 → close - Standalone-style: open → release_view → notify_attached → resize → close, and verify the released unique_ptr still backs bridge.view() - Failed-attach: open → close fires zero callbacks (balanced teardown) 7 test cases, 67 assertions, all green. Full ctest suite still 2082/2082.
- tools/templates/gain/processor.hpp.template: add a commented-out create_view() / view_size() / on_view_* block after process(). No behaviour change for existing `pulp create` output; scaffolded plugins now discover the ViewBridge extension points inline instead of needing to dig through docs. - .agents/skills/view-bridge/SKILL.md: new skill covering when to touch ViewBridge, the open → notify_attached → resize → close protocol, release_view() ownership contract, the AU v2 dual-Processor gotcha and its fix, secondary-view roles, common pitfalls, and the full test roster.
…(Codex P1 on PR #146) Codex flagged a use-after-free: the window close_callback called stop() which reset processor_, then run_event_loop() returned and we called bridge->close() on the now-dead Processor, dispatching on_view_closed(*processor_) through a dangling reference. Fix: move bridge->close() into the close_callback itself, before stop(), so on_view_closed fires while processor_ is still alive. run_event_loop() still returns on window close; the subsequent stop() after it is unchanged. bridge->close() is idempotent so the second invocation is a no-op.
CI's 'Enforce version & skill sync' gate failed on PR #146 because the new .agents/skills/view-bridge/ directory had no corresponding entry in tools/scripts/skill_path_map.json — the check asserts every skill subdir is mapped. Also adds the existing mpe/ skill which the gate flagged at the same time. The view-bridge entry covers every source file the skill documents (processor hooks, all four adapter files + CLAP/VST3 headers, standalone, AU v2/v3, examples/view-bridge-demo). The mpe entry covers core/midi MPE + UMP sources and examples/mpe-*.
…idge The version-bump gate flagged minor-bump-required for both surfaces because PR #146 ships: - SDK: new public API surface (ViewBridge, ViewSize, ViewRole, Processor::create_view / view_size / on_view_*, AU v2 kPulpEditorContextProperty, AU v3 -pulpProcessor / -pulpStore, ViewBridge::release_view / notify_attached / scripted_ui). - Plugin: new .agents/skills/view-bridge skill + CLI template hint for create_view(). Applied via tools/scripts/version_bump_check.py --mode=apply.
… gate) PR #146's macOS ARM github-hosted CI failed because the new au_v2_adapter.hpp declarations brought <AudioUnitSDK/AUUtility.h> (which #includes <expected>) into pulp-format's TUs and downstream consumers. The ausdk target already requires PUBLIC cxx_std_23, but pulp-format only forwarded the link, not the compile-features requirement, so consumers compiled with the project default of C++20 and Apple clang 17 refused to expose std::expected. Adding 'target_compile_features(pulp-format PUBLIC cxx_std_23)' inside the PULP_HAS_AUSDK guard makes the C++23 requirement transitive: every TU that links pulp::format now compiles with -std=c++23 on Apple, matching what the AU headers need. Local: build clean, 2148/2148 ctest pass. Should unblock the macOS-ARM github-hosted gate.
Previous commit (8bcbf53) only set target_compile_features(... PUBLIC cxx_std_23), but CMake 3.24+ treats the global CMAKE_CXX_STANDARD (20) as authoritative, so the standard never actually rose to 23 for pulp-format sources and AU SDK's <expected> include still failed on clang that gates it on __cplusplus >= 202302L. set_target_properties(pulp-format PROPERTIES CXX_STANDARD 23) pins the standard explicitly per-target, and target_compile_features keeps the transitive-require contract for plugin-entry TUs that link pulp::format. Local build can't verify (PULP_HAS_AUSDK is FALSE here because the AudioUnitSDK clone is missing) — CI has the SDK populated and is the authoritative check.
danielraffel
force-pushed
the
viewbridge-phase2-phase3
branch
from
April 13, 2026 12:35
0c1d9d3 to
e235fa5
Compare
This was referenced Apr 13, 2026
danielraffel
added a commit
that referenced
this pull request
Apr 14, 2026
ViewBridge Phase 3 (#146) added au_v2_cocoa_view.mm to each plugin's ${target}_AU MODULE target via PulpUtils.cmake's pulp_add_plugin helper. The .mm compiles inside that AU target, not pulp-format, so it does NOT inherit pulp-format's `target_compile_features(... PUBLIC cxx_std_23)` PUBLIC setting — CMake only propagates compile features when the consumer links against the target, not when its sources are merely `add_library(... src1 src2 ...)`-ed in alongside. AUUtility.h in AudioUnitSDK 1.4 uses std::expected / std::unexpected (C++23). Apple clang only exposes those at -std=c++23. Result on github-hosted macos-14 runners (clang 17): every plugin AU target fails to build with 'no template named unexpected in namespace std'. Pin CXX_STANDARD=23 + CXX_STANDARD_REQUIRED=ON on ${target}_AU directly. Same rationale documented at core/format/CMakeLists.txt L76 for the pulp-format target.
danielraffel
added a commit
that referenced
this pull request
Apr 14, 2026
…pulp-view (#151) * Fix Android: force-keep TalkBack JNI symbols without whole-archiving pulp-view PR #132's P1 review never actually landed on main. accessibility_android.cpp defines all Java_com_pulp_accessibility_PulpAccessibilityDelegate_native* entry points inside pulp-view (static lib). Nothing inside libpulp.so references them — the only callers are on the Kotlin side (PulpAccessibility.kt) — so the Android linker is free to strip them under default dead-code elimination. At runtime TalkBack's first node-tree walk then hits UnsatisfiedLinkError. PR #148 tried to fix this by whole-archiving pulp-view. That pulled SDL3's libSDL3.a in transitively, and SDL3 defines its own JNI_OnLoad, colliding with core/platform/src/android/jni_bridge.cpp:41. #148 was closed. This PR uses the narrower `-Wl,--undefined=<symbol>` approach: force- reference each of the nine TalkBack JNI exports in pulp-jni's link options. The linker then keeps the accessibility_android.cpp object file alive without dragging any other sections of pulp-view (or its SDL3 transitive closure) into libpulp.so. No behaviour change on non-Android builds (the whole pulp-jni target is gated on ANDROID). On Android, the nine forced references grow libpulp.so by the single accessibility_android.cpp object file — same footprint PR #127 intended when it first landed TalkBack. * Fix macOS AU v2 plugin bundles: pin CXX_STANDARD=23 on ${target}_AU ViewBridge Phase 3 (#146) added au_v2_cocoa_view.mm to each plugin's ${target}_AU MODULE target via PulpUtils.cmake's pulp_add_plugin helper. The .mm compiles inside that AU target, not pulp-format, so it does NOT inherit pulp-format's `target_compile_features(... PUBLIC cxx_std_23)` PUBLIC setting — CMake only propagates compile features when the consumer links against the target, not when its sources are merely `add_library(... src1 src2 ...)`-ed in alongside. AUUtility.h in AudioUnitSDK 1.4 uses std::expected / std::unexpected (C++23). Apple clang only exposes those at -std=c++23. Result on github-hosted macos-14 runners (clang 17): every plugin AU target fails to build with 'no template named unexpected in namespace std'. Pin CXX_STANDARD=23 + CXX_STANDARD_REQUIRED=ON on ${target}_AU directly. Same rationale documented at core/format/CMakeLists.txt L76 for the pulp-format target. * Fix macOS AU v2 (follow-up): .mm files need OBJCXX_STANDARD=23 too CXX_STANDARD=23 alone doesn't apply to .mm sources — CMake treats them as Objective-C++ (OBJCXX), a separate language with its own OBJCXX_STANDARD property. Previous commit 1e3b4f9 only set the C++ standard, so au_v2_cocoa_view.mm still compiled at the default C++17 and AUUtility.h's std::expected still failed. Add OBJCXX_STANDARD=23 + OBJCXX_STANDARD_REQUIRED=ON alongside the existing CXX_STANDARD so both C++ and Objective-C++ TUs in ${target}_AU pick up the required standard.
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
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
Phase 3 format-adapter parity for ViewBridge. Stacked on top of #140 (review that first). Lands after #140 merges.
Adapters wired through ViewBridge
core/format/src/standalone.cpp): builds editor viaViewBridge, hands ownership to the TabPanel using the newViewBridge::release_view()API, firesnotify_attached()afterWindowHost::create, andclose()after the event loop returns — guaranteeson_view_closedfires before the TabPanel-owned view is destroyed.core/format/src/au_v2_cocoa_view.mm): fixes the dual-Processor bug. The Cocoa view factory used to callregistered_factory()and spin up a secondProcessorwhose parameters silently desynced from the audio thread'sProcessor. A new private AU propertykPulpEditorContextProperty(inau_v2_adapter.{hpp,cpp}) exposes the host-sideProcessor*+StateStore*; the view factory reads it and drives aViewBridgeagainst that single instance.core/format/src/au_adapter.mm,au_view_controller_ios.mm): new ObjC accessors-pulpProcessor/-pulpStoreonPulpAudioUnit. The iOS view controller buildsViewBridge, opens → attaches →notify_attachedinviewDidLoad, forwardsviewDidLayoutSubviewstobridge->resize, closes indealloc. Falls back to an empty View whenself.audioUnitisn't set (Xcode preview).New API
ViewBridge::release_view()— transfersunique_ptr<View>ownership to the caller while retaining a raw pointer for lifecycle dispatch (resize,notify_attached,close). Documented lifetime contract: caller must keep the released view alive untilclose()runs.Out-of-scope, documented in plan
clap_webview.cpp), not a nativeview::View. Re-enters ViewBridge via Phase 4 remote views.ViewBridge::attach_secondary_view(view, ViewRole::Inspector)primitive is already in ViewBridge: Processor editor lifecycle + VST3/CLAP wiring (Feature 1, Phase 1 + core Phase 2) #140. Driving the existing paint-onlyInspectorOverlaythrough it needs a smallinspect/refactor (on_active_changedcallback + View shell) tracked as a follow-up.Tests
test/test_view_bridge.cpp— new "cross-format lifecycle invariants" test replays each adapter's call sequence (VST3, CLAP, AU v2, AU v3, Standalone, failed-attach) against a singleViewBridgestub processor and assertson_view_opened/resized/closedcounts.pulp-test-view-bridge, all green.Test plan
cmake --build build -j\$(sysctl -n hw.ncpu)clean on macOS ARM64ctest --test-dir build --output-on-failure --exclude-regex AudioWorkgroup→ 2082/2082pulp-test-view-bridge— 7/7 cases, 67 assertionsDependencies
mainafter that.