Skip to content

fix(audio): refuse capture overrides the microphone cannot deliver - #4129

Merged
kixelated merged 2 commits into
mainfrom
quest/m1/audio-capture-format
Sep 25, 2026
Merged

kixelated merged 2 commits into
mainfrom
quest/m1/audio-capture-format

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Problem

capture::Config::sample_rate and channels were documented as hints, but capture::resolve wrote them straight over the device's default config. There was no check that the device supports them, and channels as u16 wrapped: a request for 65537 channels opened a mono stream. When a backend refused the format, the error named neither the device nor the requested format.

Approach

  • Overrides are now requirements. When a request sets one, resolve reads supported_input_configs(), and a new private negotiate picks a range with the exact channel count that contains the requested rate. It prefers the default's sample format, then cpal's cmp_default_heuristics, and only accepts formats the callback can convert (f32/i16/u16). Channel counts are compared as u32, so nothing gets narrowed before a match. If nothing matches, it fails with Error::Unsupported and names the device, the requested format, and the usable ranges. It never falls back to another format.
  • A request with no overrides skips enumeration, so opening the default device behaves as before, even on a host that cannot list its ranges.
  • If building or starting the stream fails, the error now includes the device and format. UnsupportedConfig and InvalidInput from cpal become Error::Unsupported. The retry classification is unchanged.
  • On macOS, system audio that delivers a different channel count than requested now reports Unsupported instead of Capture, so both sources follow the same contract (it was already fatal).
  • Updated the Config and Error::Unsupported docs to describe the actual contract.

Tests use fake cpal capability ranges, so no audio device is needed. They cover: an unsupported rate, 65537 channels, picking from the device's ranges, keeping the default format, explicitly requesting the default, a device with no usable format, and backend open failures (fatal unsupported vs retryable busy). Nightly's --all-features nextest runs them. #4125 adds a per-PR capture-test recipe that would pick them up too.

Impact

  • Public API: no signature changes. Behavior: a sample_rate/channels override the microphone does not support now fails with Error::Unsupported before the stream opens, where before it was passed through to the backend.
  • Wire: none.
  • Platform code: screencapture.rs (macOS) has a one-token error variant change and is not compiled locally.

Alternatives

  • Pick the closest supported format (what the old doc promised). Rejected by the quest: a silent substitution hides a misconfiguration, and the encoder resamples anyway, so None is the right way to ask for "whatever works".
  • Always enumerate ranges, even with no overrides. Rejected: it costs a host call per open and could break default opens on hosts with incomplete range reporting.

Follow-ups

  • None required.

Completes quest/m1/audio-capture-format.md.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

@kixelated
kixelated marked this pull request as ready for review September 25, 2026 12:28
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-25T21:52:52.948515Z 9cbe9ba New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 4e3232c2-209f-4612-b630-1fd6dc804608

📥 Commits

Reviewing files that changed from the base of the PR and between c9f0310 and 9cbe9ba.

📒 Files selected for processing (6)
  • quest/m1/README.md
  • quest/m1/audio-capture-format.md
  • quest/m2/capture-ergonomics.md
  • rs/moq-audio/src/capture.rs
  • rs/moq-audio/src/capture/screencapture.rs
  • rs/moq-audio/src/error.rs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b18eef93-2831-4ece-81a7-2c0f2b66e2cd

📥 Commits

Reviewing files that changed from the base of the PR and between 75d615d and c9f0310.

📒 Files selected for processing (6)
  • quest/m1/README.md
  • quest/m1/audio-capture-format.md
  • quest/m2/capture-ergonomics.md
  • rs/moq-audio/src/capture.rs
  • rs/moq-audio/src/capture/screencapture.rs
  • rs/moq-audio/src/error.rs
💤 Files with no reviewable changes (3)
  • quest/m1/README.md
  • quest/m2/capture-ergonomics.md
  • quest/m1/audio-capture-format.md

Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.


Walkthrough

Microphone sample rate and channel overrides now require an exact match with a writable device format. When neither override is set, capture uses the device default without enumerating supported ranges. Unsupported configurations return contextual errors. Microphone stream-opening errors include the device and requested layout. ScreenCaptureKit channel-count mismatches now return Error::Unsupported. The capture-format quest and its related links were removed.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to c9f03

No identified issue prevents merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to c9f03

Unsupported microphone requests may now fail where capture previously attempted them. The inspected path still checks microphone permission and selects input devices only; no expanded capture authority was identified. Device and caller compatibility remains the main uncertainty.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The inspected change affects callers requesting local audio capture formats, but does not establish a new route to another device class or an expanded capture privilege.

Trust Boundaries and Controls

  • observed — Caller-specified formats are checked against supported input configurations after microphone permission and input-device selection, rather than being used to skip either control.

Resilience and Maintainability Implications

  • inferred — Rejecting an unmatched format before stream creation limits an invalid request to a capture failure. The inspected build, start, and first-buffer paths do not publish a partially opened microphone.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: rejecting microphone capture overrides that the device cannot deliver.
Description check ✅ Passed The description directly explains the capture-format validation changes, error handling, tests, compatibility impact, and related macOS behavior.
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.
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@kixelated

Copy link
Copy Markdown
Collaborator Author

MERGE

Positive improvement. Overrides that used to be silent hints (and could wrap: channels as u16 turned 65537 into mono) are now requirements. negotiate enumerates only when an override is set, prefers the default sample format then cmp_default_heuristics, accepts only callback-writable formats (f32/i16/u16), compares channels as u32, and refuses with a message that names the device, the request, and the usable ranges — never a silent nearest-format substitute. Default opens still skip enumeration, so hosts with incomplete range reporting keep working.

Error hygiene matches the contract: open/start failures go through Failure::opening so UnsupportedConfig/InvalidInput become Error::Unsupported with device+layout context, while busy stays retryable. macOS system-audio channel mismatch now reports Unsupported like the mic path. Docs on Config and Error::Unsupported catch up to reality.

Worth the complexity. The negotiate helper is small, private, and covered by fake-range unit tests (unsupported rate, wrap-safe 65537 channels, default-format preference, no-usable-format, open classification) with no hardware needed. Rejecting "always enumerate" and "pick closest" in the alternatives section is the right call given the encoder already resamples and None means "whatever works".

Would do differently: nothing required. Optional follow-up only — if a host lies about ranges and then refuses the negotiated open, the new open-path Unsupported already surfaces it; no need to dual-check here.

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated
kixelated enabled auto-merge (squash) September 25, 2026 20:59
@kixelated
kixelated force-pushed the quest/m1/audio-capture-format branch from c9f0310 to 0d90063 Compare September 25, 2026 21:02
kixelated and others added 2 commits September 25, 2026 14:48
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated force-pushed the quest/m1/audio-capture-format branch from 0d90063 to 9cbe9ba Compare September 25, 2026 21:49
@kixelated
kixelated merged commit ca0dc74 into main Sep 25, 2026
3 checks passed
@kixelated
kixelated deleted the quest/m1/audio-capture-format branch September 25, 2026 22:08
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.

1 participant