Implement WGL #64

MortimerGoro · 2016-09-19T12:17:03Z

WGL implementation, now the servo webgl triangle demo runs on windows ;)

emilio

Super exciting to see this!

I need to run and I still haven't made it to the end. A lot of formatting nits and a few questions, but I still need to reach to the core of this PR, I'll try to do it over the rest of this week.

Thanks!

emilio · 2016-09-19T12:34:23Z

src/platform/with_wgl/native_gl_context.rs

+    static ref GL_LIB: Option<HMODULEWrapper>  = {
+        let p = unsafe{kernel32::LoadLibraryA(b"opengl32.dll\0".as_ptr() as *const _)};
+        if p.is_null() {
+            debug!("opengl32.dll not found!");


nit: error! here will make it appear on the console, and seems more likely what we want.

emilio · 2016-09-19T12:35:07Z

src/platform/with_wgl/native_gl_context.rs

+
+lazy_static! {
+    static ref GL_LIB: Option<HMODULEWrapper>  = {
+        let p = unsafe{kernel32::LoadLibraryA(b"opengl32.dll\0".as_ptr() as *const _)};


nit: unsafe {, and as *const _) };.

emilio · 2016-09-19T12:39:13Z

src/platform/with_wgl/native_gl_context.rs

+
+pub struct NativeGLContext {
+    pub render_ctx: winapi::HGLRC,
+    pub device_ctx:winapi::HDC, //


nit: space after the colon. Also, either do comment, or remove the two slashes.

Please make the members private, since the intention is making NativeGLContext cross-platform. Feel free to add accessor methods if needed.

emilio · 2016-09-19T12:39:50Z

src/platform/with_wgl/native_gl_context.rs

+    }
+}
+
+pub struct NativeGLContextHandle(pub winapi::HGLRC, pub winapi::HDC);


Same here regarding pub.

emilio · 2016-09-19T12:42:19Z

src/platform/with_wgl/native_gl_context.rs

+            let p = wgl::GetProcAddress(addr) as *const _;
+            if !p.is_null() { return p; }
+            match *GL_LIB {
+                Some(ref lib) =>  kernel32::GetProcAddress(lib.0, addr) as *const _,


hmm... I'm worried we can end up mixing symbols from taken from GL_LIB and from wgl. May it be a problem?

If it is, please only call wgl::GetProcAddress when it's None, if it's not, please write a comment saying why :)

It seems strange but we have to load and mix symbols from both because wglGetProcAddress doesn't return function pointers for some legacy functions that are exported by the opengl32.dll itself (old methods coming from OpenGL 1.1). I'll add some comments to the code to explain this.

emilio · 2016-09-19T12:47:54Z

src/platform/with_wgl/native_gl_context.rs

+    }
+
+    fn create_shared(with: Option<&Self::Handle>) -> Result<NativeGLContext, &'static str> {
+        match unsafe{super::utils::create_offscreen(with, &WGLAttributes::default())}{


nit: spaces between braces, just as before.

emilio · 2016-09-19T12:47:58Z

src/platform/with_wgl/native_gl_context.rs

+                None => ptr::null_mut()
+            }
+        }
+


nit: extra newline

emilio · 2016-09-19T12:48:33Z

src/platform/with_wgl/native_gl_context.rs

+    }
+
+    fn current_handle() -> Option<Self::Handle> {
+        let handle = unsafe{ wgl::GetCurrentContext()};


nit: spacing in braces, here and in the rest of the file.

emilio · 2016-09-19T12:48:35Z

src/platform/with_wgl/native_gl_context.rs

+                device_ctx: handle.1,
+                weak: true
+            })
+        }


nit: rust style is } else {.

emilio · 2016-09-19T12:49:43Z

src/platform/with_wgl/native_gl_context.rs

+            Ok(())
+        }
+        else {
+            Err("gwl::MakeCurrent (on unbind)")


We definitely shouldn't return an error if the current context was already unbound, the error condition should be wgl::MakeCurrent failing.

emilio · 2016-09-19T12:59:44Z

(Sorry, misclick)

emilio

I need a bit more study still, but this looks great! Let me know if I can help somehow :)

