Implement new WebGL interfaces and methods #6293

emilio · 2015-06-05T14:33:30Z

This commit implements:

WebGLFramebuffer
WebGLRenderbuffer
WebGLTexture

And adds the following methods to WebGLRenderingContext:

create{Texture,Framebuffer,Renderbuffer}
bind{Texture,Framebuffer,Renderbuffer}
destroy{Buffer,Texture,Framebuffer,Renderbuffer}

Fixes:

WebGLUniform location shouldn't inherit from WebGLObject.

Known Issues:

WebGL objects have to be destroyed on drop, we may want to keep a reference to the context, or maybe a clone of the renderer to achieve this

Also refactors a huge part of the current implementation, to allow
failing on creation of different WebGL objects.

Blocked on servo/gleam#22

A reftest for most of the added functionality is not doable right now,
we need a few more functions in order to upload a texture, for example.

highfive · 2015-06-05T14:33:33Z

Warning

These commits modify unsafe code. Please review it carefully!

hoppipolla-critic-bot · 2015-06-05T14:33:36Z

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

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.

emilio · 2015-06-05T14:36:48Z

r? @jdm | @glennw | @nox

nox · 2015-06-05T23:16:00Z

Are there any interfaces extending WebGLObject that won't have a get_id() helper? If not, could it be moved there?

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

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.
Reviewed files:

components/canvas/lib.rs @ r1
components/canvas/webgl_paint_task.rs @ r1
components/canvas_traits/lib.rs @ r1
components/script/dom/mod.rs @ r1
components/script/dom/webglframebuffer.rs @ r1
components/script/dom/webglrenderbuffer.rs @ r1
components/script/dom/webglrenderingcontext.rs @ r1
components/script/dom/webgltexture.rs @ r1
components/script/dom/webgluniformlocation.rs @ r1
components/script/dom/webidls/WebGLFramebuffer.webidl @ r1
components/script/dom/webidls/WebGLRenderbuffer.webidl @ r1
components/script/dom/webidls/WebGLRenderingContext.webidl @ r1
components/script/dom/webidls/WebGLTexture.webidl @ r1

^{components/canvas/webgl_paint_task.rs, line 258 [r1] (raw file):}
"compliant".

Any harm in not including it, if it's not spec-compliant?

^{components/script/dom/webglrenderingcontext.rs, line 139 [r1] (raw file):}
Is there more harm in not supporting it at all than making it return 0 unconditionally?

^{components/script/dom/webglrenderingcontext.rs, line 402 [r1] (raw file):}
This looks like a gratuitous rewrite, I prefer the version without potentially-panicking calls (the unwrappings)

Comments from the review on Reviewable.io

emilio · 2015-06-06T07:28:33Z

Actually not, there's no interface extending WebGLObject that doesn't implement a method like that, but at this point I'm a bit sceptical about it, I don't know why neither Firefox/Chromium expose it to the window, and I'd prefer not to add functionality there until I discover why.

emilio · 2015-06-06T07:40:14Z

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.

^{components/canvas/webgl_paint_task.rs, line 258 [r1] (raw file):}
This call is not totally spec compliant (yet), since we should ensure all the shaders conform with GLES Shading Language 1.00. Right now we don't do that check, but I think removing shaders from our WebGL implementation, even if it's temporary, is a too high price.

I'd prefer to keep them, and add the sanity check after it, instead of removing the shader calls and rewrite them again later.

 A WebGL implementation must only accept shaders which conform to The OpenGL ES Shading Language, Version 1.00 [GLES20GLSL], and which do not exceed the minimum functionality mandated in Sections 4 and 5 of Appendix A. In particular:

^{components/script/dom/webglrenderingcontext.rs, line 139 [r1] (raw file):}
Well, that call is spec'd, and is totally legal not to have extension support, but I'll remove it if you want/

^{components/script/dom/webglrenderingcontext.rs, line 402 [r1] (raw file):}
We check before and early return if any of those is None, it's safe to call unwrap there. I'll change it if you want, but I think it's cleaner this way.

Comments from the review on Reviewable.io

nox · 2015-06-06T09:08:09Z

I didn't mean exposing it as a DOM method, but as a helper on WebGLObject directly, to avoid duplicating the field and the Rust method everywhere.

Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.

^{components/script/dom/webglrenderingcontext.rs, line 402 [r1] (raw file):}
The code flow is made less obvious and doesn't follows the shape of the Option type anymore. Pinging @Manishearth to decide because he like to discuss bikeshed paint.

