diff --git a/electron/native/wgc-capture/src/frame_slot_clock.h b/electron/native/wgc-capture/src/frame_slot_clock.h index f13778fef..97913a5e3 100644 --- a/electron/native/wgc-capture/src/frame_slot_clock.h +++ b/electron/native/wgc-capture/src/frame_slot_clock.h @@ -4,17 +4,19 @@ #include /** - * When the next frame of a constant-rate stream is due, on the wall clock. + * When the writer's next tick is due, on the recording clock. * - * Media Foundation's H.264 sink writer numbers its output frames at the - * nominal rate whatever SampleTime they carry, so frame N plays at N/fps: the - * frame count is the stream's clock. A writer that falls behind and then - * resyncs to "now" drops the slots it missed, and every later frame plays - * 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. The slots it owes repeat the last picture, which costs no - * readback (#925), so catching up is cheap whatever the screen does. + * The slots are a fixed grid from the first frame: slot N is due at + * origin + N/fps, computed from the origin each time and never accumulated, so + * 1/fps rounding cannot drift a long recording. + * + * A late tick does not repay the slots it missed. Every sample is stamped with + * the recording-clock time of its own tick (see main.cpp's writeVideoFrames), + * and the sink writer keeps those times (MFEncoder::submitVideoSample gives + * each sample its real duration), so a missed slot is already a + * correctly timed gap: the previous frame plays through it. Repaying it with a + * burst of back-to-back ticks would only crowd frames together after the gap. + * The next tick is due at the first slot after the late one. * * Times are recording time in 100 ns units: elapsed since the shared start, * pauses excluded, so a pause owes no frames. @@ -23,59 +25,37 @@ class FrameSlotClock { public: explicit FrameSlotClock(int fps) : fps_(std::max(1, fps)) {} - // A frame was written, or its slot given up. The first one sets the + // A tick that started at `tickHns` wrote its frame. The first one sets the // origin: the stream starts with its first frame, not before it. - void advance(int64_t nowHns) { + void advance(int64_t tickHns) { if (originHns_ < 0) { - originHns_ = nowHns; + originHns_ = tickHns; } - ++frames_; + const int64_t slot = std::max(0, tickHns - originHns_) * fps_ / 10'000'000; + slots_ = std::max(slots_ + 1, slot + 1); } - // Due at once before the first frame. Computed from the origin each time, - // never accumulated, so 1/fps rounding cannot drift a long recording. + // Due at once before the first frame. int64_t nextDueHns() const { - 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_; + return originHns_ < 0 ? 0 : originHns_ + slots_ * 10'000'000 / fps_; } private: int fps_; int64_t originHns_ = -1; - int64_t frames_ = 0; + int64_t slots_ = 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. + * The time a screen or webcam sample carries: the recording-clock instant its + * tick started, on the clock the audio follows. One rule for a new picture + * and a repeated one alike. Never a count of frames: repeats stamped + * "last + 1/fps" put a stall's time back on the frame count (#945). Never the + * WGC frame's SystemRelativeTime either: DWM's presentation estimate, it can + * lie ahead of the tick that pulls it, and mixing it with tick-stamped repeats + * made the times jitter by a refresh period. Kept strictly increasing, as the + * sink writer needs. */ -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; -}; +inline int64_t sampleTimeForTick(int64_t tickHns, int64_t lastSampleHns) { + return lastSampleHns >= 0 && tickHns <= lastSampleHns ? lastSampleHns + 1 : tickHns; +} diff --git a/electron/native/wgc-capture/src/frame_slot_clock_test.cpp b/electron/native/wgc-capture/src/frame_slot_clock_test.cpp index 89727bc44..f8699c262 100644 --- a/electron/native/wgc-capture/src/frame_slot_clock_test.cpp +++ b/electron/native/wgc-capture/src/frame_slot_clock_test.cpp @@ -17,95 +17,61 @@ void expectEqual(const char* label, int64_t actual, int64_t expected) { ++failures; } -constexpr int64_t kSecondHns = 10'000'000; - -// main.cpp's writer loop on a virtual clock: each tick costs `workHns`, one -// tick stalls `stallHns` more once the clock reaches `stallAtHns`, and the -// sleep is the same capped wait the writer takes. Returns frames written. -int64_t writeFor(int fps, int64_t durationHns, int64_t workHns, int64_t stallAtHns, int64_t stallHns) { - FrameSlotClock clock(fps); - const int64_t periodHns = kSecondHns / fps; - int64_t nowHns = 0; - int64_t frames = 0; - bool stalled = false; - while (nowHns < durationHns) { - nowHns += workHns; - if (!stalled && nowHns >= stallAtHns) { - nowHns += stallHns; - stalled = true; - } - ++frames; - clock.advance(nowHns); - nowHns += std::clamp(clock.nextDueHns() - nowHns, 0, periodHns); +void expectTrue(const char* label, bool condition, int64_t detail) { + if (condition) { + return; } - return frames; + std::printf("FAIL %s (%lld)\n", label, static_cast(detail)); + ++failures; } -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; +constexpr int64_t kSecondHns = 10'000'000; +constexpr int64_t kMsHns = 10'000; + +struct Take { + int64_t samples = 0; + int64_t lastSampleHns = -1; + int64_t minIntervalHns = INT64_MAX; + int64_t maxIntervalHns = 0; + bool increasing = true; + // Where the samples would end if their times were a count of frames at the + // nominal rate, the rule this replaces for repeated frames (#945). + int64_t countClockEndHns = 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) { +// main.cpp's screen writer on a virtual recording clock. Each tick costs +// `workHns(tickStart)`; every `dropEvery`-th tick writes nothing (the DXGI path's +// contended frame); the sleep is the writer's: to the next due slot, capped at one +// period. Every sample is stamped by sampleTimeForTick with its tick's start. +template +Take writeTake(int fps, int64_t durationHns, Work workHns, int dropEvery) { FrameSlotClock clock(fps); - PictureReadback readback; const int64_t periodHns = kSecondHns / fps; - LoadedTake take; + Take take; int64_t nowHns = 0; - int64_t pictureAtHns = 0; + int64_t ticks = 0; while (nowHns < durationHns) { const int64_t tickStartHns = nowHns; - if (!readback.readBackNow(true, repeatPastSlots && clock.isBehind(tickStartHns))) { - nowHns += repeatHns; - } else { - pictureAtHns = tickStartHns; - nowHns += readbackHns; + nowHns += workHns(tickStartHns); + ++ticks; + if (dropEvery == 0 || ticks % dropEvery != 0) { + const int64_t sampleHns = sampleTimeForTick(tickStartHns, take.lastSampleHns); + if (take.lastSampleHns >= 0) { + const int64_t interval = sampleHns - take.lastSampleHns; + take.increasing = take.increasing && interval > 0; + take.minIntervalHns = std::min(take.minIntervalHns, interval); + take.maxIntervalHns = std::max(take.maxIntervalHns, interval); + } + take.lastSampleHns = sampleHns; + ++take.samples; } - take.maxPictureLeadHns = std::max(take.maxPictureLeadHns, pictureAtHns - clock.nextDueHns()); - ++take.frames; clock.advance(tickStartHns); nowHns += std::clamp(clock.nextDueHns() - nowHns, 0, periodHns); } + take.countClockEndHns = (take.samples - 1) * kSecondHns / fps; 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 -// tick stalls `firstStallHns`. Returns the written durations, screen minus webcam. -int64_t streamsDriftFor(int64_t durationHns, int64_t firstStallHns) { - FrameSlotClock screen(60); - FrameSlotClock webcam(30); - int64_t nowHns = 0; - int64_t screenFrames = 0; - int64_t webcamFrames = 0; - bool stalled = false; - while (nowHns < durationHns) { - const int64_t tickStartHns = nowHns; - if (tickStartHns >= webcam.nextDueHns()) { - ++webcamFrames; - webcam.advance(tickStartHns); - } - nowHns += 50'000; - if (!stalled) { - nowHns += firstStallHns; - stalled = true; - } - ++screenFrames; - screen.advance(tickStartHns); - nowHns += std::clamp(screen.nextDueHns() - nowHns, 0, kSecondHns / 60); - } - return screenFrames * kSecondHns / 60 - webcamFrames * kSecondHns / 30; -} - } // namespace int main() { @@ -117,63 +83,93 @@ int main() { clock.advance(5'000'000); expectEqual("origin is the first frame", clock.nextDueHns(), 5'000'000 + 166'666); - // The regression this file exists for (#945). A writer that misses ticks - // owes their frames: the encoder numbers frames at the nominal rate, so - // the count is the video's clock. A 300 ms stall one second in, 60 fps - // for 3 s from the end of the first tick at 5 ms: 180 frames, the same as - // with no stall. The old rule, resyncing to now after the stall, wrote 163: - // the video ran 283 ms short, ahead of the audio for the rest of the take. - 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. + // A tick 2.5 periods late skips the slots it missed: the next one is due at the + // first slot after it, not at once (no catch-up burst). + clock.advance(5'000'000 + 416'666); + expectEqual("a late tick skips the missed slots", clock.nextDueHns(), 5'000'000 + 500'000); + + // A tick a hair early still moves on by one slot: the clock never stands still. + FrameSlotClock early(60); + early.advance(0); + early.advance(166'000); + expectEqual("an early tick still advances one slot", early.nextDueHns(), 333'333); + + // Samples carry their tick's time, kept strictly increasing. + expectEqual("a sample carries its tick's time", sampleTimeForTick(500, 400), 500); + expectEqual("the first sample carries its tick's time", sampleTimeForTick(0, -1), 0); + expectEqual("a sample never repeats the last time", sampleTimeForTick(400, 400), 401); + + // The regression this file exists for. Ten minutes at 60 fps under the load the + // report measured: a 300 ms stall at 1:00, then 2:30 of ticks that cost 25 ms + // (more than a period) from 5:00, and one contended frame in every 97 dropped. + // The video must end on the recording clock, within a few frames of the take's + // length (the last tick may be a dropped one), with the stall left as a gap of + // its own length and no burst after it. 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); - expectEqual("screen and webcam stay aligned after a first-tick stall", - drift > -kSecondHns / 30 && drift < kSecondHns / 30, 1); + auto load = [](int64_t tickStartHns) -> int64_t { + if (tickStartHns >= 60 * kSecondHns && tickStartHns < 60 * kSecondHns + 5 * kMsHns) { + return 300 * kMsHns; + } + if (tickStartHns >= 300 * kSecondHns && tickStartHns < 450 * kSecondHns) { + return 25 * kMsHns; + } + return 5 * kMsHns; + }; + const Take loaded = writeTake(60, tenMinutesHns, load, 97); + expectTrue("sample times strictly increase", loaded.increasing, loaded.samples); + expectTrue("the video ends on the recording clock", + loaded.lastSampleHns > tenMinutesHns - 50 * kMsHns && + loaded.lastSampleHns <= tenMinutesHns, + loaded.lastSampleHns); + expectTrue("the stall is a gap of its own length", loaded.maxIntervalHns >= 300 * kMsHns, + loaded.maxIntervalHns); + expectTrue("no burst of frames after a stall", loaded.minIntervalHns >= 5 * kMsHns, + loaded.minIntervalHns); + // The same take counted at the nominal rate would end seconds short: the overload + // alone writes ~40 frames a second for 2:30. The scenario has teeth. + expectTrue("a frame count would have lost the overload", + loaded.lastSampleHns - loaded.countClockEndHns > 30 * kSecondHns, + loaded.lastSampleHns - loaded.countClockEndHns); + + // An idle writer keeps the nominal cadence: 3 s at 60 fps is 180 samples. + const Take idle = writeTake(60, 3 * kSecondHns, [](int64_t) -> int64_t { return 50'000; }, 0); + expectEqual("steady writer keeps the cadence", idle.samples, 180); + + // Screen at 60 and webcam at 30 in one writer, both stamped with the tick's + // start and the first tick stalling 300 ms: the two streams end within one + // webcam frame of each other, on the same clock. + { + FrameSlotClock screen(60); + FrameSlotClock webcam(30); + int64_t nowHns = 0; + int64_t lastScreenHns = -1; + int64_t lastWebcamHns = -1; + bool stalled = false; + while (nowHns < 3 * kSecondHns) { + const int64_t tickStartHns = nowHns; + if (tickStartHns >= webcam.nextDueHns()) { + lastWebcamHns = sampleTimeForTick(tickStartHns, lastWebcamHns); + webcam.advance(tickStartHns); + } + nowHns += 50'000; + if (!stalled) { + nowHns += 3'000'000; + stalled = true; + } + lastScreenHns = sampleTimeForTick(tickStartHns, lastScreenHns); + screen.advance(tickStartHns); + nowHns += std::clamp(screen.nextDueHns() - nowHns, 0, kSecondHns / 60); + } + const int64_t drift = lastScreenHns - lastWebcamHns; + expectTrue("screen and webcam stay aligned after a first-tick stall", + drift > -kSecondHns / 30 && drift < kSecondHns / 30, drift); + } // Due times come from the origin, not from adding a truncated 1/fps // (166'666 hns at 60 fps) per frame, which lost 14 ms an hour. FrameSlotClock hour(60); - for (int i = 0; i < 60 * 3600; ++i) { - hour.advance(0); + for (int64_t i = 0; i < 60 * 3600; ++i) { + hour.advance(hour.nextDueHns()); } expectEqual("no rounding drift over an hour", hour.nextDueHns(), 3600 * kSecondHns); diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index 545a1f9e4..bd28c5eb1 100644 --- a/electron/native/wgc-capture/src/main.cpp +++ b/electron/native/wgc-capture/src/main.cpp @@ -1026,16 +1026,14 @@ int wmain(int argc, wchar_t* argv[]) { uint64_t latestWebcamSequence = 0; bool hasVisibleWebcamFrame = false; - // Legacy-path-only state. frameMutex guards latestFrameTexture/ - // legacyLatestFrameTimestampHns between WGC's callback thread (writer) - // and writeVideoFrames (reader); frameCv wakes the reader. Both are - // unused on the default pull-based path. + // Legacy-path-only state. frameMutex guards latestFrameTexture between + // WGC's callback thread (writer) and writeVideoFrames (reader); frameCv + // wakes the reader. Both are unused on the default pull-based path. std::timed_mutex frameMutex; std::condition_variable_any frameCv; - int64_t legacyLatestFrameTimestampHns = 0; if (legacyFrameCallback) { - session.setFrameCallback([&](ID3D11Texture2D* texture, int64_t timestampHns) { + session.setFrameCallback([&](ID3D11Texture2D* texture, int64_t /*timestampHns*/) { if (control.stopRequested || control.paused) { return; } @@ -1066,7 +1064,6 @@ int wmain(int argc, wchar_t* argv[]) { std::this_thread::sleep_for(std::chrono::milliseconds(testStallFrameCallbackMs)); } session.context()->CopyResource(latestFrameTexture.Get(), texture); - legacyLatestFrameTimestampHns = timestampHns; if (!firstFrameWritten.exchange(true)) { frameCv.notify_all(); } @@ -1080,22 +1077,14 @@ int wmain(int argc, wchar_t* argv[]) { uint64_t frameIndex = 0; int64_t lastEncodedVideoTimestampHns = -1; int64_t lastWebcamTimestampHns = -1; - // Media Foundation's H.264 encoder MFT does not honor irregular input - // sample times for a VFR source: it numbers output samples - // sequentially at its configured nominal frame rate regardless of the - // SampleTime we attach (confirmed empirically -- varying, correctly - // increasing input timestamps still produced perfectly even output - // spacing). Since we cannot make the encoder respect real capture - // time, we instead make the encoder's assumption true: feed the - // webcam encoder on a real-time-paced cadence (duplicating the - // latest available camera frame when the camera hasn't produced a - // newer one yet), so "sample N is at N/fps" is actually correct. - // The screen encoder numbers its frames the same way, so both streams - // pace to a FrameSlotClock on the recording timeline the audio follows. + // Both streams are written on a real-time cadence, one FrameSlotClock + // each, repeating the latest picture when the source has nothing newer + // (a static screen, a camera slower than its nominal rate). Every + // sample carries the recording-clock time of the tick that wrote it + // (sampleTimeForTick): the sink writer keeps those times, so the video + // stays on the clock the audio follows however late a tick runs. FrameSlotClock videoClock(config.fps); FrameSlotClock webcamClock(webcamCapture.fps()); - const int64_t nominalWebcamIntervalHns = - static_cast(10'000'000ULL / std::max(1, webcamCapture.fps())); // Real elapsed host-clock time since the shared recording start, // pauses excluded: not a synthetic frame-index clock, so a long // recording can't accumulate clock-origin drift. @@ -1106,18 +1095,11 @@ int wmain(int argc, wchar_t* argv[]) { 0, std::chrono::duration_cast(elapsed).count() - control.pausedDurationHns()); }; - 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); + int64_t tickStartHns = recordingElapsedHns(); Microsoft::WRL::ComPtr videoSample; Microsoft::WRL::ComPtr webcamSample; bool hasVideoSample = false; @@ -1151,10 +1133,12 @@ int wmain(int argc, wchar_t* argv[]) { if (control.stopRequested || encodeFailed) { break; } - if (!latestFrameTexture) { + if (control.paused || !latestFrameTexture) { continue; } - latestFrameTimestampHns = legacyLatestFrameTimestampHns; + // The wait can outlast a pause and resume: stamp this tick + // with the time it actually runs at. + tickStartHns = recordingElapsedHns(); } else { if (control.paused) { std::this_thread::sleep_for(std::chrono::milliseconds(100)); @@ -1186,7 +1170,6 @@ int wmain(int argc, wchar_t* argv[]) { // separate WGC callback thread left for it to take a // lock down with it. session.context()->CopyResource(latestFrameTexture.Get(), wgcTexture); - latestFrameTimestampHns = wgcTimestampHns; firstFrameWritten = true; } else if (!latestFrameTexture) { // No frame captured yet at all: nothing to encode @@ -1215,41 +1198,16 @@ int wmain(int argc, wchar_t* argv[]) { latestWebcamWidth, latestWebcamHeight, }; - const int64_t syntheticTimestampHns = - static_cast((frameIndex * 10'000'000ULL) / config.fps); - const int64_t sourceTimestampHns = - latestFrameTimestampHns > 0 ? latestFrameTimestampHns : syntheticTimestampHns; - if (firstFrameTimestampHns < 0) { - firstFrameTimestampHns = sourceTimestampHns; - } - int64_t frameTimestampHns = - std::max( - 0, - sourceTimestampHns - firstFrameTimestampHns - control.pausedDurationHns()); - if (lastEncodedVideoTimestampHns >= 0 && - frameTimestampHns <= lastEncodedVideoTimestampHns) { - frameTimestampHns = - lastEncodedVideoTimestampHns + static_cast(10'000'000ULL / config.fps); - } + // The tick's own time, not the WGC frame's: see sampleTimeForTick. + const int64_t frameTimestampHns = + sampleTimeForTick(tickStartHns, lastEncodedVideoTimestampHns); if (writeSeparateWebcam && webcamFrame.data) { - // Anchored to the same recording-start origin as screen video/audio. - const int64_t targetElapsedHns = tickStartHns; - // The H.264 encoder MFT does not honor irregular per-sample - // timestamps for a VFR source -- it numbers output samples - // sequentially at its configured nominal rate regardless of the - // SampleTime attached to each input sample. So the only way to - // keep the encoded webcam file in sync with real elapsed time is - // to feed the encoder *at* that nominal cadence, duplicating - // the latest available camera frame when the camera hasn't - // produced a newer one yet (VFR capture -> CFR encode resampling). - // One frame per tick: after a stall the writer's ticks come - // back to back (see the pacing below), which repays the - // slots this stream missed too. - if (targetElapsedHns >= webcamClock.nextDueHns()) { - int64_t webcamTimestampHns = targetElapsedHns; - if (lastWebcamTimestampHns >= 0 && webcamTimestampHns <= lastWebcamTimestampHns) { - webcamTimestampHns = lastWebcamTimestampHns + nominalWebcamIntervalHns; - } + // On the same clock and tick instant as the screen, so the two + // streams share an origin. One frame per due slot, repeating the + // latest camera frame when the camera has nothing newer. + if (tickStartHns >= webcamClock.nextDueHns()) { + const int64_t webcamTimestampHns = + sampleTimeForTick(tickStartHns, lastWebcamTimestampHns); // Capture the sample here, but submit it to the sink // writer OUTSIDE this block below (issue #115) so a // slow WriteSample can't hold up the next frame pull. @@ -1268,7 +1226,7 @@ int wmain(int argc, wchar_t* argv[]) { break; } lastWebcamTimestampHns = webcamTimestampHns; - webcamClock.advance(targetElapsedHns); + webcamClock.advance(tickStartHns); } } if (testStallReadbackMs > 0) { @@ -1292,17 +1250,10 @@ 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 && - !pictureReadback.readBackNow(pictureChanged, videoSlotIsPast) && + if (!usesDxgiInput && !pictureChanged && 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( @@ -1370,14 +1321,10 @@ int wmain(int argc, wchar_t* argv[]) { // sleeping a whole period on top of it made the real period // `work + 1/fps` -- 30 fps requested delivered 22.5 measured. // - // A late writer is not resynced to now: the encoder numbers frames - // 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. 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. + // A late tick skips the slots it missed rather than repaying them: + // its frame carries its own time, so the gap is already correctly + // timed (see FrameSlotClock). Capped at one period so a stop is + // never waited on longer than that. const int64_t nowHns = recordingElapsedHns(); videoClock.advance(tickStartHns); std::this_thread::sleep_for(std::min( @@ -1389,7 +1336,7 @@ int wmain(int argc, wchar_t* argv[]) { << std::chrono::duration_cast( std::chrono::steady_clock::now() - control.recordingStartedAt) .count() - << std::endl; + << " last_sample_ms=" << lastEncodedVideoTimestampHns / 10'000 << std::endl; }; std::thread videoWriterThread; diff --git a/electron/native/wgc-capture/src/mf_encoder.cpp b/electron/native/wgc-capture/src/mf_encoder.cpp index 500ff5008..09788bde1 100644 --- a/electron/native/wgc-capture/src/mf_encoder.cpp +++ b/electron/native/wgc-capture/src/mf_encoder.cpp @@ -1909,9 +1909,33 @@ bool MFEncoder::submitVideoSample(IMFSample* sample) { if (!sinkWriter_ || finalized_) { return false; } + // Each sample is written one submission late, once the next one says how + // long it really lasts. The fragmented MP4 sink closes every fragment on + // its last sample's SampleDuration and starts the next fragment right after + // it, so a nominal 1/fps there replaced the real gap to the next frame once + // a fragment: a late tick, or a 56 Hz desktop sampled at 60, lost the + // difference for good, and the screen fell 217 ms behind the audio over a + // ten-minute take (#1164). + LONGLONG pendingTime = 0; + LONGLONG nextTime = 0; + if (pendingVideoSample_ && SUCCEEDED(pendingVideoSample_->GetSampleTime(&pendingTime)) && + SUCCEEDED(sample->GetSampleTime(&nextTime)) && nextTime > pendingTime) { + pendingVideoSample_->SetSampleDuration(nextTime - pendingTime); + } + const bool written = writePendingVideoSample(); + pendingVideoSample_ = sample; + return written; +} + +bool MFEncoder::writePendingVideoSample() { + if (!pendingVideoSample_) { + return true; + } encodeStage_ = "write-sample"; - const bool written = succeeded(sinkWriter_->WriteSample(videoStreamIndex_, sample), "WriteSample"); + const bool written = + succeeded(sinkWriter_->WriteSample(videoStreamIndex_, pendingVideoSample_.Get()), "WriteSample"); encodeStage_ = "idle"; + pendingVideoSample_.Reset(); return written; } @@ -1981,8 +2005,11 @@ bool MFEncoder::finalize() { finalized_ = true; bool ok = true; if (sinkWriter_) { - ok = succeeded(sinkWriter_->Finalize(), "SinkWriter::Finalize"); + // The last frame keeps its nominal 1/fps: nothing follows it. + ok = writePendingVideoSample(); + ok = succeeded(sinkWriter_->Finalize(), "SinkWriter::Finalize") && ok; } + pendingVideoSample_.Reset(); releaseSinkWriter(); stagingTexture_.Reset(); // Before MFShutdown(), not left to the destructor. Two of the objects diff --git a/electron/native/wgc-capture/src/mf_encoder.h b/electron/native/wgc-capture/src/mf_encoder.h index 455624135..905b66ca9 100644 --- a/electron/native/wgc-capture/src/mf_encoder.h +++ b/electron/native/wgc-capture/src/mf_encoder.h @@ -217,6 +217,8 @@ class MFEncoder { // a fragmented sink has all its streams before anything can be added to it. bool configureAudioStream(const AudioInputFormat& audioFormat, const MFEncoderOptions& options); void releaseSinkWriter(); + // Writes and drops pendingVideoSample_. Under writerMutex_. + bool writePendingVideoSample(); Microsoft::WRL::ComPtr sinkWriter_; // Held only on the fragmented path, and held because a sink writer built on @@ -263,6 +265,9 @@ class MFEncoder { // conversion; reused so a frame does not allocate one. std::vector bgraScratch_; Microsoft::WRL::ComPtr lastVideoBuffer_; + // The video sample submitted last, held back until the next one gives it + // its real duration (see submitVideoSample). Under writerMutex_. + Microsoft::WRL::ComPtr pendingVideoSample_; int width_ = 0; int height_ = 0; int fps_ = 60; diff --git a/electron/native/wgc-capture/src/mf_encoder_color_test.cpp b/electron/native/wgc-capture/src/mf_encoder_color_test.cpp index 1cc20d6b0..d9278b406 100644 --- a/electron/native/wgc-capture/src/mf_encoder_color_test.cpp +++ b/electron/native/wgc-capture/src/mf_encoder_color_test.cpp @@ -358,6 +358,64 @@ void checkRepeatedFrames(ID3D11Device* device, ID3D11DeviceContext* context) { } } +// Every frame must play at the time it was stamped with, across fragment +// boundaries too. The fragmented sink closes each fragment on its last sample's +// SampleDuration and starts the next one right after it, so a nominal 1/fps +// there dropped whatever the real gap held beyond it: a writer running late at +// 40 fps, or a stall, lost time at every fragment and the video fell behind the +// audio (217 ms over a ten-minute take, #1164). +void checkSampleTimesKept(ID3D11Device* device, ID3D11DeviceContext* context, bool software) { + const std::string label = software ? "software" : "default"; + char tempDir[MAX_PATH]{}; + GetTempPathA(MAX_PATH, tempDir); + const std::string path = std::string(tempDir) + "openscreen-mf-encoder-times-" + label + ".mp4"; + DeleteFileA(path.c_str()); + + // 60 fps nominal, written every 25 ms for 4 s, with a 300 ms stall at 2 s. + std::vector stamps; + for (int64_t t = 0; t < 40'000'000; t += 250'000) { + stamps.push_back(t < 20'000'000 ? t : t + 3'000'000); + } + { + std::vector bgra(static_cast(kWidth) * kHeight * 4, 128); + const BgraFrameView frame{bgra.data(), kWidth, kHeight}; + MFEncoder encoder; + MFEncoderOptions options; + options.preferSoftwareEncoder = software; + if (!encoder.initialize(widen(path), kWidth, kHeight, 60, 2'000'000, device, context, nullptr, options)) { + expect("sample-times-initialize-" + label, false, "initialize failed"); + return; + } + bool wrote = true; + for (size_t i = 0; i < stamps.size() && wrote; i += 1) { + Microsoft::WRL::ComPtr sample; + wrote = encoder.captureBgraSample(frame, stamps[i], sample) && encoder.submitVideoSample(sample.Get()); + } + expect("sample-times-writes-" + label, wrote && encoder.finalize(), "write or finalize failed"); + } + + const std::string probe = + run("ffprobe -v error -select_streams v:0 -show_entries packet=pts_time -of csv=p=0 \"" + path + "\""); + std::vector pts; + for (size_t at = 0; at < probe.size();) { + const size_t end = probe.find_first_of("\r\n", at); + const std::string line = probe.substr(at, end == std::string::npos ? std::string::npos : end - at); + if (!line.empty()) { + pts.push_back(std::atof(line.c_str())); + } + at = end == std::string::npos ? probe.size() : end + 1; + } + std::sort(pts.begin(), pts.end()); + double worstMs = pts.size() == stamps.size() ? 0.0 : 1e9; + for (size_t i = 0; i < pts.size() && i < stamps.size(); i += 1) { + worstMs = std::max(worstMs, std::abs(pts[i] * 1000.0 - stamps[i] / 10'000.0)); + } + std::cout << "TIMES_RAW " << label << " frames=" << pts.size() << "/" << stamps.size() + << " worst pts error " << worstMs << " ms\n"; + expect("sample-times-kept-" + label, worstMs < 0.5, std::to_string(worstMs) + " ms"); + DeleteFileA(path.c_str()); +} + } // namespace int main() { @@ -386,6 +444,8 @@ int main() { checkEncoder(device.Get(), context.Get(), false); timeFullHd(device.Get(), context.Get()); checkRepeatedFrames(device.Get(), context.Get()); + checkSampleTimesKept(device.Get(), context.Get(), true); + checkSampleTimesKept(device.Get(), context.Get(), false); std::cout << "ran " << g_ran << " tests\n"; if (g_failed != 0) { diff --git a/scripts/build-windows-wgc-helper.mjs b/scripts/build-windows-wgc-helper.mjs index 3fb234d71..90b961ee7 100644 --- a/scripts/build-windows-wgc-helper.mjs +++ b/scripts/build-windows-wgc-helper.mjs @@ -158,9 +158,9 @@ const frameSlotClockTestPath = path.join(BUILD_DIR, "frame_slot_clock_test.exe") if (!fs.existsSync(frameSlotClockTestPath)) { throw new Error(`WGC helper build completed but ${frameSlotClockTestPath} was not found.`); } -// Guards the writer's cadence. The encoder numbers frames at the nominal rate, -// so a tick the writer misses must still be written or the video runs ahead -// of the audio for the rest of the take (#945). +// Guards the writer's cadence and its sample times: every frame carries the +// recording-clock time of its tick, so stalls and overload leave correctly +// timed gaps and the video stays on the audio's clock (#945). await run(frameSlotClockTestPath, [], { cwd: BUILD_DIR }); console.log(`Passed ${frameSlotClockTestPath}`);