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) { 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");