Repository navigation
Clamp the Metal render target to the pixels actually displayed - #1185
xianshijing-lk wants to merge 10 commits into
Conversation
RTCMTLVideoView sizes its CAMetalLayer drawables from the video's resolution multiplied by the screen scale, independent of the view. On a 3x iPhone a 1080p stream therefore allocated 5760x3240 drawables whether it was shown fullscreen or in a 120x68pt tile -- 324x the pixels displayed, and 213.6 MB of pool per view at BGRA8 with the default three drawables. Upstream assigns drawableSize = videoFrameSize, already ignoring the view; the fork adds the contentScaleFactor multiply (05fc61b, an xcframework bump only). autoResizeDrawable is NO, so the view's bounds never enter into it. Hand the renderer min(video pixels, displayed pixels) instead, aspect preserved and never upscaled past the source. VideoView is the only layer that knows its own geometry, so the clamp lives there and is reapplied from set(size:), performLayout, and renderer creation. The helper returns points, not pixels: the fork multiplies whatever it is given by the screen scale. That couples us to current fork behaviour, so the tests assert the resulting drawableSize rather than the helper's output -- a dependency bump that changes the contract fails loudly instead of shipping blurry video. Measured on an iPhone 14 (iOS 27.0.1, scale 3), 4 x 1080p tiles at 120x68pt, both arms in one process: before 5760x3240 213.6 MB pool/view 169.1 MB footprint/view after 362x204 0.8 MB pool/view 24.7 MB footprint/view Pool across the four tiles falls from 854.3 MB to 3.4 MB; measured footprint is 6.8x lower and draw time 4.8x faster. Tests cover aspect preservation across ten portrait/landscape/square and fit/fill combinations, tile<->fullscreen resize, orientation change, simulcast layer change, renderer recreation and track detach (weak refs confirm the old MTKView and CAMetalLayer are released), and an on-device image comparison validated by a negative control. Also ruled out, and deliberately not changed: the absent CVMetalTextureCacheFlush is not a leak -- 60 frames through a pool capped at 8 buffers starve nothing and steady-state retention is one frame; and maximumDrawableCount 3->2 now saves ~2 MB/view against real scheduling risk, since the semaphore bounds GPU work rather than drawable lifetime. CLT-3537
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A view may be reused across tracks, and the cached native size was not reset when `track` changed. Remote tracks learn that size from `VideoRendererAdapter.setSize`; local capture delivers frames straight to the renderer and never calls it. So a view moved from a remote track to a local one kept sizing its renderer from the resolution of the track it no longer showed, with nothing able to correct it. Reset it alongside the other per-track state, and take it from the frame in `render(frame:)` as well. The second half also gives local tracks a correct clamp for the first time: with `videoFrameSize` left at zero the renderer fell back to `drawableSize = bounds.size`, which is in points, so local preview rendered at a third of the display resolution on a 3x screen. Both sources agree on orientation. The adapter swaps the axes for a 90/270 rotation, and `dimensions` in `render(frame:)` already has rotation applied — the frame-derived value additionally honours `rotationOverride`, which the adapter does not see, so it matches what `-drawableSize` uses for its own flip. switchingTracksDoesNotCarryTheOldNativeSize reproduces the reported case: a 1080p view dropped to a 320x180 remote size and then handed a new track renders at 320x180 without this change, 1920x1080 with it. rotatedFramesDoNotOversizeTheDrawable covers 90 and 270 against a portrait viewport, which the earlier geometry cases missed by using portrait-shaped buffers at rotation zero. Reported by Devin on #1185. CLT-3537
|
Thanks — reviewed both. One was a real bug, the other I believe is incorrect; details below. Fixed in f89c4cc. 🔴 Previous track size distorts new video — correct, fixedConfirmed on both counts: the track setter resets Reproduced it as a test before fixing. A 1080p view dropped to a 320×180 remote size and then handed a new track: Took the recommended fix in full — reset on track change and derive the value from incoming frames. The second half turned out to matter beyond this bug: with 🟡 Rotated streams allocate oversized drawables — incorrect, but the test gap was realThe premise is that CGSize current_size = (videoFrame.rotation % 180 == 0) ?
CGSizeMake(videoFrame.width, videoFrame.height) :
CGSizeMake(videoFrame.height, videoFrame.width);So both sides of the comparison are already in display orientation — the adapter rotates, and 540×960, not the predicted ~1707×960. The worked example also doesn't follow: That said, the observation that rotation was untested was fair — my geometry cases used portrait-shaped buffers at rotation zero, which is not the same thing. One genuine edge case nearby, which the fix incidentally closes: |
|
Re-verified on device at Correctness — 14 tests, 3 suites, all pass on hardware: The track-switch fix and the rotation behaviour both hold on real hardware, not just in simulation. Benchmark unchanged by the fix — 4 × 1080p tiles at 120×68pt:
Pool across four tiles: 854.3 MB → 3.4 MB. Image comparison also unchanged: 0.100/255 on gradient and 7.690/255 on the checkerboard, against controls of 0.315 and 29.465. |
reconcileNativeRenderer replays the track's cached frame straight into a newly built renderer, bypassing render(frame:). That is the only path where a frame is drawn without anything deriving the native size from it, and it matters when the track's capture has stopped: nothing further arrives to correct the target. An MTKView that has not drawn yet still has autoResizeDrawable set -- RTCMTLRenderer clears it in addRenderingDestination:, which runs lazily on the first draw -- so an unsized renderer sizes itself to bounds x scale. That happens to equal the clamp when the source is larger than the view, and overshoots badly when it is smaller: a 320x180 track replayed into a 1200x675pt view allocated 3600x2025 drawables, 83.4 MB of pool for 0.66 MB of content. Seed the native size from the frame about to be replayed, honouring rotationOverride so it agrees with what -drawableSize uses for its own flip, then size the renderer before drawing into it. cachedFrameOnASwitchedToTrackIsSizedCorrectly covers both directions; only the smaller-source case regresses without this, which is why the first version of the test missed it. Both views are exactly 16:9: the clamp scales by the larger axis ratio so neither axis is under-sampled, which overshoots a per-axis minimum by a pixel or two when the aspects differ. detachingTheTrackReleasesTheRenderer now polls for 10s. Four extra harnesses ahead of it in a serialized suite left enough queued on the RTC executor that the default 2s was marginal. Reported by Devin on #1185. CLT-3537
The cached-frame cases pushed the file past the 400-line limit and its type past 250, which SwiftLint flags. The two halves answer different questions anyway: static geometry (aspect across fit/fill and source orientation) versus what the render target does when the view, the device orientation, the source resolution or the track changes under it. Split on that line rather than by line count. No test bodies changed. CLT-3537
|
Fixed in 36dd979. The conclusion is right — but two of the specifics aren't, and they're the reason the bug is easy to miss. The example given is the benign configurationI tested the scenario as described first — a 120×68pt view switching to a paused 1920×1080 track — and it passed: The bug needs the inverse: a source smaller than the view. 3600×2025 is 126× the pixels warranted — 83.4 MB of drawable pool for 0.66 MB of content. Had I only tested the configuration in the report I'd have pushed back, wrongly. "The Metal renderer then sizes its drawable from the frame itself" — it doesn't
That is exactly why the two configurations differ. The fixTaken as recommended: seed Also in 3756854: split the lifecycle cases into their own suite, since the new tests pushed the geometry file past the 400-line limit. One thing worth a human eye
|
`RTCMTLVideoView` multiplied `videoFrameSize` by the screen scale, but `setSize:` already reports pixels. A 1080p stream on a 3x iPhone allocated 5760x3240 drawables no matter how small the view was. The multiply came in with #165. The part of that patch that fixed livekit/client-sdk-swift#628 was the zero `videoFrameSize` fallback, which stays. `drawableSize` now uses the video's pixel size, shrunk to the pixels the view covers when the view is smaller, with aspect preserved. It never upscales. Related: livekit/client-sdk-swift#1185 (CLT-3537) works around the same bug in the Swift SDK by dividing by the screen scale before calling `setSize:`. That helper has to be removed in the Swift PR that bumps to a release with this change, otherwise the drawable ends up 3x too small. ### Testing Debug builds of this branch (iOS device and macOS) against an unmodified Swift SDK `main` `VideoView`, which passes the raw video size to `setSize:`. So the clamp below comes from this change alone. The renderer test suites were borrowed from livekit/client-sdk-swift#1185 without its `VideoView` change. Baseline is the shipped 150.7871.04. **iPhone 17 Pro Max, iOS 27.2 (3x)**, four 1080p tiles at 120x68pt: | | drawableSize | pool MB/view | footprint MB/view | |---|---|---|---| | 150.7871.04 | 5760x3240 | 213.6 | ~170 | | this branch | 363x204 | 0.8 | 5.2 | Image diff vs the unclamped render: checkerboard 7.74/255 (broken 48x27 control 29.2). Gradient differences stay within +-1 LSB. **macOS 27, built-in Retina (2x)**, views hosted in a window: | case | 150.7871.04 | this branch | displayed px | |---|---|---|---| | 1080p, 120x68pt fill | 3840x2160 | 242x136 | 240x136 | | 1080p, 300x300pt fill | 3840x2160 | 1067x600 | 1067x600 | | 1080p, 300x300pt fit | 3840x2160 | 600x338 | 600x338 | | 1080p rotated 90, 180x320pt | 2160x3840 | 360x640 | 360x640 | | 1080p, 1200x675pt | 3840x2160 | 1920x1080 | 2400x1350 | Resizing 120x68 -> 800x450 -> 120x68pt goes 242x136 -> 1600x900 -> 242x136. Geometry suite from livekit/client-sdk-swift#1185 (aspect and fill across 10 combinations, resize, orientation, source resolution change, renderer recreation, detach): fails on 150.7871.04, passes on this branch except for two cases that test the Swift side. The rotation case calls `setSize:` with the unrotated size, which `RTCVideoRendererAdapter` never does. The track switch case (iOS only) needs the Swift SDK to report local track sizes, which #1185 adds. Note that macOS runs of that suite stay at scale 1 because the views are not in a window, hence the Retina table above. ### Not yet done - Cherry-pick to m144_release
|
I'm the reporter from Sauron (the ~270 MB/stream report). Validated this branch
*At 12 tiles on 2.19.0 the device ran out of memory: iOS's jetsam report shows Visual quality: tiles look as sharp as before |
Thanks, @prashanth-sauron ! Really appreciate you reporting the issue and helping with the testing. Our current plan is to land a proper fix in WebRTC first, which should address the issue across all SDKs. We'll then update Swift to use the new WebRTC release and land the Swift PRs. We're targeting early next week. If the WebRTC release takes longer than expected, we'll go ahead with the current Swift PR as an interim fix and update it once the new WebRTC release is available. |
Five separate failures, all from the new tests.
Three are compile errors under Swift 6.1 and 26.6 that Xcode 27
accepts, so they never showed locally:
- reportEnvironment() is nonisolated but reads
VideoView.isMetalAvailable() and .shouldAvoidMetalRenderer, which
are MainActor-isolated because VideoView is a UIView/NSView
subclass. Mark it @mainactor.
- poll takes a @sendable closure, and the renderer check reads
view.subviews, which is MainActor-isolated. Use pollOnMain.
- harnesses.first.flatMap resolves `first` to the first-where
overload, which has no flatMap. Spell the lookup out.
The fourth is the device benchmark running on Mac Catalyst, which is
os(iOS) and not a simulator, so the guard let it through onto a Mac
GPU with a Mac window server. Exclude macCatalyst explicitly.
The fifth is three files failing swiftformat --lint: hoisted try and
await, inline loop bodies, indentation and a trailing blank line. CI
also runs swiftlint --strict, where warnings are errors. Both are
clean across all 483 files now.
CLT-3537
rendererReleasesFramesBackToThePool failed on the E2E macOS runner while steadyStateRetentionIsBounded, which allows a pool of 16, passed in the same run. The difference was headroom, not retention: frame delivery is asynchronous, so several frames are legitimately in flight between the capturer and the renderer at once, and a loaded machine keeps more of them there than a quiet one. Eight buffers was not enough room for that. Raising the number alone would weaken what the test is for, so it now tells the two cases apart. On exhaustion it drains and retries, counting a transient stall separately from an unrecoverable one. A renderer that genuinely holds frames does not give them back, so the retry fails too and the assertion still trips; frames merely in flight drain and the push continues. The diagnostic prints both counts, so a future failure says which. Threshold raised to 12 for headroom. CLT-3537
Picks up webrtc-sdk/webrtc#325. The Metal renderer now sizes its drawable from the video's pixel size, clamped to the pixels the view displays, instead of multiplying the video size by the screen scale.
Since 150.7871.05 the renderer clamps the drawable itself and no longer multiplies by the screen scale, so dividing by it here made the drawable 3x too small on a 3x screen. Pass the native video size to the renderers unchanged and drop the helper. The renderer relayouts on bounds and scale changes itself, so layout no longer has to push a new size.
render(frame:) and the cached frame replay rotated the size by rotationOverride. The renderer applies the override itself and swaps the axes of the size it was given when the override and the frame rotation disagree, so with an override the axes were swapped twice. A landscape frame overridden to 90 degrees in a 180x320 view got a 180x102 drawable. Rotate by the frame's rotation only, as VideoRendererAdapter does. The test harness now reports the adapter's rotated size too.
A view outside any window has no backing scale, so the macOS runs of the renderer suites all rendered at 1x and never exercised the screen scale the drawable clamp depends on. In a window they pick up the display's scale, 2x on a Retina Mac. The reporting helpers move into an extension to keep RendererProbe within the type body length limit. Also fixes a geometry test comment that described the removed Swift side clamp.
|
CI is at 27/29. Everything that was mine is fixed; the two remaining failures are pre-existing and reproduce on Fixed (mine)Nine checks were failing, from five causes — all in the new tests, none in
On the retention test
Raising the number alone would have weakened what the test is for, so it now tells the two apart: on exhaustion it drains and retries, counting a transient stall separately from an unrecoverable one. A renderer that genuinely holds frames doesn't give them back, so the retry fails too and the assertion still trips. Threshold raised to 12, and the diagnostic prints both counts so a future failure says which. Verified on macOS, both render modes: The macOS E2E job now passes. Not mine — pre-existingBoth remaining failures are visionOS, where every file in this PR is excluded by
Both visionOS jobs also failed on Happy to rerun the two visionOS jobs if you'd like confirmation, but I didn't want to churn CI to chase a flake I can't fix in this PR. |
Fixes CLT-3537.
A customer reported ~270 MB per 1080p stream on iPhone with
renderMode = .auto, attributing ~250 MB to the Metal renderer. They were right.The problem
RTCMTLVideoViewsizes itsCAMetalLayerdrawables from the video's resolution multiplied by the screen scale, independent of the view. Upstream assignsdrawableSize = videoFrameSize(already ignoring the view); our fork adds thecontentScaleFactormultiply — that arrived via 05fc61b, which touches no Swift, only an xcframework bump.RTCMTLRenderersetsautoResizeDrawable = NO, so the view's bounds never enter into it.On a 3x iPhone a 1080p stream therefore allocates 5760×3240 drawables whether shown fullscreen or in a 120×68pt tile — 324x the pixels displayed, and 213.6 MB of pool per view at BGRA8 with three drawables.
Their own "<4 MB per stream after decode" measurement was exactly one NV12 1080p frame (1920×1080×1.5 = 3.0 MB). Everything else was drawable pool.
The fix
Hand the renderer
min(video pixels, displayed pixels), aspect preserved, never upscaled past the source.VideoViewis the only layer that knows its own geometry, so the clamp lives there and is reapplied fromset(size:),performLayout, and renderer creation.The helper returns points, not pixels — the fork multiplies whatever it is given by the screen scale. That couples us to current fork behaviour, so the tests assert the resulting
drawableSizerather than the helper's output: a dependency bump that changes the contract fails loudly instead of shipping blurry video. Longer term the clamp belongs in the fork (drawableSize = min(videoFrameSize, bounds.size * contentScaleFactor)), after which this can be deleted.Measured on device
iPhone 14, iOS 27.0.1, scale 3,
.auto→.metal. Four 1080p tiles at 120×68pt, both arms in one process:Pool across the four tiles: 854.3 MB → 3.4 MB. Measured footprint 6.8x lower (144 MB/tile), draw time 4.8x faster.
Rendering correctness
Aspect ratio preserved exactly across 10 combinations of portrait/landscape/square source × portrait/landscape view × fit/fill. The
.fillmismatch cases confirm the clamp readsrendererSize(which overflows the view) rather than view bounds — using bounds there would have cropped the image.Lifecycle: tile↔fullscreen resize (exact round trip), orientation change, simulcast layer change, renderer recreation and track detach — with weak refs confirming the old
MTKViewandCAMetalLayerare released rather than ratcheting.On-device image comparison, validated by a negative control:
The control matters: without it a difference of zero is ambiguous between "identical" and "the comparison is blind". Three earlier versions of this measurement were silently broken — a flat source frame, a view outside a window so
drawHierarchyreturned stale captures, and a sleeping display. Each produced a clean-looking 0.000.Investigated and deliberately not changed
CVMetalTextureCacheFlush.RTCMTLNV12Renderernever flushes its texture cache (the symbol is absent from the framework's import table), which looked like a retention leak. It isn't: 60 frames through aCVPixelBufferPoolcapped at 8 buffers produced zero allocation failures, and steady-state retention is exactly 1 frame — identical to the sample-buffer path, confirmed on device.maximumDrawableCount3 → 2. The semaphore (kMaxInflightBuffers = 1) bounds GPU work, not drawable lifetime — a drawable returns to the pool when the compositor is done, not at command-buffer completion. After the sizing fix this saves ~2 MB/view against real scheduling risk, so it is left alone.Not covered
GPU time proper (needs Instruments' Metal System Trace;
ms/drawis CPU-side encode) and frame-drop counts.Note on the device test
VideoViewDeviceBenchmarkTestsis#if os(iOS) && !targetEnvironment(simulator)— excluded everywhere CI runs, so this PR is independent. Exercising it on hardware needs #1184, which fixes three pre-existing LKTestHost build failures. Merge that first if you want to reproduce the numbers above.Simulator and macOS: 12 tests across 3 suites pass on both. SwiftLint clean.