Implement WebGL2 Uniform Buffer Objects #19250

MortimerGoro · 2017-11-16T13:48:24Z

Implement WebGL2 Uniform Buffer Objects.

Note: Currently OSMesa compat profiles only support OpenGL 3.0 (GLSL 130). Some UBO shaders used in WPT can't be compiled with GLSL 130, so part of the implementation is not fully tested (e.g. Could not compile shader with uniform blocks without error errors in uniform-buffers.html test). All uniform-buffers.html render tests pass using the native GPU. I'll open an follow-up issue about that.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is


        Implement WebGL2 Uniform Buffer Objects

highfive · 2017-11-16T13:48:27Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/macros.rs, components/script/dom/webidls/WebGL2RenderingContext.webidl, components/script/dom/webglrenderingcontext.rs, components/script/dom/webgl2renderingcontext.rs, components/script/dom/webglbuffer.rs
@fitzgen: components/script/dom/macros.rs, components/script/dom/webidls/WebGL2RenderingContext.webidl, components/script/dom/webglrenderingcontext.rs, components/script/dom/webgl2renderingcontext.rs, components/script/dom/webglbuffer.rs
@emilio: components/script/dom/webglrenderingcontext.rs, components/canvas/webgl_thread.rs, components/script/dom/webgl2renderingcontext.rs, components/script/dom/webglbuffer.rs

highfive · 2017-11-16T13:48:29Z

Warning

These commits modify unsafe code. Please review it carefully!

anholt · 2017-11-27T03:04:58Z

Wait, we're using the compat profile? That seems like a mistake -- do we definitely need compat functionality, or is it something where OSMesa just doesn't have an interface for asking for core profiles?

glennw · 2017-11-27T03:20:29Z

I think the reason we're using compat profile is because otherwise we can't support the webgl1 fixed function requirements? (drive by comment - could be missing something completely)

glennw · 2017-11-27T03:20:46Z

I mean, I guess we could emulate them, if that is the reason we're using compat.

MortimerGoro · 2017-11-27T13:06:45Z

It's just for WebGL 1.0 backwards compatibility. If we use a core context we'll need to emulate some compatibility stuff required in WebGL 1.0. For example WebGL 1 should work without creating any VAO and a core context triggers a errors if VAOs are not created. There might be another issues like this one. I tried to pass all the WebGL tests using a 3.2 core context and there were a bunch of errors that wold require some time to research.

anholt

I don't see any fixed function GL in webgl 1, though. No TexEnv, no Begin/End, no user vertex arrays.

anholt · 2017-11-27T15:16:46Z

components/canvas/webgl_thread.rs

+            gl::TRANSFORM_FEEDBACK_BUFFER_SIZE |
+            gl::UNIFORM_BUFFER_SIZE =>
+                Ok(WebGLParameter::Int64(gl.get_integer_64iv(name, index))),
+            // Invalid parameters


Possibly drop a comment here that the _BINDING enums are handled in the front end?

anholt

Looks good! Only a couple of little bugs I think I found.

anholt · 2017-11-27T15:59:23Z

components/script/dom/webgl2renderingcontext.rs

