Repository navigation
Read the screen scale live when sizing the renderer - #1189
prashanth-sauron wants to merge 1 commit into
Conversation
`applyRendererSize()` divided by `_state.screenScale`, which stays 1 until the first `performLayout()`. It also runs before that, at renderer creation and from `set(size:)`, while `rendererSize` is still nil. In that state the video size passed through unclamped and `RTCMTLVideoView` multiplied it by the real scale: 5760x3240 drawables for 1080p on a 3x screen until layout re-applied. Read `currentScreenScale()` instead. It uses the same sources as the fork's `currentScaleFactor` in 150.7871.04 (`traitCollection.displayScale` / `window.backingScaleFactor`), so the divide and the multiply always agree. Adds a test that puts a view back in its pre-layout state and re-delivers the native size; it fails without this change (5760x3240) and passes with it (1920x1080) on an iPhone 16 Pro simulator.
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
pblazej
left a comment
pblazej
left a comment
There was a problem hiding this comment.
Thanks for finding and reproducing the pre-layout case. #1185 now moves the clamp into LiveKitWebRTC 150.7871.05, which reads the scale live and handles scale changes natively, so this production change no longer applies and the branch conflicts. The useful part is the test: can we fold an adapted version into #1185 and close this one?
| // passes through unclamped and the renderer multiplies it by the real scale. This reads | ||
| // the same sources as `RTCMTLVideoView.currentScaleFactor`, so the divide here and the | ||
| // multiply there always agree. | ||
| let screenScale = currentScreenScale() |
There was a problem hiding this comment.
issue (blocking): The current #1185 head deletes this function and passes native pixels to WebRTC .05. Dividing by the scale on top of that undersamples the video.
| let scale = harness.view.currentScreenScale() | ||
| guard scale > 1 else { | ||
| // At 1x the pre-layout default happens to be right, so there is nothing to tell apart. | ||
| print("[drawable] screen scale is 1 on this host; skipping") |
There was a problem hiding this comment.
suggestion (non-blocking,test): On macOS the view has no window, so the scale is 1 and this returns early. In a hosted 2× window it fails without the fix and passes with it, so host the view in a window.
|
Thanks, agreed. With the clamp in 150.7871.05 the applyRendererSize() change is obsolete, so I'll close this. I adapted the test to #1185's head: 5df3df0 (branch prashanth-sauron:prashanth/pre-layout-test, one commit on 234a32f). It no longer pokes _state. It collapses the renderer to zero bounds, redelivers the native size the way VideoRendererAdapter does, and asserts the drawable stays at the video's pixels. On a 3× simulator it fails on 150.7871.04 (5760×3240) and passes on .05 (1920×1080). It now uses the harness's hosted window on macOS, so it runs on a 2× Mac; mine only has 1× displays, where it skips. Feel free to cherry-pick it into #1185 so the authorship carries over. |
A small follow-up to #1185, from validating it in our app (results in #1185 (comment)).
applyRendererSize()divides by_state.screenScale, which stays1until the firstperformLayout(). It also runs before that, at renderer creation and fromset(size:), whilerendererSizeis still nil. In that state the video size passes through unclamped andRTCMTLVideoViewmultiplies it by the real scale, so a 1080p stream on a 3× screen gets 5760×3240 drawables until layout runs and re-applies. It also contradicts the doc comment's guarantee that withdisplayedSizeunknown the result "still caps at the video's native resolution".This reads
currentScreenScale()live instead. It uses the same sources as the fork'scurrentScaleFactorin 150.7871.04 (traitCollection.displayScale/window.backingScaleFactor), so the divide here and the multiply in the renderer always use the same factor, before the first layout and after a move between screens.The new test
drawableCapsAtVideoResolutionBeforeFirstLayoutputs a view back in its pre-layout state and re-delivers the native size: 5760×3240 without the change, 1920×1080 with it (iPhone 16 Pro simulator, 3×; on top of f420371). We haven't seen this on device: in our app every tile was already 609×342 at its first inspection, so it's a latent path rather than an observed regression.Tested