fix(macos): drop video frames whose timestamp does not advance - #658
Merged
Conversation
A pause/resume could end the recording. The helper shifts every sample after a resume back by the pause measured on the host clock, but a ScreenCaptureKit frame's presentation time runs a variable few milliseconds ahead of that clock (median 4.8 ms, spread ~20 ms). When the last frame before the pause led by more than the first frame after it, the shifted frame landed just behind the previous one, and AVAssetWriter rejects that in MediaToolbox's MediaSampleTimingGenerator with OSStatus -16364. The writer as a whole fails, one append later, as AVFoundationErrorDomain -11800 wrapping -16364: the "video append" toast. Every later frame is dropped while capture carries on. Measured on real ScreenCaptureKit, M1, macOS 26.5, 200 pause/resume cycles (150 ms paused, 250 ms running): 6 shifted frames behind the previous one at 1080p60 (by 0.2-1.9 ms), 7 at 4K60, and with the helper's writer settings the 4K run died at frame 623 with the exact reported error. Every such frame was captured after resume, so gating on capture time would not catch it. VideoTimestampGate refuses a video frame that is invalid or not after the last one handed to the writer. The overlap is always under one frame interval, so dropping that one frame is the whole fix for this case; audio is clocked by AudioTrackMixer and the writer accepts audio that steps back. The first refusal emits a warning with the timestamps and the pause offset, and finishWriter reports the total. A sample whose retiming fails is now dropped instead of appended unshifted: that frame would sit a whole pause ahead and the gate would then refuse every correctly shifted frame for as long as the pause lasted.
…ones Review of the gate pointed out two claims the measurements do not carry. "The overlap is always under one frame interval" was a sample of 13 events (0.2-1.9 ms), not a bound: the timestamp lead spreads about 20 ms, more than a 60 fps frame. The gate never relied on it -- it refuses until time moves past the last frame the writer received -- so this pins that with a test where two consecutive frames are refused and the third is admitted. "When the last frame before a pause led by more than the first after it" is necessary, not sufficient: the lead has to drop by more than the delivery gaps on either side of the pause, which is why about 3% of resumes trip it rather than half. The warning's doc now tells the two causes apart by pause offset instead of by an overlap size nothing bounds.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughChangesVideo timestamp gating
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ScreenCaptureRecorder
participant VideoTimestampGate
participant Writer
participant WarningReporter
ScreenCaptureRecorder->>VideoTimestampGate: check presentation timestamp
VideoTimestampGate-->>ScreenCaptureRecorder: admission verdict
ScreenCaptureRecorder->>Writer: append admitted frame
ScreenCaptureRecorder->>VideoTimestampGate: record appended timestamp
ScreenCaptureRecorder->>WarningReporter: report refused timestamps
Merge Risk: ⚪ Minimal · up to The macOS recording fix validates and drops invalid video timestamps before they reach the writer, with coverage for refusal and recovery behavior. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On macOS, pausing and resuming could end the recording. The helper then reported
video append: … AVFoundationErrorDomain -11800 … (-16364). The writer died, capture carried on, and the take was lost at Stop.Cause, measured.
MediaSampleTimingGenerator(disassembled from the macOS 26.5 dyld shared cache). It fires when a video sample's presentation time is not after the previous one.Fix.
VideoTimestampGate(OpenScreenCaptureCore, unit-tested) refuses a video frame that is invalid or not after the last frame handed to the writer.AudioTrackMixer, and the writer tolerates audio stepping back.video-frame-timestamp-refusedwarning, with the timestamps and pause offset.finishWriterreports the total. Both are warnings, so Electron's start and stop waits ignore them.Known limit. A frame is dropped rather than re-timed. If the refused frame carries the only screen change at resume and the screen then stays still, the video shows the pre-pause image until the next change. That needs a complete frame delivered in the ~20 ms before the pause, so it is narrow.
Related issue
Refs #621. It is the same silent-death shape, but that report says nothing about pausing, so whether this is its cause is unconfirmed.
Type of change
Release impact
Desktop impact
Screenshots / video
None. No UI change.
Testing
All on Mac mini M1, macOS 26.5, with real ScreenCaptureKit capture. A small window repainting at 120 Hz keeps the display producing complete frames.
Real helpers A/B at 3840×2160@60, 76.5 Mbps.
ScreenCaptureRecorderdiffers frommainonly in window exclusion and mic selection, not the video path.writer-failed-during-captureat 7.0 s with the exact reported message; stop →writer-failed; file 4.0 srecording-stopped; 13 frames refused; file 52.8 s; 3008 packets, DTS strictly increasing, PTS == DTSwriter-failed-during-captureat 2.5 s, same message; file 1.0 srecording-stopped; 8 frames refused; video 52.89 s, AAC 52.93 s (starts −44 ms, normal AAC priming); DTS strictly increasing on both tracksCombined with #655:
fix/issue-621-writer-deathbuilds, andswift testpasses.recording-stoppedwith 14 frames refused and a 52.3 s file with both tracks.Timing probe (same SCK config, host-clock pause shift, no gate):
Synthetic writer probe (320×180, the helper's writer settings):
Unit tests:
swift test --package-path electron/native/screencapturekitpasses, 39 tests including 8 new ones. One of the new tests covers two frames in a row being refused.Not tested: a real microphone (this Mac has none), Intel Macs, macOS versions other than 26.5.
Summary by CodeRabbit