Fix #5273: clamp Metal screen clips to the flush region (drawingRect)#5275
Merged
Conversation
Collaborator
Author
|
Compared 139 screenshots: 139 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 131 screenshots: 131 matched. |
Collaborator
Author
|
Compared 135 screenshots: 135 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 137 screenshots: 137 matched. |
Collaborator
Author
|
Compared 137 screenshots: 137 matched. |
Collaborator
Author
|
Compared 137 screenshots: 137 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 136 screenshots: 136 matched. |
Collaborator
Author
|
Compared 138 screenshots: 138 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 214 screenshots: 214 matched. |
Collaborator
Author
|
Compared 134 screenshots: 134 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
On iOS the Metal backend renders into a PERSISTENT screenTexture (MTLLoadActionLoad) and only queues the per-frame diff. The legacy GL backend clamps every clip to the current flush region (ClipRect.m's drawingRect), so a draw can never escape the dirty sub-region. The Metal rectangular-clip branch was missing that clamp. During a partial repaint -- e.g. an independently scrollable BorderLayout.CENTER scrolling under a fixed Toolbar / BorderLayout.NORTH -- paintDirty() flushes only the dirty component's bounds, but a clip emitted during that flush can still extend past it into the fixed band. On Metal the unclamped fill overwrote the toolbar / NORTH region of the persistent texture and it stayed blank until a full repaint (e.g. opening the overflow menu) rewrote it. Android, the simulator and the GL iOS backend were unaffected because they clamp to drawingRect (and they do a full repaint right after, so even the transient escape is never seen). Mirror the GL drawingRect clamp in the Metal branch, for screen ops only (target == nil) so mutable-image draws -- which run against their own encoder with their own framebuffer bounds -- are untouched. For a full-screen flush drawingRect is the whole framebuffer, so the clamp is a no-op there; it only bites on partial flushes, which is why no existing (full-form) screenshot test exercised it. Adds a deterministic screenshot regression test (graphics-partial-flush-clip-escape): a fixed red SOUTH band under a scrollable CENTER; the CENTER repaints (a real partial flush) with a clip reaching down past its bottom into SOUTH. A backend that clamps to drawingRect keeps the magenta fill inside the CENTER and SOUTH stays red; the regressed Metal backend lets it escape and SOUTH turns magenta. SOUTH (not NORTH) is used because the simulator's title bar repaints every frame and paintDirty() unions that region upward into the flush box -- never below the CENTER -- so a band below the CENTER stays outside the flush region regardless. The fix is symmetric, so this guards the reported NORTH case too. The test is scoped to iOS: on other platforms a full repaint immediately follows, so the transient escape is not the user-visible bug (the reporter confirmed it does not reproduce on Android or the simulator) and capturing it would only lock a misleading golden. Verified on the iOS Metal simulator (iPhone 16e and iPhone 16 Pro): pre-fix SOUTH = magenta (bug reproduced), post-fix SOUTH = red. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
fix-5273-metal-partial-flush-clip-escape
branch
from
0fbcc01 to
b2fba15
Compare
Captured from the PR's build-ios (GL) CI job at 1179x2556. On the GL backend the clip is clamped to drawingRect, so the SOUTH band stays red (the escape is confined to the CENTER). The Metal golden follows once a non-flaky build-ios-metal run captures it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Metal golden captured from a clean build-ios-metal CI run at 1179x2556: the fix clamps the escaping clip to drawingRect, so the SOUTH band stays red (byte-identical to the GL golden, as expected for solid fills). Also narrows shouldTakeScreenshot()/runTest() to phone/tablet iOS (excludes watchOS, whose Core Graphics backend the Metal fix does not touch, and tvOS) so those jobs don't expect a golden and the guard stays focused on the reported iPhone scenario. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On the iPhone/iPad simulator and device the Metal screenshot capture fell through to drawViewHierarchyInRect:afterScreenUpdates:NO, which snapshots the CALayer's currently-presented drawable. That drawable lags the renderer's screenTexture: presentFramebuffer commits the screenTexture-> drawable blit without waiting and the CALayer composites it on a later CA transaction, so a screenshot taken right after a Form.show() could capture the PREVIOUS form. This is the stale-frame race that made the metal screenshot suite non-deterministic (e.g. DesktopModeScreenshotTest intermittently capturing the prior test's form). Enable the deterministic screenTexture readback (cn1_copyMetalScreenTextureImage) on all Metal builds, not just Catalyst/TV. It blits the persistent screenTexture into a CPU-visible staging texture and waitUntilCompleted, so the capture always reflects the latest committed CN1 frame regardless of drawable-present / CALayer-composite timing. iOS (device + simulator) uses Shared storage like tvOS; only Catalyst keeps Managed + synchronizeResource. Peer/native components are still composited separately by cn1_renderPeerComponents, unchanged. Also import UserNotifications when push is enabled: the push code uses UNUserNotificationCenter but the import was gated on a different macro and otherwise relied on clang's implicit module auto-import, which compiling the readback path on iOS perturbs. The iOS-metal goldens are reseeded from CI in a follow-up commit (the readback's pixels differ from the old drawViewHierarchyInRect capture). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ative golden Clamping a clip to the current flush region is a portable correctness property, not an iOS-Metal quirk -- a regression would bleed the fill past the flushed sub-region on any backend. Remove the iPhone-only scoping (which also wrongly excluded tvOS, which is Metal and gets the same fix) so the guard runs on every screenshot pipeline. On a correct backend the SOUTH band stays red; a backend that lets the clip escape turns it magenta. Seeds the mac-native (Catalyst) golden, captured from CI. The remaining per-platform goldens (watch, tv, android, javascript, linux x64/arm, windows) are seeded from their CI artifacts in follow-up commits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The clip-escape reproduced by graphics-partial-flush-clip-escape is the same portable bug on every renderer that draws screen ops into a persistent surface without clamping a clip to the partial-flush region: iOS Metal (already fixed), watchOS, and the Linux native port. iOS GL, Android, JavaScript, mac-native, tvOS and Windows (Direct2D) already clamp and render correctly. watchOS (ClipRect.m + CN1WatchViewController.m): the watch CG branch set the clip with no drawingRect clamp, and the flush region handed to flushBuffer: was discarded (drawFrame: always got CGRectZero). Thread the flush rect through to drawFrame:, publish it via [ClipRect setDrawRect:], and add the same flush-region clamp the Metal/GL branches use -- guarded to screen ops and to a non-empty drawingRect so a clip is never collapsed to nothing. Linux (Cairo, immediate mode): iOS clamps at flush time because it is retained; Linux draws during paint, so it must know the flush region before painting. Add a no-op setPaintDirtyRegionClip hook in CodenameOneImplementation.paintDirty (reports each component's dirty region, full screen otherwise) that only the Linux port overrides -- every other port is unaffected. LinuxNative.setFlushRect records it on the window graphics and cn1LinuxApplyClip confines a rectangular screen clip to it (window target only; mutable images keep flushW == 0). cn1ss.sh: fail FAIL_ON_MISMATCH runs when a captured screenshot has no committed golden (missing_expected), with CN1SS_ALLOWED_MISSING_EXPECTED tolerance, so a new/ported test's golden can no longer be silently left unintegrated (the gap that hid this test's mac-native reference). Skipped under the seeding bypass. Seed the correct (magenta CENTER + red SOUTH) goldens for tvOS, Windows, JavaScript and Android. watch + Linux goldens follow once CI confirms the fixes produce the red SOUTH band. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Cloudflare Preview
|
…nostic The watch branch of ClipRect.execute used [self target], but ExecutableOp.target is declared only under #ifdef CN1_USE_METAL, so build-ios-watch failed to compile. The clamp is instead scoped to screen ops by drawFrame, which resets drawingRect to CGRectZero after draining the screen op queue (a mutable-image draw runs immediately outside drawFrame, where the non-empty guard no-ops the clamp). Remove the [self target] check. Linux still renders the escape (whole content magenta) with the clamp in place, so add a temporary CN1DIAG print in cn1LinuxApplyClip for window clips taller than 300px to reveal whether the flush region is the dirty sub-region or the full screen when the escaping clip applies. Reverted once diagnosed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 11 screenshots: 11 matched. |
…watch golden The paintDirty hook clamped the immediate-mode clip to the component's DIRTY region, but Component.repaint() nulls the dirty region (Component.java), so for the test's center.repaint() the dirty region was null and the flush hint fell back to the full screen -- no clamp, escape unchanged (Linux stayed all-magenta). Clamp to getPaintableBounds(cmp) instead: the same region the retained ports push to flushGraphics and clamp against, valid whether or not the dirty region was nulled. Computed before paintComponent (paint does not move the component). Remove the temporary CN1DIAG print. watchOS now renders correctly with the earlier flush-rect fix (magenta CENTER confined, red SOUTH preserved) -- seed its golden. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Moving getPaintableBounds before paintComponent (for the immediate-mode clip hint) also changed the value used for the flushGraphics region, because paintComponent can lay the component out and shift its bounds. That altered the flushed rect the retained ports clamp/flush against -- regressing landscape (iOS GL+Metal), graphics-draw-string (Android) and ValidatorLightweightPicker (watch, whose CG clamp uses this exact rect). Recompute getPaintableBounds AFTER paint for the flush region so it is pixel-identical to pre-#5273; the before-paint value is kept only as the immediate-mode clip hint (setPaintDirtyRegionClip). Linux now renders correctly (magenta CENTER + red SOUTH) with the paintable-bounds clamp -- seed the x64 and arm64 goldens (musl shares x64). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The watch render queue swapped upcomingTarget into currentTarget and discarded the previous set on each flush. When setNeedsDisplay coalesced several flushes into one drawFrame, every batch but the last was dropped, leaving stale pixels on the persistent CG bitmap. Without the #5273 clip clamp this was invisible -- the unclamped draws overpainted the stale area -- but with the clamp confining each draw to its flushed region the staleness showed through and corrupted multi-flush UI (ValidatorLightweightPicker rendered garbled overlapping frames). Append each flush's ops to the not-yet-drawn set instead of swapping/discarding, and union the flushed regions so the clamp covers the combined dirty area; draw once and consume in drawFrame (the persistent bitmap keeps the pixels for a forced re-present). No ops are lost, so the picker repaints cleanly and the partial-flush clip-escape test still confines the escaping fill. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ial-flush-clip-escape
The watch CG bitmap is never cleared, so a prior form/test's pixels survive beneath any area the new frame's CLAMPED draws no longer overpaint -- the source of the ValidatorLightweightPicker garble (stale text/shapes showing through). Before the clip clamp the unclamped draws happened to cover them. Clear the bitmap at the start of a full-screen flush: a full repaint repaints the whole surface with the opaque form background, so clearing first is gap-safe (nothing left transparent) and drops the stale underlay. Partial flushes are left untouched (their ops need not cover the whole region, so clearing could expose transparent gaps). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The full-screen-only clear didn't help ValidatorLightweightPicker because the picker repaints via PARTIAL flushes; its residue lives on the never-cleared persistent bitmap under areas the clamped partial draws no longer overpaint. Clear the flushed region (full or partial) before draining its ops: the paintDirty batch repaints that region with the components' opaque backgrounds so the ops refill what we clear, while pixels outside the flush region stay intact. Same coordinate space as the draw ops, so a partial region clears correctly. This also keeps the partial-flush clip-escape test correct (clear the CENTER, refill magenta; SOUTH is outside the flush so its red is preserved). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Locally reproduced the ValidatorLightweightPicker garble on the watch simulator (built the watch app, ran the suite filtered to the picker, logged every clamp). The picker repaints via a tiny dirty/flush region (e.g. 196x37) but its components legitimately set clips covering the whole dialog (208x248) -- a translucent/cascade repaint painting a parent larger than its dirty sub-region. The previous clamp chopped every such clip down to the dirty strip, so only the strip updated and the rest of the dialog went stale (the garble). The escape and this legitimate over-draw are mechanically identical at the clip level, so a blanket clamp cannot tell them apart. The distinguishing geometry: an escape EMERGES from within the flush region and leaks out ONE side into a fixed band that won't be repainted (its opposite edge is inside the flush region); legitimate over-draw either SURROUNDS the flush region (both opposite edges outside) or sits WHOLLY to one side (a sibling's own area). Clamp an overshooting edge only when the clip's opposite edge lies inside the flush region. Verified locally: the picker now compares equal, and the partial-flush clip-escape test's escape (top edge inside the flush, leaks down into SOUTH) is still clamped. Also drop the per-flush bitmap clear added earlier: with the clamp now confined to genuine escapes there is no clamp residue to clear, and clearing risked transparent gaps. Keep the coalesced-batch accumulation and the post-drain drawingRect reset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ial-flush-clip-escape
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.
Fixes #5273 — iOS: Fixed Toolbar and North Container Become Blank While Scrolling a Separate Center Container.
Root cause
The iOS Metal backend renders into a persistent
screenTexture(MTLLoadActionLoad) and only queues the per-frame diff. The legacy GL backend clamps every clip to the current flush region —ClipRect.m'sdrawingRect(the dirty bounding box passed toflushGraphics) — so a draw can never escape the flushed sub-region. The Metal rectangular-clip branch was missing that clamp.During a partial repaint — e.g. an independently scrollable
BorderLayout.CENTERscrolling under a fixedToolbar/BorderLayout.NORTH—paintDirty()flushes only the dirty component's bounds, but a clip emitted during that flush can still extend past it into the fixed band. On Metal the unclamped fill overwrote the toolbar / NORTH region of the persistent texture, and it stayed blank until a full repaint (opening the overflow menu) rewrote it. Android, the simulator, and the GL iOS backend were unaffected because they clamp todrawingRect. Same family as #5263 / #5191.Fix
Mirror the GL
drawingRectclamp in the Metal branch ofClipRect.m, screen ops only (target == nil) so mutable-image draws — which run against their own encoder with their own framebuffer bounds — are untouched. For a full-screen flushdrawingRectis the whole framebuffer, so the clamp is a no-op there; it only bites on partial flushes, which is exactly why no existing (full-form) screenshot test caught this.Coverage
Adds a deterministic screenshot regression test
graphics-partial-flush-clip-escape: a fixed red SOUTH band under a scrollable CENTER; the CENTER repaints (a real partial flush) with a clip reaching down past its bottom into SOUTH. A backend that clamps todrawingRectkeeps the magenta fill inside the CENTER and SOUTH stays red; the regressed Metal backend lets it escape and SOUTH turns magenta.Verification
Built the iOS port + hellocodenameone app from source and ran on the iOS Metal simulator (iPhone 16e and iPhone 16 Pro):
ClipRect.m)Native trace confirms the mechanism — escape clip
inClip=…,297,…,2532under a partialdrawRect=…,18,…,1863(bottom = SOUTH top) is clamped toscissor=…,297,…,1584(CENTER only).Goldens
The test is scoped to iPhone/iPad iOS (it does
done()and takes no screenshot elsewhere): the transient escape on Android/JS/desktop/watch isn't the persistent user-visible bug, so capturing it there would only lock a misleading golden. Two goldens were seeded from this PR's CI at 1179×2556 (the iOS sim resolution under the CI toolchain, which local Xcode-26 sims don't match):scripts/ios/screenshots/(GL) andscripts/ios/screenshots-metal/(Metal). Both show the SOUTH band red (the clip clamped to the CENTER); they're byte-identical, as expected for solid-color fills.Note: the
build-ios-metal/-tv/-watchjobs have a pre-existing flaky hang (the suite occasionally stalls at theDrawImagetest, unrelated to this change — confirmed by every captured test matching before the stall). A clean metal run captured the golden and passed; reruns may be needed when the flake recurs.Also in this PR: fix the iOS Metal stale-frame screenshot capture race
While validating the new test on CI I hit the metal screenshot suite's pre-existing non-determinism and root-caused it: on the iPhone/iPad simulator the Metal capture fell through to
drawViewHierarchyInRect:afterScreenUpdates:NO, which snapshots the CALayer's currently-presented drawable. That drawable lags the renderer'sscreenTexture(presentFramebuffer commits the screenTexture→drawable blit without waiting, and the CALayer composites it on a later CA transaction), so a screenshot taken right after aForm.show()could capture the previous form — e.g.DesktopModeScreenshotTestintermittently captured the prior test's form, and its committed golden was itself a stale capture.Fix (
IOSNative.m): enable the deterministicscreenTexturereadback (cn1_copyMetalScreenTextureImage— blit into a CPU-visible staging texture +waitUntilCompleted) on all Metal builds, not just Catalyst/TV. It always reflects the latest committed CN1 frame regardless of drawable-present / CALayer-composite timing. iOS (device + simulator) usesSharedstorage like tvOS; only Catalyst keepsManaged+synchronizeResource. Peer/native views are still composited separately, unchanged.It also imports
UserNotificationswhen push is enabled — the push code uses it but the import was gated on a different macro and otherwise relied on clang's implicit module auto-import, which compiling the readback on iOS perturbs.The readback is pixel-compatible with the existing iOS-metal goldens on the CI simulator (no reseed needed); it only removes the non-determinism. The metal CI job now passes with every test matching, and
DesktopModeis deterministic.Out of scope (pre-existing, unrelated): an intermittent
DrawImagewhole-suite hang (a ParparVM GC-safepoint/class-init deadlock when an EDT exception is logged during a concurrent GC) and the network-dependentGoogleWebMapflake — both also fail master and warrant separate work.