fix(wgc): stop holding the frame mutex across blocking WriteSample calls

[EtienneLescot]

Summary

• Fixes [Bug]: Windows native recorder never stops after stdin stop command (v1.7.0-rc.2) #115: Windows native recorder never stops after the stdin stop command when preferSoftwareEncoder: true and system audio/mic/webcam/cursor are all disabled.
• Root cause: writeVideoFrames (main.cpp) held the shared frame-state mutex across IMFSinkWriter::WriteSample, which the main thread's stop-wait also locks to check control.stopRequested. With the software H.264 fallback, WriteSample is slow enough that the writer thread could keep re-acquiring the lock every frame faster than the main thread could win it after a stop request, starving the stop-wait — matching the report that no [stop-timing] line ever printed at all.
• Fix: split MFEncoder::writeFrame/writeBgraFrame into a captureVideoSample/captureBgraSample step (GPU readback + IMFSample construction, still under the shared mutex since it touches latestFrameTexture) and a submitVideoSample step (the blocking WriteSample call, only under the encoder's own low-contention writerMutex_). main.cpp's writeVideoFrames now calls submit outside the shared mutex.

Test plan

• Built wgc-capture.exe locally via MSVC (VS 18 Insiders) + CMake/Ninja — compiles cleanly, no new warnings.
• Ran the exact repro (display capture, captureSystemAudio/captureMic/captureCursor/webcamEnabled: false, preferSoftwareEncoder: true), sent stop over stdin after 4s: helper now exits ~97ms later with a valid non-zero MP4 (previously the process could hang indefinitely).
• Not independently reproduced on hardware matching the original report (Windows 10, Intel UHD 620 + NVIDIA MX150 hybrid laptop) — the hang did not occur even on the pre-fix build on this dev machine (Windows 11, different scheduler/hardware), so this fix addresses a confirmed code-level hazard (blocking I/O under a contended mutex) but the exact reproduction on the reporter's hardware is unverified. Recommend the reporter test this branch/build.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved video and webcam frame processing to reduce blocking during recording.
  • Increased capture stability by separating frame capture from encoding submission.
  • Improved failure handling so recording stops cleanly when frame processing encounters an error.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

MFEncoder pipeline

Layer / File(s)	Summary
Capture API and sample construction electron/native/wgc-capture/src/mf_encoder.h, electron/native/wgc-capture/src/mf_encoder.cpp	Replaces combined write methods with capture methods that create IMFSample objects and update timestamp state under writerMutex_.
Sink-writer submission electron/native/wgc-capture/src/mf_encoder.cpp	Adds submitVideoSample to validate encoder state and call IMFSinkWriter::WriteSample.
Frame loop integration electron/native/wgc-capture/src/main.cpp	Captures samples under mutex, submits them after releasing it, and updates failure handling to request stop and exit the loop.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

• [Bug]: Windows native recorder never stops after stdin stop command (v1.7.0-rc.2) #115 — The refactoring separates potentially blocking sink-writer submission from frame capture and lock ownership.

Possibly related PRs

• getopenscreen/openscreen#114 — Both changes modify the MFEncoder frame-writing and IMFSinkWriter transport flow.

Suggested reviewers: my-denia

Sequence Diagram(s)

sequenceDiagram
  participant FrameWriterLoop
  participant MFEncoder
  participant IMFSinkWriter
  FrameWriterLoop->>MFEncoder: Capture video/webcam samples while mutex is held
  MFEncoder-->>FrameWriterLoop: Return IMFSample objects
  FrameWriterLoop->>MFEncoder: Submit samples after releasing mutex
  MFEncoder->>IMFSinkWriter: WriteSample
  IMFSinkWriter-->>MFEncoder: Return write status
  MFEncoder-->>FrameWriterLoop: Report success or encode failure

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description has Summary and Testing, but it misses most required template sections like Type of change, Release impact, and Desktop impact.	Add the missing template sections, including Related issue, Type of change, Release impact, Desktop impact, Screenshots/video, and any remaining testing details.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: moving WriteSample calls off the shared frame mutex.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/openscreen-issue-115-eed2b4

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

electron/native/wgc-capture/src/mf_encoder.cpp (1)

613-630: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the duplicated timestamp-advance block into a private helper.

Lines 613-630 here are byte-for-byte identical to captureBgraSample (lines 676-693): same sampleDuration, same writerMutex_-guarded sinkWriter_/finalized_ check, and same firstTimestampHns_/lastTimestampHns_ advancement. In this platform-fragile recorder path, having the timestamp-monotonicity logic in two places risks silent divergence during future edits (e.g. only one copy gets a fix), which would corrupt output ordering.

Both functions are MFEncoder members operating on the same instance state, so a single private helper cleanly captures the contract.

♻️ Suggested helper (add declaration to mf_encoder.h)

// mf_encoder.h (private section)
bool beginVideoSampleTimestamp(int64_t timestampHns,
                               int64_t& outSampleTime,
                               int64_t& outSampleDuration);

// mf_encoder.cpp
bool MFEncoder::beginVideoSampleTimestamp(int64_t timestampHns,
                                          int64_t& outSampleTime,
                                          int64_t& outSampleDuration) {
    outSampleDuration = 10'000'000LL / fps_;
    std::scoped_lock writerLock(writerMutex_);
    if (!sinkWriter_ || finalized_) {
        return false;
    }
    if (firstTimestampHns_ < 0) {
        firstTimestampHns_ = timestampHns;
    }
    int64_t sampleTime = timestampHns - firstTimestampHns_;
    if (sampleTime <= lastTimestampHns_) {
        sampleTime = lastTimestampHns_ + outSampleDuration;
    }
    lastTimestampHns_ = sampleTime;
    outSampleTime = sampleTime;
    return true;
}

Then both capture methods reduce to a single if (!beginVideoSampleTimestamp(...)) return false; before the buffer/copy work.

As per coding guidelines, this is "security-sensitive recorder code" that "must be treated as" platform-fragile, so reducing duplicated correctness-critical logic is worthwhile.

🤖 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 `@electron/native/wgc-capture/src/mf_encoder.cpp` around lines 613 - 630,
Extract the duplicated timestamp initialization and monotonicity logic from
captureBgraSample and its neighboring capture method into a private MFEncoder
helper named beginVideoSampleTimestamp. Have the helper compute duration, lock
writerMutex_, validate sinkWriter_ and finalized_, update firstTimestampHns_ and
lastTimestampHns_, and return the sample time and duration through output
parameters; update both callers to use it and return false when it fails.

Source: Coding guidelines

🤖 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 `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 613-630: Extract the duplicated timestamp initialization and
monotonicity logic from captureBgraSample and its neighboring capture method
into a private MFEncoder helper named beginVideoSampleTimestamp. Have the helper
compute duration, lock writerMutex_, validate sinkWriter_ and finalized_, update
firstTimestampHns_ and lastTimestampHns_, and return the sample time and
duration through output parameters; update both callers to use it and return
false when it fails.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 35496426-79d0-4e8a-9979-03d5bfdebc63

📥 Commits

Reviewing files that changed from the base of the PR and between d5966ed and da61792.

📒 Files selected for processing (3)

• electron/native/wgc-capture/src/main.cpp
• electron/native/wgc-capture/src/mf_encoder.cpp
• electron/native/wgc-capture/src/mf_encoder.h