Skip to content

feat(audio): renderer-bus feeder — song audio into engine output under exclusive mode (Phase 2)#828

Merged
byrongamatos merged 3 commits into
mainfrom
feat/renderer-bus-phase2
Jul 9, 2026
Merged

feat(audio): renderer-bus feeder — song audio into engine output under exclusive mode (Phase 2)#828
byrongamatos merged 3 commits into
mainfrom
feat/renderer-bus-phase2

Conversation

@OmikronApex

@OmikronApex OmikronApex commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Stacked on #824 (Phase 1) — merge that first; this PR then reduces to one commit.

Under exclusive-style output the Phase 1 native transport carries loose /audio/ songs and feedpak full-mixes, but not the stems plugin's multi-stem WebAudio graph or tracks JUCE hard-rejected. This adds _installRendererBusFeeder to static/app.js:

  • Captures the renderer-side master (stems plugin's published audioGraph, or the core <audio> element as fallback) with an AudioWorklet tap.
  • Re-points the owning AudioContext at a null sink (setSinkId({type:'none'})) so the graph keeps rendering while the device is exclusive-locked — analysers/visualizations stay live.
  • Pushes ~10 ms chunks over IPC into the desktop engine's renderer bus (feedBack-desktop feat/renderer-bus-phase2), mixed like a backing track (~10–20 ms added latency on song audio only; guitar path untouched).
  • Reuses feat(audio): route feedpak full-mix natively under WASAPI-exclusive output (Phase 1) #824's window._juceOutputIsExclusive predicate; inert in the Docker sphere and in shared mode (no double audio — covered by tests).

Validated by the fix12 tester spike (null-sink rendering works, clocks hold, drift → 0, zero overflow over 8 min) and shipped end-to-end in the fix13 tester build.

Companion PRs: feedBack-desktop feat/renderer-bus-phase2 (engine-side bus), feedBack-plugin-stems feat/publish-audio-graph (graph publication).

Tests: 7 new sandbox tests (tests/js/renderer_bus_feeder.test.js); full JS suite 955/955; #824's python suite 19/19 on the merged result.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Fixed silent audio in desktop exclusive-output modes by keeping playback active and properly routing audio to the system.
    • Improved handling when switching between exclusive and shared output, so audio now resumes and stops more reliably.
    • Added support for both live stem playback and song playback when the renderer needs to supply audio to the desktop engine.

OmikronApex and others added 2 commits July 9, 2026 19:33
Song playback runs through the renderer, which WASAPI-exclusive (and
ASIO) output silences. Route single-mix feedpaks (stem-less
original_audio packs AND single-stem packs) onto the engine's backing
transport when the output device type is exclusive-style, and migrate
back to HTML5 when it isn't. Extends /api/audio-local-path to resolve
/api/sloppak/.../file/... URLs via the same containment guards as
serve_sloppak_file. Multi-stem packs stay on the WebAudio path
(Phase 2). Includes [feedpak-route] transition-gated diagnostics
logging.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e output (Phase 2)

Under exclusive-style output the native backing transport (Phase 1, #824)
carries loose /audio/ songs and feedpak full-mixes, but not the stems
plugin's multi-stem WebAudio graph or tracks JUCE rejected. The feeder taps
the renderer-side master with an AudioWorklet, re-points the owning
AudioContext at a null sink so it keeps rendering without a device, and
pushes ~10 ms chunks over IPC into the desktop engine's renderer bus
(feedBack-desktop#90 follow-up). Inert in the Docker sphere and in shared
mode. Validated by the fix12 tester spike: null-sink rendering works,
clocks hold, no overflow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a renderer-side audio bus feeder in static/app.js that taps stems or core audio element output via AudioWorklet/AudioContext, redirects audio to a null sink in exclusive output modes, and pushes PCM chunks to the desktop engine bus. Includes a corresponding test suite validating engagement/disengagement behavior.

Changes

Renderer Bus Feeder

Layer / File(s) Summary
Feeder installation and engagement state machine
static/app.js
Adds _installRendererBusFeeder() IIFE creating an AudioWorklet tap, engaging either stems masterNode graph or core <audio> element via lazily created AudioContext, switching sink via setSinkId for exclusive vs shared output, and pushing ~10ms PCM chunks to api.pushRendererAudio while active; exposes window._reevaluateRendererBus.
Visibility/reevaluation scheduling
static/app.js
Wires periodic and visibility-change triggered reevaluation of engagement state and publishes the reevaluation hook on window.
Test harness and mocks
tests/js/renderer_bus_feeder.test.js
Extracts the feeder IIFE from source via brace-balanced slicing, builds a VM sandbox with faked AudioContext, DOM/audio element access, mocked feedBackDesktop.audio API, and a stems graph factory.
Engagement behavior test cases
tests/js/renderer_bus_feeder.test.js
Validates bus enable/disable and sink switching for stems-engaged exclusive/shared output, element-backed capture context creation, native transport (_juceMode) inactivity, mid-engagement graph replacement, and bus disable on engine stop.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

Possibly related PRs

  • got-feedBack/feedBack#824: Shares reliance on the window._juceOutputIsExclusive predicate, which the renderer bus feeder uses to determine engagement state.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the new renderer-bus feeder and exclusive-mode song-audio routing.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/renderer-bus-phase2

Comment @coderabbitai help to get the list of available commands.

@byrongamatos

Copy link
Copy Markdown
Contributor

Reviewed — ship-ready, one retarget.

The off/element/stems state machine is sound: _busy reentrancy guard, stemsGraphChanged re-engage, and disengage-restores-sink are all correct and the 7 sandbox tests cover exactly the tricky transitions (exclusive engage, return-to-shared disengage, native-transport/_juceMode inertness, element fallback, graph-swap, engine-stop). Namespace fallback (window.feedBack || window.slopsmith)?.stems?.audioGraph matches the stems plugin (which publishes on slopsmith) — no skew. The permanent createMediaElementSource reroute is a real one-way side effect but acknowledged, and sink-toggling keeps it transparent in shared mode.

Before merge: retarget to main. Base is feat/feedpak-exclusive-native-routing (#824's branch), which is already merged — this should collapse to one commit against main. CodeRabbit skipped this PR for the same feature-branch reason, so it's had no bot review; the tests are the safety net and CI needs to actually run once it's on main.

Minor (optional): the tap worklet postMessages every 128-frame quantum unconditionally — the active gate is on the main-thread receiver, so an initialized-but-inactive element tap keeps allocating + posting ~375×/s whenever _elCtx runs. Harmless, just avoidable; push the active flag into the worklet if it ever shows up in a profile.

@OmikronApex
OmikronApex changed the base branch from feat/feedpak-exclusive-native-routing to main July 9, 2026 20:37
@OmikronApex

Copy link
Copy Markdown
Contributor Author

Retargeted to main and synced — the diff is now exactly the feeder + its tests (static/app.js +203, tests/js/renderer_bus_feeder.test.js +195; the main merge picked up one post-#824 touch in static/capabilities/audio-effects.js as ancestry, not diff). Full JS suite 955/955 locally; CI should now run for real, and CodeRabbit can review against the default branch.

On the worklet nit (inactive tap still posting ~375×/s while _elCtx runs): agreed it's avoidable — pushing the active flag into the worklet via a port message is the fix. Deferring per your "if it ever shows up in a profile"; noted as a follow-up so it doesn't get lost. Worth remembering the exposure is bounded anyway: the element tap only exists after the first exclusive-mode engagement, and the stems tap's context is suspended whenever no song is loaded.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@static/app.js`:
- Around line 5455-5457: The reevaluate failure path in the renderer bus should
restore the sink state before forcing `_mode = 'off'`, because `_setSink(ctx,
true)` may already have disengaged the context and left `_disengage()` unable to
clean up on the next pass. Update the `catch` block in the reevaluation flow to
explicitly clean up/reset the sink or bus using the existing helpers around
`_setSink` and `_disengage` before changing `_mode`, so the context does not
remain stuck on `{type:'none'}` after a failed `reevaluate`.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7a8e53d0-19bf-4d32-a50c-f6455019a900

📥 Commits

Reviewing files that changed from the base of the PR and between 1b31780 and a732523.

📒 Files selected for processing (2)
  • static/app.js
  • tests/js/renderer_bus_feeder.test.js

Comment thread static/app.js
Comment on lines +5455 to +5457
} catch (e) {
console.warn('[renderer-bus] reevaluate failed (will retry):', e);
_mode = 'off';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

file=static/app.js
wc -l "$file"
echo '--- around _disengage / mode logic ---'
rg -n "_disengage|_setSink|_mode|reevaluate failed|want === 'off'|type:'none'|type: 'none'" "$file" | head -n 80
echo '--- lines 5360-5485 ---'
sed -n '5360,5485p' "$file"

Repository: got-feedBack/feedBack

Length of output: 6764


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '5368,5475p' static/app.js

Repository: got-feedBack/feedBack

Length of output: 4973


Restore the sink when reevaluate fails. If _setSink(ctx, true) already ran, clearing _mode here leaves _disengage() with nothing to do on the next pass, so the context can stay on {type:'none'} and audio remains muted until a successful re-engage. Clean up the sink/bus before setting _mode = 'off'.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@static/app.js` around lines 5455 - 5457, The reevaluate failure path in the
renderer bus should restore the sink state before forcing `_mode = 'off'`,
because `_setSink(ctx, true)` may already have disengaged the context and left
`_disengage()` unable to clean up on the next pass. Update the `catch` block in
the reevaluation flow to explicitly clean up/reset the sink or bus using the
existing helpers around `_setSink` and `_disengage` before changing `_mode`, so
the context does not remain stuck on `{type:'none'}` after a failed
`reevaluate`.

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