fix(main): permission handler was denying whole-app loopback capture - #100
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughElectron display capture now grants only available screen sources with main-frame audio, denies invalid requests, and logs outcomes. Renderer media permissions deny video requests while allowing non-video requests, with centralized diagnostic logging and existing origin restrictions retained. ChangesDesktop capture and runtime diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant PermissionHandler
participant DisplayMediaHandler
participant desktopCapturer
participant MainWindow
Renderer->>PermissionHandler: request media permission
PermissionHandler-->>Renderer: allow non-video or deny video
Renderer->>DisplayMediaHandler: request display media
DisplayMediaHandler->>desktopCapturer: enumerate screen sources
desktopCapturer-->>DisplayMediaHandler: return sources
DisplayMediaHandler->>MainWindow: provide screen video and main-frame audio
DisplayMediaHandler-->>Renderer: deny when window or sources are unavailable
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
getDisplayMedia (renderer-bus whole-app loopback capture) rides the 'media' permission request with EMPTY mediaTypes. The audio-only rule denied it, so the display-media handler never ran and exclusive/ASIO output lost all page audio (song previews, element-song fallback). Allow media requests unless they explicitly ask for 'video' (camera stays blocked; getDisplayMedia video is the app's own frame), mirror the policy in the permission-check handler, and add [asio-diag] logs to every deny path plus the display-media handler so future denials name their stage in tester logs. Verified packaged: display-media granted → renderer-bus engaged, engine busEnabled/busFlowing=true on ASIO. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ef0c988 to
bcbb0e3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/audio/AudioEngine.h (1)
624-634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment claims
destis cleared first, but implementation never clears it.The comment says drains "into
dest(stereo, bus gain applied, dest cleared first)", butpullRendererBus's implementation (AudioEngine.cpp) never callsdest.clear(). On the success path this is harmless since the write loop overwrites every sample in[0, numSamples). On the gated/early-return paths (disabled, priming, underflow, undersized scratch),destis left with whatever stale data it held from the previous block — safe today only because every call site additionally gates onrendererFrames > 0before readingrendererBusPullScratch. If a future caller reads the buffer without that gate, this comment would incorrectly imply it's safe to do so.📝 Suggested comment fix
// Shared consumer step for the duplex and split output paths: drain one // block from the renderer-bus ring into `dest` (stereo, bus gain applied, - // dest cleared first). Returns numSamples on success, 0 when gated + // dest fully overwritten on success, left untouched on the gated paths + // below — callers must check the return value before reading `dest`). + // Returns numSamples on success, 0 when gated // (disabled, priming, underflow, scratch undersized). Single consumer —🤖 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 `@src/audio/AudioEngine.h` around lines 624 - 634, Update the documentation for pullRendererBus and rendererBusPullScratch to remove the claim that dest is cleared first, or revise it to accurately describe the current behavior on gated and early-return paths. Keep the implementation unchanged unless needed to make the documented contract accurate.
🤖 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.
Nitpick comments:
In `@src/audio/AudioEngine.h`:
- Around line 624-634: Update the documentation for pullRendererBus and
rendererBusPullScratch to remove the claim that dest is cleared first, or revise
it to accurately describe the current behavior on gated and early-return paths.
Keep the implementation unchanged unless needed to make the documented contract
accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a5030cae-ea7f-4b19-87f5-94c663578a60
📒 Files selected for processing (5)
src/audio/AudioEngine.cppsrc/audio/AudioEngine.hsrc/main/audio-bridge.tssrc/main/main.tssrc/main/preload.ts
Problem
Tester report (2026-07-11, Focusrite USB ASIO): song-preview audio silent / on the wrong device with ASIO output. Log showed
[renderer-bus] loopback capture unavailable — NotAllowedErrorwith no display-media handler activity.Root cause:
getDisplayMedia(the renderer-bus whole-app loopback capture, #98) arrives atsetPermissionRequestHandleras amediarequest with emptymediaTypes. The audio-only rule (empty ⇒ deny) rejected it beforesetDisplayMediaRequestHandlerever ran. Loopback went sticky-unavailable; song_preview's private<audio>element has no surface-tap fallback → previews played to the default WASAPI endpoint.Fix
mediapermission requests unless they explicitly include'video'(camera stays blocked — getDisplayMedia's video is the app's own frame; real camera requests always carrymediaTypes:['video']).setPermissionCheckHandler.[asio-diag]logging on every permission deny path and in the display-media handler (received / granted / DENIED with reason) so any future denial names its stage in tester logs.Verification
Packaged portable build (fix16), ASIO4ALL:
Tester confirms preview audio now on ASIO + streamer WASAPI output.
Companion core PR: raw-audio capture constraints (tin-can quality fix) in feedBack.
🤖 Generated with Claude Code
Summary by CodeRabbit