EGL Rework #17

Drakulix · 2017-05-18T20:29:49Z

Alright, it is late and I am not sure, if I did not miss anything important.
Docs are also largely missing.
Other then that, I think this is good for review/first feedback.

Partly addresses #12.

elinorbgr · 2017-05-19T08:12:37Z

Cargo.toml

 nix = "0.7.0"
 xkbcommon = "0.2.1"
 tempfile = "2.1.5"
 slog = { version = "2.0.0" }
 slog-stdlog = "2.0.0-0.2"
-glutin = { version = "~0.7.4", optional = true }
+libloading = "0.4.0"
+wayland-client = { version = "~0.8.6", optional = true }


Having wayland-client 0.8 and wayland-server 0.9 actually compiles? O.o

Yes when I set both to use dlopen.
And wayland-client 0.8 is sadly required, as winit is still expecting that.

elinorbgr · 2017-05-19T08:13:11Z

Cargo.toml

-default = ["backend_glutin", "backend_libinput", "renderer_glium"]
-backend_glutin = ["glutin", "wayland-server/dlopen"]
+default = ["backend_winit", "backend_libinput", "renderer_glium"]
+backend_winit = ["winit", "wayland-server/dlopen", "wayland-client", "wayland-client/dlopen"]


I don't think we need both wayland-client and wayland-client/dlopen here, do we?

See comment above.

But, don't wayland-client/dlopen imply wayland-client ?

Not sure, good point. I will test that.

elinorbgr · 2017-05-19T08:14:10Z

src/backend/glutin.rs

