Update gleam, and use a dynamic GL function table. #81

emilio · 2017-02-13T11:55:02Z

emilio · 2017-02-13T11:55:55Z

kvark · 2017-02-14T16:34:15Z

So gl::Gl is a trait, and thus the new code works with the trait object, right?
Is there any benefit of using the unsafe raw pointers versus just Rc/Weak?

emilio · 2017-02-14T16:38:03Z

@kvark That's right. It avoids some refcount churn. In this case it seemed easy enough to do it, but I'd be fine also with Rc. It may make sense to share gl function tables across threads though, that's something that Rc would prevent, but we don't use that at all I think so it may be ok.

emilio · 2017-02-14T16:47:06Z

Oh, and I hadn't been following the gleam PR btw, the generic proposal looks ok to me. It's kind of sad to use dynamic dispatch, but I was taking that for granted already. Couldn't we make Gl a static table and add convenience methods to it instead? I did something similar for clang-sys for bindgen (see the functions struct in KyleMayes/clang-sys#45). It'd be a matter of adding all the glue that would be inlined and would just look like:

#[inline]
fn flush(&self) {
    (self.functions.expect("glFlush not loaded"))()
}

sotaroikeda · 2017-02-16T00:02:50Z

Couldn't we make Gl a static table and add convenience methods to it instead?

It might be better for performance. But I want to handle it as another issue, since we want to enable Graphics branch soon.

kvark · 2017-02-16T05:43:07Z

@emilio

It avoids some refcount churn.

The extra refcount work would only apply to resource creation/deletion, right? I wouldn't be too concerned and preferred the code clarity (and less unsafety) instead.

It may make sense to share gl function tables across threads though

Could you explain why we'd need that? It sounds like a hack.

Couldn't we make Gl a static table and add convenience methods to it instead?

Possible, although I'd rather want to see static dispatch here in the first place. Considering the deadlines, this will have to wait.

emilio · 2017-02-16T05:57:26Z

On Wed, Feb 15, 2017 at 09:43:07PM -0800, Dzmitry Malyshau wrote: @emilio > It avoids some refcount churn. The extra refcount work would only apply to resource creation/deletion, right? I wouldn't be too concerned and preferred the code clarity (and less unsafety) instead.

Right, and that's a fair point.

> It may make sense to share gl function tables across threads though Could you explain why we'd need that? It sounds like a hack.

Didn't have a specific use case in mind, but yeah, I guess context creation time is not a big deal. I could imagine us sharing the same function hooks or something, but yeah, as I said I don't expect it to be super-useful.

> Couldn't we make Gl a static table and add convenience methods to it instead? Possible, although I'd rather want to see static dispatch here in the first place. Considering the deadlines, this will have to wait.

That's fair, as I said I don't love dynamic dispatch, but we can probably live with it.

kvark · 2017-02-16T06:28:11Z

@emilio thanks!
I've just published gleam-0.3.0, so let's have this PR updated.

sotaroikeda · 2017-02-16T06:38:32Z

@emilio, thank you!

emilio · 2017-02-16T10:37:48Z

Done, feel free to take a look, let me know when do you want to merge this.

kvark · 2017-02-16T15:31:55Z

src/gl_context.rs

        let native_context = try!(Native::create_shared_with_dispatcher(shared_with, dispatcher));
+        // XXX how to handle GLES instantiation?
+        let gl_ = gl::GlFns::load_with(|s| GLContext::<Native>::get_proc_address(s) as *const _);


it appears that we need to sort this out before merging

Oh, right, I just grabbed that from the original Sotaro's patch. I guess the idea here would be doing so per-backend (that is, the egl back-end would load gles functions, etc). That being said, in which case would it make a difference? I guess we can provide only the GLES-like interface given this is primarily for WebGL and we create compat contexts, alternatively, I can make it generic, but that cries to avoid the virtual dispatch then.

alternatively, I can make it generic, but that cries to avoid the virtual dispatch then

Well, doing static dispatch is still on the table, just for a different time frame - servo/gleam#107
So maybe it's not the worst idea to be ready for that ;)

