From 635d47a980bee8cc3e815010023d19933e0c04b5 Mon Sep 17 00:00:00 2001 From: David Date: Sun, 11 Oct 2026 19:09:59 -0700 Subject: [PATCH 1/2] fix(gl): never repaint on the app's own NSOpenGLView context (#1947) A handle app that binds an NSOpenGLView hands the runtime that view, and comp_gl_window_macos_setup_external adopted the view's openGLContext - the very CGL context the app renders with. The #868 repaint thread then replayed the weave on it while the app thread was drawing (RenderScene runs between xrBeginFrame and xrEndFrame, outside c->mutex and outside app_frame_in_progress), so one GLDContextRec was driven from two threads: SIGSEGV in GLRResourceList::addResource under the repaint's glBlitFramebuffer, or a Metal-validation abort in the app's draw. Hiding the OS cursor was only a timing trigger. With DXR_WEAVE_REPAINT_FORCE=1 the old code crashes 10/10 launches with the cursor extension off as well. No lock the runtime takes can cover the app's own GL calls, so the repaint thread is not started when the compositor's context is app-owned (one WARN says so). Private contexts (own window, plain NSView, offscreen texture) keep the repaint, now under CGLLockContext so it cannot interleave with AppKit's -[NSOpenGLContext update] on the main thread. Co-Authored-By: Claude Opus 5.5 --- docs/adr/ADR-046-depth-aware-cursor.md | 20 ++++++------ src/xrt/compositor/gl/comp_gl_compositor.cpp | 30 ++++++++++++++++++ src/xrt/compositor/gl/comp_gl_window_macos.h | 29 +++++++++++++++++ src/xrt/compositor/gl/comp_gl_window_macos.m | 33 ++++++++++++++++++++ 4 files changed, 103 insertions(+), 9 deletions(-) diff --git a/docs/adr/ADR-046-depth-aware-cursor.md b/docs/adr/ADR-046-depth-aware-cursor.md index e9c900a0b6..e35319a529 100644 --- a/docs/adr/ADR-046-depth-aware-cursor.md +++ b/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. diff --git a/src/xrt/compositor/gl/comp_gl_compositor.cpp b/src/xrt/compositor/gl/comp_gl_compositor.cpp index 1c3ad8732f..5bd10bf76c 100644 --- a/src/xrt/compositor/gl/comp_gl_compositor.cpp +++ b/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); } diff --git a/src/xrt/compositor/gl/comp_gl_window_macos.h b/src/xrt/compositor/gl/comp_gl_window_macos.h index 18d92a8957..aad6af974f 100644 --- a/src/xrt/compositor/gl/comp_gl_window_macos.h +++ b/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. * diff --git a/src/xrt/compositor/gl/comp_gl_window_macos.m b/src/xrt/compositor/gl/comp_gl_window_macos.m index 5b75d6bdf6..cfde0bde80 100644 --- a/src/xrt/compositor/gl/comp_gl_window_macos.m +++ b/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) { From 6c3eee2b03eafbf66572291ab083eba7b06fb328 Mon Sep 17 00:00:00 2001 From: David Date: Sun, 11 Oct 2026 19:10:01 -0700 Subject: [PATCH 2/2] feat(cube_handle_gl_macos): depth cursor default-on again Undo the #1946 opt-in (DISPLAYXR_CURSOR_DEPTH=1) now that the crash it worked around is fixed in the GL compositor: like the Metal and Vulkan cubes, the cursor is on by default and DISPLAYXR_CURSOR_DEPTH=0 opts out. Co-Authored-By: Claude Opus 5.5 --- test_apps/handle/cube_handle_gl_macos/main.mm | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/test_apps/handle/cube_handle_gl_macos/main.mm b/test_apps/handle/cube_handle_gl_macos/main.mm index b36286e4d5..ef45d79b1e 100644 --- a/test_apps/handle/cube_handle_gl_macos/main.mm +++ b/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");