@@ -21,7 +21,7 @@ use std::rc::Rc;
 /// Create a new `GlutinHeadlessRenderer` which implements the `OpenglRenderer` graphics
 /// backend trait
 pub fn init_headless_renderer() -> Result<GlutinHeadlessRenderer, CreationError> {
-    init_headless_renderer_from_builder(HeadlessRendererBuilder::new(1024, 600))
+    init_headless_renderer_from_builder(HeadlessRendererBuilder::new(1024, 600)


unclosed delimiter ( 😛

Actually, is this file needed at all now?

No, I just forgot to remove it.

elinorbgr · 2017-05-19T08:15:37Z

src/backend/graphics/egl.rs

+/// `044e651edf67a2029eecc650dd42546af1501414`
+///
+/// It therefor falls under glutin's Apache 2.0 license
+/// (see https://github.com/tomaka/glutin/blob/master/LICENSE)


"therefore"

Also I guess this should be good enough. But to be safe, let's ask @tomaka his opinion on the matter.

Well, yeah, I don't see a problem with this.

elinorbgr · 2017-05-19T08:29:26Z

src/backend/graphics/egl.rs

+pub enum Native {
+    X11(ffi::NativeDisplayType, ffi::NativeWindowType),
+    Wayland(ffi::NativeDisplayType, ffi::NativeWindowType),
+    Gbm(ffi::NativeDisplayType, ffi::NativeWindowType),


Is this what will be used for the native/modesetting backed?

Yes. This was the main motivation of doing this change now, as this is essential to make anything useful out of the drm implementation I am working on.

elinorbgr · 2017-05-19T08:48:40Z

src/backend/winit.rs

+    /// input events will be received by a set `InputHandler`.
+    ///
+    /// Returns an error if the `Window` the window has been closed. Calling
+    /// `process_new_events` again after the `Window` has been closed is considered an


dispatch_new_events

elinorbgr · 2017-05-19T09:02:09Z

I don't have much to say other than this. This is quite a lot of boilerplate to plug everything together, not the easiest thing to review. I'll try to give a few more passes on it.

Also, making the travis build pass would be a good thing to do as well. 😉

Drakulix · 2017-05-19T10:06:29Z

Thanks for the first review. Typos, Docs and Travis are definitely my main priority today.

Drakulix · 2017-05-21T20:42:13Z

Alright, I should have addressed most of the things.

The example is still not working right (immediately closes on wayland and panics on x11), but its getting somewhere.

elinorbgr · 2017-05-22T09:52:05Z

The example is still not working right (immediately closes on wayland and panics on x11), but its getting somewhere.

Closes without panicking on wayland? Well, that's weird, is it some segfault or stuff like that?

elinorbgr · 2017-05-22T09:58:55Z

src/backend/winit.rs

+                                 match event {
+                                     WindowEvent::Resized(x, y) => {
+                                         window.set_inner_size(x, y);
+                                         if let Some(wl_surface) = surface.as_ref() {


To avoid confusion, this is a wl_egl_surface, not a wl_surface

elinorbgr · 2017-05-22T10:01:14Z

I'd like to have the example running without crashing before merging, as this is likely some weird bug somewhere and we'll probably regret not hunting it if we merge it now.

Drakulix · 2017-05-22T10:07:05Z

Alright, I will hunt it.
No segfault, it just closes and returns a bad error code. Maybe we should integrate dispatch_new_events into the event_loop somehow. Probably with a timer.

elinorbgr · 2017-05-22T10:30:21Z

I mean, given the source of this example, it finishing by an other way than panicking is very weird, and likely not a good sign at all...

Drakulix · 2017-05-22T10:59:43Z

Well testing on wayland is quite painful right now for me, because I need to switch my DE (see Cloudef/wlc#235), which is why I was hoping, that you might have an idea.

elinorbgr · 2017-05-22T12:30:11Z

What I do, is I run weston as an X11 client, and then start my wayland apps as weston clients.

I can try to run your code like this this evening on my computer, if you want.

Drakulix · 2017-05-22T12:44:00Z

What I do, is I run weston as an X11 client, and then start my wayland apps as weston clients.

Good idea. Will try that.

I can try to run your code like this this evening on my computer, if you want.

This will probably be a rather nasty bug, so if you are willing to help debugging (even just by testing), that would be highly appreciated.

Drakulix

Debugged it. Basically context creation is failing currently, which breaks both (X11 & Wayland), but a possible bug in wayland-client lets the wayland variant segfault.

Drakulix · 2017-05-23T08:56:54Z

src/backend/winit.rs

+                            color_bits: Some(24),
+                            alpha_bits: Some(8),
+                            ..Default::default()
+                        })?


Okay I have debugged it. This line causes a segfault, if context creation fails (why it fails is a different story, see other comment). The reason is, that the wl_egl_surface is dropped, which causes the segfault in this specific case. Any thoughts @vberger ?

if the wl_egl_surface is dropped, this destroys the EGL surface, and the pointer you retrieve line 82 becomes invalid. Could this be the reason?

The crash happens in wl_egl_window_destroy though.

Uh. So, that means the EGL surface ends up in an invalid state following the failed initialization, and then it blows up when it is destroyed, I'd guess?

Probably. I will check, what kind of cleanup should be done in this case.

Drakulix · 2017-05-23T08:58:49Z

src/backend/winit.rs

+                                       version: None,
+                                       profile: None,
+                                       debug: cfg!(debug_assertions),
+                                       robustness: Robustness::TryRobustLoseContextOnReset,


This causes context creation to fail, because apparently my EGL implementation reports, that it supports robustness, but creation later fails with BadAttribute, if it is set to any other value then NotRobust.

Okay, this is far beyond my knowledge of EGL 😅

It just sounds like a typical driver bug. I have changed it to default to NotRobust now.

elinorbgr · 2017-05-23T11:48:14Z

src/backend/graphics/egl.rs

+            };
+
+            if surface.is_null() {
+                return Err(CreationError::OsError(String::from("eglCreateWindowSurface failed")));


This check should be done in the Gbm branch of the match, otherwise providing a null pointer in input with one of the others backend may trigger it...

An other assert checking that they are not null can be added to replace this one, too.

Are you aware, that the match statement, does the same for all backend types? The rustfmt format might have tricked you there.

Oh! Right, indeed. My bad. 😅

elinorbgr · 2017-05-29T21:19:54Z

src/backend/graphics/egl.rs

+                return Err(CreationError::OsError(String::from("eglCreateWindowSurface failed")));
+            }
+            surface
+        };


Good news, I found the cause for the segfault!

The reason is, because of the reentrancy of this function(see the match on line 146, it calls itself), this par ends up called twice. Especially, egl.CreateWindowSurface() is called twice on the same native surface, and apparently this is not good.

The simplest fix is to move this whole let surface = { .... } block down in the function, right before the end, until after the EGL context has been successfully created (so line 500 of this diff). :)

Note it only solves the segfault, not the panic 😅

- Don't initialize a surface twice, if context creation fails for one version - Don't let the loaded egl library go out of scope and thus invalidating the function pointers

elinorbgr · 2017-06-02T14:05:44Z

👍

First EGL Rework draft

b950714

elinorbgr reviewed May 19, 2017

View reviewed changes

Bugfixes

5bc6418

Drakulix added 2 commits May 21, 2017 22:50

rustfmt fixes

786d719

rustfmt example

b2f7d6f

Drakulix changed the title ~~[WIP] First EGL Rework draft~~ First EGL Rework draft May 21, 2017

Drakulix changed the title ~~First EGL Rework draft~~ EGL Rework May 21, 2017

elinorbgr reviewed May 22, 2017

View reviewed changes

Drakulix commented May 23, 2017

View reviewed changes

Drakulix added 2 commits May 23, 2017 11:02

Fix broken robost implementation

94d6a0f

Rename variable to avoid confusion

dbaf4f7

elinorbgr reviewed May 23, 2017

View reviewed changes

elinorbgr reviewed May 29, 2017

View reviewed changes

Drakulix added 6 commits June 2, 2017 13:54

Update winit

115eb4d

Flesh out glium compatibility

2c9f6a7

Move glium.rs

e191c08

Fix egl initialization segfaults

f890b40

- Don't initialize a surface twice, if context creation fails for one version - Don't let the loaded egl library go out of scope and thus invalidating the function pointers

Provide a little more fleshed out and working example

ca5076a

Remove robustness for now

a762861

Fix example formatting

e28b237

elinorbgr merged commit 1925dc9 into Smithay:master Jun 2, 2017

EGL Rework #17

EGL Rework #17

Conversation

Drakulix commented May 18, 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

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

elinorbgr commented May 19, 2017

Drakulix commented May 19, 2017

Drakulix commented May 21, 2017

elinorbgr commented May 22, 2017

Choose a reason for hiding this comment

elinorbgr commented May 22, 2017

Drakulix commented May 22, 2017

elinorbgr commented May 22, 2017 • edited Loading

Drakulix commented May 22, 2017 • edited Loading

elinorbgr commented May 22, 2017

Drakulix commented May 22, 2017 • edited Loading

Drakulix 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

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

elinorbgr commented Jun 2, 2017

Drakulix commented May 18, 2017 •

edited

Loading

elinorbgr commented May 22, 2017 •

edited

Loading

Drakulix commented May 22, 2017 •

edited

Loading

Drakulix commented May 22, 2017 •

edited

Loading