Merge branch 'wip/damaged-but-no-frost-bite' into 'master'

x11: defer _NET_WM_FRAME_DRAWN update until frame usable by compositor

See merge request GNOME/gtk!1982

config.h.meson

@@ -123,6 +123,9 @@
 /* Have the Xcursor library */
 #mesondefine HAVE_XCURSOR
 
+/* Have the XDAMAGE X extension */
+#mesondefine HAVE_XDAMAGE
+
 /* Have the XFIXES X extension */
 #mesondefine HAVE_XFIXES
 

gdk/x11/gdkdisplay-x11.c

@@ -1603,6 +1603,14 @@ gdk_x11_display_open (const gchar *display_name)
   }
 #endif
 
+#ifdef HAVE_XDAMAGE
+  display_x11->have_damage = FALSE;
+  if (XDamageQueryExtension (display_x11->xdisplay,
+                             &display_x11->damage_event_base,
+                             &display_x11->damage_error_base))
+    display_x11->have_damage = TRUE;
+#endif
+
   display->clipboard = gdk_x11_clipboard_new (display, "CLIPBOARD");
   display->primary_clipboard = gdk_x11_clipboard_new (display, "PRIMARY");
 

gdk/x11/gdkdisplay-x11.h

@@ -149,6 +149,13 @@ struct _GdkX11Display
   guint has_glx_multisample : 1;
   guint has_glx_visual_rating : 1;
   guint has_glx_create_es2_context : 1;
+  guint has_async_glx_swap_buffers : 1;
+
+#ifdef HAVE_XDAMAGE
+  gint damage_event_base;
+  gint damage_error_base;
+  guint have_damage;
+#endif
 };
 
 struct _GdkX11DisplayClass

gdk/x11/gdkglcontext-x11.c

