Add multiple WebGL calls and improve error detection #6770

emilio · 2015-07-25T19:25:21Z

Since it probably won't merge until multiprocess lands, I plan to use this PR to keep improving WebGL support until it can land.

Main TODOs are integration of tests, since it seems KhronosGroup/WebGL#1105 is going nowhere, adding missing calls and proper painting via native surfaces instead of readback.

I can't resolve conflicts right now because of time but I will do it soon.

hoppipolla-critic-bot · 2015-07-25T19:25:26Z

Critic review: https://critic.hoppipolla.co.uk/r/5656

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

Ms2ger · 2015-07-25T19:27:49Z

We'll likely impose a moratorium on new WebGL features until e10s (electrolysis, multiprocess Servo) lands.

emilio · 2015-07-25T19:29:26Z

@Ms2ger yep, that's why I marked it as WIP and didn't take the time to resolve conflicts :P

pcwalton · 2015-07-25T20:07:26Z

NB: e10s for WebGL has landed, so full speed ahead here!

emilio · 2015-07-26T15:10:55Z

@pcwalton Nice then :) in that case this can be reviewed and I'll keep working on top of it when merged, I just rebased it.

r? @pcwalton @jdm @Ms2ger

pcwalton · 2015-07-27T18:22:30Z

@bors-servo: r+

bors-servo · 2015-07-27T18:22:32Z

📌 Commit d303af2 has been approved by pcwalton

bors-servo · 2015-07-27T23:08:03Z

⌛ Testing commit d303af2 with merge 0173d72...


        Auto merge of #6770 - ecoal95:webgl-again, r=pcwalton

Add multiple WebGL calls and improve error detection Since it probably won't merge until multiprocess lands, I plan to use this PR to keep improving WebGL support until it can land. Main TODOs are integration of tests, since it seems KhronosGroup/WebGL#1105 is going nowhere, adding missing calls and proper painting via native surfaces instead of readback. I can't resolve conflicts right now because of time but I will do it soon.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6770)

bors-servo · 2015-07-27T23:13:01Z

💔 Test failed - gonk

emilio · 2015-07-27T23:23:46Z

Arrg, forgot to update gleam with my local version. My fault, I'll do it ASAP.

emilio · 2015-07-28T14:18:55Z

This is blocked until gleam is published to crates.io

metajack · 2015-07-29T03:57:11Z

@glennw Can you publish a new gleam? Or do we have to wait for upstream?

glennw · 2015-07-29T04:14:57Z

@metajack We are the upstream :) I've published 0.1.6 on crates.io.

emilio · 2015-07-29T12:42:07Z

Once #6830 lands this should be mergeable :)

metajack · 2015-07-31T04:52:12Z

@bors-servo retry

This is ready to go now.

bors-servo · 2015-07-31T04:52:15Z

⌛ Testing commit d303af2 with merge 2f41022...


        Auto merge of #6770 - ecoal95:webgl-again, r=pcwalton

Add multiple WebGL calls and improve error detection Since it probably won't merge until multiprocess lands, I plan to use this PR to keep improving WebGL support until it can land. Main TODOs are integration of tests, since it seems KhronosGroup/WebGL#1105 is going nowhere, adding missing calls and proper painting via native surfaces instead of readback. I can't resolve conflicts right now because of time but I will do it soon.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6770)

bors-servo · 2015-07-31T05:00:39Z

💔 Test failed - android

bors-servo · 2015-08-22T12:02:16Z

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

emilio · 2015-08-23T23:11:46Z

Unbitrotted again, for some reason the S-needs-rebase flag didn't disappear (cc/ @jdm)

jdm · 2015-08-24T01:45:42Z

I'll try to review this tomorrow.

jdm · 2015-08-25T13:25:31Z

-S-awaiting-review +S-needs-code-changes

Reviewed 3 of 3 files at r1, 12 of 12 files at r2.
Review status: all files reviewed at latest revision, 19 unresolved discussions, all commit checks successful.

^{components/canvas/webgl_paint_task.rs, line 164 [r2] (raw file):}
Let's make this message take a u32 instead of whatever it currently does.

^{components/script/dom/canvasrenderingcontext2d.rs, line 396 [r2] (raw file):}
I would prefer separate window and canvas bindings here.

^{components/script/dom/webglbuffer.rs, line 21 [r2] (raw file):}
The target to which this buffer was bound the first time.

^{components/script/dom/webglbuffer.rs, line 60 [r2] (raw file):}
Let's not add these annotations unless we discover performance problems that they solve.

^{components/script/dom/webglbuffer.rs, line 66 [r2] (raw file):}
We could enforce this by creating BufferTarget(u32) and ValidatedBufferTarget(u32) types, with a validate(self) -> ValidatedBufferTarget method present on BufferTarget, then make this method only take ValidatedBufferTarget and only pass validated ones in the webgl messages. That's fine as a followup issue, though.

^{components/script/dom/webglbuffer.rs, line 79 [r2] (raw file):}
Remove this unless it's solving a known problem.

^{components/script/dom/webglrenderingcontext.rs, line 65 [r2] (raw file):}
I'm not sure that this macro is pulling its weight.

