Fix graphics screenshot tests: Scale, AffineScale, TransformPerspective, TransformCamera#4875
Open
shai-almog wants to merge 11 commits intomasterfrom
Open
Fix graphics screenshot tests: Scale, AffineScale, TransformPerspective, TransformCamera#4875shai-almog wants to merge 11 commits intomasterfrom
shai-almog wants to merge 11 commits intomasterfrom
Conversation
…tive/camera The Scale, AffineScale, TransformPerspective, and TransformCamera grid tests produced empty cells in the screenshot pipelines because each test had a structural defect: - Scale + AffineScale crossed the axes in the scale formula (xScale=0.01*height, yScale=0.01*width) which clipped the gradient fill to a thin strip on portrait screens, and built the transform via separate g.translate + g.scale calls -- but g.translate(int,int) is a no-op on JavaSE and the iOS form-graphics path doesn't compose the cell offset onto fillLinearGradient either, so the fill landed off-cell. Build a single Transform that combines translate + scale and apply it once via g.setTransform. - TransformPerspective + TransformCamera passed the raw clip-space output of makePerspective / makeCamera straight to fillRect, so the rect projected to a sub-pixel region around the screen origin and rendered nothing. They also used the static Transform.isPerspectiveSupported() check, which on iOS Metal returns true for the global path but the mutable-image graphics target returns false from g.isPerspectiveTransformSupported(), so the bottom 2 cells of the 2x2 grid silently no-oped. Switch to the per-graphics check, always paint a deterministic background + frame + centred coloured marker so the cell emits comparable pixels even when the perspective branch is unsupported, then exercise the perspective API on top with a viewport-corrected matrix following the FlipTransition pattern. Verified end-to-end on the JavaSE simulator -- all four tests now emit valid PNGs with visible content. Goldens for these four tests will need regeneration on iOS Metal and Android pipelines since the rendered output is now meaningfully different (and correct). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
iOS Metal screenshot updatesCompared 84 screenshots: 80 matched, 4 updated.
Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
iOS screenshot updatesCompared 89 screenshots: 85 matched, 4 updated.
Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
…e output The first attempt at fixing TransformPerspective and TransformCamera followed FlipTransition.paint()'s viewport-mapping pattern verbatim. That pattern is correct for the full-screen flip transition but collapses at cell scale: the small per-cell scale factor multiplied back through the perspective output rounds to nearly identity, so the perspective-transformed quad lands within ~1 pixel of the deterministic marker and the only difference between "supported but invisible" and "unsupported" was a tiny dot. Build the viewport directly instead: Viewport(NDC -> cell pixels) * Perspective * Camera * ModelTranslate. The viewport is a translate- then-scale matrix that maps NDC (-1..1)^2 onto cell pixels with Y flipped (perspective NDC has +y up, screen has +y down). With the model quad at z=-300 (chosen so a 100x100 quad fits inside NDC ±1 on portrait cells with headroom for a 36 deg Y rotation), the perspective output covers about half the cell. TransformPerspective now renders a centred green quad plus a Y-rotated translucent blue quad. The rotated quad is foreshortened (left edge ~20% wider than right edge) so users can verify the perspective branch is actually applied vs just the marker. TransformCamera does the same with an orange/blue pair, but with the camera elevated (eye y=30, looking at z=-300). The ~5.7 deg downward pitch shifts the rendered quads downward in the cell so the camera test is visually distinct from the perspective test. Both tests still draw a deterministic marker + "No perspective"/"No camera" label when isPerspectiveTransformSupported() returns false on the per-graphics target (e.g., iOS Metal mutable images). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 90 screenshots: 90 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
The previous attempt built a Viewport*Perspective*Camera*ModelTranslate matrix and applied it via g.setTransform(mvp) followed by fillRect. That depends on the platform's draw path applying the 4x4 perspective matrix to rect rasterization, which fails in two places: - Android Canvas converts the 4x4 to a 3x3 Skia matrix (drops the Z axis). canvas.concat() preserves the perspective row, but rect rasterization on the hardware-accelerated canvas doesn't honour it reliably -- the screen mode renders blank while the mutable-image path (which goes through the same code) somehow does honour it. - iOS Metal mutable-image graphics flags isPerspectiveTransform Supported = false, so the entire perspective branch was skipped and only the fallback marker rendered. Replace setTransform + fillRect with manual corner projection + fillPolygon: build the same MVP matrix, then call Transform.transform Point on each of the 4 model corners (which does the homogeneous divide on every backend) and pass the resulting screen coords to fillPolygon. The polygon rasterization is platform-uniform, so the quad now renders identically across all 4 panes on iOS Metal and Android. Switch the gate from g.isPerspectiveTransformSupported() (per-graphics) to Transform.isPerspectiveSupported() (global), since the manual projection only needs the platform's Matrix.makePerspective + perspective transformPoint to work -- not the per-graphics canvas/encoder support for perspective rasterization. JavaSE still returns false and falls back to the deterministic marker. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The NativeGraphics.setTransform helper at IOSImplementation.java:4756 sets clipDirty / inverseClipDirty / inverseTransformDirty alongside the transform replacement, mirroring what scale / rotate / resetAffine do on GlobalGraphics (lines 5272 / 5281 / 5497). The Override-level impl.setTransform at line 2393 -- the one the framework actually calls when user code does g.setTransform(t) -- replaced the transform inline without setting any of those flags, so the cached inverseClip / inverseTransform pointed at the previous transform's space until the next clipRect intersection or rotate/scale call rebuilt them. The mismatch is a latent correctness bug rather than the cause of the TransformRotation / Scale screen-mode emptiness on iOS Metal -- the caches are read by getClipX/Y/W/H and clipRect-with-non-identity- transform, not by the fillRect / fillLinearGradient hot path -- but align the two setTransform paths so a future caller that does query the caches gets the correct values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The screenshot pipeline silently shipped TabsTheme_light's image bytes under MultiButtonTheme_light's filename on iOS Metal in PR #4875 -- both decoded streams reassembled to the same MD5, but the comparator had no way to tell that the bytes attributed to MultiButtonTheme_light were actually a previous test's pixels. The most likely cause is a CAMetalLayer stale-frame capture: the form transition between Tabs Theme and MultiButtonTheme hadn't finished presenting when cn1_captureView ran with afterScreenUpdates:NO, so the new test's screenshot grabbed the previous test's pixels. Add a detection signal at the emit boundary: - Cn1ssDeviceRunnerHelper computes a 64-bit FNV-1a hash of every emitted PNG and logs `png_fnv1a64=<hex>` on the existing CN1SS:INFO line. - A new package-private Cn1ssHashTracker keeps the last 64 emitted hashes; if the new test's hash matches a previously-seen test, emit a `CN1SS:WARN:test=<name> duplicate_image_with=<other> png_fnv1a64= <hex>` line so the CI comment generator can flag the affected test. - Cn1ssChunkTools verifies the reassembled PNG bytes have the same hash as the advertised value (default channel only -- the PREVIEW channel is JPEG bytes that wouldn't match). Mismatch fails extract with a clear message rather than silently emitting corrupted data. The hash is FNV-1a rather than SHA-256 / CRC32 to avoid pulling java.security or java.util.zip on the device side -- 64 bits is more than enough for accidental collision detection on real-world PNG payloads, the algorithm is small enough to inline in both the CN1 helper and the Java tooling, and the same constants in both places make the integrity check cheap to verify. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 7 screenshots: 7 matched. |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
The previous Cn1ssHashTracker used `private static final Map<String, String> hashToTest = new LinkedHashMap<>()` to track recently-emitted screenshot hashes. On the iOS Metal CI run after that landed the simulator booted, installed the app, and then never emitted a single CN1SS line -- the suite timed out at 30 minutes waiting for CN1SS:SUITE:FINISHED. Cn1ssDeviceRunner.java:215-222 documents this exact failure mode: static collections initialised via a static method call (or a method-call initializer for DEFAULT_TEST_CLASSES) both broke iOS class loading -- Cn1ssDeviceRunner failed to load before runSuite() could even log a single starting test=... entry, leaving the suite to time out at the 300s end-marker deadline. Keep all skip lookups inline to avoid triggering the same static-init failure path. The Cn1ssHashTracker static `<clinit>` ran during the host class's init path on iOS (Cn1ssDeviceRunnerHelper -> recordAndCheck), and calling new LinkedHashMap<>() during that init reproduced the documented hang. Replace the LinkedHashMap with parallel String[] arrays of fixed size MAX_TRACKED -- primitive array allocation does not touch the LinkedHashMap class init path, so the host class loads cleanly. Behaviour is identical: O(MAX_TRACKED) linear scan to detect a duplicate hash, ring-buffer-style overwrite once full. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The fix-graphics-screenshot-tests rewrites of Scale, AffineScale,
TransformPerspective and TransformCamera produce different bytes than
the previous golden set (which were captured against the broken pre-
fix tests). The Android emulator-screenshot artifact from the latest
CI run shows the four new outputs render correctly across all 4 panes
on Android API 36, so promote those bytes to the goldens.
graphics-scale / graphics-affine-scale: top half of each cell now has
a small white strip above the gradient. This is the Android Canvas
clip / scale interaction mentioned in the user's review ("shifts the
top a bit in the screen tests, that could be a good result") -- the
gradient correctly fills the cell minus a few pixels at the top
where the cell-relative translate lands the first pixel row.
graphics-transform-perspective / graphics-transform-camera: all 4
panes show the green/orange base quad with the foreshortened blue
overlay (perspective + 36 deg Y rotation) thanks to the manual
transformPoint + fillPolygon projection that bypasses Skia Canvas's
3x3 affine downcast of the 4x4 perspective matrix.
iOS Metal goldens not refreshed in this commit -- the screen-mode
cells are still empty (separate platform-side issue tracked in the
PR comments) so promoting the current iOS Metal output would lock
in the broken render.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The hash verification I added in c3011a7 used a `\b` word boundary to terminate the test-name match in the CN1SS:INFO regex: "CN1SS:INFO:test=" + Pattern.quote(testName) + "\\b[^\\n]*?\\bpng_fnv1a64=..." `\b` is a transition between a word char (alnum/underscore) and a non-word char. Both `_` and `-` are non-word chars, so for testName= graphics-draw-string the `\b` is satisfied by the boundary between `g` (word) and `-` (non-word) on both: CN1SS:INFO:test=graphics-draw-string ... CN1SS:INFO:test=graphics-draw-string-decorated ... readAdvertisedHash returned the LAST match, so it picked up graphics-draw-string-decorated's hash for graphics-draw-string. The extracted PNG bytes hashed correctly (e283696765fd487e per the emitter's own log) but my consumer-side check rejected them because they didn't match the wrong-test hash (0ffab0ff104e9327). Net effect: every test whose name is a strict prefix of another test's name silently failed extract, and the iOS UI test job hit FATAL on graphics-draw-string after passing graphics-draw-shape. Replace `\b` with `(?![A-Za-z0-9_.\-])` -- the same character class the chunk pattern uses for test names. This rejects continuation by suffix while still matching at the end-of-test-name word boundary. Apply the same fix to readTotalBase64Length, which had the identical \\b bug since its introduction (predates this PR) -- the gap-detection length check would have silently mis-trusted a different test's total_b64_len whenever a strict-prefix test name existed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Fixes #4200 |
When the primary device-runner.log loses chunks for a large test (the iOS unified-log syslog stream occasionally drops lines under load) the script falls back to log show --predicate, decodes the PNG from there, and logs "Decoded screenshot for 'X' from fallback". The fallback path was missing two things compared to the primary path: 1. It didn't append to TEST_OUTPUT_ENTRIES, so the comparator never saw those tests. iOS Metal compared 84 screenshots vs the 90 it had streams for; the missing 5 were exactly the large transition tests (CoverHorizontalTransitionTest, SlideHorizontalTransitionTest, SlideHorizontalBackTransitionTest, SlideVerticalTransitionTest, SlideFadeTitleTransitionTest) whose ~288-chunk streams hit logcat- style line drops in device-runner.log but survived in the syslog fallback. 2. It didn't decode the PREVIEW channel from the fallback log, so the PR comment for those tests had no inline thumbnail when the fallback was needed. Mirror both steps from the primary path in the fallback branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…/camera The transformPoint+fillPolygon rewrite from a6570dd produces visible foreshortened quads on all 4 panes on iOS Metal, matching the Android output. Promote the latest CI artifact bytes to the iOS Metal goldens so subsequent runs match cleanly. graphics-affine-scale / graphics-scale goldens are NOT updated -- the top half of the cell (form Graphics path) is still empty on iOS Metal because g.setTransform(t) for non-translation transforms isn't applied to fillRect / fillLinearGradient on the screen encoder, while the bottom half (mutable image path) renders correctly. That's a platform bug in the iOS Metal port, separate from this PR's test-fix scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
setTransform's default branch (TYPE_UNKNOWN composed transform) copies the native matrix data via impl.copyTransform but doesn't mark the Transform's cached state as dirty. The TRANSLATION / SCALE / IDENTITY branches all set `dirty = true` so getNativeTransform() will re-run initNativeTransform on next access. Match that contract in the default branch -- for TYPE_UNKNOWN initNativeTransform's switch hits default break and doesn't actually resync the matrix data, but the dirty flag is the externally-observable signal that the native cache is fresh. This is the lowest-risk fix attempt for the iOS Metal port bug where g.setTransform(t) with composed transforms (TYPE_UNKNOWN) silently fails to apply on the form-Graphics screen encoder while g.rotate / g.scale / g.translate (which go through ng.rotate etc.) work correctly. Both paths construct identical 4x4 matrix data in the end and call nativeSetTransform with the same 16 floats, so the exact failure mechanism is still mysterious -- but the dirty-flag contract diverges between the working and failing paths and matching it is a sane defensive change. See memory note project_metal_settransform_screen_unrendered for the open investigation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Four graphics screenshot tests in
scripts/hellocodenameone/common/.../tests/graphics/produced empty cells in the iOS and Android screenshot pipelines because each test had a structural defect.xScale = 0.01 * bounds.heightwas applied to the X axis (andyScalefromwidthto Y) — axes swapped, so the 100×100 logical fill was clipped to a thin strip on portrait screens. Also relied ong.translate+g.scaleseparately, which doesn't compose becauseg.translate(int, int)is a no-op on JavaSE and the iOS form-graphics path doesn't carry the translate intofillLinearGradient.makePerspective/makeCamerastraight tofillRect, so the rect projected to a sub-pixel region around the screen origin and rendered nothing visible. Also used the staticTransform.isPerspectiveSupported()(the global check) instead of the per-graphicsg.isPerspectiveTransformSupported()— on iOS Metal, mutable-image graphics return false for the per-graphics check, so the bottom 2 cells of each 2×2 grid silently no-oped.Fixes
All four tests now:
Transform(translate × scale) applied viag.setTransform(t)instead ofg.translate+g.scale.FlipTransition.paint()pattern.g.isPerspectiveTransformSupported()and emit a clear "No perspective" / "No camera" label when the per-graphics target doesn't support perspective.Verified end-to-end on the JavaSE simulator — all four tests now emit valid 65–72 KB PNGs with visible content (previously the cells were mostly empty).
Test plan
graphics-scale,graphics-affine-scale,graphics-transform-perspective,graphics-transform-cameraon each pipeline (the new pixel output differs from the previously broken goldens)🤖 Generated with Claude Code