[blockie710]

VST3 Plugin - Code Review for PR #12752

Executive Summary

The implementation in PR #12752 by pkv adds a complete native VST3 host (obs-vst3) alongside the existing VST2 plugin (obs-vst). The code is well-structured, follows OBS conventions, and passes build verification on Windows (MSVC). This review identifies areas for improvement and potential issues.

---

Architecture Review

✅ Strengths

1. Clean separation of concerns: Host app, component holder, plugin core, scanner, editor window
2. Cross-platform UI abstraction: VST3EditorWindow with platform-specific implementations
3. Lock-free parameter transfer: ParameterChangeTransfer with Qt queued connections
4. Proper SDK integration: Compiles required SDK sources directly (no external deps)
5. Comprehensive sidechain support: With audio resampling for channel mismatch
6. State serialization: Chunk-based + parameter fallback
7. Caching: Plugin scan results cached to JSON for fast startup

⚠️ Areas for Improvement

1. Memory Management - Raw Pointers in VST3EditorWindow

File: VST3EditorWindow.cpp:132, 155

viewContainer_ = new QWidget(this);  // Qt parent ownership - OK
resizeLinuxTimer_ = new QTimer(this); // Qt parent ownership - OK

Assessment: Qt parent ownership handles cleanup, but explicit std::unique_ptr would be clearer for non-Qt objects.

2. Missing Virtual Destructors

File: VST3ComponentHolder.h:54