Comments from the review on Reviewable.io

emilio · 2015-06-06T09:31:56Z

Yes, I supposed so, but what I was trying to say (sorry), is that gecko for example doesn't even have the interface (they have a base class for them, but not a DOM binding).

Anyways: The WebGL2 spec does spec some objects without id, like queries, so I think that even if now it could make sense to move it there, that change will have to be rolled back in the future.

Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.

Comments from the review on Reviewable.io

emilio · 2015-06-06T09:42:26Z

I updated the tests and corrected the typo :)

SimonSapin · 2015-06-06T09:51:15Z

Review status: 12 of 14 files reviewed, 1 unresolved discussion, all commit checks successful.

^{components/script/dom/webglrenderingcontext.rs, line 402 [r1] (raw file):}
I also prefer the previous code, fwiw. Another option is to use the unwrap_or_return! macro from the mac crate. (That crate is already used by some of Servo’s dependencies.)

Comments from the review on Reviewable.io

nox · 2015-06-06T09:52:53Z

Perfect, no other tests because we need to integrate Khronos test suite, right?

-S-awaiting-answer -S-awaiting-review

Update the bikeshed snippet and it can be merged.

Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.
Reviewed files:

components/canvas/webgl_paint_task.rs @ r2
tests/wpt/mozilla/tests/mozilla/interfaces.html @ r2

Comments from the review on Reviewable.io


        Implement new WebGL interfaces and methods

This commit implements: * WebGLFramebuffer * WebGLRenderbuffer * WebGLTexture And adds the following methods to `WebGLRenderingContext`: * create{Texture,Framebuffer,Renderbuffer} * bind{Texture,Framebuffer,Renderbuffer} * destroy{Buffer,Texture,Framebuffer,Renderbuffer} Fixes: * WebGLUniform location shouldn't inherit from WebGLObject. Known Issues: * WebGL objects have to be destroyed on drop, we may want to keep a reference to the context, or maybe a clone of the renderer to achieve this Also refactors a huge part of the current implementation, to allow failing on creation of different WebGL objects. Blocked on servo/gleam#22 A reftest for most of the added functionality is not doable right now, we need a few more functions in order to upload a texture, for example.

emilio · 2015-06-06T10:18:03Z

Reverted the changes then :P

You're right, I can't do reftests with this until I implement the tex* set of functions, and making simple html tests when we have to integrate more serious tests is not worth it I think.

emilio · 2015-06-06T10:23:46Z

By the way it's still blocked on servo/gleam#22, I don't know if you can review it.

nox · 2015-06-06T12:18:54Z

Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.
Reviewed files:

components/script/dom/webglrenderingcontext.rs @ r3

Comments from the review on Reviewable.io

nox · 2015-06-08T17:44:25Z

#6305

nox · 2015-06-08T20:55:17Z

@bors-servo: r+

-S-blocked-on-external

#6305 got merged.

Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.

Comments from the review on Reviewable.io

bors-servo · 2015-06-08T20:55:17Z

📌 Commit 9f94d39 has been approved by nox

bors-servo · 2015-06-08T20:55:19Z

⌛ Testing commit 9f94d39 with merge d01d962...


        Auto merge of #6293 - ecoal95:webgl-objects, r=nox

This commit implements: * WebGLFramebuffer * WebGLRenderbuffer * WebGLTexture And adds the following methods to `WebGLRenderingContext`: * create{Texture,Framebuffer,Renderbuffer} * bind{Texture,Framebuffer,Renderbuffer} * destroy{Buffer,Texture,Framebuffer,Renderbuffer} Fixes: * WebGLUniform location shouldn't inherit from WebGLObject. Known Issues: * WebGL objects have to be destroyed on drop, we may want to keep a reference to the context, or maybe a clone of the renderer to achieve this Also refactors a huge part of the current implementation, to allow failing on creation of different WebGL objects. Blocked on servo/gleam#22 A reftest for most of the added functionality is not doable right now, we need a few more functions in order to upload a texture, for example.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6293)

bors-servo · 2015-06-08T21:36:44Z

💔 Test failed - linux2

nox · 2015-06-09T15:06:35Z

@bors-servo: retry

bors-servo · 2015-06-09T15:06:38Z

⌛ Testing commit 9f94d39 with merge e09c555...


        Auto merge of #6293 - ecoal95:webgl-objects, r=nox

