Repository navigation
Conversation
|
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 configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⬇️ Low Change: Bug fix Merge Risk: ⚪ Minimal · up to This change makes Windows non-integer audio resampling consistent across packet boundaries. It also resets interpolation history when a starved source resumes. No concrete defect remains open. The main residual caveat is that the native tests are not exercised by CI. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
描述包含详细的缺陷说明、实现范围、测试结果和限制，但没有按模板提供 Type of change、Release impact、Desktop impact 等必需章节。描述还使用了“Fixes #920”，同时说明本 PR 只覆盖连续性子问题，未明确表示完整解决该问题。 |
按模板补充 Summary、Related issue、Type of change、Release impact、Desktop impact、Screenshots / video 和 Testing 章节。将“Fixes #920”改为“Refs #920”或“Related to #920”，除非本 PR 完整解决 issue #920。 |
✅ Passed checks (3 passed)
-
-
Check name Status Explanation Title check 标题准确描述了主要改动：修复 Windows 非整数重采样的跨数据包连续性。标题没有使用模糊表述，也没有声称已经发布或完全修复。 Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
-
✨ Finishing Touches
-
-
🧪 Generate unit tests (beta)
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
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. |
![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: c5a5a0f613
ℹ️ 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".
c5a5a0f to
5764aa2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
5764aa2 to
2ae4dcc
Compare
fix(windows): preserve non-integer resampling continuity across packets
Fixes the Windows capture helper's audio path for issue getopenscreen#920, item 24 (study issue; this PR is the continuity slice only). Upstream-candidate staging PR: single clean commit on the latest upstream/main, carrying everything the earlier fork review PR (#23) verified — its 6-commit history was review scaffolding, not the submission shape.
The defect
convertAudioWithGain's non-integer-rate branch kept no cross-packet state: every packet recomputed its output-frame count withllround(packetFrames × rateRatio)and restarted interpolation phase at zero. The same signal produced different output depending only on packet boundaries — deterministic counterexample at the patch's base commit (mono float32→PCM16, oneAudioDecimatorStateper stream, asAudioMixerowns them):Upside: exact-multiple packet geometry — including the standard 10 ms/480-frame WASAPI packet — was already correct; the defect bites on non-aligned packet geometry (non-10 ms endpoint periods, partial first packets, odd rates). The integer-factor decimation path (96/192k→48k) already carried cross-packet state upstream and is untouched.
The change (4 files)
electron/native/wgc-capture/src/audio_sample_utils.{h,cpp}+audio_sample_utils_test.cpp, plus a real-machine results row intechnical-documentation/testing/manual-e2e-checklist.md:K(N)=max(1,round½↑(N·T/S)),C(N)=min(K,⌈(N−1)T/S⌉)— output counts never depend on the partition; checked counters fail safe.finalizeAudioConversion: append-only tail emission so the pre-existing stateless one-shot overload keeps its exact whole-stream contract through ONE conversion path (it is the stateless wrapper's internal step; the streaming/production path never finalizes — its documented bounded tail is dropped at reset/stop, and that behavior is pinned by tests).friend struct AudioMixerTestAccess.--resample-continuity-onlyfocused mode and anOS920_PRISTINE_REGRESSIONbuild switch that compiles the new tests against pristine sources to produce assertion-based old-FAIL.Evidence
2896c1f7; Windows-local MSVC builds — GitHub CI has no native C++ job and does not build or run the wgc-capture suite);fc /bbyte controls — one-shot outputs and B/C packetized outputs identical old-vs-fixed; A/D packetized differ exactly by the fix.OPENSCREEN_WGC_FORCE_AAC_SOURCE_RATEsurface; old vs fixed decoded output is time-aligned at zero offset, 97.4 % byte-identical with residual maxdiff 170 (AAC block-boundary quantization), each build repeating byte-identically. This hardware cannot produce the non-aligned packet geometry the defect class needs, so the old-FAIL/new-PASS defect proof remains the deterministic suites; the real pass evidences end-to-end no-regression on the non-integer path. Aliasing on non-integer downsampling (case E: 23 kHz→21.1 kHz alias survives) is a known limitation, deliberately out of scope. Starvation-resume state continuity is fixed and behavior-tested (deterministic mixer-path tests with integer/same-rate controls); on real hardware only the startup empty-queue padding (~70 ms) was observed — a full starve-with-history-then-resume event was not reproduced outside the deterministic tests.Base branch note
This PR's base is the auxiliary branch
base/os920-upstream-2896c1f7pinned at upstream/main2896c1f7b9ad46cf53419e6edd360da2d2d80ac3(the latest tip at delivery time), so the diff shows exactly the four intended files with no drift (forkmainis 400+ commits behind upstream — deliberately untouched). The three audio files' blobs are identical between this base and the earlier validated basea997ce76(upstream did not touch them in between); the checklist row is applied on top of upstream's own newer checklist edits. One commit, no fork-only CI scaffolding.Not in this PR
Anti-alias filtering for non-integer downsampling; mixer clock/cushion changes; anything from getopenscreen#1076. Cross-platform (macOS/Linux) audio paths don't share this code.