Implement WebGL getFrameBufferAttachmentParameter API #20317

gootorov · 2018-03-16T20:24:42Z

Implementation of getFramebufferAttachmentParameter as in WebGL1 specification.

Part of #10209.

r? emilio or jdm.

./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

This change is

highfive · 2018-03-16T20:24:47Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webgl2renderingcontext.rs
@fitzgen: components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webgl2renderingcontext.rs
@KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webgl2renderingcontext.rs

highfive · 2018-03-16T20:24:49Z

Warning

These commits modify unsafe code. Please review it carefully!

jdm · 2018-03-16T22:13:04Z

@bors-servo try

bors-servo · 2018-03-16T22:13:09Z

⌛ Trying commit c99d3c2 with merge 610d0b5...

…er, r=<try> Implement WebGL getFrameBufferAttachmentParameter API  Implementation of `getFramebufferAttachmentParameter` as in WebGL1 specification. Part of #10209. r? emilio or jdm. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20317)

emilio

Pending going through the GLES specs and such, looks good! I only have a couple minor suggestions.

Let's see what bors thinks, thanks for working on this!

emilio · 2018-03-16T22:04:56Z

components/script/dom/webgl2renderingcontext.rs

@@ -142,6 +142,14 @@ impl WebGL2RenderingContextMethods for WebGL2RenderingContext {
        self.base.GetExtension(cx, name)
    }

+    #[allow(unsafe_code)]
+    /// https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.4
+    unsafe fn GetFramebufferAttachmentParameter(&self, cx: *mut JSContext,


nit: Indentation looks off, let's use:

unsafe fn GetFramebufferAttachmentParameter( &self, cx: *mut JSContext, // ... ) -> JSVal {

emilio · 2018-03-16T22:05:38Z

components/script/dom/webglrenderingcontext.rs

+    // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.6
+    unsafe fn GetFramebufferAttachmentParameter(&self, _cx: *mut JSContext,
+                                         target: u32, attachment: u32, pname: u32) -> JSVal
+    {


nit: Ditto about indentation.

emilio · 2018-03-16T22:06:49Z

components/script/dom/webglrenderingcontext.rs

+        if let None = self.bound_framebuffer.get() {
+            self.webgl_error(InvalidOperation);
+            return NullValue();
+        }


It's probably easier and faster if we do the validation here, to keep it all in the same place.

The WebGL thread should assert that glGetError returns GL_NO_ERROR in debug builds, in case we've messed up.

Does that sound fine?

Though now I see it we're very inconsistent about it, I think it's nicer. wdyt? (also, @jdm wdyt?)

Yes, it's a bit inconsistent in the code base. I noticed that usually on the receiver channel side we validate input that we pass to GLES and on the sender side (here) we check what GLES returned. If it returns something we don't expect, we panic. At least that's how I understood it.

Validating everything here wouldn't be any harder, we could do that if that's faster.

Looking at it again today, I think it will look nicer, I agree. If that's faster too, then it's a win-win. We could move all validation for other APIs here too as a follow-up.

@jdm What do you think? Sounds good?

emilio · 2018-03-16T22:07:02Z

components/script/dom/webglrenderingcontext.rs

+                                         target: u32, attachment: u32, pname: u32) -> JSVal
+    {
+        // Check if currently bound framebuffer is non-zero as per spec.
+        if let None = self.bound_framebuffer.get() {


nit: self.bound_framebuffer.get().is_none()

bors-servo · 2018-03-16T22:41:47Z

💔 Test failed - linux-rel-css

gootorov · 2018-03-16T22:49:56Z

Bors fails at WebGL2 tests that we talked about on IRC. How can we disable them? The WebGL1 ones are all passing.
There are some other tests as well that are expected to timeout but are OK now. Shall we enable them now?

gootorov · 2018-03-16T23:02:25Z

~~Nevermind, the WebGL2 tests has actually always been disabled.~~
WebGL2 tests are disabled, but bors still runs them, why is that?

Enabled rapid-resizing that is unexpectedly passing now and looking into wpt/CSS2/visudet/line-height-204 that started to fail.

emilio · 2018-03-17T01:09:46Z

Note that the line-height test is known flake. You should look at the filtered-wpt-errorsummary.log file instead: http://build.servo.org/builders/linux-rel-css/builds/8248/steps/shell__4/logs/filtered-wpt-errorsummary.log

gootorov · 2018-03-17T01:21:14Z

Oh, okay. Errors in filtered-wpt-errorsummary.log are expected. Those are failures from WebGL2 tests that we discussed, but I don't know why bors complains about them.

servo/tests/wpt/mozilla/meta/webgl/conformance-2.0.0/conformance2/renderbuffers/framebuffer-test.html.ini

Lines 1 to 4 in fc90e61

    
           [framebuffer-test.html] 
        
             expected: ERROR 
        
             [WebGL test #1: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).] 
        
               expected: FAIL

emilio · 2018-03-17T04:14:26Z

Why wouldn't it run them? The annotation in that file just means that the test failed because it threw before the end. That may be possible, since you just added an interface that could've made the test throw, so expectations need to be updated.

bors-servo · 2018-03-18T04:35:53Z

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

gootorov · 2018-03-18T14:22:50Z

@emilio Should be good now! :)

emilio · 2018-03-18T14:38:16Z

components/script/dom/webglrenderingcontext.rs

+
+        // Note: commented out stuff is for the WebGL2 standard.
+        let target_matches = match target {
+            //constants::READ_FRAMEBUFFER |


Maybe add a space after //?

emilio · 2018-03-18T14:40:54Z

components/canvas/webgl_thread.rs

+        target: u32,
+        attachment: u32,
+        pname: u32,
+        chan: WebGLSender<WebGLResult<WebGLParameter>>


So, there's no reason this chan returns a WebGLResult now, right? I'd make it just return an integer, and remove the dead code from the handle_potential_webgl_error stuff.

The channels require a type parameter and we can't pass WebGLSender to get_framebuffer_attachment_parameter(). We can simplify that to WebGLSender<WebGLParameter> and the code below would become

match receiver.recv().unwrap() { WebGLParameter::Int(value) => Int32Value(value), _ => panic!("Framebuffer attachment parameter must be an integer"), }

Sounds good?

Or, we could do WebGLSender<i32>, and then Int32Value(receiver.recv().unwrap()) would be possible. What do you think?

Yes, if it will always return an integer then WebGLSender<i32> makes sense.

emilio · 2018-03-18T14:41:34Z

components/script/dom/webglrenderingcontext.rs

+            WebGLParameter::Float(_) => panic!("Framebuffer attachment parameter should not be float"),
+            WebGLParameter::FloatArray(_) => panic!("Framebuffer attachment parameter should not be float array"),
+            WebGLParameter::String(_) => panic!("Framebuffer attachment parameter should not be string"),
+            WebGLParameter::Invalid => {


With the change suggested above, a;; this block becomes:

Int32Value(receiver.recv().unwrap())

emilio · 2018-03-18T14:43:29Z

components/script/dom/webglrenderingcontext.rs

+            //constants::FRAMEBUFFER_ATTACHMENT_STENCIL_SIZE |
+            //constants::FRAMEBUFFER_ATTACHMENT_TEXTURE_LAYER |
+            constants::FRAMEBUFFER_ATTACHMENT_OBJECT_NAME |
+            constants::FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE |


So, according to the spec, when OBJECT_TYPE is passed, we need to return a WebGLTexture or WebGLRenderbuffer, right? Not an integer which happens to be the name of that texture / rb on the GPU.

WebGL and GLES specs say slightly different things, for some reason.
According to GLES spec, it returns one of NONE, TEXTURE, or RENDERBUFFER, which are typedef'ed unsigned integers as can be seen in this table:
https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14

emilio

Really close! :)

emilio · 2018-03-20T00:23:52Z

components/script/dom/webglrenderingcontext.rs

+            constants::FRAMEBUFFER_ATTACHMENT_OBJECT_NAME => {
+                if self.bound_renderbuffer.get().is_some() {
+                    return object_binding_to_js_or_null!(cx, &self.bound_renderbuffer);
+                } else {


nit: else after return is unnecessary.

emilio · 2018-03-20T00:24:11Z

components/script/dom/webglrenderingcontext.rs

@@ -2343,6 +2343,19 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
            return NullValue();
        }

+        // special case that returns `WebGLRenderbuffer` or `WebGLTexture` dom object
+        match pname {


nit: just if pname == constants::FRAMEBUFFER_ATTACHMENT_OBJECT_NAME?

emilio · 2018-03-20T00:26:18Z

components/script/dom/webglrenderingcontext.rs

+                    return object_binding_to_js_or_null!(cx, &self.bound_renderbuffer);
+                } else {
+                    let texture = self.bound_texture(constants::TEXTURE_2D);
+                    return optional_root_object_to_js_or_null!(cx, texture);


So, the spec says:

If the value of FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE is NONE, then querying any other pname will generate INVALID_ENUM.

Which means that if there's no texture and no renderbuffer, we should return and signal an invalid enum error for all the parameter names too.

emilio · 2018-03-20T17:49:32Z

components/script/dom/webglrenderingcontext.rs

+                return object_binding_to_js_or_null!(cx, &self.bound_renderbuffer);
+            }
+            self.webgl_error(InvalidEnum);
+            return NullValue();


I think this should be instead:

// From the GLES2 spec: // // If the value of FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE is NONE, // then querying any other pname will generate INVALID_ENUM. // if pname != constants::FRAMEBUFFER_ATTACHMENT_OBJECT_TYPE && self.bound_texture(..).is_none() && self.bound_renderbuffer.get().is_none() { self.webgl_error(InvalidEnum); return NullValue(); }

That is, my reading of that quote is that InvalidEnum should be returned for all other constants, not only for OBJECT_NAME.

emilio · 2018-03-21T01:45:50Z

Looks great, thanks!

@bors-servo r+

jdm · 2018-03-21T15:29:14Z

@bors-servo r+

bors-servo · 2018-03-21T15:29:14Z

📌 Commit da01db9 has been approved by jdm

…er, r=jdm Implement WebGL getFrameBufferAttachmentParameter API  Implementation of `getFramebufferAttachmentParameter` as in WebGL1 specification. Part of #10209. r? emilio or jdm. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20317)

bors-servo · 2018-03-21T16:15:00Z

⌛ Testing commit da01db9 with merge 6bfcff7...

bors-servo · 2018-03-21T16:47:33Z

💔 Test failed - linux-rel-wpt

jdm · 2018-03-21T17:17:19Z

Looks like /_mozilla/webgl/conformance-1.0.3/conformance/misc/object-deletion-behaviour.html still needs expectation updates.

jdm · 2018-03-22T16:28:26Z

@bors-servo: r+
Thanks!

bors-servo · 2018-03-22T16:28:26Z

📌 Commit ee5bdbb has been approved by jdm

bors-servo · 2018-03-22T16:28:32Z

⌛ Testing commit ee5bdbb with merge 4aaac61...

…er, r=jdm Implement WebGL getFrameBufferAttachmentParameter API  Implementation of `getFramebufferAttachmentParameter` as in WebGL1 specification. Part of #10209. r? emilio or jdm. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20317)

bors-servo · 2018-03-22T17:48:19Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 4aaac61 to master...

highfive assigned emilio Mar 16, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 16, 2018

emilio reviewed Mar 16, 2018

View reviewed changes

highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 16, 2018

highfive removed the S-tests-failed The changes caused existing tests to fail. label Mar 16, 2018

gootorov force-pushed the webgl-getFramebufferAttachmentParameter branch from c567d0d to 10b9a79 Compare March 17, 2018 00:02

emilio mentioned this pull request Mar 17, 2018

Fully implement WebGL 1.0 #10209

Open

highfive added the S-needs-rebase There are merge conflict errors. label Mar 18, 2018

gootorov force-pushed the webgl-getFramebufferAttachmentParameter branch from 419f791 to c10fef9 Compare March 18, 2018 14:21

emilio reviewed Mar 18, 2018

View reviewed changes

gootorov force-pushed the webgl-getFramebufferAttachmentParameter branch from c10fef9 to a687d50 Compare March 18, 2018 18:48

emilio reviewed Mar 20, 2018

View reviewed changes

gootorov force-pushed the webgl-getFramebufferAttachmentParameter branch from 28b9346 to a91642a Compare March 20, 2018 16:50

emilio reviewed Mar 20, 2018

View reviewed changes

gootorov force-pushed the webgl-getFramebufferAttachmentParameter branch from a91642a to db015a3 Compare March 21, 2018 01:11

emilio approved these changes Mar 21, 2018

View reviewed changes

highfive assigned jdm and unassigned emilio Mar 21, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 21, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Mar 21, 2018

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 22, 2018

gootorov force-pushed the webgl-getFramebufferAttachmentParameter branch 2 times, most recently from 5800857 to 2182d9b Compare March 22, 2018 16:19

Implement WebGL getFramebufferAttachmentParameter API

ee5bdbb

gootorov force-pushed the webgl-getFramebufferAttachmentParameter branch from 2182d9b to ee5bdbb Compare March 22, 2018 16:26

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 22, 2018

jdm added the This Week In Servo (TWiS) label Mar 22, 2018

bors-servo mentioned this pull request Mar 22, 2018

Fall down the WebGL rabbit hole and fix some bugs there #20375

Merged

bors-servo merged commit ee5bdbb into servo:master Mar 22, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 22, 2018

gootorov deleted the webgl-getFramebufferAttachmentParameter branch March 22, 2018 17:49

Implement WebGL getFrameBufferAttachmentParameter API #20317

Implement WebGL getFrameBufferAttachmentParameter API #20317

Conversation

gootorov commented Mar 16, 2018 • edited by emilio Loading

highfive commented Mar 16, 2018

highfive commented Mar 16, 2018

jdm commented Mar 16, 2018

bors-servo commented Mar 16, 2018

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gootorov Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Mar 16, 2018

gootorov commented Mar 16, 2018 • edited Loading

gootorov commented Mar 16, 2018 • edited Loading

emilio commented Mar 17, 2018

gootorov commented Mar 17, 2018 • edited Loading

emilio commented Mar 17, 2018

bors-servo commented Mar 18, 2018

gootorov commented Mar 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Mar 21, 2018

jdm commented Mar 21, 2018

bors-servo commented Mar 21, 2018

bors-servo commented Mar 21, 2018

bors-servo commented Mar 21, 2018

jdm commented Mar 21, 2018

jdm commented Mar 22, 2018

bors-servo commented Mar 22, 2018

bors-servo commented Mar 22, 2018

bors-servo commented Mar 22, 2018

gootorov commented Mar 16, 2018 •

edited by emilio

Loading

gootorov Mar 16, 2018 •

edited

Loading

gootorov commented Mar 16, 2018 •

edited

Loading

gootorov commented Mar 16, 2018 •

edited

Loading

gootorov commented Mar 17, 2018 •

edited

Loading