fix(compositor): keep privacy blur on its content under zoom and 3D tilt - #679
Merged
Merged
Conversation
A blur/mosaic annotation was anchored on the unzoomed screen box (s_ann) while the footage it covers is magnified in the zoomed box (s_dst) and, under a 3D preset, warped through the tilted quad. The mask stayed on a fixed output rectangle and the content it hid slid out from under it, in the preview and in the export. Rendered on D3D11: with a 2x zoom, up to 100% of the covered content was visible. FrameGeometry::privacy_mask now places the mask on the content, once for all three backends: - on the zoomed box, where the zoom-independent cut puts the content; - through the same TiltedQuad as the screen under a 3D preset, drawn by mode 10 with the corners in dst_prev/src_prev and mb.z = 1 (HLSL, MSL and WGSL); - widened to the previous frame's rect while velocity motion blur is on; - padded by one pixel, with the oval feather moved outside the ellipse; - with the blur radius / mosaic block scaled by the content's magnification, so a zoomed mosaic does not get finer. Text, figures, images and captions keep their unzoomed anchor. Tests: five geometry tests (they fail on the old placement) and an env-gated D3D11 pixel test that renders a red secret under zoom, iso and left presets and counts what stays visible.
Review of the previous commit found two places where the mask could still under-cover its content: - An oval inscribed in the rect widened to the motion-blur trail does not contain the current oval, so its rim showed through on zoom-ramp frames. A widened mask now falls back to its rectangle, like freehand. - The 1 px pad covers the half-texel bilinear fringe only at rest; under a zoom the fringe grows with the content. The pad now scales with the magnification, on the upright and the tilted path. Also corrects the doc sentence that put captions on the unzoomed box (they are anchored on the output frame).
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 ignored due to path filters (1)
📒 Files selected for processing (11)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughChangesPrivacy mask alignment
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable privacy-mask regression is established from the available evidence, so the change is ready for normal merge checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 73.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 7 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use 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 |
EtienneLescot
marked this pull request as draft
September 16, 2026 09:11
16 tasks
EtienneLescot
marked this pull request as ready for review
September 16, 2026 09:20
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
A blur/mosaic annotation is a privacy mask, but it did not follow the footage it covers. It was drawn on the screen box without the zoom (
s_ann), while the content is magnified in the zoomed box (s_dst) and, under a 3D preset, warped through the tilted quad. The mask stayed on a fixed output rectangle and what it hid slid out from under it — in the preview and in the exported file. On a D3D11 render with a 2× zoom, 89 % to 100 % of the covered content was visible.FrameGeometry::privacy_masknow places the mask on the content, once, for all three backends:s_dst. The drawn cut is taken at zoom 1, so content at fractionfof the screen lands exactly ats_dst.xy + f * s_dst.wh.TiltedQuadas the screen. Mode 10 gets the four corners indst_prev(TL, TR) andsrc_prev(BR, BL) withmb.z = 1, and inverse-warps them with the existingquad_inverse_bilinear. HLSL, MSL and WGSL are changed identically;LayerCBkeeps its 128-byte layout.Text, arrows, images and captions keep their current anchoring.
Known follow-up, not in this PR: a selected blur's outline in the editor still sits on the unzoomed rect, so it disagrees with the rendered mask while the playhead is inside a zoom. The same mismatch already exists for
ZoomFocusOverlay.Related issue
None filed.
Type of change
Release impact
Existing projects: blurs under a zoom now move and grow with the content. That is the fix, but it is a visible change.
Desktop impact
Screenshots / video
No images attached. Set
OPENSCREEN_PRIVACY_OUTwhen runningprivacy_blur_under_zoomto get one PPM per case.Testing
Windows (D3D11), this machine
cargo test -p openscreen-compositor --lib: 209 passed (204 before + 5 new geometry tests).The five geometry tests follow the content independently, through the cut and
cut_ref. Three of them fail on the old placement (mutation run).New env-gated pixel test
tests/privacy_blur_under_zoom.rsrenders a synthetic red box and counts the red pixels still visible under the mask:App export, end to end:
electron . exporton eight legacy projects with the rebuilt addon (depth 4 zoom, with and without the mask). Red pixels at 3 s with the mask: 0 in every case (rest, centred, off-centre, iso). Without the mask: 42 777 to 207 933. Logged inmanual-e2e-checklist.md.npm run build-vite(tsc + vite), Biome on the touched TS file,npm run docs:check.Linux, under WSL
nix develop .#compositor-view, with the pinned ffmpeg SDK and nix lavapipe.cargo test -p openscreen-compositor --lib --tests: 227 passed. GPU tests ran on llvmpipe; the only skips were the three tests that need external memory, ONNX or VAAPI.naga30 validation. CI uses naga 24.macOS
every_shader_entry_point_compileson the macOS job is the check.Review
Not covered
compose_framewith the preview.🤖 Generated with Claude Code
Summary by CodeRabbit