fix(mac): stop forcing a keyframe on backpressure drops#117
Conversation
A frame skipped BEFORE the encoder never breaks the P-frame chain: the encoder only references frames it was actually fed, and TCP delivers every encoded frame in order, so the receiver stays in sync without any resync IDR. The forced keyframe was actively harmful: the drop fires exactly when the socket is already backed up, and an IDR is a multi-hundred-KB bitrate spike — each drop made the congestion worse, feeding a drop -> IDR -> drop spiral under sustained pressure (measured on a 120Hz fork of this pipeline: delivered fps collapsed to 0 while capture held 80+). Keyframes are still forced where they are actually needed: reconnect, display rebuild, and explicit receiver requests.
peetzweg
left a comment
There was a problem hiding this comment.
Verdict: looks correct, approving. Traced the drop site and I agree the forced keyframe was pure waste, not a safety net.
The check in stream(_:didOutputSampleBuffer:of:) (Mac/MacSender.swift, around the if pendingSends > maxPendingSends block) fires before encode(pixelBuffer:...) is ever called. So a dropped frame never reaches VTCompressionSessionEncodeFrame — the encoder's reference chain only ever consists of frames it actually encoded, and with AllowFrameReordering = false and no B-frames each P-frame only references the last frame it was fed, not the last frame captured. Combined with TCP (and usbmux, which is also an ordered reliable stream) delivering every encoded frame in order, the receiver's decode state stays consistent — there's no missing reference to recover from. So removing the forced IDR here is safe: this is a pre-encode skip, not a post-encode frame going missing off the wire.
I also checked whether anything else was leaning on needsKeyframe being set from this call site — it isn't. becomeReady() sets it on reconnect, reconfigure() sets it on display rebuild, the phone's explicit kf control message sets it, and the watchdog's static-screen replay only reads it. All four are untouched by this diff. On the receiver side, PhoneReceiver.decodeAndRender already has its own independent resync path — any real decode failure (e.g. joining mid-GOP) calls requestKeyframeIfNeeded() regardless of why the decoder got out of sync. That safety net isn't being removed, so even in a scenario this change didn't anticipate, the receiver still self-heals.
One thing worth a second look, not blocking: the non-Metal display path (displayLayer.enqueue, PhoneReceiver.swift ~line 621) flushes on .failed status but doesn't call requestKeyframeIfNeeded() the way the Metal decode path does. That's a pre-existing gap unrelated to this change (this PR doesn't introduce any missing-reference scenario for it to matter), so not something to fix here, but flagging in case it's ever relevant when debugging a stuck AVSampleBufferDisplayLayer.
Nice write-up in the PR description with real measurements from the 120Hz fork, and the comment left in the code explains the drop→IDR→drop spiral clearly for the next person who touches this. No functional concerns. Thanks for tracking this down and for the clean, single-purpose diff — appreciate the sibling-PR heads up too, rebasing against #118/#119 should be trivial given the small diff.
What
Removes
needsKeyframe = truefrom the backpressure drop path inMacSender.Why
A frame skipped before the encoder never breaks the P-frame chain: the encoder only references frames it was actually fed, and TCP delivers every encoded frame in order — the receiver stays in sync without any resync IDR.
The forced keyframe was actively harmful: the drop fires exactly when the socket is already backed up, and an IDR is a multi-hundred-KB bitrate spike. Each drop made the congestion worse, feeding a drop → IDR → drop spiral under sustained pressure. Measured on a private 120Hz experiment on top of this pipeline: delivered fps collapsed to ~0 while capture held 80+; removing the forced IDR restored delivery to what the wire sustains. At upstream's 60fps the spiral is milder but the mechanism is identical — every backpressure drop today costs an unnecessary keyframe right when bandwidth is scarcest.
Keyframes are still forced where they are actually needed: reconnect, display rebuild, and explicit receiver
keyframerequests.Testing
xcodebuild -scheme OpenSidecarMac -configuration Debugbuilds clean (only pre-existing Swift 6 concurrency warnings).Two sibling PRs touch adjacent lines (silent-encode-failure logging, encoder in-flight gate) — trivial conflicts if merged together, happy to rebase whichever lands last.