Correct RGBA8 texture readback coordinates and row order by bkaradzic-microsoft · Pull Request #1892 · BabylonJS/BabylonNative

bkaradzic-microsoft · 2026-09-18T22:16:16Z

Summary

Convert WebGL bottom-origin crop coordinates to the backend's texture origin before blitting a readback rectangle.
Return bottom-up RGBA8 rows: flip D3D/Metal/Vulkan storage, not OpenGL storage.
Reject invalid mip levels, empty/out-of-range rectangles, and invalid face/layer indices before calculating requested storage sizes or allocating an automatic destination buffer, as well as before Y conversion and GPU submission.
Add asymmetric full-image, corner, edge, interior, mip-level, invertY, destination-buffer-offset, and wide odd-height regressions. Invalid-input coverage includes a 65535-by-65535 crop with a null destination.

Independence

Based on upstream master. Uses published Babylon.js 9.21.2 and unchanged native dependency pins. No protocol changes, float readback API, fixture/reference/tolerance changes, or dependency on the other open Native PRs.

Extracts only the RGBA8 readback portion of the shotgun fixes.

Validation

Latest allocation-order follow-up, Windows x64 / D3D11 / Chakra / RelWithDebInfo:

Rebuilt the checked-in JavaScript bundle and native UnitTests target.
JavaScript.All: 118 passing, including all three readback cases and the new huge null-destination rejection.
The base-level cases compare every channel exactly. The mip case allows one unit of existing mip-generation rounding for the green channel while distinguishing opposite ends of the mip (0 versus 255).

Verified runtime evidence from the prior published-head CI run, before this allocation-order follow-up:

Backend/configuration	Readback regression evidence
D3D11 / Windows / Chakra	All three named cases passed; JavaScript suite: 118 passing.
OpenGL / Ubuntu / Clang / JavaScriptCore	All three named cases passed; JavaScript suite: 118 passing.
Vulkan / Ubuntu	All three named cases passed; JavaScript suite: 115 passing, 3 pending.
Metal / macOS	Build-only evidence for these GPU regressions: CI uses a no-op Metal device and skips them. No Metal runtime pixel validation is claimed.
D3D12 / Windows	Build-only evidence: that CI job skipped Unit Tests and Validation Tests. No D3D12 runtime readback validation is claimed.

Fresh cross-platform CI for the allocation-order follow-up is pending; the OpenGL/Vulkan results above are explicitly from the previous head.

Copilot

Copilot review overview

🟡 Changes recommended

Address the rectangle-validation issue and update both test gates to account for disabled native image loading.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Validate rectangle values before narrowing to uint16_t · New

What changed in this PR

Corrects RGBA8 texture readback coordinates, row ordering, and rectangle validation across backends.

Changes:

Converts crop coordinates and normalizes row order.
Adds mip, crop, inversion, offset, and bounds regressions.
Regenerates the compiled JavaScript test bundle.

File	Summary	Review status
`Plugins/NativeEngine/Source/NativeEngine.cpp`	Implements readback coordinate and row-order fixes.	Changes required: validate original rectangle values before narrowing.
`Apps/UnitTests/JavaScript/src/tests.javaScript.all.ts`	Adds readback regression coverage.	Changes required: gate the test on native image loading.
`Apps/UnitTests/JavaScript/dist/tests.javaScript.all.js`	Updates the compiled test bundle.	Changes required: regenerate with the corrected test gate.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 · 1

Open (2)

The new validation also rejects non-integers / non-finite values (e.g., 0.5, NaN, Infinity),… · New
With this change, FlipImage will run on D3D/Metal/Vulkan (likely the common case) rather than… · New

Resolved since last review (1)

Validate rectangle values before narrowing to uint16_t

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 3 · 1

Open (4)

