From b6ca052f24b3cba90d333dae59328d9b3dd7a166 Mon Sep 17 00:00:00 2001 From: David Peicho Date: Tue, 18 Feb 2025 20:10:02 +0100 Subject: [PATCH] GL: new "apple-crashy-msaa-default-framebuffer" workaround. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First spotted in July 2024 with magnum-player, and back then the only reliable solution was to disable MSAA altogether. Fortunately this fixes it as well, unfortunately there's no way to know if the default framebuffer is actually multisampled, so it's done always. Co-authored-by: Vladimír Vondruš --- src/Magnum/GL/Context.cpp | 9 +++- src/Magnum/GL/Context.h | 1 + .../GL/Implementation/driverSpecific.cpp | 44 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/Magnum/GL/Context.cpp b/src/Magnum/GL/Context.cpp index 559b25d64..6f34fc368 100644 --- a/src/Magnum/GL/Context.cpp +++ b/src/Magnum/GL/Context.cpp @@ -1002,7 +1002,10 @@ bool Context::tryCreate(const Configuration& configuration) { _extensionRequiredVersion[extension.index()] = Version::None; /* Setup driver workarounds (increase required version for particular - extensions), see Implementation/driverWorkarounds.cpp */ + extensions), see Implementation/driverWorkarounds.cpp. Workarounds done + here are what affects state creation below, there's another set of + workarounds that instead depend on the created state, which are then + set up in setupDriverWorkaroundsWithStateCreated() afterwards. */ setupDriverWorkarounds(); /* Set this context as current */ @@ -1033,6 +1036,10 @@ bool Context::tryCreate(const Configuration& configuration) { _stateData = Utility::move(state.first()); _state = &*state.second(); + /* Setup driver workarounds that need the state to be created already. + Counterpart to the setupDriverWorkarounds() call above. */ + setupDriverWorkaroundsWithStateCreated(); + /* Print a list of used workarounds */ if(!_driverWorkarounds.isEmpty()) { Debug{output} << "Using driver workarounds:"; diff --git a/src/Magnum/GL/Context.h b/src/Magnum/GL/Context.h index b4ababbab..d77e1331d 100644 --- a/src/Magnum/GL/Context.h +++ b/src/Magnum/GL/Context.h @@ -897,6 +897,7 @@ class MAGNUM_GL_EXPORT Context { /* Defined in Implementation/driverSpecific.cpp */ MAGNUM_GL_LOCAL void setupDriverWorkarounds(); + MAGNUM_GL_LOCAL void setupDriverWorkaroundsWithStateCreated(); #ifndef MAGNUM_TARGET_GLES MAGNUM_GL_LOCAL static bool isCoreProfileImplementationDefault(Context& self); diff --git a/src/Magnum/GL/Implementation/driverSpecific.cpp b/src/Magnum/GL/Implementation/driverSpecific.cpp index c8eeaddf1..e13ae6b98 100644 --- a/src/Magnum/GL/Implementation/driverSpecific.cpp +++ b/src/Magnum/GL/Implementation/driverSpecific.cpp @@ -33,6 +33,13 @@ #include "Magnum/GL/Extensions.h" #include "Magnum/Math/Range.h" +#if defined(CORRADE_TARGET_APPLE) && !defined(MAGNUM_TARGET_GLES) +/* For the "apple-crashy-msaa-default-framebuffer" workaround */ +#include "Magnum/GL/Framebuffer.h" +#include "Magnum/GL/Renderbuffer.h" +#include "Magnum/GL/RenderbufferFormat.h" +#endif + #if defined(MAGNUM_TARGET_WEBGL) && defined(CORRADE_TARGET_EMSCRIPTEN) /* Including any Emscripten header should also make __EMSCRIPTEN_major__ etc macros available, independently of whether they're passed implicitly (before @@ -109,6 +116,20 @@ constexpr Containers::StringView KnownWorkarounds[]{ "apple-buffer-texture-unbind-on-buffer-modify"_s, #endif +#if defined(CORRADE_TARGET_APPLE) && !defined(MAGNUM_TARGET_GLES) +/* Creating a multisampled default framebuffer in both SDL and GLFW causes a + crash in gldUpdateReadFramebuffer() inside Apple's GL-over-Metal + implementation when the window gets moved or resized. Disabling + multisampling fixes it, but it's also fixed by creating and then immediately + destroying a custom framebuffer with a multisampled attachment. Ideally the + workaround would only be enabled if the default framebuffer is multisampled, + but that's unfortunately only queryable since GL 4.3+, while Apple is stuck + on 4.1. So the workaround is enabled always. + + The assumption is that this affects only desktop GL, not GLES on iOS. */ +"apple-crashy-msaa-default-framebuffer"_s, +#endif + #if defined(MAGNUM_TARGET_GLES) && !defined(MAGNUM_TARGET_GLES2) /* Qualcomm Adreno drivers V@0615.65 (and possibly others) report __VERSION__ as 300 even for GLSL ES 3.10 and 3.20, breaking version-dependent shader @@ -877,6 +898,29 @@ void Context::setupDriverWorkarounds() { } } #endif + + /* Additional workarounds done in setupDriverWorkaroundsWithStateCreated() + below */ +} + +void Context::setupDriverWorkaroundsWithStateCreated() { + /* Compared to setupDriverWorkarounds(), which is called even before the + context is current and the internal state tracker is created, everything + here depends on the context being current and state ready. */ + + #if defined(CORRADE_TARGET_APPLE) && !defined(MAGNUM_TARGET_GLES) + /* Creating and then immediately destructing is what fixes it, yes. Enable + this only on Apple's own drivers, not alternatives like ANGLE or Zink. + Also ideally would be done only if the default framebuffer is + multisampled, but that's only possible to query since GL 4.3, and Apple + is stuck at 4.1, so doing this always. */ + if(vendorString() == "Apple"_s && !isDriverWorkaroundDisabled("apple-crashy-msaa-default-framebuffer"_s)) { + GL::Renderbuffer renderbuffer; + renderbuffer.setStorageMultisample(4, GL::RenderbufferFormat::RGBA8, Vector2i{1}); + GL::Framebuffer framebuffer{{{}, Vector2i{1}}}; + framebuffer.attachRenderbuffer(GL::Framebuffer::ColorAttachment{0}, renderbuffer); + } + #endif } Context::Configuration& Context::Configuration::addDisabledWorkarounds(const Containers::StringIterable& workarounds) {