This commit implements: * WebGLFramebuffer * WebGLRenderbuffer * WebGLTexture And adds the following methods to `WebGLRenderingContext`: * create{Texture,Framebuffer,Renderbuffer} * bind{Texture,Framebuffer,Renderbuffer} * destroy{Buffer,Texture,Framebuffer,Renderbuffer} Fixes: * WebGLUniform location shouldn't inherit from WebGLObject. Known Issues: * WebGL objects have to be destroyed on drop, we may want to keep a reference to the context, or maybe a clone of the renderer to achieve this Also refactors a huge part of the current implementation, to allow failing on creation of different WebGL objects. Blocked on servo/gleam#22 A reftest for most of the added functionality is not doable right now, we need a few more functions in order to upload a texture, for example.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6293)

bors-servo · 2015-06-09T15:49:50Z

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

emilio · 2015-06-09T18:11:29Z

@nox Thanks for taking the time to update gleam and merge this, I'm in my final exams period so I couldn't take the time :)

nox · 2015-06-09T18:46:26Z

@ecoal95 Thanks for your contribution, I hope you'll have good marks. :)

highfive added the S-awaiting-review label Jun 5, 2015

nox added the S-blocked-on-external label Jun 5, 2015

nox added S-awaiting-answer S-needs-code-changes and removed S-awaiting-review labels Jun 5, 2015

emilio force-pushed the emilio:webgl-objects branch from c8638d0 to b84d494 Jun 6, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 6, 2015

nox added S-needs-code-changes and removed S-awaiting-answer S-awaiting-review labels Jun 6, 2015

emilio force-pushed the emilio:webgl-objects branch from b84d494 to 9f94d39 Jun 6, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 6, 2015

nox removed the S-awaiting-review label Jun 6, 2015

nox self-assigned this Jun 6, 2015

nox removed the S-blocked-on-external label Jun 8, 2015

nox added the S-awaiting-merge label Jun 8, 2015

nox mentioned this pull request Jun 9, 2015

Intermittent pass for /XMLHttpRequest/timeout-cors-async.htm #6312

Closed

bors-servo merged commit 9f94d39 into servo:master Jun 9, 2015
2 checks passed

2 checks passed

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

homu Test successful
Details

emilio deleted the emilio:webgl-objects branch Jun 9, 2015

servo / servo

Implement new WebGL interfaces and methods #6293

Implement new WebGL interfaces and methods #6293

emilio commented Jun 5, 2015

highfive commented Jun 5, 2015

hoppipolla-critic-bot commented Jun 5, 2015

emilio commented Jun 5, 2015

nox commented Jun 5, 2015

emilio commented Jun 6, 2015

emilio commented Jun 6, 2015

nox commented Jun 6, 2015

emilio commented Jun 6, 2015

emilio commented Jun 6, 2015

SimonSapin commented Jun 6, 2015

nox commented Jun 6, 2015

emilio commented Jun 6, 2015

emilio commented Jun 6, 2015

nox commented Jun 6, 2015

nox commented Jun 8, 2015

nox commented Jun 8, 2015

bors-servo commented Jun 8, 2015

bors-servo commented Jun 8, 2015

bors-servo commented Jun 8, 2015

nox commented Jun 9, 2015

bors-servo commented Jun 9, 2015

bors-servo commented Jun 9, 2015

emilio commented Jun 9, 2015

nox commented Jun 9, 2015

servo / servo

Join GitHub today

Implement new WebGL interfaces and methods #6293

Implement new WebGL interfaces and methods #6293

Conversation

emilio commented Jun 5, 2015

highfive commented Jun 5, 2015

hoppipolla-critic-bot commented Jun 5, 2015

emilio commented Jun 5, 2015

nox commented Jun 5, 2015

emilio commented Jun 6, 2015

emilio commented Jun 6, 2015

nox commented Jun 6, 2015

emilio commented Jun 6, 2015

emilio commented Jun 6, 2015

SimonSapin commented Jun 6, 2015

nox commented Jun 6, 2015

emilio commented Jun 6, 2015

emilio commented Jun 6, 2015

nox commented Jun 6, 2015

nox commented Jun 8, 2015

nox commented Jun 8, 2015

bors-servo commented Jun 8, 2015

bors-servo commented Jun 8, 2015

bors-servo commented Jun 8, 2015

nox commented Jun 9, 2015

bors-servo commented Jun 9, 2015

bors-servo commented Jun 9, 2015

emilio commented Jun 9, 2015

nox commented Jun 9, 2015