Created attachment 290780 [details] [review]
Work in progress patch

Work in progress: implement GdkGLContext for Quartz backend

Current problems:
* size of rendering is doubled on Retina display
* something wrong with paint updates; gdkgears demo only updates every couple of seconds but reports ~30fps
* window doesn't resize properly; lots of dirty textures
* had to disable glInvalidateFramebuffer() call because doesn't exist on Mac OS X legacy OpenGL mode?

Created attachment 290781 [details] [review]
Additional patch to call the GdkGLContext's update method on window resize

This gets the window resizing more or less correctly.

Created attachment 290808 [details] [review]
Work in progress patch (updated)

Updated patch partially fixes Retina display.

Other widgets render at low resolution, which may be due to upstream cairo issues <https://www.libreoffice.org/bugzilla/show_bug.cgi?id=69796> ?

Also fixed releasing of Obj-C objects as ARC is not in use here, so have to do it manually.

Created attachment 290819 [details] [review]
Additional patch to fix GL scale on Retina (but... breaks non-GL views)

This patch fixes gdk_quartz_ref_cairo_surface () to respect the window scale factor, which fixes the resolution of widgets in GL-rendered windows on Retina display... BUT non-GL-rendered windows on Retina display show an extra doubling that shouldn't be there.

Per ebassi on IRC this is probably correct behavior of ref_cairo_surface () (matches X11) and the source of the further-doubling bug should be tracked down and fixed...

I _think_ I've fixed this with a patch to cairo, posted on https://www.libreoffice.org/bugzilla/show_bug.cgi?id=69796

Quartz CGContexts have an implicit device scaling factor applied by default, which seems to do bad things when we combine it with an explicit device scaling factor applied at the cairo level from the GDK level.

Patch there applies an inverse scaling transform on the CGContext so when cairo draws, it's drawing just with the cairo device scale factor.

These two together _seem_ to work, but I'm not really sure if it's right. :D

