-
Notifications
You must be signed in to change notification settings - Fork 80
fix(wgc): GPU DXGI encode path for Windows capture, with a fallback at every step #305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -607,6 +607,15 @@ int main(int argc, char* argv[]) { | |||||||||||||||||||||||||||||||||||||
| MFEncoderOptions encoderOptions{}; | ||||||||||||||||||||||||||||||||||||||
| encoderOptions.preferSoftwareEncoder = config.preferSoftwareEncoder; | ||||||||||||||||||||||||||||||||||||||
| encoderOptions.injectDefaultSinkWriterFailureOnce = injectDefaultSinkWriterFailureOnce; | ||||||||||||||||||||||||||||||||||||||
| // Keep the CPU path for software encoding and inline webcam PiP: both need | ||||||||||||||||||||||||||||||||||||||
| // the frame in system memory, which is the one thing the DXGI path does not | ||||||||||||||||||||||||||||||||||||||
| // produce. The env var is the escape hatch for a machine where the GPU path | ||||||||||||||||||||||||||||||||||||||
| // misbehaves in a way the encoder's own probes do not catch -- a support | ||||||||||||||||||||||||||||||||||||||
| // answer instead of a hotfix. | ||||||||||||||||||||||||||||||||||||||
| encoderOptions.useDxgiInput = | ||||||||||||||||||||||||||||||||||||||
| !config.preferSoftwareEncoder && | ||||||||||||||||||||||||||||||||||||||
| (!webcamActive || writeSeparateWebcam) && | ||||||||||||||||||||||||||||||||||||||
| readEnvInt("OPENSCREEN_WGC_DISABLE_DXGI_INPUT", 0) == 0; | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+610
to
+618
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
A recording with Use the configuration value that is already resolved at this point. 🐛 Proposed fix encoderOptions.useDxgiInput =
!config.preferSoftwareEncoder &&
- (!webcamActive || writeSeparateWebcam) &&
+ (!config.webcamEnabled || writeSeparateWebcam) &&
readEnvInt("OPENSCREEN_WGC_DISABLE_DXGI_INPUT", 0) == 0;
Verify with a webcam recording on real Windows hardware: CI runs only on Linux. Based on coding guidelines: "Native capture changes require a manual smoke test on real macOS or Windows, because CI runs only on Linux." 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| MFEncoder encoder; | ||||||||||||||||||||||||||||||||||||||
| if (!encoder.initialize( | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -622,15 +631,22 @@ int main(int argc, char* argv[]) { | |||||||||||||||||||||||||||||||||||||
| std::cerr << "ERROR: Failed to initialize Media Foundation encoder" << std::endl; | ||||||||||||||||||||||||||||||||||||||
| return 1; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| // `videoInput` reports what the encoder settled on, not what was asked for: | ||||||||||||||||||||||||||||||||||||||
| // it silently degrades to the CPU readback on any machine the GPU path does | ||||||||||||||||||||||||||||||||||||||
| // not fit, and a bug report that cannot tell the two apart is a bug report | ||||||||||||||||||||||||||||||||||||||
| // about the wrong path. | ||||||||||||||||||||||||||||||||||||||
| const bool usesDxgiInput = encoder.usesDxgiInput(); | ||||||||||||||||||||||||||||||||||||||
| std::cout << "{\"event\":\"encoder-selection\",\"schemaVersion\":2,\"video\":\"" | ||||||||||||||||||||||||||||||||||||||
| << encoder.videoEncoderSelection() | ||||||||||||||||||||||||||||||||||||||
| << "\",\"videoInput\":\"" << (usesDxgiInput ? "dxgi-nv12" : "cpu-rgb32") | ||||||||||||||||||||||||||||||||||||||
| << "\",\"preferSoftwareEncoder\":" | ||||||||||||||||||||||||||||||||||||||
| << (config.preferSoftwareEncoder ? "true" : "false") | ||||||||||||||||||||||||||||||||||||||
| << "}" << std::endl; | ||||||||||||||||||||||||||||||||||||||
| MFEncoder webcamEncoder; | ||||||||||||||||||||||||||||||||||||||
| if (writeSeparateWebcam) { | ||||||||||||||||||||||||||||||||||||||
| MFEncoderOptions webcamEncoderOptions = encoderOptions; | ||||||||||||||||||||||||||||||||||||||
| webcamEncoderOptions.injectDefaultSinkWriterFailureOnce = false; | ||||||||||||||||||||||||||||||||||||||
| webcamEncoderOptions.useDxgiInput = false; | ||||||||||||||||||||||||||||||||||||||
| const int webcamPixels = std::max(1, webcamCapture.width()) * std::max(1, webcamCapture.height()); | ||||||||||||||||||||||||||||||||||||||
| const int webcamBitrate = webcamPixels >= 1280 * 720 ? 8'000'000 : 4'000'000; | ||||||||||||||||||||||||||||||||||||||
| if (!webcamEncoder.initialize( | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -652,6 +668,10 @@ int main(int argc, char* argv[]) { | |||||||||||||||||||||||||||||||||||||
| CaptureControl control; | ||||||||||||||||||||||||||||||||||||||
| std::atomic<bool> firstFrameWritten = false; | ||||||||||||||||||||||||||||||||||||||
| std::atomic<bool> encodeFailed = false; | ||||||||||||||||||||||||||||||||||||||
| // Frames the GPU bridge was too busy to take. Reported at stop rather than | ||||||||||||||||||||||||||||||||||||||
| // per frame: a handful over a recording is normal contention, a stream of | ||||||||||||||||||||||||||||||||||||||
| // them is the next bug report, and neither is worth a log line each. | ||||||||||||||||||||||||||||||||||||||
| std::atomic<uint64_t> contendedFrames = 0; | ||||||||||||||||||||||||||||||||||||||
| Microsoft::WRL::ComPtr<ID3D11Texture2D> latestFrameTexture; | ||||||||||||||||||||||||||||||||||||||
| int64_t latestFrameTimestampHns = 0; | ||||||||||||||||||||||||||||||||||||||
| int64_t firstFrameTimestampHns = -1; | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -802,22 +822,40 @@ int main(int argc, char* argv[]) { | |||||||||||||||||||||||||||||||||||||
| std::this_thread::sleep_for(std::chrono::milliseconds(testStallReadbackMs)); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (latestFrameTexture) { | ||||||||||||||||||||||||||||||||||||||
| // captureVideoSample performs the GPU readback | ||||||||||||||||||||||||||||||||||||||
| // (CopyResource/Map) from latestFrameTexture, which must | ||||||||||||||||||||||||||||||||||||||
| // stay serialized (via `mutex`) against the WGC | ||||||||||||||||||||||||||||||||||||||
| // frame-arrival callback above, which writes new data | ||||||||||||||||||||||||||||||||||||||
| // into the same texture on another thread. | ||||||||||||||||||||||||||||||||||||||
| hasVideoSample = encoder.captureVideoSample( | ||||||||||||||||||||||||||||||||||||||
| latestFrameTexture.Get(), | ||||||||||||||||||||||||||||||||||||||
| frameTimestampHns, | ||||||||||||||||||||||||||||||||||||||
| !writeSeparateWebcam && webcamFrame.data ? &webcamFrame : nullptr, | ||||||||||||||||||||||||||||||||||||||
| videoSample); | ||||||||||||||||||||||||||||||||||||||
| if (!hasVideoSample) { | ||||||||||||||||||||||||||||||||||||||
| // Both entry points do their GPU work on latestFrameTexture, | ||||||||||||||||||||||||||||||||||||||
| // which must stay serialized (via `mutex`) against the WGC | ||||||||||||||||||||||||||||||||||||||
| // frame-arrival callback above, which writes new data into | ||||||||||||||||||||||||||||||||||||||
| // the same texture on another thread. Which one is live is | ||||||||||||||||||||||||||||||||||||||
| // the encoder's answer, not this struct's request: it falls | ||||||||||||||||||||||||||||||||||||||
| // back to the CPU path on its own when the GPU path does | ||||||||||||||||||||||||||||||||||||||
| // not fit the machine. | ||||||||||||||||||||||||||||||||||||||
| bool captured = false; | ||||||||||||||||||||||||||||||||||||||
| if (usesDxgiInput) { | ||||||||||||||||||||||||||||||||||||||
| captured = encoder.captureDxgiSample( | ||||||||||||||||||||||||||||||||||||||
| latestFrameTexture.Get(), | ||||||||||||||||||||||||||||||||||||||
| frameTimestampHns, | ||||||||||||||||||||||||||||||||||||||
| videoSample); | ||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||
| captured = encoder.captureVideoSample( | ||||||||||||||||||||||||||||||||||||||
| latestFrameTexture.Get(), | ||||||||||||||||||||||||||||||||||||||
| frameTimestampHns, | ||||||||||||||||||||||||||||||||||||||
| !writeSeparateWebcam && webcamFrame.data ? &webcamFrame : nullptr, | ||||||||||||||||||||||||||||||||||||||
| videoSample); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (!captured) { | ||||||||||||||||||||||||||||||||||||||
| encodeFailed = true; | ||||||||||||||||||||||||||||||||||||||
| control.requestStop(); | ||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| lastEncodedVideoTimestampHns = frameTimestampHns; | ||||||||||||||||||||||||||||||||||||||
| // The DXGI path returns success with no sample when the | ||||||||||||||||||||||||||||||||||||||
| // GPU bridge was momentarily busy. That costs one frame, | ||||||||||||||||||||||||||||||||||||||
| // which beats ending a recording that is otherwise fine. | ||||||||||||||||||||||||||||||||||||||
| hasVideoSample = videoSample != nullptr; | ||||||||||||||||||||||||||||||||||||||
| if (hasVideoSample) { | ||||||||||||||||||||||||||||||||||||||
| lastEncodedVideoTimestampHns = frameTimestampHns; | ||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||
| contendedFrames += 1; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
@@ -1080,8 +1118,12 @@ int main(int argc, char* argv[]) { | |||||||||||||||||||||||||||||||||||||
| // sleep still be killed. | ||||||||||||||||||||||||||||||||||||||
| if (stopElapsedMs() >= currentStepDeadlineMs.load() && !shutdownComplete.load()) { | ||||||||||||||||||||||||||||||||||||||
| const char* step = currentStopStep.load(); | ||||||||||||||||||||||||||||||||||||||
| // The encoder stage is what turns "video-writer-join was | ||||||||||||||||||||||||||||||||||||||
| // abandoned" into something actionable: it names the call the | ||||||||||||||||||||||||||||||||||||||
| // writer thread is sitting in, instead of leaving the next | ||||||||||||||||||||||||||||||||||||||
| // report to guess the way issue #252 had to. | ||||||||||||||||||||||||||||||||||||||
| std::cerr << "[stop-timing] step=" << step << " elapsed_ms=" << stopElapsedMs() | ||||||||||||||||||||||||||||||||||||||
| << " phase=abandoned" << std::endl; | ||||||||||||||||||||||||||||||||||||||
| << " phase=abandoned encode_stage=" << encoder.encodeStage() << std::endl; | ||||||||||||||||||||||||||||||||||||||
| std::cout << "{\"event\":\"stop-timeout\",\"schemaVersion\":2,\"step\":\"" << step | ||||||||||||||||||||||||||||||||||||||
| << "\"}" << std::endl; | ||||||||||||||||||||||||||||||||||||||
| std::cout.flush(); | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -1124,6 +1166,9 @@ int main(int argc, char* argv[]) { | |||||||||||||||||||||||||||||||||||||
| beginStopStep("video-writer-join", stepBudgetMs); | ||||||||||||||||||||||||||||||||||||||
| stopVideoWriter(); | ||||||||||||||||||||||||||||||||||||||
| logStopStep("video-writer-join"); | ||||||||||||||||||||||||||||||||||||||
| if (usesDxgiInput) { | ||||||||||||||||||||||||||||||||||||||
| std::cerr << "[frame-drops] gpu_bridge_contended=" << contendedFrames.load() << std::endl; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| // No frame lock here, and the ordering above is what makes that safe rather | ||||||||||||||||||||||||||||||||||||||
| // than incidental: stopVideoWriter() joined the only thread that calls into | ||||||||||||||||||||||||||||||||||||||
| // the encoder's GPU readback, and audioMixer->stop() joined the only other | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The "shared keyed-mutex texture" is not a fallback point. All three places state that the GPU path degrades to the CPU path when the shared keyed-mutex texture is unavailable. It does not.
MFEncoder::convertBgraTextureToNv12creates that texture on the first frame (electron/native/wgc-capture/src/mf_encoder.cpplines 941-994), afterinitialize()already configured the sink writer for NV12, so a failure there fails the recording. Either move the bridge creation intoinitializeDxgiPipeline()as proposed onelectron/native/wgc-capture/src/mf_encoder.cpplines 941-994, or correct all three statements.electron/native/README.md#L88-L88: remove "the shared bridge texture" from the list of automatic degrade conditions, or state that a bridge failure stops the recording.technical-documentation/architecture/recording.md#L72-L72: remove "no shared keyed-mutex texture" from the "degrades to the CPU one on its own at every step" list, or state the exception.electron/native/wgc-capture/src/mf_encoder.h#L32-L37: drop the claim that a driver which refuses shared keyed-mutex textures "records exactly as it did before the path existed", or qualify it.📍 Affects 3 files
electron/native/README.md#L88-L88(this comment)technical-documentation/architecture/recording.md#L72-L72electron/native/wgc-capture/src/mf_encoder.h#L32-L37🤖 Prompt for AI Agents