fix(compositor): anchor Linux annotations on the un-zoomed screen rect - #398
Merged
Conversation
Rapport utilisateur : « les captions sont sensibles au zoom et rotation 3d sous linux, alors que ce n'est pas le cas pour les autres plateformes ». Les captions ne sont pas un calque a part -- `captionCuesToTextRegions` les projette en annotations texte -- donc c'est le placement des annotations qui derive, et seulement sur ce backend. `plan_frame` expose deux rects pour cette raison precise : `s_dst`, la boite ecran, que le zoom deplace et agrandit depuis l'issue #179, et `s_ann`, la meme boite AVANT le `remap_box` du zoom. Le contrat de `SceneAnnotation` veut le second (« deliberately NOT affected by the zoom crop »). 81839f0 l'avait pose et corrige les deux backends qui existaient alors ; il touche exactement `compositor_macos.rs` et `compositor_windows.rs`. Le port Linux a repris la formule d'avant le correctif et calculait ses rects, ainsi que sa taille de police, sur `s_dst` : sous un zoom 2.5 la bande deborde du cadre, et un preset de rotation 3D -- propriete d'une region de zoom -- la deplace lui aussi sans pour autant la poser sur le plan incline. Le natif etant desormais la seule source de pixels de l'apercu, la derive se voyait des l'edition, pas seulement a l'export. Plutot que de reparer la troisieme copie de la meme arithmetique, `FrameGeometry` expose ce dont un backend a besoin -- `annotation_dst` et `annotation_anchor_h_px` --, qui ne lisent que `s_ann` : le choix disparait du backend, ce qui est ce qui a rate ici. Deux tests. `the_annotation_rect_and_font_ignore_zoom_and_rotation` (`frame_geometry.rs`) tourne en CI sur les trois plateformes et epingle le contrat sur les accesseurs, avec ses deux garde-fous (le zoom doit bouger `s_dst`, le preset iso doit produire une rotation) sans lesquels les egalites seraient vraies pour la mauvaise raison. L'existant `the_annotation_anchor_ignores_the_zoom` prouvait que `plan_frame` CALCULE la bonne ancre, jamais qu'un backend s'en sert -- c'est exactement l'interstice ou ce bug a vecu. `compose_linux_annotation_ancree_hors_zoom` (`tests/compose_linux.rs`) le mesure sur GPU : une bande a plaque opaque rendue sans zoom, avec zoom, puis avec zoom + iso, chacune diffee contre son propre rendu sans annotation, boite englobante a 2 px pres. Opt-in comme tout ce fichier. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAnnotation placement and text sizing now use zoom-independent geometry on all native compositor platforms. Shared geometry tests and opt-in Linux composition tests cover zoomed and rotated scenes. ChangesAnnotation anchoring
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change is localized, and no actionable merge-blocking risk remains after the normal required test checks are completed. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…ends Review of #398 found the fix correct but its two supporting claims not. `annotation_dst`'s doc said it existed « pour que le backend n'ait pas le choix », yet only Linux was converted: Metal and D3D still open-coded the same four lines and the same `font_size_rel * rect[3] * rh`, and both took the anchor as a parameter named `screen_dst` — pointing at the wrong field, one token away from re-shipping #179 on two more platforms. The arithmetic now lives once, in `annotation_dst_in`. It is a free function and not a method because Windows destructures `FrameGeometry` on arrival and has no `&self` left to offer; the method forwards to it for Linux, which does hold the geometry. Both `draw_annotations` parameters are renamed `s_ann`, so the name states the contract instead of contradicting it. What this does NOT do, and the doc now says so: remove the rect CHOICE from the Metal and D3D call sites. Only rendering pixels can catch that, which is the opt-in Linux test. The unit test grew the two assertions that make it more than a tautology over `s_ann`: that `annotation_dst_in` agrees with the method (the path Metal and D3D take is now covered on all three CI jobs), and that the same function fed `s_dst` returns a DIFFERENT rect — without which a constant implementation would satisfy everything above it. Comment corrections, all of them things the review caught as false: - `compose_linux.rs` claimed the drift was invisible in the preview and hit only the export. The native compositor is the preview's sole pixel source (`AnnotationOverlay.tsx` paints selection chrome, not pixels), so it was visible while editing — and the claim contradicted the comment this same branch added to `compositor_linux.rs`. - « son empreinte est le rect entier » was wrong: the plate comes from `glyphs.plate`, hugged to the laid-out lines. The measurement is still sound, the stated reason was not. - « frame 90 tombe au coeur de la region » was wrong twice: `FPS` is a hard 60, so frame 90 is 1.5 s, and `set_timeline_time(3.0)` is what actually drives sampling. Fixed here and in `compose_linux_ecran_tilte`, which is where the error was copied from. - The 2 px tolerance note undersold the error by an order of magnitude: the band drops ~450 px and leaves the frame, so « bande absente » is what fires. That assert now names `s_dst` instead of leaving the reader to guess why a band went missing. One `seek_to` for the six renders instead of six identical ones, as `compose_linux_ecran_tilte` already does — same AVFrame, so the decoder cannot contribute a difference that reads as a displacement. Not compiled locally: the crate's build.rs needs vendored ffmpeg headers absent from this machine. The three CI Rust jobs are the verification. Co-Authored-By: Claude <noreply@anthropic.com>
1 task
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
On Linux the compositor anchored annotations — and therefore captions, which are projected into text annotations — on
s_dst, the screen box that the zoom moves and grows, instead ofs_ann, the same box before the zoom. So a caption drifted and swelled under a zoom, ran past the frame edge at a high scale, and moved again under a 3D rotation preset (which is a property of a zoom region, so it brings the box with it). Windows and macOS render the same project with the caption nailed in place. Since the native compositor is the preview's sole pixel source, this was visible while editing, not only at export.This is #179's regression, fixed once in 81839f0 for the two backends that existed then. That commit touched
compositor_macos.rsandcompositor_windows.rs; the Linux backend was ported from the pre-fix code and kept the old formula.The fix is to stop letting each backend redo the arithmetic. It now lives once, in
frame_geometry::annotation_dst_in, and all three backends call it: Linux throughFrameGeometry::annotation_dst, Metal and D3D directly.annotation_anchor_h_pxdoes the same for the font size, which is a fraction of that rect's height.What the second commit changes, and why
The first commit fixed Linux by adding two accessors and calling them there. Reviewing it turned up that the two claims holding it up were both weaker than stated, so a31452b makes them true rather than leaving the comments asserting them.
The arithmetic was centralized for one backend of three.
annotation_dst's doc said it existed « pour que le backend n'ait pas le choix », butcompositor_macos.rsandcompositor_windows.rsstill open-coded the same four lines and the samefont_size_rel * rect[3] * rh. Worse, both received the anchor as a parameter namedscreen_dst— named after the wrong field, one token away from re-shipping #179 on two more platforms. Both now call the shared function and both parameters are renameds_ann.It is a free function rather than a method because Windows destructures
FrameGeometryon arrival atcompose_frameand has no&selfleft to offer by the time it draws annotations; the method forwards to it for Linux, which does hold the geometry.What is still not enforced, now stated in the doc instead of contradicted by it: the choice of rect at the Metal and D3D call sites. Their
draw_annotationstakes the anchor as a parameter, so passings_dstremains possible on two backends of three. Only rendering pixels can catch that, which is the opt-in Linux test below. The parameter name and the tests are what stand in for a type-level guarantee.The unit test was close to a tautology.
annotation_dst/annotation_anchor_h_pxare pure functions ofs_ann, andthe_annotation_anchor_ignores_the_zoomalready proveds_annignores the zoom — so the equalities held by construction, and the rotation half could not fail at all (s_anniss_base, which has no rotation term). It now also asserts thatannotation_dst_inagrees with the method — putting the path Metal and D3D take under test on all three CI jobs, where it previously had none — and that the same function feds_dstreturns a different rect, without which a constant implementation would satisfy everything above it.Four comments said things that were not true.
compose_linux.rsclaimed nobody noticed the bug because « la preview (web) reste juste » and only the export diverged. The native compositor is the preview's sole pixel source —AnnotationOverlay.tsxpaints the selection chrome, not the annotation pixels — so the drift was visible while editing. The claim also contradicted the comment this same branch added tocompositor_linux.rs.glyphs.plate, hugged to the laid-out lines, not the 0.92x0.22 box. The measurement is still sound (the plate is a deterministic function ofdstandfont_size_px, boths_ann-anchored); the stated reason was not.FPSis a hard-coded 60, so frame 90 is 1.5 s, andset_timeline_time(Some(3.0))is what actually drives sampling — the90.0argument reaches nothing in that test. Corrected here and incompose_linux_ecran_tilte, where the error was copied from.bande absente— which now namess_dstinstead of leaving the reader to guess why a band went missing.One
seek_tofor the six renders instead of six identical ones, matchingcompose_linux_ecran_tilte: same AVFrame, so the decoder cannot contribute a difference that reads as a displacement.Related issue
Fixes #397
Type of change
Release impact
Desktop impact
s_ann; the arithmetic they now call is identical to what they open-coded)Testing
the_annotation_rect_and_font_ignore_zoom_and_rotation(frame_geometry.rs) — runs in CI on all three platforms. Plans the golden scene plain, zoomed, and zoomed+iso, then assertsannotation_dst,annotation_dst_inandannotation_anchor_h_pxreturn the same rect and height in all three. Four guards keep it honest:s_dstmust actually differ under zoom, theisopreset must produce a non-identity rotation, the free function must agree with the method, and feeding its_dstmust move the rect.compose_linux_annotation_ancree_hors_zoom(tests/compose_linux.rs) — the on-device proof, and the only test that covers the call site: renders a caption band with an opaque plate three times (no zoom / zoom 2.5 / zoom 2.5 +iso), each diffed against its own no-annotation render, asserting the bounding box lands within 2 px of the un-zoomed one. Opt-in (OPENSCREEN_LINUX_COMPOSE=1+crates/fixture/screen.mp4), so CI compiles it but does not run it.annotation_scenegrew anannotation_scene_with_zoomsibling; the four existing callers are unchanged and delegate through it.Not verified locally, and I want to be explicit about it. Written on Windows:
compositor_linux.rsandtests/compose_linux.rsarecfg-gated to Linux and cannot be compiled here,compositor_macos.rslikewise, and the crate'sbuild.rsneeds vendored ffmpeg headers this machine doesn't have — socargo checkdoes not run either. All four files were syntax-checked withrustfmt, which is not a substitute for the borrow checker. The threeci.ymlRust jobs (Linux test, Windows check, macOS test) are the verification; theframe_geometrytest runs in all three and the Linux job compiles the newcompose_linuxcase. If anything is red I'll fix it on this branch.No TypeScript changed, so the JS suite and lint are untouched.
Summary by CodeRabbit
Bug Fixes
Tests