fix(gl): repaint thread vs AppKit cursor hide race (#1947)

docs/adr/ADR-046-depth-aware-cursor.md

@@ -659,15 +659,17 @@ sprite and the depth, and the runtime owns the drawing. Apps get the same contra
   on the same app geometry), vk_native −22.0 px in its 1280 × 1440 atlas; nothing drawn at
   0.1,0.1; the arrow's fill reads (255,255,255) and its outline (5,5,8), the custom ring
   (0,220,255) — exactly the authored bytes, so no encode is lost or doubled.
-- **Known issue (GL on macOS, not caused by the draw):** `cube_handle_gl_macos` crashes in
-  roughly 1 of 5 launches once the depth cursor is ACTIVE — in Apple's GL driver, on the #868
-  repaint thread (`gl_repaint_thread` → `glBlitFramebuffer`) or in the app's own draw. It
-  follows the app's `[NSCursor hide]`, not the runtime draw: app-draw mode crashes too; the
-  runtime draw with the OS cursor left visible (0/10), `DXR_WEAVE_REPAINT=0` (0/8) and the
-  extension off (0/20) do not. The GL cube never hid the OS cursor before this adoption, so this
-  is a latent repaint-thread / AppKit race in the GL compositor surfacing — runtime#1947. Until it
-  is fixed the GL cube's depth cursor is **opt-in** (`DISPLAYXR_CURSOR_DEPTH=1`); the Metal and
-  Vulkan cubes stay default-on.
+- **GL on macOS: a crash found here was the repaint thread, not the cursor (fixed).** While
+  this landed, `cube_handle_gl_macos` crashed in roughly 1 of 5 launches once the depth cursor was
+  active, on the #868 repaint thread (`gl_repaint_thread` → `glBlitFramebuffer`) or in the app's
+  own draw. The cause was unrelated to cursors: an app that binds an `NSOpenGLView` hands the
+  runtime that view, and the GL compositor adopted the view's `openGLContext`, which is the
+  context the app renders with. The repaint thread then replayed on it while the app thread drew,
+  so one GL context was in use on two threads. Hiding the OS cursor only changed frame timing
+  enough to open the repaint gate. With `DXR_WEAVE_REPAINT_FORCE=1` the old build crashes 10/10
+  launches even with the extension off. The fix: the GL compositor never starts the repaint
+  thread on an app-owned context (it stays on for the private contexts of the own-window, plain-`NSView` and offscreen
+  paths). The GL cube's depth cursor is default-on again, like the Metal and Vulkan cubes.
 - **Found in passing (Metal, #1941):** on the Vulkan-app-on-Metal fallback a custom sprite is
   handed over as the client swapchain and cast to the native Metal type — runtime#1948.
 

src/xrt/compositor/gl/comp_gl_compositor.cpp

@@ -5387,9 +5387,15 @@ gl_repaint_thread(void *ptr)
 		// Release unconditionally — see the note above about wedging the app.
 		wglMakeCurrent(NULL, NULL);
 #elif defined(__APPLE__)
+		// Only ever a compositor-PRIVATE context here: an app-owned one keeps
+		// this thread from starting at all (#1947, see the create path). The
+		// CGL lock is the one AppKit takes around -[NSOpenGLContext update]
+		// when the view's surface changes on the main thread.
+		comp_gl_window_macos_lock(c->macos_window);
 		comp_gl_window_macos_make_current(c->macos_window);
 		rp_start_ns = os_monotonic_get_ns();
 		gl_window_present(c, c->repaint.atlas_tex, c->repaint.last_dt, /*is_repaint=*/true);
+		comp_gl_window_macos_unlock(c->macos_window);
 #endif
 
 		c->repaint.count++;
@@ -8833,6 +8839,30 @@ comp_gl_compositor_create(struct xrt_device *xdev,
 			U_LOG_W("#868: DXR_WEAVE_REPAINT_FORCE=1 — repainting every refresh regardless "
 			        "of app rate. Correctness probe; it WILL cost frame rate.");
 		}
+#ifdef __APPLE__
+		/*
+		 * #1947: never repaint on the APP's own GL context.
+		 *
+		 * A handle app that binds an NSOpenGLView hands us that view, and
+		 * comp_gl_window_macos_setup_external adopts the view's openGLContext
+		 * — the same CGL context the app renders with. The app thread uses it
+		 * freely between its xr* calls (RenderScene runs between xrBeginFrame
+		 * and xrEndFrame, outside c->mutex and outside app_frame_in_progress,
+		 * which only spans xrEndFrame), so a repaint replaying on it from this
+		 * thread drives one GLDContextRec from two threads at once. Measured:
+		 * SIGSEGV in GLRResourceList::addResource under the repaint's
+		 * glBlitFramebuffer while the main thread was in RenderScene's
+		 * flushContext, ~1 launch in 5. No lock the runtime takes can cover
+		 * the app's own GL calls, so the replay is off for this configuration;
+		 * the app's frames weave and present exactly as before. Private
+		 * contexts (own window, plain NSView, offscreen texture) keep it.
+		 */
+		if (c->repaint.enabled == 1 && comp_gl_window_macos_context_is_app_owned(c->macos_window)) {
+			c->repaint.enabled = 0;
+			U_LOG_W("#868/#1947: weave repaint OFF — the compositor presents on the app's own "
+			        "NSOpenGLView context, which only the app thread may use");
+		}
+#endif
 		if (c->repaint.enabled == 1) {
 			os_thread_helper_start(&c->repaint_thread, gl_repaint_thread, c);
 		}

src/xrt/compositor/gl/comp_gl_window_macos.h

@@ -88,6 +88,35 @@ comp_gl_window_macos_create_offscreen(void *app_cgl_ctx,
 void *
 comp_gl_window_macos_get_cgl_context(struct comp_gl_window_macos *win);
 
+/*!
+ * #1947: is the compositor's GL context the APP's own context?
+ *
+ * True when the app bound an NSOpenGLView and the compositor adopted that
+ * view's openGLContext instead of creating a private (share-group) context.
+ * The app keeps rendering with that context on its own thread whenever it
+ * likes, without any lock the runtime could take, so the runtime must not
+ * issue GL on it from any other thread (the #868 repaint thread).
+ *
+ * @param win The window handle.
+ */
+bool
+comp_gl_window_macos_context_is_app_owned(struct comp_gl_window_macos *win);
+
+/*!
+ * CGLLockContext / CGLUnlockContext on the compositor's context — the lock
+ * AppKit itself takes around -[NSOpenGLContext update] when the view's
+ * surface changes on the main thread. Taken by off-app-thread GL work on a
+ * view-attached context so it cannot interleave with that update.
+ *
+ * @param win The window handle.
+ */
+void
+comp_gl_window_macos_lock(struct comp_gl_window_macos *win);
+
+//! @copydoc comp_gl_window_macos_lock
+void
+comp_gl_window_macos_unlock(struct comp_gl_window_macos *win);
+
 /*!
  * Make the compositor's GL context current.
  *

src/xrt/compositor/gl/comp_gl_window_macos.m

@@ -45,6 +45,15 @@
 
 	//! True if we created the window ourselves.
 	bool owns_window;
+
+	/*!
+	 * #1947: true when gl_context is NOT ours but the app's own: the external
+	 * view is an NSOpenGLView and we adopted its openGLContext — the context
+	 * the app renders with. The runtime may then issue GL on it only from the
+	 * app's thread, inside the app's own xr* calls. See
+	 * comp_gl_window_macos_context_is_app_owned().
+	 */
+	bool context_is_app_owned;
 };
 
 static void
@@ -234,6 +243,10 @@
 		if ([view isKindOfClass:[NSOpenGLView class]]) {
 			NSOpenGLView *glView = (NSOpenGLView *)view;
 			win->gl_context = [glView openGLContext];
+			// #1947: the view's context belongs to the app (it is normally the
+			// very context the app renders with — cube_handle_gl_macos binds
+			// [g_glView openGLContext]). Borrowed, not ours.
+			win->context_is_app_owned = (win->gl_context != nil);
 			if (win->gl_context == nil) {
 				// Create context for the existing NSOpenGLView
 				NSOpenGLContext *share_ctx = nil;
@@ -426,6 +439,26 @@
 	return (void *)[win->gl_context CGLContextObj];
 }
 
+bool
+comp_gl_window_macos_context_is_app_owned(struct comp_gl_window_macos *win)
+{
+	return win != NULL && win->context_is_app_owned;
+}
+
+void
+comp_gl_window_macos_lock(struct comp_gl_window_macos *win)
+{
+	if (win == NULL || win->gl_context == nil) return;
+	CGLLockContext([win->gl_context CGLContextObj]);
+}
+
+void
+comp_gl_window_macos_unlock(struct comp_gl_window_macos *win)
+{
+	if (win == NULL || win->gl_context == nil) return;
+	CGLUnlockContext([win->gl_context CGLContextObj]);
+}
+
 bool
 comp_gl_window_macos_make_current(struct comp_gl_window_macos *win)
 {

test_apps/handle/cube_handle_gl_macos/main.mm

@@ -654,7 +654,7 @@ static void EnsureFBO(GLRenderer &r, uint32_t w, uint32_t h)
 // DISPLAYXR_CURSOR_DEPTH_SPRITE=custom - and the app hides the OS cursor
 // exactly while XrCursorDepthDrawStateDXR::willDraw.
 // DISPLAYXR_CURSOR_DEPTH_DRAW=app draws a crosshair from the placement
-// instead. OPT-IN here: DISPLAYXR_CURSOR_DEPTH=1 (runtime#1947); DISPLAYXR_CURSOR_DEPTH_UV=u,v
+// instead. DISPLAYXR_CURSOR_DEPTH=0 opts out; DISPLAYXR_CURSOR_DEPTH_UV=u,v
 // scripts the cursor for headless capture.
 // ============================================================================
 
@@ -1406,18 +1406,14 @@ static bool InitializeOpenXR(AppXrSession &app)
     if (app.hasViewRigExt) {
         enabledExts.push_back(XR_DXR_VIEW_RIG_EXTENSION_NAME);
     }
-    // XR_DXR_cursor_depth (ADR-046): OPT-IN on this app only -
-    // DISPLAYXR_CURSOR_DEPTH=1 enables it (the runtime otherwise does no
-    // cursor work at all). Unlike the Metal and Vulkan cubes (default on), the
-    // GL cube crashes ~1 in 5 launches in Apple's GL driver while the OS cursor
-    // is hidden - a GL compositor repaint-thread vs AppKit race, runtime#1947.
-    // Flip back to default-on once that is fixed.
+    // XR_DXR_cursor_depth (ADR-046): on by default, DISPLAYXR_CURSOR_DEPTH=0
+    // opts out (the runtime then does no cursor work at all).
     if (app.hasCursorDepthExt) {
         const char *opt = getenv("DISPLAYXR_CURSOR_DEPTH");
-        if (opt != nullptr && strcmp(opt, "1") == 0) {
-            enabledExts.push_back(XR_DXR_CURSOR_DEPTH_EXTENSION_NAME);
-        } else {
+        if (opt != nullptr && strcmp(opt, "0") == 0) {
             app.hasCursorDepthExt = false;
+        } else {
+            enabledExts.push_back(XR_DXR_CURSOR_DEPTH_EXTENSION_NAME);
         }
     }
     LOG_INFO("XR_DXR_cursor_depth: %s", app.hasCursorDepthExt ? "ENABLED" : "off");