This test doesn’t appear to depend on native image loading (it only uses RawTexture + GPU… · New
Same gating concern as the previous test: this case exercises mip readback and validation paths… · New
std::swap_ranges swaps byte-by-byte here, which is typically slower than swapping whole rows via… · New
The new validation errors are correct but not very actionable because they don’t identify which… · New

Resolved since last review (2)

The new validation also rejects non-integers / non-finite values (e.g., 0.5, NaN, Infinity),…
With this change, FlipImage will run on D3D/Metal/Vulkan (likely the common case) rather than…

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 3

Open (3)

Using a thread_local scratch buffer can retain the peak row size for the lifetime of the thread,… · New
The mip extents calculation is duplicated (it’s recomputed later as mipWidth/mipHeight for… · New
The static_cast<uint16_t>(mipHeight - y - height) can truncate if mipHeight exceeds… · New

Resolved since last review (4)

std::swap_ranges swaps byte-by-byte here, which is typically slower than swapping whole rows via…
Same gating concern as the previous test: this case exercises mip readback and validation paths…
This test doesn’t appear to depend on native image loading (it only uses RawTexture + GPU…
The new validation errors are correct but not very actionable because they don’t identify which…

Extract the RGBA8 readback fixes without changing the published JavaScript buffer contract. Reject rectangles outside their mip before converting Y. Add asymmetric full-image, edge, interior, mip, and buffer-offset controls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae

Validate mip and rectangle inputs before integer conversion, including 16-bit and 32-bit wraps, fractions, negative and non-finite values. Cover rejected calls with unchanged destination sentinels and gate both pixel regressions on native image-loading support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae

Report the finite integer requirements and numeric bounds before narrowing readback coordinates, with exact-message coverage for invalid inputs. Swap image rows in place instead of allocating a temporary row buffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae

Restore optimized whole-row copies after measuring GCC's byte-wise swap_ranges regression. Keep scratch storage thread-local so repeated readbacks reuse its capacity without cross-thread sharing. Name the invalid rectangle parameter, verify exact errors, and add a wide odd-height readback regression. Document why RawTexture fixtures require the native image-loading build option. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305

Review-Group: E2 Source: PR BabylonJS#1892 Squashed final review changes, including regressions and review follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae

Reject invalid mip levels, rectangles, and face/layer indices before calculating requested storage sizes or allocating a null destination. Cover a 65535-by-65535 invalid crop with a null output buffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e6c3c123-c355-4790-a3d4-94337ed6e052

Update the generated JavaScript test bundle to match the source regressions for stricter readTexture input and buffer validation. Keep the executable test artifact separate from the native readback implementation. Review-Group: E2 Source: PR BabylonJS#1892 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae Split-From: ac3f4081ed11b26f509693c60f97b296790451e2 Split-Part: support

Reject invalid buffer offsets, lengths, face/layer indices, and uninitialized textures before readback work begins. Check full source and destination storage sizes before bgfx's uint32 conversion. Link bimg independently of image decoding so these checks also build with image loading disabled. Review-Group: E2 Source: PR BabylonJS#1892 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae Split-From: ac3f4081ed11b26f509693c60f97b296790451e2 Split-Part: code

Copilot AI lite review requested due to automatic review settings September 18, 2026 22:16

Copilot started reviewing on behalf of bkaradzic-microsoft September 18, 2026 22:24 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp Outdated

bkaradzic-microsoft requested a balanced review from Copilot September 18, 2026 23:37

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp Outdated

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp

bkaradzic-microsoft requested review from CedricGuillemet and bghgary and a balanced review from Copilot September 19, 2026 00:12

Copilot AI reviewed Sep 19, 2026

View reviewed changes

Comment thread Apps/UnitTests/JavaScript/src/tests.javaScript.all.ts Outdated

Comment thread Apps/UnitTests/JavaScript/src/tests.javaScript.all.ts Outdated

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp

Copilot started reviewing on behalf of bkaradzic-microsoft September 19, 2026 00:35 View session

Copilot started reviewing on behalf of bkaradzic-microsoft September 19, 2026 00:39 View session

bkaradzic-microsoft requested a balanced review from Copilot September 19, 2026 00:59

Copilot AI reviewed Sep 19, 2026

View reviewed changes

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp Outdated

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp Outdated

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp Outdated

Copilot started reviewing on behalf of bkaradzic-microsoft September 19, 2026 01:21 View session

bkaradzic-microsoft and others added 4 commits September 22, 2026 13:05

bkaradzic-microsoft force-pushed the pr/native-rgba8-readback branch from 4c88350 to dee3cb1 Compare September 22, 2026 20:05

Address RGBA8 readback review feedback

7d45f26

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d35d0a8b-b073-4f2a-bbd3-a0b1d3584305

bghgary approved these changes Oct 1, 2026

View reviewed changes

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp Outdated

Comment thread Apps/UnitTests/JavaScript/src/tests.javaScript.all.ts

bkaradzic-microsoft merged commit a93c85f into BabylonJS:master Oct 1, 2026
67 of 68 checks passed

Navigation Menu

Repository navigation

Correct RGBA8 texture readback coordinates and row order - #1892

Correct RGBA8 texture readback coordinates and row order#1892
bkaradzic-microsoft merged 6 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:pr/native-rgba8-readback

bkaradzic-microsoft commented Sep 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bkaradzic-microsoft commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Independence

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bkaradzic-microsoft commented Sep 18, 2026 •

edited

Loading