fix: snap illegal AAC sample rates to 48 kHz on Windows - #2
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
The new Windows-native test executable has multiple early-return failure paths that can leave stray temp files, and there’s a newly introduced comment referencing an incorrect env var name.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adjusts the Windows WGC helper’s AAC audio path so WASAPI loopback/mic formats that report non-AAC-compatible PCM sample rates (e.g., 96 kHz) are snapped to 48 kHz before Media Foundation sink-writer configuration, preventing SetInputMediaType(audio) failures during recording start.
Changes:
- Introduces explicit AAC sample-rate snapping (keep known-good AAC rates; snap everything else to 48 kHz) in
makeAacCompatibleAudioFormat. - Adds Media Foundation–backed Windows-native test executable (
audio_sample_utils_test) and runs it as part of the WGC helper build script. - Adds test-only encoder options/env plumbing to optionally skip snapping and to probe the failing MF code path for diagnostics.
File summaries
| File | Description |
|---|---|
| scripts/build-windows-wgc-helper.mjs | Runs the new audio_sample_utils_test.exe after building to validate MF behavior. |
| electron/native/wgc-capture/src/mf_encoder.h | Adds test-only encoder options and threads options into audio-stream configuration. |
| electron/native/wgc-capture/src/mf_encoder.cpp | Applies rate snapping earlier (output-type build + input-type set), and adds an optional MF probe for invalid AAC rates. |
| electron/native/wgc-capture/src/main.cpp | Adds env-driven diagnostics to force/disable AAC rate snap and inject the probe; logs these in JSON events. |
| electron/native/wgc-capture/src/audio_sample_utils.cpp | Implements the AAC-compatible sample-rate snapping logic used by the encoder path. |
| electron/native/wgc-capture/src/audio_sample_utils_test.cpp | Adds a Windows-native test harness validating snap behavior, resampling, and actual MF acceptance/rejection of sample rates. |
| electron/native/wgc-capture/CMakeLists.txt | Builds and links the new audio_sample_utils_test executable against MF/COM libs. |
Review details
Suppressed comments (2)
electron/native/wgc-capture/src/audio_sample_utils_test.cpp:71
- trySetAacPcmRate() returns early on MFCreateMediaType(outputType) failure without deleting the temp output file path, which can leave a stray temp file on failure paths.
Microsoft::WRL::ComPtr<IMFMediaType> outputType;
hr = MFCreateMediaType(&outputType);
if (FAILED(hr)) {
return hr;
}
electron/native/wgc-capture/src/audio_sample_utils_test.cpp:125
- trySetAacPcmRateWithVideo() returns early on MFCreateMediaType(videoOut) failure without deleting the temp file path, which can leave stray files on failure paths.
Microsoft::WRL::ComPtr<IMFMediaType> videoOut;
hr = MFCreateMediaType(&videoOut);
if (FAILED(hr)) {
return hr;
}
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Microsoft::WRL::ComPtr<IMFSinkWriter> writer; | ||
| HRESULT hr = MFCreateSinkWriterFromURL(path.c_str(), nullptr, attributes, &writer); | ||
| if (FAILED(hr)) { | ||
| return hr; | ||
| } |
| Microsoft::WRL::ComPtr<IMFSinkWriter> writer; | ||
| HRESULT hr = MFCreateSinkWriterFromURL(path.c_str(), nullptr, nullptr, &writer); | ||
| if (FAILED(hr)) { | ||
| return hr; | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 569c7cb5a7
ℹ️ 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".
| if (!fs.existsSync(audioUtilsTestPath)) { | ||
| throw new Error(`WGC helper build completed but ${audioUtilsTestPath} was not found.`); | ||
| } | ||
| await run(audioUtilsTestPath, [], { cwd: BUILD_DIR }); |
There was a problem hiding this comment.
Running this executable unconditionally makes npm run build:native:win depend on the Media Foundation transforms installed on the build host, rather than only on compilation. On Windows N/KN or Server installations without the Media Feature Pack, the test's required 48 kHz AAC/H.264 setup fails; conversely, a registered third-party AAC transform that accepts 96 kHz makes the required rejection fail. Either case aborts an otherwise successful helper build, so this runtime codec probe should not be part of the mandatory packaging command unless it explicitly activates the intended transform or skips unsupported hosts.
Useful? React with 👍 / 👎.
| AudioInputFormat format{}; | ||
| format.subtype = MFAudioFormat_PCM; | ||
| format.sampleRate = source.sampleRate > 0 ? source.sampleRate : 48000; | ||
| format.sampleRate = aacCompatibleSampleRate(source.sampleRate); |
There was a problem hiding this comment.
When a 96 or 192 kHz WASAPI source is snapped here, AudioMixer::append now sends every packet through convertAudioWithGain, whose rate conversion selects one nearest source frame per output frame without a low-pass filter. Downsampling therefore aliases source energy above 24 kHz into the audible 48 kHz recording—for example, 30 kHz microphone noise becomes an 18 kHz tone—so the newly supported high-rate-device path can produce audible artifacts. Use an anti-aliased, stateful resampler when the rate changes.
Useful? React with 👍 / 👎.
| AudioInputFormat format{}; | ||
| format.subtype = MFAudioFormat_PCM; | ||
| format.sampleRate = source.sampleRate > 0 ? source.sampleRate : 48000; | ||
| format.sampleRate = aacCompatibleSampleRate(source.sampleRate); |
There was a problem hiding this comment.
This changes the Windows native capture and audio-encoding path, but the commit does not append a Windows capture-to-export result to technical-documentation/testing/manual-e2e-checklist.md. The repository requires that pass after every native-capture change and says the run does not count until its platform, build, coverage, and skipped checks are recorded, so add the corresponding results-log row before treating this fix as validated.
AGENTS.md reference: AGENTS.md:L84-L90
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff60bdb240
ℹ️ 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".
| targetFormat, | ||
| targetFrame, | ||
| channel, | ||
| (sum / static_cast<double>(factor)) * gain); |
There was a problem hiding this comment.
On 96/192 kHz devices with energy above the 24 kHz output Nyquist frequency, averaging only each 2- or 4-frame group is a box filter, not sufficient anti-alias filtering. For example, a 30 kHz component at 96 kHz retains about 56% amplitude through this average and then aliases to an audible 18 kHz component in the 48 kHz recording. The fresh follow-up replaces nearest-neighbor sampling with this box average, but that new code therefore does not resolve the previously reported aliasing; use a proper low-pass resampler before decimation.
Useful? React with 👍 / 👎.
| if (sourceFormat.sampleRate > targetFormat.sampleRate && | ||
| sourceFormat.sampleRate % targetFormat.sampleRate == 0) { | ||
| const UINT32 factor = sourceFormat.sampleRate / targetFormat.sampleRate; | ||
| const size_t targetFrames = sourceFrames / factor; |
There was a problem hiding this comment.
When a 96 kHz WASAPI packet has an odd frame count, or a 192 kHz packet has a count not divisible by four, this integer division discards the packet's remaining source frames. AudioMixer::append invokes this conversion independently for every GetBuffer result and retains no remainder or resampling phase, so repeated non-aligned packets lose samples and introduce discontinuities or accumulated timing error in the captured audio. Carry the unused frames into the next callback instead of truncating each packet independently.
Useful? React with 👍 / 👎.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Superseded by the upstream pull request getopenscreen#565 |
Summary
Windows WASAPI loopback often reports 96 kHz or 192 kHz. Those rates are legal PCM mix rates but not AAC encoder input rates, so Media Foundation SetInputMediaType fails with MF_E_INVALIDMEDIATYPE (0xc00d36b4). The capture helper now snaps illegal AAC rates to 48 kHz and leaves 8 / 11.025 / 12 / 16 / 22.05 / 24 / 32 / 44.1 / 48 kHz unchanged. The mixer already resamples when the source rate differs.
Related issue
Windows capture AAC sample-rate mismatch. This fork PR does not use a GitHub closing keyword.
Type of change
Release impact
Desktop impact
Screenshots / video
Not applicable (native capture helper).
Testing
On Windows with system audio: