Fix video rendering performance degradation - #98
Draft
andrusha wants to merge 13 commits into
Draft
Conversation
The statistics HUD previously reported server render FPS and WebRTC decode FPS as game/stream FPS, while AVFoundation presentation drops were removed as supposedly redundant. Those counters describe different pipeline stages, so a 60 fps decoder could conceal a much lower visible cadence when AVSampleBufferVideoRenderer replaced or dropped frames. Report callback, enqueue, and effective AV presentation rates over fresh intervals; expose application drops, superseded frames, renderer backpressure, pending depth, AV frame delay, and optimized-compositing usage. Extend AVVideoPerformanceMetrics capture with optimized frame counts and reset rate counters after each snapshot while retaining stable decoded-format state. This restores the stage attribution required by docs/latency/02-video-pipeline.md and docs/latency/05-stream-runtime-overhead.md before changing decoder or presentation policy.
The HDR-preserving H.265 decoder converted every Annex-B access unit by first copying Data into a byte array, allocating subdata for each NAL, appending those payloads into an intermediate AVCC Data, and finally copying the result into a CMBlockBuffer. The work scaled with compressed frame size, so bitrate spikes during explosions or major scene changes could stall WebRTC's decoder thread and release otherwise-60-fps output in bursts. Parse the contiguous Data storage in place, point VideoToolbox parameter-set creation directly at the source bytes, and write length-prefixed NAL payloads straight into the final CMBlockBuffer. Submit hardware decode asynchronously, protect the callback shared with asynchronous output, surface delayed decode failures on the next input, and preserve render/RTP timing in both the VideoToolbox sample and LKRTCVideoFrame. This removes the scene-complexity-dependent allocation path identified by docs/latency/02-video-pipeline.md while retaining native HEVC Main/Main10 output and color metadata.
The immediate-presentation path replaced queued images without preserving source cadence. When scene-complexity-dependent H.265 work or transport jitter delivered otherwise-60-fps callbacks in bursts, AVFoundation could collapse several decoded frames into one visible update, matching the reported 20-fps motion while server and decoder counters remained at 60 fps. Map WebRTC frame timestamps (with RTP rollover-aware fallback) onto the host clock, rebase discontinuities instead of accumulating stale latency, and make this paced mode the default. Keep the former DisplayImmediately behavior behind CLOUDNOW_VIDEO_PRESENTATION_MODE=immediate for physical-device A/B validation, and surface the active mode in diagnostics. Serialize enqueue and flush operations on a user-interactive queue. Respect AVSampleBufferVideoRenderer readiness with a single-slot newest-frame mailbox, count backpressure and superseded frames, and stop readiness callbacks as soon as the mailbox drains. This keeps application-side buffering bounded while preserving failure recovery and format-cache invalidation. The policy follows the shallow-queue and stable-cadence guidance in docs/latency/deep-research-report-video-latency.md, while retaining the explicit A/B requirement from docs/latency/02-video-pipeline.md and docs/latency/09-video-latency-plan.md.
Apply SwiftFormat 0.62.1's five-digit numeric-literal convention to the 90 kHz RTP clock constants and restore the required scope spacing in the renderer. This is behavior-neutral and keeps the decoder and presentation commits aligned with the exact formatter version enforced by CI.
Physical playback showed that the host-timestamp pacing experiment did not present frames correctly. Make DisplayImmediately/latest-image presentation the production fallback again so decoded frames are shown as soon as AVFoundation can accept them and stale queued images retain replacement semantics. Keep CLOUDNOW_VIDEO_PRESENTATION_MODE=paced as an internal diagnostic override rather than deleting the experiment. The H.265 copy-amplification fix, asynchronous decode, end-stage metrics, serialized flush/enqueue handling, and single-frame backpressure mailbox remain active.
The asynchronous VideoToolbox flag allowed decode() to return before its output callback and placed no application-side bound on submitted samples. During complex scenes, compressed frames could accumulate faster than hardware completed them, retaining CMSampleBuffers and decoded surfaces while visible output fell further behind. That failure mode matches the observed progression from lag to process termination under memory pressure. Clear the asynchronous flag so VideoToolbox must finish the output callback before decode() returns, restoring the decoder's original one-frame backpressure and immediate error reporting. Retain the lower-copy Annex-B parser and direct CMBlockBuffer conversion, timestamp propagation, callback synchronization, and immediate latest-frame renderer policy.
Device crash reports consistently ended in a stack overflow through thousands of RTCVideoDecoderCallback reabstraction thunks. The callback had been routed through a generic Mutex value lookup on every decoded frame, and each lookup crossed the imported Objective-C block bridge. Store and invoke the concrete RTCVideoDecoderCallback directly. Decode is synchronous because VTDecompressionSessionWaitForAsynchronousFrames completes before decode() returns, so callback replacement and frame delivery do not overlap. This removes the per-frame generic bridge while preserving decoder ordering and the existing per-decode failure lock.
The latency research calls for latest-frame-wins backpressure, but the previous mailbox ran only after each frame had already been captured by enqueueQueue.async. During high-motion decode bursts, every queued closure retained its CMSampleBuffer and 4K CVPixelBuffer, allowing latency and memory to grow before pending-frame replacement occurred. Move admission ahead of dispatch with a lock-protected one-frame mailbox and a single drain token. New frames supersede the retained frame immediately, renderer readiness drives one serial drain cycle, and flush atomically drops the mailbox. This keeps queueing latency bounded while retaining immediate presentation and AVSampleBufferVideoRenderer readiness handling.
Reconnect could be entered independently by signaling and ICE callbacks, while each attempt launched an untracked Task. A stale offer or closed peer delegate could then overwrite the active peer connection, and reconnect teardown left the old renderer track, receiver, signaling handler, and completed task handles retained. Track and coalesce reconnect and offer tasks, invalidate replacements with a connection generation, and check peer/data-channel identity at every asynchronous callback boundary. Centralized transport teardown now detaches video and channel delegates, closes signaling and WebRTC, and releases receivers before reclaiming. The signaling client now nils cancelled task handles and event callbacks. Its heartbeat weakly captures the client, while the receive loop owns only NWConnection during its suspension, removing the client-task retain cycles identified during the memory audit.
The memory audit found that GFNAudioDevice.initialize replaced its route-change observer token without removing a prior registration. Reinitialization could therefore retain duplicate NotificationCenter registrations for the process lifetime even though terminate removed only the newest token. Make observer installation idempotent and reuse the same removal path during termination. Also flush excess I420 conversion-pool buffers when a track resets or tvOS signals memory pressure, reducing the renderer high-water mark without disturbing in-use pixel buffers or the bounded six-buffer allocation policy.
SwiftFormat 0.62.1 requires multiline guard else clauses to wrap according to the repository configuration. Apply only those mechanical changes to the reconnect lifecycle implementation; behavior is unchanged.
Recent regressions showed that apparently small structural changes in per-frame code can have nonlinear effects: dispatching before frame admission retained one 4K buffer per queued closure, and reading the imported decoder callback through generic synchronization accumulated Objective-C reabstraction thunks until the decoder stack overflowed. Document why renderer admission must remain latest-frame-wins and occur before dispatch, why only one frame mailbox and drain token are allowed, and why queued drain closures must never capture frame ownership. Record that H.265 decode intentionally stays synchronous and invokes the concrete RTCVideoDecoderCallback directly. Mark the software I420 conversion, real-time audio nodes, and controller input delivery as protected paths as well. Their comments preserve pooled/vectorized conversion, preallocated lock-free audio callbacks, one input queue hop, and bounded snapshot coalescing. Extend AGENTS.md with a higher review and evidence bar for future agents: avoid per-callback tasks, actor hops, logs, serialization, blocking, and large allocations; explain hot-path changes in commits; and validate behavioral rewrites with a high-motion 4K60 device workload, queue-depth/drop metrics, memory high-water, and crash logs. This commit changes documentation only.
Integrate upstream's microphone stabilization, route recovery, lock-free activity telemetry, localized statistics HUD redesign, and stream-view updates with the video latency work on this branch. Conflict resolution preserves deterministic transport teardown and generation-plus-identity guards around signaling and peer-connection work, preventing obsolete async offers from mutating a replacement stream. The realtime capture callback remains preallocated and bounded, and both route and engine observers are removed before reinitialization to avoid duplicate notifications and retained device lifetimes. The redesigned developer HUD also retains the video pipeline's callback, enqueue, presentation, drop, supersession, backpressure, and compositor counters so scene-change regressions remain diagnosable without adding work to the render callback.
aarikmudgal
self-requested a review
July 23, 2026 11:54
Collaborator
|
Hi @andrusha , thanks for the PR. Please find below some review comments for you to cross check, verify and potentially implement. 1. Input-latency telemetry is disabled after reconnect
2. Renderer readiness callback can leave a frame stranded
3. Overlapping metrics captures can report incorrect display FPS
4. Paced H.265 timing bypasses RTP rollover handling
5. SDP failure paths leave transport resources active
Performance validation requestedThe mailbox and lower-copy decoder changes are structurally sound, but we should validate this hot-path rewrite with physical-device evidence. Maybe if possible for you to attach before and after screenshot, it would be nice because for some reason in my local testing, I couldn't pinpoint or observed the changes, but that is something I will retest again and after the changes again as well. |
aarikmudgal
marked this pull request as draft
July 31, 2026 07:22
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
2 participants
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.
Since the video rendering optimization PRs there were some which touched this subsystem and had introduced performance regressions (gfn streaming 60fps, ±30 are displayed). Those fixes aim to fix the issue and hopefully prevent it in the future.
Main cause is buffer overflow in CMSampleBuffer which is owned by closure. And recursive callback calls in h265 path.