Skip to content

Commit 5af7d53

Browse files
committed
fix(wgc): give the audio write its own breadcrumb instead of the video's
An adversarial review of 4d1a0cc caught this, and three independent passes landed on it separately: naming the audio write was right, putting it in encodeStage_ was not. encodeStage_ is a single slot. Almost all of the video thread's stages -- the whole DXGI bridge sequence in convertBgraTextureToNv12 and captureDxgiSample -- are set with no MFEncoder lock held; only submitVideoSample's stamp sits under writerMutex_. So the previous commit's claim that "both stamps are serialized by the mutex they sit under" was true of the two WriteSample stamps and of nothing else. The consequence ran backwards from the intent. writeAudio runs on the audio-mixer thread, which emits roughly every 10 ms and ends each call by storing "idle". A video thread wedged in bridge-copy therefore had its breadcrumb erased within milliseconds, and the watchdog line -- the single piece of evidence this instrumentation exists to produce -- would have printed encode_stage=idle for precisely the hang it was added to identify. Worse on the system-audio configuration than anywhere else, which is the configuration reproducing #252 most consistently. One slot per writing thread. encodeStage_ is single-writer again (the video thread), audioStage_ belongs to the mixer, and the abandoned-step line prints both. A report showing audio_stage=write-audio next to a video stage stuck on a bridge call now says the two are contending for writerMutex_, which is a thing no shared slot could have expressed. Verified at runtime on the previous commit: a 10-minute DXGI recording (16,177 frames, gpu_bridge_contended=0) and a 15-second one both stopped cleanly, so the surrounding path this touches is exercised, not just compiled.
1 parent ef2d59d commit 5af7d53

3 files changed

Lines changed: 31 additions & 3 deletions

File tree

electron/native/wgc-capture/src/main.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1130,8 +1130,15 @@ int main(int argc, char* argv[]) {
11301130
// abandoned" into something actionable: it names the call the
11311131
// writer thread is sitting in, instead of leaving the next
11321132
// report to guess the way issue #252 had to.
1133+
// Both threads are named, because either can be the one that is
1134+
// stuck and each has its own slot: encode_stage is the video
1135+
// writer, audio_stage the mixer. A report showing audio_stage
1136+
// parked on write-audio while encode_stage sits at a bridge
1137+
// call says the two are fighting over writerMutex_, which no
1138+
// single-slot breadcrumb could ever have shown.
11331139
std::cerr << "[stop-timing] step=" << step << " elapsed_ms=" << stopElapsedMs()
1134-
<< " phase=abandoned encode_stage=" << encoder.encodeStage() << std::endl;
1140+
<< " phase=abandoned encode_stage=" << encoder.encodeStage()
1141+
<< " audio_stage=" << encoder.audioStage() << std::endl;
11351142
std::cout << "{\"event\":\"stop-timeout\",\"schemaVersion\":2,\"step\":\"" << step
11361143
<< "\"}" << std::endl;
11371144
std::cout.flush();

electron/native/wgc-capture/src/mf_encoder.cpp

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,10 @@ const char* MFEncoder::encodeStage() const {
366366
return encodeStage_.load();
367367
}
368368

369+
const char* MFEncoder::audioStage() const {
370+
return audioStage_.load();
371+
}
372+
369373
int64_t MFEncoder::nextSampleTime(int64_t timestampHns, int64_t sampleDuration) {
370374
// On `timestampMutex_` and not `writerMutex_`, deliberately. Every caller
371375
// of this runs under main.cpp's frame lock, and `writerMutex_` is held
@@ -1365,10 +1369,19 @@ bool MFEncoder::writeAudio(const BYTE* data, DWORD byteCount, int64_t timestampH
13651369
// Named too, for the same reason the video write is: this is a synchronous
13661370
// encode holding writerMutex_, so it is a place the process can be stuck,
13671371
// and a watchdog report that only ever names video writes cannot say so.
1368-
encodeStage_ = "write-audio";
1372+
//
1373+
// Its own slot, not encodeStage_. This runs on the audio-mixer thread,
1374+
// which emits roughly every 10 ms, while most of the video thread's stages
1375+
// (the whole DXGI bridge sequence) are set outside writerMutex_. Sharing
1376+
// one slot meant a video thread wedged in bridge-copy had its breadcrumb
1377+
// overwritten with "idle" within milliseconds, so the watchdog reported the
1378+
// absence of a stage instead of the call that hung -- the exact opposite of
1379+
// what the breadcrumb is for, on the exact configuration (#252 with system
1380+
// audio) it was added to diagnose.
1381+
audioStage_ = "write-audio";
13691382
const bool written =
13701383
succeeded(sinkWriter_->WriteSample(audioStreamIndex_, sample.Get()), "WriteSample(audio)");
1371-
encodeStage_ = "idle";
1384+
audioStage_ = "idle";
13721385
return written;
13731386
}
13741387

electron/native/wgc-capture/src/mf_encoder.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,14 @@ class MFEncoder {
9393
// writer was never configured for.
9494
bool usesDxgiInput() const;
9595
// A breadcrumb, not state: safe to read from another thread at any time.
96+
// One slot per writing thread, deliberately. encodeStage() names what the
97+
// video-writer thread is inside; audioStage() names what the audio-mixer
98+
// thread is inside. A single shared slot cannot do both: most of the video
99+
// stages are set outside writerMutex_, so an audio write landing every few
100+
// milliseconds would overwrite a wedged video stage with "idle" and the
101+
// watchdog would report the absence of the very call it is trying to name.
96102
const char* encodeStage() const;
103+
const char* audioStage() const;
97104

98105
private:
99106
// Contended is not Failed: the bridge is a two-key handshake and a missed
@@ -156,6 +163,7 @@ class MFEncoder {
156163
// step overruns. `video-writer-join phase=abandoned` says which thread is
157164
// stuck; this says which call it is stuck in.
158165
std::atomic<const char*> encodeStage_{"idle"};
166+
std::atomic<const char*> audioStage_{"idle"};
159167
DWORD videoStreamIndex_ = 0;
160168
DWORD audioStreamIndex_ = 0;
161169
bool hasAudioStream_ = false;

0 commit comments

Comments
 (0)