emilio · 2016-09-20T14:44:49Z

src/platform/with_wgl/native_gl_context.rs

+    fn current_handle() -> Option<Self::Handle> {
+        let handle = unsafe { wgl::GetCurrentContext() };
+        if !handle.is_null() {
+            let hdc = unsafe { wgl::GetCurrentDC() };


Should we assert! here that hdc is not null (or return an error)?

emilio · 2016-09-20T14:46:23Z

src/platform/with_wgl/utils.rs

+    if !share.is_null() {
+        if wgl::ShareLists(share as *const c_void, ctx) == 0 {
+            return Err(format!("wglShareLists failed: {}",
+                               format!("{}", io::Error::last_os_error())));


nit: this second format! (here and above) shouldn't be needed.

emilio · 2016-09-20T14:47:07Z

src/platform/with_wgl/utils.rs

+        }
+    };
+
+    return Ok((ctx as winapi::HGLRC, hdc));


nit: no need the return keyword at the end of the function, you can avoid the semicolon at the end of the line.

emilio · 2016-09-20T14:49:50Z

src/platform/with_wgl/utils.rs

+}
+
+// creates a full context: attempts to use optional ext WGL functions
+unsafe fn create_full_context(settings: &WGLAttributes,


This seems sensible enough, though, I'll need to study a bit more since I'm not confident enough with the windows API. Thanks for pointing to glutin.

Also, can you add proper attribution to the glutin code?

I have added more attribution to glutin code and link to their repo in the last commit. Maybe you could create a licence/author files in this repo and we can add more attribution there

emilio

The code looks reasonable, I can't test it, but I'm happy with it. Please rebase, do a squash and then I'll merge it.

If you find a way to run the tests this using AppVeyor or similar, that'd be awesome :-)

emilio · 2016-09-26T09:54:15Z

src/platform/with_wgl/native_gl_context.rs

+    fn unbind(&self) -> Result<(), &'static str> {
+        unsafe {
+            if self.is_current() && wgl::MakeCurrent(ptr::null_mut(), ptr::null_mut()) == 0 {
+                Err("gwl::MakeCurrent (on unbind)")


nit: wgl instead of gwl.

emilio · 2016-09-26T09:58:54Z

src/platform/with_wgl/utils.rs

+
+    // Disable or enable vsync
+    if extensions.split(' ').find(|&i| i == "WGL_EXT_swap_control").is_some() {
+        let _guard = try!(CurrentContextGuard::make_current(hdc, ctx as winapi::HGLRC));


Are you sure you need the CurrentContextGuard here? It seems to me we're going to bind the context right away after this, so we can rather bind it directly.

emilio · 2016-09-26T09:59:12Z

src/platform/with_wgl/utils.rs

+}
+
+unsafe fn create_hidden_window() -> Result<winapi::HWND, &'static str> {
+


nit: Remove this extra line.

…ing gl symbols after a context creation: wglGetProcAddress only works in the presence of a valid GL context, so we use a dummy ctx when the caller calls wglGetProcAddress without a valid GL context bound

emilio

Sorry I hadn't look more carefully the error paths, I've left a few comments on that to prevent leaks.

emilio · 2016-09-27T17:06:34Z

src/platform/with_wgl/native_gl_context.rs

+        let addr = CString::new(addr.as_bytes()).unwrap();
+        let addr = addr.as_ptr();
+        unsafe {
+


nit: extra line.

emilio · 2016-09-27T17:06:43Z

src/platform/with_wgl/native_gl_context.rs

+                None => ptr::null_mut(),
+            }
+        }
+


nit: Here too

emilio · 2016-09-27T17:07:50Z

src/platform/with_wgl/utils.rs

+            let mut placement: winapi::WINDOWPLACEMENT = mem::zeroed();
+            placement.length = mem::size_of::<winapi::WINDOWPLACEMENT>() as winapi::UINT;
+            if user32::GetWindowPlacement(window, &mut placement) == 0 {
+                panic!();


please panic with a descriptive message, or return an error.

emilio · 2016-09-27T17:08:39Z

src/platform/with_wgl/utils.rs

+
+        let hdc = user32::GetDC(win);
+        if hdc.is_null() {
+            let err = Err(format!("GetDC function failed: {}", io::Error::last_os_error()));


nit: return Err(..). Also, it seems you need to clean up the window you just created.

emilio · 2016-09-27T17:09:00Z

src/platform/with_wgl/utils.rs

+                               io::Error::last_os_error()));
+        }
+
+        let hdc = user32::GetDC(win);


I assume hdc doesn't need any cleanup, am I right?

emilio · 2016-09-27T17:11:54Z

src/platform/with_wgl/utils.rs

+            .map_err(|_| "Pixel format not available".to_owned()))
+    } else {
+        try!(choose_native_pixel_format(hdc, &settings.pixel_format)
+            .map_err(|_| "Pixel format not available".to_owned()))


Would be nice using two different errors here.

emilio · 2016-09-27T17:12:50Z

src/platform/with_wgl/utils.rs

+    }
+
+    match reqs.multisampling {
+        Some(0) => (),


nit: Some(0) | None => (), instead of two braces.

emilio · 2016-09-27T17:13:16Z

src/platform/with_wgl/utils.rs

+
+    // querying back the capabilities of what windows told us
+    let mut output: winapi::PIXELFORMATDESCRIPTOR = mem::zeroed();
+    if gdi32::DescribePixelFormat(hdc,


does pf_id need cleanup?

No, its just a pixel format index and doesn't require a release call

emilio · 2016-09-27T17:15:03Z

src/platform/with_wgl/wgl_attributes.rs

+// Attributes to use when creating an OpenGL context.
+#[derive(Clone, Debug)]
+pub struct WGLAttributes {
+    //


nit: Empty comment, please remove.

emilio · 2016-09-27T17:15:37Z

src/platform/with_wgl/utils.rs

+    }
+
+    if !share.is_null() {
+        if wgl::ShareLists(share as *const c_void, ctx) == 0 {


You need to clean up the context if this fails.

MortimerGoro · 2016-09-28T18:48:02Z

I have improved WGL & winapi disposal in error paths using a scoped struct. Also got rid of CurrentContextGuard code.

emilio · 2016-10-02T01:34:38Z

src/platform/with_wgl/utils.rs

+    });
+
+    // Unbind the current context so WGLScopedContext can release resources properly
+    // wgl::DeleteContex must be called with unbound contexts


nit: DeleteContext

emilio · 2016-10-02T01:37:06Z

src/platform/with_wgl/utils.rs


+    let result = create_full_context(settings, &extra, &extensions, ctx.device_ctx, shared_with);
+    if result.is_ok() {
+        ctx.dispose = false; // Everything is ok, don't dispose the scoped ctx


probably something like removing the dispose flag, and use mem::forget is better here? It's probably fine anyway.

I like the mem::forget approach, it seems more rusty

emilio · 2016-10-02T01:39:40Z

src/platform/with_wgl/native_gl_context.rs

@@ -54,10 +54,13 @@ impl Drop for NativeGLContext {
    fn drop(&mut self) {
        unsafe {
            if !self.weak {
+                self.unbind().unwrap(); //wglDeleteContext requires unbind


Does this mean that the context to be deleted needs to be unbound? Or that there must be no current context.

I guess this is the first one, but please be clearer in this regard in the comment. Also, space after //.

Improved comments

emilio · 2016-10-02T01:44:56Z

src/platform/with_wgl/native_gl_context.rs

                wgl::DeleteContext(self.render_ctx as *const _);
                let window = user32::WindowFromDC(self.device_ctx);
-                user32::ReleaseDC(window, self.device_ctx);
-                user32::DestroyWindow(window);
+                if !window.is_null() {


what has suddenly making window nullable? If it happened before and is something you've fixed, I guess that's fine.

But if it's because we can create multiple contexts associated with the same device context, and then have the window deleted, that can mean multiple things if I'm correct:

The context doesn't require the window to stay alive, in which case we can delete it earlier.

The context does require the window to stay alive, in which case this is wrong.

The context does require the window to stay alive at least until we do sharing? That doesn't make sense to me but could be it.

Can you tell me which one is the relevant one?

We are not creating multiple contexts associated with the same HDC. It was just a extra safety check because null is a possible return value of WindowFromDC. It shouldn't happen in our code, but it seems safer to check for null.

Can we instead assert that it's not null, and handle it only if under some circumstances the assertion fails?

emilio

Ok, let's do it. I don't think I can say too much right now with my limited wgl/winapi experience, and this has already stayed here for really long.

Sorry it took so long @MortimerGoro! Hopefully I wasn't too nitpicky :)

Thanks for this work!

If you look into how to test this (with AppVeyor, for example), so it doesn't regress, it'd be really nice :)

WebGL support on Windows  This is the final step to provide WebGL support on Windows ;) Some Related PRs already merged in webrender and offscreen-gl-context: servo/surfman#64 servo/webrender#432 --- - [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 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/13840)

…render_dispatcher); r=emilio  This is the final step to provide WebGL support on Windows ;) Some Related PRs already merged in webrender and offscreen-gl-context: servo/surfman#64 servo/webrender#432 --- - [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 OR - [ ] These changes do not require tests because _____ Source-Repo: https://github.com/servo/servo Source-Revision: 4216224f9cc7f3430db2b59f4d77061824ba7a1e

…render_dispatcher); r=emilio  This is the final step to provide WebGL support on Windows ;) Some Related PRs already merged in webrender and offscreen-gl-context: servo/surfman#64 servo/webrender#432 --- - [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 OR - [ ] These changes do not require tests because _____ Source-Repo: https://github.com/servo/servo Source-Revision: 4216224f9cc7f3430db2b59f4d77061824ba7a1e UltraBlame original commit: c57538159933d9eb1840437dfa9c9860220dd805

emilio requested changes Sep 19, 2016

View reviewed changes

emilio closed this Sep 19, 2016

emilio reopened this Sep 19, 2016

emilio reviewed Sep 20, 2016

View reviewed changes

MortimerGoro force-pushed the wgl branch from 3a6ef18 to f80277a Compare September 20, 2016 14:55

emilio approved these changes Sep 26, 2016

View reviewed changes

MortimerGoro added 6 commits September 27, 2016 17:36

Implement WGL

29c7281

Minor fixes and improve rust formatting

94d7eac

Add attributions to the glutin code and format functions

1cb36fe

Check for null HDC value

a949520

Use a dummy context for wglGetProcAddress when needed instead of load…

ca5fce1

…ing gl symbols after a context creation: wglGetProcAddress only works in the presence of a valid GL context, so we use a dummy ctx when the caller calls wglGetProcAddress without a valid GL context bound

Remove unneeded wglMakeCurrent & minor fixes

46fda29

MortimerGoro force-pushed the wgl branch from fac156e to 46fda29 Compare September 27, 2016 16:47

emilio requested changes Sep 27, 2016

View reviewed changes

MortimerGoro force-pushed the wgl branch from dc5868f to 35e3534 Compare September 28, 2016 21:58

emilio reviewed Oct 2, 2016

View reviewed changes

MortimerGoro force-pushed the wgl branch from 35e3534 to 044a2a0 Compare October 3, 2016 10:11

Improve disposed winapi & WGL resources in error paths

0876fc3

MortimerGoro force-pushed the wgl branch from 044a2a0 to 0876fc3 Compare October 3, 2016 14:36

emilio approved these changes Oct 3, 2016

View reviewed changes

emilio merged commit 0a4c86a into servo:master Oct 3, 2016

MortimerGoro mentioned this pull request Oct 19, 2016

WebGL support on Windows servo/servo#13840

Merged

5 tasks

		}

		unsafe fn create_hidden_window() -> Result<winapi::HWND, &'static str> {

Implement WGL #64

Implement WGL #64

Uh oh!

Conversation

MortimerGoro commented Sep 19, 2016

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MortimerGoro Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio commented Sep 19, 2016

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MortimerGoro Sep 20, 2016 •

edited

Loading

emilio Sep 26, 2016 •

edited

Loading