Commit cf94da2ca1 removed the 'update' method; Alex what do you recommend I use instead to update the surface state on window resize? (I'll also need to call that when switching between screens of different resolutions, I think.)

Brion: I don't understand exactly why you need the backend-independent code to call into the backend during a resize. The backend is in full control of all native window resizing. (As in, it either happens from the backend in GdkWindowImpl->move_resize, or it happens externally from the app and the app gets told about it via events similar to ConfigureNotify on X11.)

There are two types of GdkGLContexts. 

One type (attached==true) is the internal context used by gdk to paint. This is inherently tied to a specific native window and you can easily read it off GdkWindow->gl_paint_context when you resize the window, and call update on it as needed.

The other type (attached==false) are the ones we pass of to users. These are created as shared with a paint context, but are not directly tied to a specific front/back buffer. You can only use these with fb objects that are then drawn with the paint context. These never need to be "updated" in any way.

Review of attachment 290808 [details] [review]:

I don't have an OSX machine and never uses GL on it, but here are some comments :)

::: gdk/quartz/gdkglcontext-quartz.c
@@ +61,3 @@
+  /* If nothing else is known, repaint everything so that the back
+	 buffer is fully up-to-date for the swapbuffer */
+  cairo_region_union_rectangle (update_area, &window_rect);

You really should try to find some approach similar to the buffer age extension so that you know what parts of the old frames are still valid. Otherwise this will keep repainting the entire window even if only a small spinner is changing.

@@ +108,3 @@
+  NSOpenGLPixelFormatAttribute attrs[] =
+    {
+      kCGLPFAOpenGLProfile, NSOpenGLProfileVersionLegacy,

Please do support profile GDK_GL_PROFILE_3_2_CORE==NSOpenGLProfileVersion3_2Core.
Gtk internally uses this and it is much more efficient.

@@ +143,3 @@
+	    [view setWantsBestResolutionOpenGLSurface:YES];
+
+      [ctx setView:view];

I believe you want to set NSOpenGLCPSwapInterval to 1 here, to make frame switching synced to the framerate.

::: gdk/quartz/gdkquartzglcontext.h
@@ +44,3 @@
+gboolean        gdk_quartz_display_get_glx_version (GdkDisplay *display,
+                                                    gint       *major,
+                                                    gint       *minor);

glx version? I think you should drop this one.

::: gtk/gtkglarea.c
@@ +508,3 @@
     invalidate[i++] = GL_COLOR_ATTACHMENT0;
 
+  //glInvalidateFramebuffer (GL_FRAMEBUFFER, i, invalidate);

This needs to be protected with a check for the GL_ARB_invalidate_subdata extension

Review of attachment 290808 [details] [review]:

::: gdk/quartz/gdkglcontext-quartz.c
@@ +61,3 @@
+  /* If nothing else is known, repaint everything so that the back
+	 buffer is fully up-to-date for the swapbuffer */
+  cairo_region_union_rectangle (update_area, &window_rect);

Maybe you can read back the NSOpenGLPFABackingStore value?

I've rescued these patches from bitrotting to the point where they compile again on gtk-3-16, but beyond that I don't really have any idea what I'm doing; perhaps someone could point me in the right direction?

In any case, here are three patches:

- One with a minimal non-implementation of GL for Quartz that simply returns an error if you try to use it. It would be nice if this could be committed to gtk-3-16 before this bug is solved, since the current behaviour is to crash.

- One update of Brion's patch with some of the comments addressed. I must be doing something wrong though, because all non-GL parts of the window are just black. I didn't yet address Alex's comment about improving the repainting and the buffer age extension.

- One patch to call update during window resize as per Alex's comment above (#7).

Created attachment 301257 [details] [review]
quartz: Non-implementation of GdkGLContext

This makes sure the Quartz backend returns an error when asked to create
a GL context, rather than crashing because of a NULL vfunc.

Created attachment 301258 [details] [review]
Work in progress: implement GdkGLContext for Quartz backend

Current problems:
 * other widgets in a GL-painted window are low-resolution on Retina display
 * something wrong with paint updates; gdkgears demo only updates every couple
   of seconds but reports ~30fps

Created attachment 301259 [details] [review]
quartz: Update GL context on window resize

The previous version of this patch sent an update message to the
NSOpenGLContext in a GdkGLContext::update vfunc, but that vfunc does not
exist any more.

Review of attachment 301257 [details] [review]:

sure. Makes sense until we have a working implementation.

Comment on attachment 301257 [details] [review]
quartz: Non-implementation of GdkGLContext

Attachment 301257 [details] pushed as 0c4deb6 - quartz: Non-implementation of GdkGLContext

I've tried these patches on my MBP but the gdk_gl_context_check_extensions() function finds none of the required GL extensions:

(gtk-play:24581): Gdk-WARNING **: GL implementation doesn't support any form of non-power-of-two textures
Gdk-Message: OpenGL version: 4.1 (core)
* GLSL version: 4.10
* Extensions checked:
 - GL_ARB_texture_non_power_of_two: no
 - GL_ARB_texture_rectangle: no
 - GL_EXT_framebuffer_blit: no
 - GL_GREMEDY_frame_terminator: no
* Using texture rectangle: no

I'm no GL expert at all but would it be possible to use different extensions in macOS? I'll attach log of my OpenGL setup.

Created attachment 355867 [details]
OpenGL extensions report

Forgot to mention I use the 3-22 branch. I haven't checked with master yet.

(In reply to Philippe Normand from comment #16)
> I've tried these patches on my MBP but the gdk_gl_context_check_extensions()
> function finds none of the required GL extensions:
> 
> (gtk-play:24581): Gdk-WARNING **: GL implementation doesn't support any form
> of non-power-of-two textures
> Gdk-Message: OpenGL version: 4.1 (core)
> * GLSL version: 4.10
> * Extensions checked:
>  - GL_ARB_texture_non_power_of_two: no
>  - GL_ARB_texture_rectangle: no
>  - GL_EXT_framebuffer_blit: no

These extensions are part of the GL core API, so they should definitely be available.

>  - GL_GREMEDY_frame_terminator: no

This one is entirely optional, and does not matter much, except for debugging.

> * Using texture rectangle: no
> 
> I'm no GL expert at all but would it be possible to use different extensions
> in macOS? I'll attach log of my OpenGL setup.

That's not right.

A GL profile with version 4.1 (core) should have all those extensions available as part of the core API.

This means that something else is not right when the extensions are queried. It's possible, for instance, that the GL context is not actually bound to something drawable, and thus feature discovery outside of the GL context version is failing.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/517.