fix(windows): preserve WGC duration through static frames#551
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds MFEncoder::extendLastFrameToLocked to repeat the last buffered NV12 frame across intermediate timestamps (with readiness checks and fps-based durations); writeFrame now calls this helper and returns false if extension fails. Also updates wgc-capture helper binary metadata in the Windows manifest. ChangesFrame Extension Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
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 `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 255-256: The code computes frameDurationHns using fps_ before
validating fps_, which will divide by zero if fps_ == 0; update the logic around
the computation that sets frameDurationHns so you first check/guard fps_ (e.g.,
if (fps_ <= 0) return false;) then compute const int64_t frameDurationHns =
10000000LL / fps_; and keep the existing check for frameDurationHns <= 0 if
still needed; reference fps_ and frameDurationHns when making this change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 085d32e8-5967-4c53-9ce5-4b3e427b01bc
⛔ Files ignored due to path filters (1)
electron/native/bin/win32-x64/wgc-capture.exeis excluded by!**/*.exe
📒 Files selected for processing (3)
electron/native/bin/win32-x64/helpers-manifest.jsonelectron/native/wgc-capture/src/mf_encoder.cppelectron/native/wgc-capture/src/mf_encoder.h
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
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 `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 255-257: The code only checks fps_ when padding frames, but
writeNv12SampleLocked() also computes frameDurationHns = 10000000LL / fps_
(risking divide-by-zero) and initialize() currently doesn't validate fps_; add
validation of fps_ in initialize() to ensure fps_ > 0 (or set a safe default)
and/or add a guard at the start of writeNv12SampleLocked() to return an error if
fps_ <= 0; update any callers (e.g., extendLastFrameToLocked()) to rely on this
invariant so frameDurationHns computation is always safe.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c27185c9-d5ba-4904-ae6a-32f4abf5a36c
⛔ Files ignored due to path filters (1)
electron/native/bin/win32-x64/wgc-capture.exeis excluded by!**/*.exe
📒 Files selected for processing (2)
electron/native/bin/win32-x64/helpers-manifest.jsonelectron/native/wgc-capture/src/mf_encoder.cpp
✅ Files skipped from review due to trivial changes (1)
- electron/native/bin/win32-x64/helpers-manifest.json
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
wgc-capture.exehelper manifest and staged helper binary from a temp CMake build.Why
Refs #375. Users are reporting recordings that are shorter in the editor/export, with one signal pointing at static screen sections. WGC can stop delivering frames while the captured scene is unchanged; if the MP4 only receives sparse samples, the editor can see a shorter effective timeline. This keeps the encoded video timeline continuous without touching the broader export/NVIDIA path.
Validation
cmake -S electron/native/wgc-capture -B %TEMP%/recordly-wgc-build-* -G "Visual Studio 17 2022" -A x64cmake --build %TEMP%/recordly-wgc-build-* --config Releasenpx tsc --noEmitnpm test -- src/hooks/useScreenRecorder.test.ts src/lib/mediaTiming.test.tsgit diff --checkKnown unrelated baseline:
npm test -- electron/ipc/recording/diagnostics.test.ts src/hooks/useScreenRecorder.test.tscurrently has one diagnostics expectation mismatch ongetCompanionAudioFallbackPathswhere code returns the.system.wavsidecar but the test expects only video + mic.Summary by CodeRabbit
Bug Fixes
Refactor
Chores