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:51:19Z

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:51:19Z

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:51:19Z

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:51:19Z

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:51: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:51:19Z

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:51:19Z

src/platform/with_wgl/native_gl_context.rs

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


nit: extra newline

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

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:51:19Z

src/platform/with_wgl/native_gl_context.rs

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


nit: rust style is } else {.

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

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:51:35Z

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:51:35Z

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:51:35Z

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:51:35Z

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-26T10:02:33Z

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-26T10:02:33Z

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-26T10:02:33Z

src/platform/with_wgl/utils.rs

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


nit: Remove this extra line.


        Implement WGL


        Minor fixes and improve rust formatting


        Add attributions to the glutin code and format functions


        Check for null HDC value


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

…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

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:17:51Z

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:17:51Z

src/platform/with_wgl/native_gl_context.rs

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


nit: Here too

emilio · 2016-09-27T17:17:51Z

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:17:51Z

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:17:51Z

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:17:51Z

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:17:51Z

src/platform/with_wgl/utils.rs

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


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

emilio · 2016-09-27T17:17:51Z

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:17:51Z

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:17:51Z

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:45:15Z

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:45:15Z

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:45:15Z

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:45:15Z

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?


        Improve disposed winapi & WGL resources in error paths

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 :)


        Auto merge of #13840 - MortimerGoro:webrender_dispatcher, r=emilio

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)


        servo: Merge #13840 - WebGL support on Windows (from MortimerGoro:web…

…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


        servo: Merge #13840 - WebGL support on Windows (from MortimerGoro:web…

…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


        servo: Merge #13840 - WebGL support on Windows (from MortimerGoro:web…

…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


        servo: Merge #13840 - WebGL support on Windows (from MortimerGoro:web…

…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 changes

emilio closed this Sep 19, 2016

emilio reopened this Sep 19, 2016

emilio reviewed Sep 20, 2016

View changes

MortimerGoro force-pushed the MortimerGoro:wgl branch from 3a6ef18 to f80277a Sep 20, 2016

emilio approved these changes Sep 26, 2016

View changes

MortimerGoro added 6 commits Sep 19, 2016

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

Loading status checks…

46fda29

MortimerGoro force-pushed the MortimerGoro:wgl branch from fac156e to 46fda29 Sep 27, 2016

emilio requested changes Sep 27, 2016

View changes

MortimerGoro force-pushed the MortimerGoro:wgl branch from dc5868f to 35e3534 Sep 28, 2016

emilio reviewed Oct 2, 2016

View changes

MortimerGoro force-pushed the MortimerGoro:wgl branch from 35e3534 to 044a2a0 Oct 3, 2016

Improve disposed winapi & WGL resources in error paths

Loading status checks…

0876fc3

MortimerGoro force-pushed the MortimerGoro:wgl branch from 044a2a0 to 0876fc3 Oct 3, 2016

emilio approved these changes Oct 3, 2016

View changes

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

1 check failed

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

MortimerGoro mentioned this pull request Oct 19, 2016

WebGL support on Windows servo/servo#13840

Merged

3 of 5 tasks complete

		}

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

servo / surfman

Join GitHub today

Implement WGL #64

Implement WGL #64

Conversation

MortimerGoro commented Sep 19, 2016

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MortimerGoro Sep 20, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Sep 19, 2016

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio Sep 26, 2016 • edited

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MortimerGoro commented Sep 28, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio left a comment

MortimerGoro Sep 20, 2016 •

edited

emilio Sep 26, 2016 •

edited