Really though, If this newly created context is shared with some existing one, we should use Gl function loader if that existing one is GL, and GLES correspondingly. If it's an entirely new one (shared_with.is_none() == true), this is probably where we want the gleam::GlType flag to be passed in for run-time context type selection.

That sounds reasonable, though probably for WebGL it's enough with loading unconditionally just the gles functions.

tl;dr; I'll prepare a patch with that.

kvark · 2017-02-16T15:33:39Z

src/draw_buffer.rs

-        match *self {
-            ColorAttachment::Renderbuffer(id) => gl::delete_renderbuffers(&[id]),
-            ColorAttachment::Texture(tex_id) => gl::delete_textures(&[tex_id]),
+    fn destroy(self, gl: &gl::Gl) {


why do we need to destroy now instead of drop?

Didn't want to grow them by an extra pointer given we only create one per draw buffer, but I can definitely do it I guess.

Ah, you are passing gl in, I see. And the function is not public, so there is no concern about calling it twice.
Let's leave it with destroy then :)

kvark · 2017-02-17T02:53:15Z

src/draw_buffer.rs

-        match *self {
-            ColorAttachment::Renderbuffer(id) => gl::delete_renderbuffers(&[id]),
-            ColorAttachment::Texture(tex_id) => gl::delete_textures(&[tex_id]),
+    fn destroy(self, gl: &gl::Gl) {


Ah, you are passing gl in, I see. And the function is not public, so there is no concern about calling it twice.
Let's leave it with destroy then :)

kvark · 2017-02-17T02:59:23Z

src/gl_context.rs

-        -> Result<GLContext<Native>, &'static str> {
-        Self::new_shared_with_dispatcher(size, attributes, color_attachment_type, shared_with, None)
+        -> Result<Self, &'static str> {
+        Self::new_shared_with_dispatcher(size,


I'm a bit confused about this. create_shared_with_dispatcher calls new_shared_with_dispatcher, and new_shared_with_dispatcher calls create_shared_with_dispatcher, both seem unconditional.

It... doesn't? create_shared_with_dispatcher calls the relevant method on the underlying platform-dependent context.

I see now, you are correct.

kvark · 2017-02-17T03:02:36Z

src/gl_context.rs

        let native_context = try!(Native::create_shared_with_dispatcher(shared_with, dispatcher));
+        // XXX how to handle GLES instantiation?
+        let gl_ = gl::GlFns::load_with(|s| GLContext::<Native>::get_proc_address(s) as *const _);


alternatively, I can make it generic, but that cries to avoid the virtual dispatch then

Well, doing static dispatch is still on the table, just for a different time frame - servo/gleam#107
So maybe it's not the worst idea to be ready for that ;)

Really though, If this newly created context is shared with some existing one, we should use Gl function loader if that existing one is GL, and GLES correspondingly. If it's an entirely new one (shared_with.is_none() == true), this is probably where we want the gleam::GlType flag to be passed in for run-time context type selection.

sotaroikeda · 2017-02-24T06:04:08Z

What is a current status?

emilio · 2017-02-25T18:48:54Z

Whoops, totally forgot about this, there it is.

kvark

Thanks @emilio , looks good!
I got a few minor things, which are fine to address separately, if you wish.

kvark · 2017-02-26T01:14:27Z

src/draw_buffer.rs

@@ -113,8 +116,9 @@ impl DrawBuffer {

        try!(draw_buffer.init(context, color_attachment_type));

-        debug_assert!(gl::check_frame_buffer_status(gl::FRAMEBUFFER) == gl::FRAMEBUFFER_COMPLETE);
-        debug_assert!(gl::get_error() == gl::NO_ERROR);
+        debug_assert!(


debug_assert_eq! would be much better here since it would print the actual error/status

kvark · 2017-02-26T01:15:20Z

src/draw_buffer.rs

-        //   ColorAttachment
-        gl::delete_renderbuffers(&[self.stencil_renderbuffer, self.depth_renderbuffer]);
+    fn gl(&self) -> &gl::Gl {
+        // See the comment on top the GLContext field to see why our usage of


perhaps, there is no longer a point in this comment? There is no unsafe here, so safety is assumed by default

kvark · 2017-02-26T01:17:04Z

src/gl_context.rs

-        -> Result<GLContext<Native>, &'static str> {
-        Self::new_shared_with_dispatcher(size, attributes, color_attachment_type, shared_with, None)
+        -> Result<Self, &'static str> {
+        Self::new_shared_with_dispatcher(size,


I see now, you are correct.

kvark · 2017-02-26T01:19:12Z

src/tests.rs

-#[cfg(not(target_os="android"))]
-static LOAD_GL: Once = ONCE_INIT;
+#[cfg(target_os = "android")]
+const GL_API_TYPE: gl::GlType = gl::GlType::Gles;


could use default(), which is already implemented on GlType

emilio · 2017-02-26T12:51:39Z

Thanks for the review @kvark! Landed and published as 0.7.0

sotaroikeda · 2017-02-27T03:43:52Z

Thank you!

emilio mentioned this pull request Feb 14, 2017

CGLCreateContext fails when creating a context through WebRender from WebGL. #82

Open

kvark mentioned this pull request Feb 14, 2017

Run-time selection between GL and GLES servo/gleam#106

Merged

emilio force-pushed the gleamup branch 2 times, most recently from b29c2a9 to 4d2f294 Compare February 16, 2017 10:36

kvark suggested changes Feb 16, 2017

View reviewed changes

kvark suggested changes Feb 17, 2017

View reviewed changes

emilio force-pushed the gleamup branch from 4d2f294 to 2e6c489 Compare February 25, 2017 18:48

kvark approved these changes Feb 26, 2017

View reviewed changes

emilio force-pushed the gleamup branch 2 times, most recently from c885e6a to 23d0368 Compare February 26, 2017 12:50

Update gleam, and use a dynamic GL function table.

bde5260

emilio force-pushed the gleamup branch from 23d0368 to bde5260 Compare February 26, 2017 12:50

emilio merged commit 26096cb into master Feb 26, 2017

emilio deleted the gleamup branch February 26, 2017 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update gleam, and use a dynamic GL function table. #81

Update gleam, and use a dynamic GL function table. #81

emilio commented Feb 13, 2017

emilio commented Feb 13, 2017

kvark commented Feb 14, 2017

emilio commented Feb 14, 2017

emilio commented Feb 14, 2017

sotaroikeda commented Feb 16, 2017

kvark commented Feb 16, 2017

emilio commented Feb 16, 2017 via email

kvark commented Feb 16, 2017

sotaroikeda commented Feb 16, 2017

emilio commented Feb 16, 2017

kvark Feb 16, 2017

emilio Feb 16, 2017

kvark Feb 17, 2017 •

edited

Loading

emilio Feb 17, 2017

emilio Feb 17, 2017

kvark Feb 16, 2017

emilio Feb 16, 2017

kvark Feb 17, 2017

kvark Feb 17, 2017

kvark Feb 17, 2017

emilio Feb 17, 2017

kvark Feb 26, 2017

kvark Feb 17, 2017 •

edited

Loading

sotaroikeda commented Feb 24, 2017

emilio commented Feb 25, 2017

kvark left a comment

kvark Feb 26, 2017

kvark Feb 26, 2017

kvark Feb 26, 2017

kvark Feb 26, 2017

emilio commented Feb 26, 2017

sotaroikeda commented Feb 27, 2017

Update gleam, and use a dynamic GL function table. #81

Update gleam, and use a dynamic GL function table. #81

Conversation

emilio commented Feb 13, 2017

emilio commented Feb 13, 2017

kvark commented Feb 14, 2017

emilio commented Feb 14, 2017

emilio commented Feb 14, 2017

sotaroikeda commented Feb 16, 2017

kvark commented Feb 16, 2017

emilio commented Feb 16, 2017 via email

kvark commented Feb 16, 2017

sotaroikeda commented Feb 16, 2017

emilio commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark Feb 17, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

sotaroikeda commented Feb 24, 2017

emilio commented Feb 25, 2017

kvark 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 Feb 26, 2017

sotaroikeda commented Feb 27, 2017

kvark Feb 17, 2017 •

edited

Loading

kvark Feb 17, 2017 •

edited

Loading