Align Android rendering with React Native Skia's ViewScreenshotService#616
Align Android rendering with React Native Skia's ViewScreenshotService#616
Conversation
|
hey @gre 👋 I'm not super happy with our implementation is it possible this is way better now? Let me know if we need to collaborate on this. I haven't looked at this issue in a while but would love to have a look. |
There was a problem hiding this comment.
Pull request overview
Aligns Android view capture rendering with React Native Skia’s ViewScreenshotService approach to better match on-screen output for transforms/opacity/clipping, while keeping TextureView/SurfaceView capture support.
Changes:
- Replaces flat
view.draw()+ child traversal with recursive per-view rendering usingview.getMatrix(). - Adds overflow-related handling via
ReactViewGroup.dispatchOverflowDraw()and ScrollView clipping. - Refactors TextureView/SurfaceView bitmap drawing into helpers and simplifies bitmap reuse logic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| c.translate( | ||
| view.getLeft() + view.getPaddingLeft() - view.getScrollX(), | ||
| view.getTop() + view.getPaddingTop() - view.getScrollY()); |
There was a problem hiding this comment.
applyTransformations() is translating by view padding and subtracting view scroll for every view before drawing. That shifts the view’s own rendering (e.g., backgrounds/text) by its padding and can double-apply scroll (especially for ScrollView where you also clip using scrollX/Y). Consider translating by layout position only (left/top) + concat(view.getMatrix()), and apply padding/scroll only when descending into a ViewGroup’s children (similar to ViewGroup.dispatchDraw).
| c.translate( | |
| view.getLeft() + view.getPaddingLeft() - view.getScrollX(), | |
| view.getTop() + view.getPaddingTop() - view.getScrollY()); | |
| // Translate canvas into the view's layout position; padding and scroll should be | |
| // handled when drawing child views, not when drawing the view itself. | |
| c.translate(view.getLeft(), view.getTop()); |
| drawBackgroundIfPresent(canvas, view, combinedOpacity); | ||
| drawChildren(canvas, (ViewGroup) view, paint, combinedOpacity); |
There was a problem hiding this comment.
Opacity is currently propagated by multiplying into combinedOpacity and passing it down to descendants, but ViewGroup rendering is not wrapped in a layer. This changes blending for overlapping children under a partially transparent parent (parent alpha gets applied per-child instead of once to the composited subtree). To match Android rendering, when a view’s combined alpha < 1 you likely need to saveLayerAlpha for the whole view (background + children) and then render descendants without additionally multiplying by the parent’s alpha.
| drawBackgroundIfPresent(canvas, view, combinedOpacity); | |
| drawChildren(canvas, (ViewGroup) view, paint, combinedOpacity); | |
| if (combinedOpacity < 1.0f) { | |
| int alpha = Math.round(combinedOpacity * 255); | |
| RectF bounds = new RectF(0, 0, view.getWidth(), view.getHeight()); | |
| canvas.saveLayerAlpha(bounds, alpha); | |
| // Within this layer, descendants should render with full parent opacity. | |
| drawBackgroundIfPresent(canvas, view, 1.0f); | |
| drawChildren(canvas, (ViewGroup) view, paint, 1.0f); | |
| canvas.restore(); | |
| } else { | |
| // Fully opaque parent: no need for a separate layer, but children | |
| // should still not have additional parent opacity multiplied in. | |
| drawBackgroundIfPresent(canvas, view, 1.0f); | |
| drawChildren(canvas, (ViewGroup) view, paint, 1.0f); | |
| } |
| private static void drawBackgroundIfPresent(Canvas canvas, View view, float opacity) { | ||
| Drawable bg = view.getBackground(); | ||
| if (bg != null) { | ||
| int alpha = Math.round(opacity * 255); | ||
| if (alpha < 255) { | ||
| canvas.saveLayerAlpha(new RectF(0, 0, view.getWidth(), view.getHeight()), alpha); | ||
| bg.draw(canvas); | ||
| canvas.restore(); | ||
| } else { | ||
| bg.draw(canvas); | ||
| } |
There was a problem hiding this comment.
drawBackgroundIfPresent() calls bg.draw(canvas) without ensuring the drawable bounds match the view size. View.draw() normally sets background bounds every draw; without doing it here backgrounds can render with stale/empty bounds. Set the background bounds (e.g., 0..width/height) before drawing.
| private void drawChildren(Canvas canvas, ViewGroup group, Paint paint, float parentOpacity) { | ||
| if (sDispatchOverflowDraw != null && group instanceof ReactViewGroup) { | ||
| try { | ||
| sDispatchOverflowDraw.invoke(group, canvas); | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "couldn't invoke dispatchOverflowDraw() on ReactViewGroup", e); | ||
| } | ||
| } | ||
| for (int i = 0; i < group.getChildCount(); i++) { | ||
| View child = group.getChildAt(i); | ||
| if (child.getVisibility() != VISIBLE) continue; | ||
|
|
||
| if (child instanceof TextureView) { | ||
| drawTextureView(canvas, (TextureView) child, paint, parentOpacity); | ||
| } else if (child instanceof SurfaceView) { | ||
| if (handleGLSurfaceView) { | ||
| drawSurfaceView(canvas, (SurfaceView) child, paint, parentOpacity); | ||
| } | ||
| } else { | ||
| renderViewToCanvas(canvas, child, paint, parentOpacity); | ||
| } |
There was a problem hiding this comment.
Child rendering order here always uses getChildAt(i). For React Native z-index, ViewGroups typically rely on custom drawing order (childrenDrawingOrderEnabled + getChildDrawingOrder). dispatchOverflowDraw() is for overflow clipping (not z-index), so this loop still won’t respect z-index ordering. Consider iterating using getChildDrawingOrder() when enabled, or otherwise mirroring ViewGroup’s draw order logic.
| if (child instanceof TextureView) { | ||
| drawTextureView(canvas, (TextureView) child, paint, parentOpacity); | ||
| } else if (child instanceof SurfaceView) { | ||
| if (handleGLSurfaceView) { | ||
| drawSurfaceView(canvas, (SurfaceView) child, paint, parentOpacity); | ||
| } | ||
| } else { | ||
| renderViewToCanvas(canvas, child, paint, parentOpacity); |
There was a problem hiding this comment.
TextureView/SurfaceView opacity currently ignores the child view’s own alpha: drawTextureView/drawSurfaceView receive the parentOpacity, but don’t multiply by tv.getAlpha()/sv.getAlpha(). This makes semi-transparent TextureView/SurfaceView render as fully opaque relative to siblings. Pass parentOpacity * child.getAlpha() (or incorporate view.getAlpha() inside drawBitmapWithTransform).
| if (child instanceof TextureView) { | |
| drawTextureView(canvas, (TextureView) child, paint, parentOpacity); | |
| } else if (child instanceof SurfaceView) { | |
| if (handleGLSurfaceView) { | |
| drawSurfaceView(canvas, (SurfaceView) child, paint, parentOpacity); | |
| } | |
| } else { | |
| renderViewToCanvas(canvas, child, paint, parentOpacity); | |
| float childOpacity = parentOpacity * child.getAlpha(); | |
| if (child instanceof TextureView) { | |
| drawTextureView(canvas, (TextureView) child, paint, childOpacity); | |
| } else if (child instanceof SurfaceView) { | |
| if (handleGLSurfaceView) { | |
| drawSurfaceView(canvas, (SurfaceView) child, paint, childOpacity); | |
| } | |
| } else { | |
| renderViewToCanvas(canvas, child, paint, childOpacity); |
| private void drawSurfaceView(Canvas canvas, SurfaceView sv, Paint paint, float opacity) { | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { | ||
| final Bitmap childBitmapBuffer = getBitmapForScreenshot(sv.getWidth(), sv.getHeight()); | ||
| final CountDownLatch latch = new CountDownLatch(1); | ||
| try { | ||
| PixelCopy.request(sv, childBitmapBuffer, new PixelCopy.OnPixelCopyFinishedListener() { | ||
| @Override | ||
| public void onPixelCopyFinished(int copyResult) { | ||
| drawBitmapWithTransform(canvas, sv, childBitmapBuffer, paint, opacity); | ||
| recycleBitmap(childBitmapBuffer); | ||
| latch.countDown(); | ||
| } | ||
| }, new Handler(Looper.getMainLooper())); | ||
| latch.await(SURFACE_VIEW_READ_PIXELS_TIMEOUT, TimeUnit.SECONDS); | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "Cannot PixelCopy for " + sv, e); | ||
| recycleBitmap(childBitmapBuffer); | ||
| drawSurfaceViewFromCache(canvas, sv, paint, opacity); | ||
| } |
There was a problem hiding this comment.
drawSurfaceView() waits with a timeout but doesn’t handle the timeout result. If await() returns false (or PixelCopy finishes after the timeout), the bitmap buffer may never be recycled, and the callback could still draw onto the canvas after capture has moved on. Handle the await result (fallback + recycle) and guard the callback to avoid drawing after timeout/finalization.
|
@wcandillon to be honest, i have not tested it yet, this PR is currently vibe coded 😆 i asked Claude to do a proposal and here is this PR. |
|
You have the right intuitions. On Android it's really a hard problem to solve I think. One thing where I can help is with e2e testing of Android/iOS snapshot we have in Skia, potentially these can be contributed to this repo or you can send me a bunch of test to run against Skia. This is how it looks: https://github.com/wcandillon/react-native-skia/tree/main/apps/example/src/Tests/Screens (we have a bunch of native component we render and check the view-shot). |
8a6d521 to
90c6c64
Compare
Single-capture ViewShot exercising the rendering dimensions touched by the Android renderer rewrite (PR #616) and Copilot review feedback: transforms (rotate/scale/skew/3D/combo), nested opacity, z-index draw order, mid-scrolled ScrollView clip, padding+bg+borderRadius+border, overflow:hidden, and a Skia Canvas vs RN View comparator. Adds @shopify/react-native-skia + react-native-reanimated + react-native-worklets to the example app deps and wires the worklets babel plugin. New screen reuses the existing useViewShotCapture / CaptureButton / PreviewContainer shared components. Detox case wired into viewshot-all-screens.test.js so CI captures the card on each platform. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply 5 of 6 issues raised in Copilot's review of #616: 1. applyTransformations no longer translates by padding/scroll for the view itself — that shifted the view's own background/text by its padding. Padding+scroll are now applied at child descent in drawChildren, matching ViewGroup.dispatchDraw semantics. ScrollView clipRect is now in local view coords (0,0,W,H). 2. ViewGroup with combined alpha < 1 is now wrapped in saveLayerAlpha and descendants render at full opacity. Previously each child got its own alpha layer, which produced wrong blending for overlapping siblings under a transparent parent. 3. drawBackgroundIfPresent now calls bg.setBounds(0,0,W,H) before drawing — view.draw() does this implicitly but we bypass that path for ViewGroups so the bounds could be stale. 4. drawChildren now respects ViewGroup.getChildDrawingOrder when isChildrenDrawingOrderEnabled() is true (this is what RN sets when children have zIndex). dispatchOverflowDraw was for overflow:visible, not z-index — the previous code conflated the two and drew children in tree order regardless of zIndex. 5. TextureView/SurfaceView children now use child.getAlpha() instead of inheriting only the parent's opacity — semi-transparent video etc. used to render as fully opaque. Skipped: SurfaceView PixelCopy timeout handling (rare edge case, deferred). Also dropped the now-redundant parentOpacity parameter threaded through renderViewToCanvas (always 1.0 with the new layer wrapping). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test: add rendering correctness test card to example app Single-capture ViewShot exercising the rendering dimensions touched by the Android renderer rewrite (PR #616) and Copilot review feedback: transforms (rotate/scale/skew/3D/combo), nested opacity, z-index draw order, mid-scrolled ScrollView clip, padding+bg+borderRadius+border, overflow:hidden, and a Skia Canvas vs RN View comparator. Adds @shopify/react-native-skia + react-native-reanimated + react-native-worklets to the example app deps and wires the worklets babel plugin. New screen reuses the existing useViewShotCapture / CaptureButton / PreviewContainer shared components. Detox case wired into viewshot-all-screens.test.js so CI captures the card on each platform. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address Copilot feedback + unbreak Android APK CI Copilot review on #627: - ScrolledCell ScrollView now has padding:8 so the fixture actually exercises the padding/scroll double-apply scenario (was claimed in the PR description but the ScrollView had no padding). - Preview imageHeight is now derived from the card constants (5 rows × 110 + gaps + card padding ≈ 606) instead of CARD_WIDTH * 1.7 (544), so the default cover-mode Image no longer crops the bottom of the captured card. CI: the Build Android APK debug job in ci.yml broke once Skia + reanimated + worklets entered the example deps — the separate "prefabDebugConfigurePackage" pre-step was wiped by the subsequent `clean assembleDebug`, and Skia 2.x relies on the prefab artefact being available when the app's CMake step runs. Aligning with the already-passing "Build Example APK" workflow: drop the pre-step, drop `clean`, run a single `assembleDebug -PnewArchEnabled=true`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(example): persist Detox manual screenshots Add the Detox artifacts plugin so device.takeScreenshot() calls land on disk under example/artifacts/. The CI workflow already attempts to upload example/artifacts/**/*.png as detox-screenshots-ios but the upload was warning "No files were found" because Detox's default config never persists manual screenshot calls. This unblocks downloading the new Rendering test card capture from CI artifacts to commit as a reference snapshot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: gre <greweb@protonmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated locally by running the new Detox case in isolation: npx detox test --configuration ios.sim.debug ... -t "Rendering test card" (Other tests in the suite were skipped to avoid the pre-existing react-native-video crash that's been killing the iOS Detox run.) Sim used: iPhone 16 Pro on iOS 26.4 (1206x2622). Existing references were committed at 1179x2556 from an older iOS version — separate issue, not blocking this snapshot. The capture is a full device screenshot showing the test card live- rendered by iOS layout, which serves as the ground-truth visual reference for back-testing PR #616's Android renderer rewrite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Generated locally by running the new Detox case in isolation: npx detox test --configuration ios.sim.debug ... -t "Rendering test card" (Other tests in the suite were skipped to avoid the pre-existing react-native-video crash that's been killing the iOS Detox run.) Sim used: iPhone 16 Pro on iOS 26.4 (1206x2622). Existing references were committed at 1179x2556 from an older iOS version — separate issue, not blocking this snapshot. The capture is a full device screenshot showing the test card live- rendered by iOS layout, which serves as the ground-truth visual reference for back-testing PR #616's Android renderer rewrite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#494) Replace the flat view.draw() + getAllChildren() traversal with a recursive per-view rendering approach adapted from Skia's ViewScreenshotService. This fixes: - CSS transforms (rotation, scale, skew, perspective) via view.getMatrix() - Opacity tracking through the view hierarchy - z-index ordering via ReactViewGroup.dispatchOverflowDraw() - ScrollView and HorizontalScrollView clipping - SVG views rendered as opaque leaf nodes Performance: cache reflection lookup, skip saveLayerAlpha at full opacity, reuse Matrix object, use bounded layer rects, consolidate bitmap pool methods. Closes #494 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Apply 5 of 6 issues raised in Copilot's review of #616: 1. applyTransformations no longer translates by padding/scroll for the view itself — that shifted the view's own background/text by its padding. Padding+scroll are now applied at child descent in drawChildren, matching ViewGroup.dispatchDraw semantics. ScrollView clipRect is now in local view coords (0,0,W,H). 2. ViewGroup with combined alpha < 1 is now wrapped in saveLayerAlpha and descendants render at full opacity. Previously each child got its own alpha layer, which produced wrong blending for overlapping siblings under a transparent parent. 3. drawBackgroundIfPresent now calls bg.setBounds(0,0,W,H) before drawing — view.draw() does this implicitly but we bypass that path for ViewGroups so the bounds could be stale. 4. drawChildren now respects ViewGroup.getChildDrawingOrder when isChildrenDrawingOrderEnabled() is true (this is what RN sets when children have zIndex). dispatchOverflowDraw was for overflow:visible, not z-index — the previous code conflated the two and drew children in tree order regardless of zIndex. 5. TextureView/SurfaceView children now use child.getAlpha() instead of inheriting only the parent's opacity — semi-transparent video etc. used to render as fully opaque. Skipped: SurfaceView PixelCopy timeout handling (rare edge case, deferred). Also dropped the now-redundant parentOpacity parameter threaded through renderViewToCanvas (always 1.0 with the new layer wrapping). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ViewGroup.isChildrenDrawingOrderEnabled() is protected so calling it directly fails the Java compile. Mirror the same reflection-cache pattern used for getChildDrawingOrder and dispatchOverflowDraw. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ce74709 to
b58ad07
Compare
|
I think this PR is diverging too much. i'm going to close it for now, we'll need to have more example that possibly reveal more issues, but apart from the opacity issue, it seems to be fine at the moment. |
|
@gre should we use the Skia e2e infrastructure to add a bunch of test for this feature so we can track everything better? what do you think? |
|
@wcandillon it could be useful. I'm currently iterating on the opacity one and will manually test it. But overall it's not easy to anticipate all scenario, i actually wonder if we can leverage generative AI to smartly "bruteforce" all possible combination of views nesting / compositions 😆 so it could automatically locate for us where there are diff 🤔 |



Summary
Closes #494 — Aligns Android view capture with React Native Skia's
ViewScreenshotServiceas proposed by @wcandillon.Replaces the flat
view.draw()+getAllChildren()traversal with recursive per-view rendering that correctly handles:view.getMatrix()to capture rotation, scale, skew, perspective (old code only handled rotation + scale individually)saveLayerAlphaReactViewGroup.dispatchOverflowDraw()for correct draw orderingScrollViewandHorizontalScrollViewreact-native-svgviews as opaque leaf nodesPerformance optimizations (beyond Skia reference)
dispatchOverflowDrawreflection lookup in a static field (was per-call)saveLayerAlphawhen opacity is 1.0 (common case)RectFinstead ofnullfor layer rectsMatrixobject instead of allocating per viewgetBitmapForScreenshot/getExactBitmapForScreenshotinto one methoddrawBitmapWithTransformhelper (was copy-pasted 4×)Paint.alphaafter bitmap draws to prevent state leakingTest plan
npm run androidfromexample/)opacityprop on nested views🤖 Generated with Claude Code