Skip to content

Fix readPixels PBO segfault & implement getBufferSubData for WebGL2 - #334

Open
Determinated738 wants to merge 3 commits into
stackgl:masterfrom
Determinated738:master
Open

Determinated738 wants to merge 3 commits into
stackgl:masterfrom
Determinated738:master

Conversation

@Determinated738

Copy link
Copy Markdown

This PR fixes a segfault that happens when using PBOs, as well as implementing getBufferSubData as it was missing.

Minimal code for recreating the segfault:

import createGL from "gl";

const width = 512;
const height = 512;
const size = width * height * 4;

const gl = createGL(width, height, { createWebGL2Context: true });

try {
    const pbo = gl.createBuffer();
    gl.bindBuffer(gl.PIXEL_PACK_BUFFER, pbo);
    gl.bufferData(gl.PIXEL_PACK_BUFFER, size, gl.STREAM_READ);
    gl.bindBuffer(gl.PIXEL_PACK_BUFFER, null);

    gl.clearColor(1, 0, 0, 1);
    gl.clear(gl.COLOR_BUFFER_BIT);

    gl.bindBuffer(gl.PIXEL_PACK_BUFFER, pbo);
    gl.readPixels(0, 0, width, height, gl.RGBA, gl.UNSIGNED_BYTE, 0);

    const pixels = new Uint8Array(size);
    gl.getBufferSubData(gl.PIXEL_PACK_BUFFER, 0, pixels);
    console.log(pixels);
} catch (e) {
    console.error("Caught JS Error:", e);
}

@Determinated738

Copy link
Copy Markdown
Author

Hey @dhritzkiv, I went through the failed tests, they seem to be unrelated to my PR, if you need any help going through each one and fixing them, i'm more than happy to do a separate PR for this.

@dhritzkiv

Copy link
Copy Markdown
Member

Hey! No worries about the failed tests. Your latest fixes will be integrated into other fixes that will be in an upcoming release

dhritzkiv added a commit that referenced this pull request Nov 5, 2026
#334 put two early returns on one line, unlike the rest of the file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dhritzkiv added a commit that referenced this pull request Nov 5, 2026
#334 declared and loaded glGetBufferSubData, which isn't part of OpenGL ES and isn't called; getBufferSubData maps the buffer instead.
The header and loader stay as ANGLE ships them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dhritzkiv added a commit that referenced this pull request Nov 5, 2026
Since #334, native readPixels queried the pixel pack buffer binding on every call.
WebGL1 has no such binding, so every WebGL1 readPixels set INVALID_ENUM.
It now reads into a pixel pack buffer only when given a number, and checks the binding only then, so a number can't be written through as a pointer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dhritzkiv added a commit that referenced this pull request Nov 5, 2026
Since #334, native readPixels threw a TypeError for any array without a data pointer, which includes empty ones, so reading a 0×0 rectangle threw and crashed the reading_read-pixels-pack-alignment conformance test.
Empty arrays now go to GL like any other, and null sets INVALID_VALUE, as in browsers.
WebGL2's offset form sets INVALID_OPERATION when no pixel pack buffer is bound and INVALID_VALUE for a negative offset, and anything else that isn't an ArrayBufferView throws a TypeError.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dhritzkiv added a commit that referenced this pull request Nov 5, 2026
…SubData

#334 dropped the target checks, so a bad target got no error from bufferData and INVALID_OPERATION from bufferSubData instead of INVALID_ENUM.
With no buffer bound, bufferData called getError() right after setting its error, which cleared it.
This failed the more_functions_bufferDataBadArgs conformance test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dhritzkiv added a commit that referenced this pull request Nov 5, 2026
#334 added PIXEL_PACK_BUFFER and PIXEL_UNPACK_BUFFER and kept their bindings in JS, where deleteBuffer left them pointing at the deleted buffer.
bindBuffer, bufferData and bufferSubData now take all six WebGL2 targets: COPY_READ_BUFFER, COPY_WRITE_BUFFER, PIXEL_PACK_BUFFER, PIXEL_UNPACK_BUFFER, TRANSFORM_FEEDBACK_BUFFER and UNIFORM_BUFFER.
Their bindings are read back from GL, which also keeps the ones bindBufferBase and bindBufferRange change, so getParameter returns the WebGLBuffer instead of 0 or the GL id.
WebGL2 only keeps element array buffers apart from other data, so a buffer is no longer limited to the first target it was bound to, and bindBufferBase and bindBufferRange check this too.
deleteBuffer unbinds the buffer from these targets when its deletion has to wait for a vertex array object.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dhritzkiv added a commit that referenced this pull request Nov 5, 2026
#334 added STREAM_READ, STATIC_READ and DYNAMIC_READ; STREAM_COPY, STATIC_COPY and DYNAMIC_COPY still set INVALID_ENUM.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dhritzkiv added a commit that referenced this pull request Nov 5, 2026
#334's getBufferSubData threw an Error whenever GL couldn't map the range, such as when reading past the end of the buffer, where WebGL sets INVALID_VALUE.
A dstOffset past the end of dstBuffer threw a RangeError, a length past the end read into the bytes after the view, and a bad target set INVALID_OPERATION instead of INVALID_ENUM.
It now checks dstOffset and length against dstBuffer, leaves range errors to GL, reads nothing for an empty range, and throws a TypeError only when dstBuffer isn't an ArrayBufferView.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dhritzkiv added a commit that referenced this pull request Nov 5, 2026
This is the case #334 fixed, along with the errors around it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dhritzkiv added a commit that referenced this pull request Nov 5, 2026
#334 made PIXEL_UNPACK_BUFFER bindable, but texture uploads ignored it: the offset forms of texImage2D, texSubImage2D, texImage3D and texSubImage3D threw a TypeError, and null pixels uploaded from the start of the bound buffer.
Uploads now take a byte offset into the pixel unpack buffer while one is bound.
As in browsers, an offset without one, or pixels, null or an image with one, sets INVALID_OPERATION, and so do UNPACK_FLIP_Y_WEBGL and UNPACK_PREMULTIPLY_ALPHA_WEBGL, since the pixels never pass through headless-gl.
The native methods check the binding before using an offset, so it can't be read as a pointer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants