Initial implementation of WebGLQueries #24178

mmatyas · 2019-09-11T10:20:48Z

This patch adds initial support for WeGLQueries. Most related WebGL functions and objects (1) are implemented; what's still missing is:

syncing the result of getQueryParameter with the event loop
EXT_disjoint_timer_query_webgl2 support

cc @jdm @zakorgy

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

highfive · 2019-09-11T10:20:52Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGL2RenderingContext.webidl, components/script/dom/webidls/WebGLQuery.webidl, components/script/dom/mod.rs, components/script/dom/bindings/trace.rs and 2 more
@jgraham: tests/wpt/webgl/meta/conformance2/query/occlusion-query.html.ini, tests/wpt/webgl/meta/conformance2/query/query.html.ini
@KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGL2RenderingContext.webidl, components/script/dom/webidls/WebGLQuery.webidl, components/script/dom/mod.rs, components/script/dom/bindings/trace.rs and 2 more

highfive · 2019-09-11T10:20:53Z

Warning

These commits modify unsafe code. Please review it carefully!

mmatyas · 2019-09-11T10:21:19Z

Note: the patch doesn't pass test-tidy at the moment, because in my opinion this:

match target {
    constants::ANY_SAMPLES_PASSED |
    constants::ANY_SAMPLES_PASSED_CONSERVATIVE => {
        &self.occlusion_query
    },
    constants::TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN => {
        &self.primitives_query
    },

is more readable than this:

match target {
    constants::ANY_SAMPLES_PASSED | constants::ANY_SAMPLES_PASSED_CONSERVATIVE => {
        &self.occlusion_query
    },
    constants::TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN => &self.primitives_query,

Of course, I can apply test-tidy's changes in case you disagree.

Also I'm pretty sure many Option check could be simplified with some creative use of and_then and co., I'd be happy to hear your thoughs about them.

jdm

The rest of the changes look good!

jdm · 2019-09-16T15:06:40Z

components/script/dom/webgl2renderingcontext.rs

+        );
+        match query.get_parameter(&self.base, pname) {
+            Ok(value) => match pname {
+                constants::QUERY_RESULT => UInt32Value(value),


To get the event loop integration right, I recommend this strategy:

add Option<u32> and Option<bool> members for storing cached versions of these results

if both these fields are None, get the query parameter from the GL thread and store the result in the appropriate field, and return the result

if either field is Some, get the query parameter from the GL thread, queue a task that will update the cached values, and return the current cached value

I see, but what do you mean exactly by queuing a task? The query results should not change until the next time we're in the event loop; do we have a way for scheduling a callback to run at that time? Or you mean connect it somehow to the DOM's global scheduler?

I mean using the task! macro to declare a closure that will run asynchronously on the script thread, and using the task source API to enqueue it to run later. This code is one example of many in the script/ code of queueing a task.

jdm · 2019-09-16T15:08:57Z

As for the question about tidy, you can add a #[cfg(rustfmt_skip) annotation if you would prefer.

mmatyas · 2019-09-23T13:48:57Z

Ok, I've updated the patch with the task queue (thanks!), and I've also added a Drop implementation. The timing tests now pass, but the actual query results might fail (wtu.drawUnitQuad produces a crash on my PC, unrelated to this patch). I wonder if it passes on the CI.

jdm · 2019-09-23T13:50:47Z

@bors-servo try=wpt

bors-servo · 2019-09-23T13:50:52Z

⌛ Trying commit bea2682 with merge d4fb458...

@jdm

Initial implementation of WebGLQueries This patch adds initial support for WeGLQueries. Most related WebGL functions and objects ([1]) are implemented; what's still missing is: - syncing the result of `getQueryParameter` with the event loop - `EXT_disjoint_timer_query_webgl2` support [1]: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.12  cc @jdm @zakorgy ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24178)

bors-servo · 2019-09-23T13:58:22Z

💔 Test failed - linux-rel-wpt

The following pull requests depend on it: servo#24250, servo#24178.

bors-servo · 2019-09-24T02:41:32Z

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

Update sparkle crate The following pull requests depend on it: #24250, #24178.  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  - [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/24266)

mmatyas · 2019-09-25T12:14:08Z

Rebased to master. I've also improved the Drop impl making it support the fallible case. In the commit message I've removed the note about the missing event loop sync implementation, as it now works too.

jdm · 2019-09-30T14:31:30Z

components/script/dom/webidls/WebGLQuery.webidl

+//
+
+[Exposed=Window]
+interface WebGLQuery : WebGLObject {


This needs to be hidden behind the webgl2 preference:

servo/components/script/dom/webidls/WebGL2RenderingContext.webidl

Line 583 in 86d971d

[Pref="dom.webgl2.enabled"]

jdm · 2019-09-30T14:39:28Z

Here's the reason for those failures - the test harness automatically adds a #N prefix to each test message in this code. Since it only calls testFailed if a method is missing, it now executes testFailed fewer times than previously since there are new methods present in the interface, to the test messages contain different #N prefixes than before. You should delete tests/wpt/webgl/meta/conformance2/context/methods-2.html.ini and regenerate it in order to update the expected error messages.

jdm · 2019-09-30T14:40:25Z

Also some other interface test expectations needs updating:

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WebGLQuery interface: existence and properties of interface object", 
    "test": "/webgl/idlharness.any.html", 
    "line": 332663, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WebGLQuery interface object length", 
    "test": "/webgl/idlharness.any.html", 
    "line": 332664, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WebGLQuery interface object name", 
    "test": "/webgl/idlharness.any.html", 
    "line": 332665, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WebGLQuery interface: existence and properties of interface prototype object", 
    "test": "/webgl/idlharness.any.html", 
    "line": 332666, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WebGLQuery interface: existence and properties of interface prototype object's \"constructor\" property", 
    "test": "/webgl/idlharness.any.html", 
    "line": 332667, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "WebGLQuery interface: existence and properties of interface prototype object's @@unscopables property", 
    "test": "/webgl/idlharness.any.html", 
    "line": 332668, 
    "action": "test_result", 
    "expected": "FAIL"
}

This patch adds initial support for WeGLQueries. Most related WebGL functions and objects are implemented [1]. What's still missing is the `EXT_disjoint_timer_query_webgl2` support. [1]: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.12

mmatyas · 2019-10-01T10:39:55Z

Thanks, updated with the methods-2 and Pref changes. However, I couldn't reproduce the idlharness fails, even after regenerating its ini, hm.

jdm · 2019-10-01T12:21:41Z

@bors-servo r+

bors-servo · 2019-10-01T12:21:42Z

📌 Commit f2e2b3d has been approved by jdm

bors-servo · 2019-10-01T12:21:46Z

⌛ Testing commit f2e2b3d with merge cfcff8e...

@jdm

Initial implementation of WebGLQueries This patch adds initial support for WeGLQueries. Most related WebGL functions and objects ([1]) are implemented; what's still missing is: - syncing the result of `getQueryParameter` with the event loop - `EXT_disjoint_timer_query_webgl2` support [1]: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.12  cc @jdm @zakorgy ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24178)

bors-servo · 2019-10-01T14:59:20Z

💔 Test failed - status-taskcluster

jdm · 2019-10-01T15:29:00Z

@bors-servo retry

bors-servo · 2019-10-01T17:47:16Z

⌛ Testing commit f2e2b3d with merge d3475e5...

@jdm

Initial implementation of WebGLQueries This patch adds initial support for WeGLQueries. Most related WebGL functions and objects ([1]) are implemented; what's still missing is: - syncing the result of `getQueryParameter` with the event loop - `EXT_disjoint_timer_query_webgl2` support [1]: https://www.khronos.org/registry/webgl/specs/latest/2.0/#3.7.12  cc @jdm @zakorgy ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24178)

bors-servo · 2019-10-01T19:29:47Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing d3475e5 to master...

highfive assigned Manishearth Sep 11, 2019

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 11, 2019

jdm requested changes Sep 16, 2019

View reviewed changes

jdm assigned jdm and unassigned Manishearth Sep 16, 2019

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 16, 2019

jdm mentioned this pull request Sep 20, 2019

Initial implementation of WebGLSync #24250

Merged

3 tasks

mmatyas force-pushed the webgl_fns_query branch from 0d459bd to bea2682 Compare September 23, 2019 13:38

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 23, 2019

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 23, 2019

imiklos pushed a commit to imiklos/servo that referenced this pull request Sep 23, 2019

Update sparkle crate

1c23e54

The following pull requests depend on it: servo#24250, servo#24178.

imiklos mentioned this pull request Sep 23, 2019

Update sparkle crate #24266

Merged

3 tasks

highfive added the S-needs-rebase There are merge conflict errors. label Sep 24, 2019

mmatyas force-pushed the webgl_fns_query branch from bea2682 to 676a52d Compare September 25, 2019 12:07

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 25, 2019

jdm requested changes Sep 30, 2019

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Sep 30, 2019

mmatyas force-pushed the webgl_fns_query branch from 86d971d to f2e2b3d Compare October 1, 2019 10:31

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. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 1, 2019

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 Oct 1, 2019

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 Oct 1, 2019

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

bors-servo merged commit f2e2b3d into servo:master Oct 1, 2019

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 1, 2019

bors-servo mentioned this pull request Oct 1, 2019

Add WebGLSampler support #24333

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of WebGLQueries #24178

Initial implementation of WebGLQueries #24178

mmatyas commented Sep 11, 2019 •

edited by SimonSapin

highfive commented Sep 11, 2019

highfive commented Sep 11, 2019

mmatyas commented Sep 11, 2019

jdm left a comment

jdm Sep 16, 2019

mmatyas Sep 17, 2019

jdm Sep 17, 2019

jdm commented Sep 16, 2019

mmatyas commented Sep 23, 2019

jdm commented Sep 23, 2019

bors-servo commented Sep 23, 2019

bors-servo commented Sep 23, 2019

bors-servo commented Sep 24, 2019

mmatyas commented Sep 25, 2019

jdm Sep 30, 2019

jdm commented Sep 30, 2019

jdm commented Sep 30, 2019

mmatyas commented Oct 1, 2019

jdm commented Oct 1, 2019

bors-servo commented Oct 1, 2019

bors-servo commented Oct 1, 2019

bors-servo commented Oct 1, 2019

jdm commented Oct 1, 2019

bors-servo commented Oct 1, 2019

bors-servo commented Oct 1, 2019

Initial implementation of WebGLQueries #24178

Initial implementation of WebGLQueries #24178

Conversation

mmatyas commented Sep 11, 2019 • edited by SimonSapin

highfive commented Sep 11, 2019

highfive commented Sep 11, 2019

mmatyas commented Sep 11, 2019

jdm left a comment

Choose a reason for hiding this comment

jdm Sep 16, 2019

Choose a reason for hiding this comment

mmatyas Sep 17, 2019

Choose a reason for hiding this comment

jdm Sep 17, 2019

Choose a reason for hiding this comment

jdm commented Sep 16, 2019

mmatyas commented Sep 23, 2019

jdm commented Sep 23, 2019

bors-servo commented Sep 23, 2019

bors-servo commented Sep 23, 2019

bors-servo commented Sep 24, 2019

mmatyas commented Sep 25, 2019

jdm Sep 30, 2019

Choose a reason for hiding this comment

jdm commented Sep 30, 2019

jdm commented Sep 30, 2019

mmatyas commented Oct 1, 2019

jdm commented Oct 1, 2019

bors-servo commented Oct 1, 2019

bors-servo commented Oct 1, 2019

bors-servo commented Oct 1, 2019

jdm commented Oct 1, 2019

bors-servo commented Oct 1, 2019

bors-servo commented Oct 1, 2019

mmatyas commented Sep 11, 2019 •

edited by SimonSapin