GNOME Bugzilla – Bug 740346
Mir backend needs support for GdkGLContext
Last modified: 2014-11-20 03:42:55 UTC
Preliminary work is pushed in wip/mir-gdkgl branch, it mostly works [0], but at least here (using radeon), I'm getting some texture corruptions when enabling egl_swap_buffers_with_damage (and thus using eglSwapBuffersWithDamageEXT instead of eglSwapBuffers) as you can see at [2] or [3]. Not sure whether this is a mir or gdk issue yet (by the way, having damage swapping would improve performances, but it's not fundamental to have this working). [0] https://www.youtube.com/watch?v=xQyfvEvQ_G0 [1] https://git.gnome.org/browse/gtk+/log/?h=wip/mir-gdkgl [2] http://people.ubuntu.com/~3v1n0/gdkgl-mir-bad1.avi [3] http://people.ubuntu.com/~3v1n0/gdkgl-mir-bad2.avi
Created attachment 291047 [details] [review] mir: add support for gdkgl Proposed patch
Created attachment 291049 [details] [review] mir: add support for gdkgl Fixed the patch, removing unneeded commits
Review of attachment 291049 [details] [review]: Generally looks good, thanks Marco! Please update the commit message to refer to this bug and perhaps add some more information. ::: gdk/mir/gdkmir.h @@ +36,2 @@ GDK_AVAILABLE_IN_3_10 +MirConnection *gdk_mir_display_get_mir_connection (GdkDisplay *display); This change seems unrelated - is it just a typo that needs fixing? ::: gdk/mir/gdkmirdisplay.c @@ +689,3 @@ display_class->text_property_to_utf8_list = gdk_mir_display_text_property_to_utf8_list; display_class->utf8_to_string_target = gdk_mir_display_utf8_to_string_target; + display_class->make_gl_context_current = _gdk_mir_display_make_gl_context_current; _gdk_mir_display_make_gl_context_current should be in this file ::: gdk/mir/gdkmirwindowimpl.c @@ +1398,3 @@ impl_class->set_opaque_region = gdk_mir_window_impl_set_opaque_region; impl_class->set_shadow_width = gdk_mir_window_impl_set_shadow_width; + impl_class->create_gl_context = _gdk_mir_window_create_gl_context; This should be _gdk_mir_window_impl_create_gl_context otherwise it is confusing to find. @@ +1399,3 @@ impl_class->set_shadow_width = gdk_mir_window_impl_set_shadow_width; + impl_class->create_gl_context = _gdk_mir_window_create_gl_context; + impl_class->invalidate_for_new_frame = _gdk_mir_window_invalidate_for_new_frame; This should be _gdk_mir_window_impl_invalidate_for_new_frame otherwise it is confusing to find.
Review of attachment 291049 [details] [review]: As explained on IRC, about moving the display or window functions that have something to do with GL context in the proper file, I just I followed what gdk was already doing with other backends (and so keeping all the GL related stuff together), but if we'd prefer to expose the GdkMirGLCOntext struct in a private header, I'm happy with that. ::: gdk/mir/gdkmir.h @@ +36,2 @@ GDK_AVAILABLE_IN_3_10 +MirConnection *gdk_mir_display_get_mir_connection (GdkDisplay *display); Yes, it's not strictly needed here, I can move this out to another commit...
Created attachment 291050 [details] [review] mir: add OpenGL support Updated according to your comments
Review of attachment 291050 [details] [review]: Looks good: - Add https://bugzilla.gnome.org/show_bug.cgi?id=740346 into the bug comment as per other commits. Should also drop the [PATCH 1/2]. Otherwise looks good to go to me. Thanks! ::: gdk/mir/gdkmirwindowimpl.c @@ +1608,3 @@ impl_class->set_opaque_region = gdk_mir_window_impl_set_opaque_region; impl_class->set_shadow_width = gdk_mir_window_impl_set_shadow_width; + impl_class->create_gl_context = gdk_mir_window_create_gl_context; Rename gdk_mir_window_create_gl_context -> gdk_mir_window_impl_create_gl_context @@ +1609,3 @@ impl_class->set_shadow_width = gdk_mir_window_impl_set_shadow_width; + impl_class->create_gl_context = gdk_mir_window_create_gl_context; + impl_class->invalidate_for_new_frame = gdk_mir_window_invalidate_for_new_frame; Rename gdk_mir_window_invalidate_for_new_frame -> gdk_mir_window_impl_invalidate_for_new_frame
Created attachment 291052 [details] [review] mir: add OpenGL support Oooops... I missed the impl! ;)
Review of attachment 291052 [details] [review]: All good!