+            _ => return self.base.BindBuffer(target, buffer),
+        };
+
+        if let Some(buffer) = buffer {


I think you still want to call down to self.base.BindBuffer() here -- you've lost the validation of the enum to be either array or element_array.

anholt · 2017-11-27T15:59:23Z

components/script/dom/webgl2renderingcontext.rs

    fn BufferData_(&self, target: u32, size: i64, usage: u32) -> Fallible<()> {
-        self.base.BufferData_(target, size, usage)
+        let bound_buffer = match target {


It would be nice if we could have a single helper function that returned the binding point (not the bound buffer) given the target enum.

anholt · 2017-11-27T15:59:23Z

components/script/dom/webgl2renderingcontext.rs

-        self.base.DeleteBuffer(buffer)
+        self.base.DeleteBuffer(buffer);
+        if let Some(buffer) = buffer {
+            handle_object_deletion!(self, self.bound_uniform_buffer, buffer,


You've added the TFB buffer elsewhere in the patch, want to do so here as well?

anholt · 2017-11-27T15:59:23Z

components/script/dom/webgl2renderingcontext.rs

+        let value = match target {
+            constants::TRANSFORM_FEEDBACK_BUFFER_BINDING =>
+                return object_binding_to_js_or_null!(cx, &self.bound_transform_feedback_buffer),
+            constants::UNIFORM_BUFFER_BINDING =>


These should be returning the indexed bound buffer, not the non-indexed binding's buffer. We would need to track a vec of bindings in BindBufferBase/Range according to the context limits, and also validate that the index is not greater than that array size, and then return them here.

anholt · 2017-11-27T15:59:23Z

components/script/dom/webglbuffer.rs

+        self.currently_bound.get()
+    }
+
+    pub fn set_currently_bound(&self, value: bool) {


I don't think this quite works. Bind a buffer to COPY_READ, then COPY_WRITE, then unbind from COPY_WRITE, and now it's "not currently bound" and we would allow it to be bound to TFB, right?

anholt · 2017-11-27T15:59:23Z

components/script/dom/webglrenderingcontext.rs

@@ -1156,6 +1111,111 @@ impl WebGLRenderingContext {
            }
        }
    }
+


It would be nice if the refactors were split out from the functional content of the commit.

anholt · 2017-11-27T16:36:37Z

The typical process for handling core GL's VAO requirement is that you just have a single VAO that you bind when you're not otherwise doing VAO-based rendering (this is also essentially what Mesa does internally for the compat path). We could allocate a VAO at context creation, and bind it when the JS side hasn't bound one.

If there's a branch you could share where you started this work, I might be interested in following up on it.

bors-servo · 2018-03-06T15:50:39Z

☔ The latest upstream changes (presumably #20144) made this pull request unmergeable. Please resolve the merge conflicts.

dralley · 2020-04-20T15:19:10Z

Is this PR still relevant?

jdm · 2020-04-20T15:27:48Z

This has been superseded by recent WebGL 2 work.

Implement WebGL2 Uniform Buffer Objects

Loading status checks…

077ee75

highfive assigned emilio Nov 16, 2017

highfive added the S-awaiting-review label Nov 16, 2017

anholt reviewed Nov 27, 2017

View changes

anholt suggested changes Nov 27, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 27, 2017

highfive added the S-needs-rebase label Mar 6, 2018

jdm closed this Apr 20, 2020

servo / servo

Implement WebGL2 Uniform Buffer Objects #19250

Implement WebGL2 Uniform Buffer Objects #19250

MortimerGoro commented Nov 16, 2017 •

edited

highfive commented Nov 16, 2017

highfive commented Nov 16, 2017

anholt commented Nov 27, 2017

glennw commented Nov 27, 2017

glennw commented Nov 27, 2017

MortimerGoro commented Nov 27, 2017

anholt left a comment

This comment has been minimized.

anholt left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

anholt commented Nov 27, 2017

bors-servo commented Mar 6, 2018

dralley commented Apr 20, 2020

jdm commented Apr 20, 2020

@@ @@ -1156,6 +1111,111 @@ impl WebGLRenderingContext { @@
                     }
                 }
             }

servo / servo

Join GitHub today

Implement WebGL2 Uniform Buffer Objects #19250

Implement WebGL2 Uniform Buffer Objects #19250

Conversation

MortimerGoro commented Nov 16, 2017 • edited

highfive commented Nov 16, 2017

highfive commented Nov 16, 2017

anholt commented Nov 27, 2017

glennw commented Nov 27, 2017

glennw commented Nov 27, 2017

MortimerGoro commented Nov 27, 2017

anholt left a comment

This comment has been minimized.

anholt Nov 27, 2017

anholt left a comment

This comment has been minimized.

anholt Nov 27, 2017

This comment has been minimized.

anholt Nov 27, 2017

This comment has been minimized.

anholt Nov 27, 2017

This comment has been minimized.

anholt Nov 27, 2017

This comment has been minimized.

anholt Nov 27, 2017

This comment has been minimized.

anholt Nov 27, 2017

anholt commented Nov 27, 2017

bors-servo commented Mar 6, 2018

dralley commented Apr 20, 2020

jdm commented Apr 20, 2020

MortimerGoro commented Nov 16, 2017 •

edited