^{components/script/dom/webglrenderingcontext.rs, line 80 [r2] (raw file):}
I think we can use #[derive(HeapSizeOf)] on TextureUnpacking instead.

^{components/script/dom/webglrenderingcontext.rs, line 87 [r2] (raw file):}
Same here for deriving JSTraceable.

^{components/script/dom/webglrenderingcontext.rs, line 148 [r2] (raw file):}
Remove this unless it is solving a problem.

^{components/script/dom/webglrenderingcontext.rs, line 670 [r2] (raw file):}
http://doc.servo.org/num/traits/trait.Float.html#tymethod.is_nan exists; why not use it now?

^{components/script/dom/webglrenderingcontext.rs, line 840 [r2] (raw file):}
Let's have a let global in here to break this up.

^{components/script/dom/webglrenderingcontext.rs, line 848 [r2] (raw file):}
let window and let canvas, please.

^{components/script/dom/webglrenderingcontext.rs, line 936 [r2] (raw file):}
Remove this.

^{components/script/dom/webgltexture.rs, line 23 [r2] (raw file):}
The target to which this texture...

^{components/script/dom/webgltexture.rs, line 63 [r2] (raw file):}
Let's make an enum here instead of two Option parameters.

^{components/script/dom/webgltexture.rs, line 67 [r2] (raw file):}
Remove this.

^{components/script/dom/webgltexture.rs, line 87 [r2] (raw file):}
Remove thi.

^{tests/ref/basic.list, line 380 [r2] (raw file):}
Does using reftest-wait make this test more reliable? http://mxr.mozilla.org/servo/source/tests/ref/iframe/hide_and_show.html?force=1#17

Comments from the review on Reviewable.io

emilio · 2015-08-25T14:59:09Z

Review status: 8 of 15 files reviewed at latest revision, 19 unresolved discussions, all commit checks successful.

^{components/canvas/webgl_paint_task.rs, line 164 [r2] (raw file):}
Done.

^{components/script/dom/canvasrenderingcontext2d.rs, line 396 [r2] (raw file):}
Done.

^{components/script/dom/webglbuffer.rs, line 21 [r2] (raw file):}
Done.

^{components/script/dom/webglbuffer.rs, line 60 [r2] (raw file):}
Done.

^{components/script/dom/webglbuffer.rs, line 66 [r2] (raw file):}
At the end the valid buffer targets are just ARRAY_BUFFER or ELEMENT_ARRAY_BUFFER, we could create an enum to enforce type safety there, but since we'll need to convert back to a u32, I don't know if it's worth it. Anyways I agree it can be done as a followup.

^{components/script/dom/webglbuffer.rs, line 79 [r2] (raw file):}
Done.

^{components/script/dom/webglrenderingcontext.rs, line 65 [r2] (raw file):}
You're right, I imported the WebGLError variants and renamed handle_webgl_error to just webgl_error.

^{components/script/dom/webglrenderingcontext.rs, line 80 [r2] (raw file):}
True, sorry! I tried to put them before the macro and the compiler didn't like it.

^{components/script/dom/webglrenderingcontext.rs, line 87 [r2] (raw file):}
Done.

^{components/script/dom/webglrenderingcontext.rs, line 148 [r2] (raw file):}
Done.

^{components/script/dom/webglrenderingcontext.rs, line 670 [r2] (raw file):}
Done.

^{components/script/dom/webglrenderingcontext.rs, line 840 [r2] (raw file):}
Done.

^{components/script/dom/webglrenderingcontext.rs, line 848 [r2] (raw file):}
Done.

^{components/script/dom/webglrenderingcontext.rs, line 936 [r2] (raw file):}
Done.

^{components/script/dom/webgltexture.rs, line 23 [r2] (raw file):}
Done.

^{components/script/dom/webgltexture.rs, line 63 [r2] (raw file):}
True, that was dumb on my side.

^{components/script/dom/webgltexture.rs, line 67 [r2] (raw file):}
Done.

^{components/script/dom/webgltexture.rs, line 87 [r2] (raw file):}
Done.

^{tests/ref/basic.list, line 0 [r2] (raw file):}
Yes, totally! Didn't know about that, thanks! :)

Comments from the review on Reviewable.io


        Update gleam to 0.1.9


        webgl: Implement multiple calls and improve error detection

This commit implements WebGL's: * cullFace * frontFace * enable * disable * depthMask * colorMask * clearDepth * clearStencil * depthFunc * depthRange * hint * lineWidth * pixelStorei * polygonOffset * texParameteri * texParameterf * texImage2D (partially) It inlines a lot of OpenGL calls to keep the file `components/canvas/webgl_paint_task.rs` as small as possible while keeping readability. It also improves error detection on previous calls, and sets node damage on the canvas in the drawing calls. It adds a `TexImage2D` reftest, even though it's not enabled because: * WebGL paints the image when it loads (asynchronously), so the reftest doesn't wait for it and it finishes early * If we change the source for the base64 src of the image it works as expected in non-headless mode, but the test harness locks

jdm · 2015-08-25T15:22:57Z

@bors-servo: r+
Thanks for the PR!