class VST3ComponentHolder : public IComponentHandler, public IUnitHandler {
    // Has virtual methods but no virtual destructor declared
    // Base classes (IComponentHandler, IUnitHandler) have virtual destructors via FUnknown

Assessment: Low risk - FUnknown provides virtual destructor chain, but explicit declaration recommended.

3. Exception Safety in Initialization

File: VST3Plugin.cpp:247-277

module = VST3::Hosting::Module::create(path, error);
if (!module) { ... return false; }
...
plugProvider = owned(new OBSPlugProvider(...));
if (plugProvider->setup(...) == false) {
    plugProvider = nullptr;  // Leak if setup fails after construction
}

Fix: Use std::unique_ptr with custom deleter or ensure cleanup on failure.

4. Thread Safety - GUI↔DSP Parameter Transfer

File: VST3Plugin.cpp:380-427

void VST3Plugin::drainDspToGui() {
    uiDrainScheduled.store(false, std::memory_order_release);
    while (dspToGui.getNextChange(id, value, sampleOffset)) {
        if (editController) {
            editController->setParamNormalized(id, value);  // Called from Qt thread
        }
    }
}

Assessment: The uiDrainScheduled atomic flag prevents re-entrancy. Good design.

5. Linux RunLoop Integration

File: RunLoopImpl.cpp

• Uses QSocketNotifier for FD events
• Uses QTimer for plugin timers
• Risk: No explicit handling of Qt event loop re-entrancy during plugin callbacks

6. Wayland Support Stub

File: obs-vst3.cpp:118-124

const auto platform = obs_get_nix_platform();
if (platform == OBS_NIX_PLATFORM_WAYLAND) {
    backend = VST3Backend::Wayland;  // Set but not implemented
}

Assessment: Correctly disabled in practice (no Wayland VST3s exist), but should have compile-time guard.

---

Code Quality Issues

Style/Formatting (Fixed by clang-format)

• ✅ Missing final newline in RunLoopImpl.h - FIXED
• ✅ Indentation consistency (tabs for C++, spaces for ObjC) - APPLIED

Static Analysis (cppcheck)

• ✅ Uninitialized member arrays copyBuffers/sidechainCopyBuffers - FIXED with = {}
• ⚠️ SDK headers have many "uninitialized member" warnings (not our code)
• ⚠️ uid.h:168 - "Reference to temporary returned" in SDK (not our code)

---

Functional Testing Gaps

1. Missing Automated Tests

No unit/integration tests for:

• Plugin scanner cache invalidation
• Sidechain resampling accuracy
• State serialization round-trip
• Parameter change transfer under load
• Multi-instance isolation

2. Edge Cases Not Tested

Scenario	Risk
Plugin crashes during process()	Host should survive
Plugin takes >1 block to initialize	Block size negotiation
Rapid parameter automation (1000+/sec)	Queue overflow
Sample rate change mid-stream	Reinitialization
Channel layout change (stereo→5.1)	Bus reconfiguration

---

Licensing Compliance ✅

• VST3 SDK v3.8.0+: MIT license (dual MIT/Proprietary)
• OBS Studio: GPLv2+
• Compatibility: MIT is GPL-compatible ✅
• Logo usage: Clarified by Steinberg (no longer mandatory) ✅
• Headers: All files have proper GPLv3 headers ✅

---

Build System Review

✅ CMake Integration

• FindVST3SDK.cmake properly locates SDK
• SDK_WARN_FLAGS suppresses SDK warnings
• Platform-specific source files via generator expressions
• macOS ARC enabled for .mm files
• Windows resource file included

⚠️ Flatpak Configuration

// build-aux/com.obsproject.Studio.json
"-DENABLE_VST3=OFF"  // Correctly disabled

Reason: Flatpak sandbox restricts SDK paths to /usr and $HOME. Correct decision.

---

Performance Considerations

Audio Thread Safety

• No allocations in process() ✅
• Lock-free parameter queues ✅
• Deque-based buffering for variable block sizes ✅
• Timestamp-based stream discontinuity detection ✅

Potential Optimization

// VST3Plugin.cpp:454 - Could avoid memcpy for in-place processing
tresult result = audioEffect->process(processData);
// Currently copies input→SDK buffers→output
// Could optimize for plugins supporting in-place

---

Recommendations

High Priority (Before Merge)

1. Add virtual destructor to VST3ComponentHolder
2. Fix exception safety in VST3Plugin::init() - use RAII for plugProvider
3. Add compile-time guard for Wayland backend (currently runtime only)

Medium Priority (Follow-up PR)

1. Add unit tests using cmocka framework (test/cmocka/)
2. Integrate Host Checker into CI
3. Add ASAN/Valgrind to CI for memory checking
4. Benchmark suite for performance regression detection

Low Priority (Nice to Have)

1. In-place processing optimization for compatible plugins
2. Wayland support when SDK matures
3. Plugin sandboxing (separate process) for crash isolation

---

Comparison with VST2 Plugin (obs-vst)

Aspect	obs-vst (VST2)	obs-vst3 (VST3)	Winner
API	Legacy C API	Modern C++ interfaces	VST3
Parameter automation	setParameter/getParameter	IEditController + IParameterChanges	VST3
Sidechain	Not supported	Full support	VST3
State	Chunk only	Chunk + parameter	VST3
Multi-bus	Single stereo	Multiple buses (limited to 1+1+1)	VST3
UI embedding	HWND/NSView/X11	Same + better resize	Tie
Latency reporting	Manual	IAudioProcessor::getLatencySamples()	VST3
MIDI	Not in OBS	API exists (unused)	Tie
Plugin scanning	Directory walk	moduleinfo.json cache	VST3

---

Verdict

Status: Ready for Review with minor fixes

The implementation is production-quality and follows OBS conventions. The three high-priority items are straightforward fixes. The VST3 plugin provides significant user value (sidechain, modern plugin support, better parameter handling) and correctly coexists with the legacy VST2 plugin.

Suggested Commit for Fixes

git commit -m "fix(vst3): address review feedback

- Add virtual destructor to VST3ComponentHolder
- Fix exception safety in VST3Plugin::init with RAII
- Add compile-time guard for Wayland backend
- Apply clang-format to all new files"

---

Testing Sign-off

Test Category	Status
Build (Windows MSVC)	✅ Pass
Build (Linux GCC/Clang)	⚠️ Pending
Build (macOS Xcode)	⚠️ Pending
Host Checker (SDK)	⚠️ Pending
Manual Plugin Matrix (8 plugins)	⚠️ Pending
Sidechain Functionality	⚠️ Pending
Memory Leak Check	⚠️ Pending

Run full test matrix before final approval

[Fenrirthviti]

Yikes.

[blockie710]

Apologies for the noise—I was testing build fixes and patches on top of pkviet's work (#12752) locally and accidentally opened this against master instead of keeping it in my own fork/reviewing #12752.

Definitely didn't intend to open a competing PR. Thanks for keeping the repo clean.