fix(windows): repeat the owed screen slots so a loaded writer catches up

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

@@ -13,8 +13,8 @@
  * early against the audio, which follows the wall clock
  * (getopenscreen/openscreen#945). This clock never resyncs: a late writer
  * finds the next frame already due and writes it at once, until the count
- * has caught up. Repeating an unchanged screen costs no readback (#925), so
- * catching up is cheap.
+ * has caught up. The slots it owes repeat the last picture, which costs no
+ * readback (#925), so catching up is cheap whatever the screen does.
  *
  * Times are recording time in 100 ns units: elapsed since the shared start,
  * pauses excluded, so a pause owes no frames.
@@ -38,8 +38,44 @@ class FrameSlotClock {
         return originHns_ < 0 ? 0 : originHns_ + frames_ * 10'000'000 / fps_;
     }
 
+    // Whether the next frame's slot is already in the past at nowHns: the slot
+    // after it is due too. Such a slot is owed, not live, so it takes the
+    // picture already encoded instead of a new readback. Catching up with
+    // readbacks only works while a readback costs less than a period; under
+    // load a changing screen makes every tick a readback that costs more, the
+    // count never catches up, and the video plays ever earlier against the audio.
+    bool isBehind(int64_t nowHns) const {
+        return originHns_ >= 0 && nowHns >= originHns_ + (frames_ + 1) * 10'000'000 / fps_;
+    }
+
 private:
     int fps_;
     int64_t originHns_ = -1;
     int64_t frames_ = 0;
 };
+
+/**
+ * Whether a tick reads the screen back or repeats the last picture.
+ *
+ * A changed screen is read back, an unchanged one repeated (#925). A slot
+ * already in the past (FrameSlotClock::isBehind) repeats either way, and the
+ * change it skipped stays pending until a live slot reads it: WGC delivers a
+ * frame only when the screen changes, so a change seen on a past slot and
+ * followed by a still screen would otherwise never reach the video.
+ */
+class PictureReadback {
+public:
+    // `changed`: a new picture arrived this tick. True when this tick must
+    // read back, which also takes the pending change off the books.
+    bool readBackNow(bool changed, bool slotIsPast) {
+        pending_ = pending_ || changed;
+        if (!pending_ || slotIsPast) {
+            return false;
+        }
+        pending_ = false;
+        return true;
+    }
+
+private:
+    bool pending_ = false;
+};

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

@@ -41,6 +41,42 @@ int64_t writeFor(int fps, int64_t durationHns, int64_t workHns, int64_t stallAtH
     return frames;
 }
 
+struct LoadedTake {
+    int64_t frames = 0;
+    // How far the newest picture written ran ahead of the slot it was written
+    // in: the video shows an event that much before the audio plays it.
+    int64_t maxPictureLeadHns = 0;
+};
+
+// The writer under load on a screen that changes every tick: a readback costs
+// `readbackHns`, a repeat of the last picture `repeatHns`. With
+// `repeatPastSlots`, a slot the clock says is already past repeats instead of
+// reading back, through the same PictureReadback main.cpp asks; without it,
+// every tick reads back.
+LoadedTake writeLoadedFor(int fps, int64_t durationHns, int64_t readbackHns, int64_t repeatHns,
+                          bool repeatPastSlots) {
+    FrameSlotClock clock(fps);
+    PictureReadback readback;
+    const int64_t periodHns = kSecondHns / fps;
+    LoadedTake take;
+    int64_t nowHns = 0;
+    int64_t pictureAtHns = 0;
+    while (nowHns < durationHns) {
+        const int64_t tickStartHns = nowHns;
+        if (!readback.readBackNow(true, repeatPastSlots && clock.isBehind(tickStartHns))) {
+            nowHns += repeatHns;
+        } else {
+            pictureAtHns = tickStartHns;
+            nowHns += readbackHns;
+        }
+        take.maxPictureLeadHns = std::max(take.maxPictureLeadHns, pictureAtHns - clock.nextDueHns());
+        ++take.frames;
+        clock.advance(tickStartHns);
+        nowHns += std::clamp<int64_t>(clock.nextDueHns() - nowHns, 0, periodHns);
+    }
+    return take;
+}
+
 // Both streams of one take, the screen at 60 fps and the webcam at 30, on the writer's
 // rule: each clock is advanced with the instant its tick STARTED, so a stall inside the
 // tick (the screen readback) cannot give the two streams different origins. The first
@@ -90,6 +126,43 @@ int main() {
     expectEqual("steady writer keeps the count", writeFor(60, 3 * kSecondHns, 50'000, 3 * kSecondHns, 0), 180);
     expectEqual("stalled writer catches up", writeFor(60, 3 * kSecondHns, 50'000, kSecondHns, 3'000'000), 180);
 
+    // Before the first frame nothing is owed, and a writer on time owes only the
+    // slot it is about to write.
+    FrameSlotClock owed(60);
+    expectEqual("nothing owed before the first frame", owed.isBehind(10 * kSecondHns), 0);
+    owed.advance(0);
+    expectEqual("live slot is not past", owed.isBehind(166'666), 0);
+    expectEqual("slot past once the next one is due", owed.isBehind(333'333), 1);
+
+    // A still screen repeats, a changed one reads back once.
+    PictureReadback readback;
+    expectEqual("still screen repeats", readback.readBackNow(false, false), 0);
+    expectEqual("changed screen reads back", readback.readBackNow(true, false), 1);
+    expectEqual("read change is not read again", readback.readBackNow(false, false), 0);
+    // A change that lands on a past slot, then a screen that stays still: WGC
+    // delivers no frame for the live slot, yet the change must reach it.
+    expectEqual("past slot repeats a changed screen", readback.readBackNow(true, true), 0);
+    expectEqual("change skipped on a past slot is read on the live one",
+                readback.readBackNow(false, false), 1);
+    expectEqual("and only once", readback.readBackNow(false, false), 0);
+
+    // The v2.1.0 candidate's take (#945 follow-up): 10 min at 60 fps under a
+    // CPU/GPU load, screen 217 ms short of the webcam and the audio at the end.
+    // A readback that costs 20 ms on a changing screen, against a 16.7 ms
+    // period: catching up with readbacks wrote 30000 frames, 100 s of video
+    // played early. Repeating the past slots writes all 36000, and no picture
+    // runs more than one period ahead of its slot.
+    const int64_t tenMinutesHns = 600 * kSecondHns;
+    const LoadedTake readbackEveryTick = writeLoadedFor(60, tenMinutesHns, 200'000, 40'000, false);
+    expectEqual("readback catch-up falls behind under load", readbackEveryTick.frames, 30000);
+    const LoadedTake repeatPast = writeLoadedFor(60, tenMinutesHns, 200'000, 40'000, true);
+    expectEqual("repeated past slots keep the count under load", repeatPast.frames, 36000);
+    expectEqual("no picture leads its slot by more than a period",
+                repeatPast.maxPictureLeadHns <= kSecondHns / 60, 1);
+    // Only just over budget, as on that machine: still the full count.
+    expectEqual("readback just over a period keeps the count",
+                writeLoadedFor(60, tenMinutesHns, 170'000, 40'000, true).frames, 36000);
+
     // A first tick that stalls 300 ms must not set the screen's origin 300 ms after the
     // webcam's: both streams of a 3 s take come out within one webcam frame of each other.
     const int64_t drift = streamsDriftFor(3 * kSecondHns, 3'000'000);

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

@@ -1108,12 +1108,16 @@ int wmain(int argc, wchar_t* argv[]) {
         };
         int64_t firstFrameTimestampHns = -1;
         int64_t latestFrameTimestampHns = 0;
+        PictureReadback pictureReadback;
 
         while (!control.stopRequested && !encodeFailed) {
             // One instant per tick for both clocks: read after the screen readback, the
             // screen's would trail the webcam's by any stall in it, and the first tick
             // would give the two streams different origins.
             const int64_t tickStartHns = recordingElapsedHns();
+            // A writer behind by more than this slot fills it with the last
+            // picture: only the latest owed slot gets a readback (#945).
+            const bool videoSlotIsPast = videoClock.isBehind(tickStartHns);
             Microsoft::WRL::ComPtr<IMFSample> videoSample;
             Microsoft::WRL::ComPtr<IMFSample> webcamSample;
             bool hasVideoSample = false;
@@ -1288,10 +1292,17 @@ int wmain(int argc, wchar_t* argv[]) {
                     // 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 && !pictureChanged &&
+                    if (!usesDxgiInput &&
+                        !pictureReadback.readBackNow(pictureChanged, videoSlotIsPast) &&
                         encoder.repeatLastVideoSample(frameTimestampHns, videoSample)) {
                         // Nothing new to read back (#925): the static screen,
                         // most of a demo, costs no GPU copy and no conversion.
+                        // A slot already in the past repeats too, changed
+                        // screen or not: under load a readback can cost more
+                        // than a period, and a writer that catches up with
+                        // readbacks then never catches up at all (#945). The
+                        // new picture waits in latestFrameTexture, pending,
+                        // for the first slot that is live.
                         captured = true;
                     } else if (usesDxgiInput) {
                         captured = encoder.captureDxgiSample(
@@ -1363,7 +1374,8 @@ int wmain(int argc, wchar_t* argv[]) {
             // at the nominal rate, so a skipped slot would put every later
             // frame early against the audio (#945). The next frame is simply
             // already due, and ticks run back to back until the count has
-            // caught up; on a screen that did not change they are repeats.
+            // caught up. Every slot but the live one is a repeat, which costs
+            // an encode and no readback, so the count does catch up.
             // Capped at one period so a stop is never waited on longer than
             // that, whatever the count says.
             const int64_t nowHns = recordingElapsedHns();