Reviewed 10 of 10 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from the review on Reviewable.io

bors-servo · 2015-08-25T15:22:58Z

📌 Commit 6341c77 has been approved by jdm

bors-servo · 2015-08-25T15:23:01Z

⌛ Testing commit 6341c77 with merge a109a33...


        Auto merge of #6770 - ecoal95:webgl-again, r=jdm

Add multiple WebGL calls and improve error detection Since it probably won't merge until multiprocess lands, I plan to use this PR to keep improving WebGL support until it can land. Main TODOs are integration of tests, since it seems KhronosGroup/WebGL#1105 is going nowhere, adding missing calls and proper painting via native surfaces instead of readback. I can't resolve conflicts right now because of time but I will do it soon.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6770)

bors-servo · 2015-08-25T16:04:38Z

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2, mac3

highfive added the S-awaiting-review label Jul 25, 2015

emilio force-pushed the emilio:webgl-again branch from e6009d0 to d303af2 Jul 26, 2015

emilio changed the title ~~[WIP] Improve WebGL~~ Add multiple WebGL calls and improve error detection Jul 26, 2015

jdm removed the S-awaiting-review label Jul 27, 2015

emilio mentioned this pull request Jul 27, 2015

Add more gl calls for WebGL servo/gleam#34

Merged

jdm added the S-needs-code-changes label Jul 28, 2015

metajack added S-blocked-on-external S-awaiting-merge and removed S-needs-code-changes labels Jul 29, 2015

metajack assigned pcwalton Jul 29, 2015

metajack removed the S-blocked-on-external label Jul 31, 2015

emilio force-pushed the emilio:webgl-again branch from d35471b to 63dc9ad Aug 20, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Aug 22, 2015

emilio force-pushed the emilio:webgl-again branch 2 times, most recently from cbc9ee1 to aa85e28 Aug 23, 2015

jdm removed the S-needs-rebase label Aug 24, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Aug 25, 2015

emilio force-pushed the emilio:webgl-again branch from aa85e28 to ec23099 Aug 25, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 25, 2015

emilio force-pushed the emilio:webgl-again branch from ec23099 to 883baf6 Aug 25, 2015

emilio added 2 commits Aug 23, 2015

Update gleam to 0.1.9

af3310f

emilio force-pushed the emilio:webgl-again branch from 883baf6 to 6341c77 Aug 25, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 25, 2015

bors-servo merged commit 6341c77 into servo:master Aug 25, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors-servo mentioned this pull request Aug 25, 2015

make dom_struct derive HeapSizeOf #7361

Merged

jdm mentioned this pull request Aug 25, 2015

Make handle_potential_webgl_error more ergonomic #7360

Merged

servo / servo

Add multiple WebGL calls and improve error detection #6770

Add multiple WebGL calls and improve error detection #6770

emilio commented Jul 25, 2015

hoppipolla-critic-bot commented Jul 25, 2015

Ms2ger commented Jul 25, 2015

emilio commented Jul 25, 2015

pcwalton commented Jul 25, 2015

emilio commented Jul 26, 2015

pcwalton commented Jul 27, 2015

bors-servo commented Jul 27, 2015

bors-servo commented Jul 27, 2015

bors-servo commented Jul 27, 2015

emilio commented Jul 27, 2015

emilio commented Jul 28, 2015

metajack commented Jul 29, 2015

glennw commented Jul 29, 2015

emilio commented Jul 29, 2015

metajack commented Jul 31, 2015

bors-servo commented Jul 31, 2015

bors-servo commented Jul 31, 2015

bors-servo commented Aug 22, 2015

emilio commented Aug 23, 2015

jdm commented Aug 24, 2015

jdm commented Aug 25, 2015

emilio commented Aug 25, 2015

jdm commented Aug 25, 2015

bors-servo commented Aug 25, 2015

bors-servo commented Aug 25, 2015

bors-servo commented Aug 25, 2015

servo / servo

Join GitHub today

Add multiple WebGL calls and improve error detection #6770

Add multiple WebGL calls and improve error detection #6770

Conversation

emilio commented Jul 25, 2015

hoppipolla-critic-bot commented Jul 25, 2015

Ms2ger commented Jul 25, 2015

emilio commented Jul 25, 2015

pcwalton commented Jul 25, 2015

emilio commented Jul 26, 2015

pcwalton commented Jul 27, 2015

bors-servo commented Jul 27, 2015

bors-servo commented Jul 27, 2015

bors-servo commented Jul 27, 2015

emilio commented Jul 27, 2015

emilio commented Jul 28, 2015

metajack commented Jul 29, 2015

glennw commented Jul 29, 2015

emilio commented Jul 29, 2015

metajack commented Jul 31, 2015

bors-servo commented Jul 31, 2015

bors-servo commented Jul 31, 2015

bors-servo commented Aug 22, 2015

emilio commented Aug 23, 2015

jdm commented Aug 24, 2015

jdm commented Aug 25, 2015

emilio commented Aug 25, 2015

jdm commented Aug 25, 2015

bors-servo commented Aug 25, 2015

bors-servo commented Aug 25, 2015

bors-servo commented Aug 25, 2015