@@ -183,6 +183,22 @@ gdk_x11_gl_context_end_frame (GdkDrawContext *draw_context,
 
   gdk_x11_surface_pre_damage (surface);
 
+#ifdef HAVE_XDAMAGE
+  if (context_x11->xdamage != 0)
+    {
+      g_assert (context_x11->frame_fence == 0);
+
+      context_x11->frame_fence = glFenceSync (GL_SYNC_GPU_COMMANDS_COMPLETE, 0);
+
+      /* We consider the frame still getting painted until the GL operation is
+       * finished, and the window gets damage reported from the X server.
+       * It's only at this point the compositor can be sure it has full
+       * access to the new updates.
+       */
+      _gdk_x11_surface_set_frame_still_painting (surface, TRUE);
+    }
+#endif
+
   glXSwapBuffers (dpy, drawable);
 
   if (context_x11->do_frame_sync && info != NULL && display_x11->has_glx_video_sync)
@@ -565,6 +581,81 @@ create_legacy_context (GdkDisplay   *display,
   return res;
 }
 
+#ifdef HAVE_XDAMAGE
+static gboolean
+on_gl_surface_xevent (GdkGLContext   *context,
+                      XEvent         *xevent,
+                      GdkX11Display  *display_x11)
+{
+  GdkX11GLContext *context_x11 = GDK_X11_GL_CONTEXT (context);
+  GdkSurface *surface = gdk_gl_context_get_surface (context);
+  XDamageNotifyEvent *damage_xevent;
+
+  if (!context_x11->is_attached)
+    return FALSE;
+
+  if (xevent->type != (display_x11->damage_event_base + XDamageNotify))
+    return FALSE;
+
+  damage_xevent = (XDamageNotifyEvent *) xevent;
+
+  if (damage_xevent->damage != context_x11->xdamage)
+    return FALSE;
+
+  if (context_x11->frame_fence)
+    {
+      GLenum wait_result;
+
+      wait_result = glClientWaitSync (context_x11->frame_fence, 0, 0);
+
+      switch (wait_result)
+        {
+          /* We assume that if the fence has been signaled, that this damage
+           * event is the damage event that was triggered by the GL drawing
+           * associated with the fence. That's, technically, not necessarly
+           * always true. The X server could have generated damage for
+           * an unrelated event (say the size of the window changing), at
+           * just the right moment such that we're picking it up instead.
+           *
+           * We're choosing not to handle this edge case, but if it does ever
+           * happen in the wild, it could lead to slight underdrawing by
+           * the compositor for one frame. In the future, if we find out
+           * this edge case is noticeable, we can compensate by copying the
+           * painted region from gdk_x11_gl_context_end_frame and subtracting
+           * damaged areas from the copy as they come in. Once the copied
+           * region goes empty, we know that there won't be any underdraw,
+           * and can mark painting has finished. It's not worth the added
+           * complexity and resource usage to do this bookkeeping, however,
+           * unless the problem is practically visible.
+           */
+          case GL_ALREADY_SIGNALED:
+          case GL_CONDITION_SATISFIED:
+          case GL_WAIT_FAILED:
+            if (wait_result == GL_WAIT_FAILED)
+              g_warning ("failed to wait on GL fence associated with last swap buffers call");
+            glDeleteSync (context_x11->frame_fence);
+            context_x11->frame_fence = 0;
+            _gdk_x11_surface_set_frame_still_painting (surface, FALSE);
+            break;
+
+          /* We assume that if the fence hasn't been signaled, that this
+           * damage event is not the damage event that was triggered by the
+           * GL drawing associated with the fence. That's only true for
+           * the Nvidia vendor driver. When using open source drivers, damage
+           * is emitted immediately on swap buffers, before the fence ever
+           * has a chance to signal.
+           */
+          case GL_TIMEOUT_EXPIRED:
+            break;
+          default:
+            g_assert_not_reached ();
+        }
+    }
+
+  return FALSE;
+}
+#endif
+
 static gboolean
 gdk_x11_gl_context_realize (GdkGLContext  *context,
                             GError       **error)
@@ -737,6 +828,24 @@ gdk_x11_gl_context_realize (GdkGLContext  *context,
   context_x11->attached_drawable = info->glx_drawable ? info->glx_drawable : gdk_x11_surface_get_xid (surface);
   context_x11->unattached_drawable = info->dummy_glx ? info->dummy_glx : info->dummy_xwin;
 
+#ifdef HAVE_XDAMAGE
+  if (display_x11->have_damage && display_x11->has_async_glx_swap_buffers)
+    {
+      gdk_x11_display_error_trap_push (display);
+      context_x11->xdamage = XDamageCreate (dpy,
+                                            gdk_x11_surface_get_xid (surface),
+                                            XDamageReportRawRectangles);
+      if (gdk_x11_display_error_trap_pop (display))
+        context_x11->xdamage = 0;
+      else
+        g_signal_connect_object (G_OBJECT (display),
+                                 "xevent",
+                                 G_CALLBACK (on_gl_surface_xevent),
+                                 context,
+                                 G_CONNECT_SWAPPED);
+    }
+#endif
+
   context_x11->is_direct = glXIsDirect (dpy, context_x11->glx_context);
 
   GDK_DISPLAY_NOTE (display, OPENGL,
@@ -766,6 +875,10 @@ gdk_x11_gl_context_dispose (GObject *gobject)
       context_x11->glx_context = NULL;
     }
 
+#ifdef HAVE_XDAMAGE
+  context_x11->xdamage = 0;
+#endif
+
   G_OBJECT_CLASS (gdk_x11_gl_context_parent_class)->dispose (gobject);
 }
 
@@ -841,6 +954,21 @@ gdk_x11_screen_init_gl (GdkX11Screen *screen)
   display_x11->has_glx_visual_rating =
     epoxy_has_glx_extension (dpy, screen_num, "GLX_EXT_visual_rating");
 
+  if (g_strcmp0 (glXGetClientString (dpy, GLX_VENDOR), "NVIDIA Corporation") == 0)
+    {
+      /* With the mesa based drivers, we can safely assume the compositor can
+       * access the updated surface texture immediately after glXSwapBuffers is
+       * run, because the kernel ensures there is an implicit synchronization
+       * operation upon texture access. This is not true with the Nvidia vendor
+       * driver. There is a window of time after glXSwapBuffers before other
+       * processes can see the updated drawing. We need to take special care,
+       * in that case, to defer telling the compositor our latest frame is
+       * ready until after the GPU has completed all issued commands related
+       * to the frame, and that the X server says the frame has been drawn.
+       */
+      display_x11->has_async_glx_swap_buffers = TRUE;
+    }
+
   GDK_DISPLAY_NOTE (display, OPENGL,
             g_message ("GLX version %d.%d found\n"
                        " - Vendor: %s\n"

gdk/x11/gdkglcontext-x11.h

@@ -24,6 +24,10 @@
 #include <X11/X.h>
 #include <X11/Xlib.h>
 
+#ifdef HAVE_XDAMAGE
+#include <X11/extensions/Xdamage.h>
+#endif
+
 #include <epoxy/gl.h>
 #include <epoxy/glx.h>
 
@@ -44,6 +48,11 @@ struct _GdkX11GLContext
   GLXDrawable attached_drawable;
   GLXDrawable unattached_drawable;
 
+#ifdef HAVE_XDAMAGE
+  GLsync frame_fence;
+  Damage xdamage;
+#endif
+
   guint is_attached : 1;
   guint is_direct : 1;
   guint do_frame_sync : 1;

gdk/x11/gdkprivate-x11.h

@@ -104,6 +104,11 @@ void _gdk_x11_surface_grab_check_unmap   (GdkSurface *window,
                                           gulong     serial);
 void _gdk_x11_surface_grab_check_destroy (GdkSurface *window);
 
+#ifdef HAVE_XDAMAGE
+void _gdk_x11_surface_set_frame_still_painting (GdkSurface *surface,
+                                                gboolean    painting);
+#endif
+
 gboolean _gdk_x11_display_is_root_window (GdkDisplay *display,
                                           Window      xroot_window);
 

gdk/x11/gdksurface-x11.c

@@ -360,6 +360,77 @@ gdk_x11_surface_begin_frame (GdkSurface *surface,
     }
 }
 
+static gboolean
+should_sync_frame_drawing (GdkSurface *surface)
+{
+  GdkX11Surface *impl = GDK_X11_SURFACE (surface);
+
+  /* disabled client side */
+  if (!impl->frame_sync_enabled)
+    return FALSE;
+
+  /* disabled compositor side */
+  if (!gdk_x11_screen_supports_net_wm_hint (GDK_SURFACE_SCREEN (surface),
+                                            g_intern_static_string ("_NET_WM_FRAME_DRAWN")))
+    return FALSE;
+
+  return TRUE;
+}
+
+static void
+sync_counter_for_end_frame (GdkSurface *surface)
+{
+  GdkX11Surface *impl = GDK_X11_SURFACE (surface);
+
+  g_assert (!impl->toplevel->in_frame);
+  g_assert (impl->toplevel->extended_update_counter != None);
+  g_assert ((impl->toplevel->current_counter_value % 2) == 0);
+
+  set_sync_counter (GDK_SURFACE_XDISPLAY (surface),
+                    impl->toplevel->extended_update_counter,
+                    impl->toplevel->current_counter_value);
+}
+
+static void
+maybe_sync_counter_for_end_frame (GdkSurface *surface)
+{
+  GdkX11Surface *impl = GDK_X11_SURFACE (surface);
+  gboolean frame_sync_negotiated = should_sync_frame_drawing (surface);
+  gboolean frame_done_painting = !impl->toplevel->frame_pending;
+
+#ifdef HAVE_XDAMAGE
+  frame_done_painting = !impl->toplevel->frame_still_painting && frame_sync_negotiated;
+#endif
+
+  if (!impl->toplevel->frame_pending)
+    {
+      if (!frame_sync_negotiated || frame_done_painting)
+        sync_counter_for_end_frame (surface);
+    }
+  else
+    {
+      if (frame_done_painting)
+        sync_counter_for_end_frame (surface);
+    }
+}
+
+#ifdef HAVE_XDAMAGE
+void
+_gdk_x11_surface_set_frame_still_painting (GdkSurface *surface,
+                                           gboolean    painting)
+{
+  GdkX11Surface *impl = GDK_X11_SURFACE (surface);
+
+  if (impl->toplevel->frame_still_painting == painting)
+    return;
+
+  impl->toplevel->frame_still_painting = painting;
+
+  if (!impl->toplevel->frame_still_painting)
+    maybe_sync_counter_for_end_frame (surface);
+}
+#endif
+
 static void
 gdk_x11_surface_end_frame (GdkSurface *surface)
 {
@@ -405,13 +476,9 @@ gdk_x11_surface_end_frame (GdkSurface *surface)
       else
         impl->toplevel->current_counter_value += 1;
 
-      set_sync_counter(GDK_SURFACE_XDISPLAY (surface),
-		       impl->toplevel->extended_update_counter,
-		       impl->toplevel->current_counter_value);
+      maybe_sync_counter_for_end_frame (surface);
 
-      if (impl->frame_sync_enabled &&
-          gdk_x11_screen_supports_net_wm_hint (GDK_SURFACE_SCREEN (surface),
-					       g_intern_static_string ("_NET_WM_FRAME_DRAWN")))
+      if (should_sync_frame_drawing (surface))
         {
           impl->toplevel->frame_pending = TRUE;
           gdk_surface_freeze_updates (surface);

gdk/x11/gdksurface-x11.h

@@ -124,6 +124,9 @@ struct _GdkToplevelX11
 
   guint in_frame : 1;
 
+  /* If we're waiting for damage from the X server after painting a frame */
+  guint frame_still_painting : 1;
+
   /* If we're expecting a response from the compositor after painting a frame */
   guint frame_pending : 1;
 

gdk/x11/meson.build

@@ -65,6 +65,7 @@ gdk_x11_deps = [
   xext_dep,
   x11_dep,
   xcursor_dep,
+  xdamage_dep,
   xfixes_dep,
   xcomposite_dep,
   xrandr_dep,

meson.build

@@ -501,6 +501,7 @@ if x11_enabled
   xi_dep         = dependency('xi')
   xext_dep       = dependency('xext')
   xcursor_dep    = dependency('xcursor', required: false)
+  xdamage_dep    = dependency('xdamage', required: false)
   xfixes_dep     = dependency('xfixes', required: false)
   xcomposite_dep = dependency('xcomposite', required: false)
   fontconfig_dep = dependency('fontconfig')
@@ -513,6 +514,9 @@ if x11_enabled
   if xcursor_dep.found()
     x11_pkgs += ['xcursor']
   endif
+  if xdamage_dep.found()
+    x11_pkgs += ['xdamage']
+  endif
   if xfixes_dep.found()
     x11_pkgs += ['xfixes']
   endif
@@ -523,6 +527,7 @@ if x11_enabled
   atk_pkgs += ['atk-bridge-2.0']
 
   cdata.set('HAVE_XCURSOR', xcursor_dep.found())
+  cdata.set('HAVE_XDAMAGE', xdamage_dep.found())
   cdata.set('HAVE_XCOMPOSITE', xcomposite_dep.found())
   cdata.set('HAVE_XFIXES', xfixes_dep.found())