GL: new apple-buffer-texture-detach-on-data-modify workaround.

doc/changelog.dox

@@ -65,6 +65,10 @@ See also:
     that attempted to fix this by doing an explicit buffer binding in some
     cases. See @ref opengl-workarounds and [mosra/magnum#405](https://github.com/mosra/magnum/pull/405)
     for more information.
+-   A @cpp "apple-buffer-texture-detach-on-data-modify" @ce workaround that
+    fixes crashes on Apple macOS when attempting to modify a @ref GL::Buffer
+    that's attached to a @ref GL::BufferTexture. See @ref opengl-workarounds
+    for more information.
 
 @subsubsection changelog-latest-new-math Math library
 

src/Magnum/GL/Buffer.cpp

@@ -29,6 +29,9 @@
 #include <Corrade/Containers/Array.h>
 #include <Corrade/Utility/Debug.h>
 
+#if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+#include "Magnum/GL/BufferTexture.h"
+#endif
 #include "Magnum/GL/Context.h"
 #include "Magnum/GL/Extensions.h"
 #include "Magnum/GL/Implementation/State.h"
@@ -546,6 +549,92 @@ bool Buffer::unmapImplementationDSA() {
 #endif
 #endif
 
+#if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+/* If this buffer is attached to a buffer texture, we need to temporarily
+   detach it to avoid crashes in the macOS driver when doing buffer-modifying
+   operations. See the apple-buffer-texture-detach-on-setdata workaround for
+   more info. */
+void Buffer::textureWorkaroundAppleBefore() {
+    /* No buffer texture attached or the texture no longer exists, nothing to
+       do */
+    if(!_bufferTexture || !glIsTexture(_bufferTexture)) {
+        _bufferTexture = 0; /* Avoid doing unnecessary work next time */
+        return;
+    }
+
+    /* Bind the buffer texture so we can ask for its state (as there's no
+       DSA on Apple to have a shortcut). The state tracking is a bit
+       complicated for textures, so playing it safe and using (friended)
+       private AbstractTexture APIs for that. */
+    BufferTexture t = BufferTexture::wrap(_bufferTexture);
+    t.bindInternal();
+
+    /* Check the current buffer binding for the texture. If it is no longer
+       our buffer, the buffer might get detached since or replaced with
+       another (which is fine, and much easier than maintaining the state
+       explicitly). */
+    GLuint currentBufferBinding;
+    glGetTexLevelParameteriv(GL_TEXTURE_BUFFER, 0, GL_TEXTURE_BUFFER_DATA_STORE_BINDING, reinterpret_cast<GLint*>(&currentBufferBinding));
+    if(currentBufferBinding != _id) {
+        _bufferTexture = 0; /* Avoid doing unnecessary work next time */
+        return;
+    }
+
+     /* In a saner bug workaround, i would just query
+        GL_TEXTURE_INTERNAL_FORMAT here. However, that's also broken,
+        returning GL_R8 always, so instead we have to cache it in the
+        Buffer instance. "Fortunately" macOS doesn't support
+        ARB_texture_range, so we don't need to store the offset + size,
+        just the format. */
+     CORRADE_INTERNAL_ASSERT(!Context::current().isExtensionSupported<Extensions::ARB::texture_buffer_range>());
+
+     /* Temporarily detach the buffer. To avoid hitting more corner
+        cases, keep the same format as before. */
+     glTexBuffer(GL_TEXTURE_BUFFER, _bufferTextureFormat, 0);
+}
+
+void Buffer::textureWorkaroundAppleAfter() {
+    /* Put the buffer back, if we are supposed to be attached to a texture.
+       Assumes textureWorkaroundAppleBefore() was called and thus the texture
+       is bound. In case the state was stale, _bufferTexture was set to 0, so
+       this will get executed only when needed. */
+    if(_bufferTexture) glTexBuffer(GL_TEXTURE_BUFFER, _bufferTextureFormat, _id);
+}
+
+void Buffer::dataImplementationApple(const GLsizeiptr size, const GLvoid* const data, const BufferUsage usage) {
+    textureWorkaroundAppleBefore();
+    dataImplementationDefault(size, data, usage);
+    textureWorkaroundAppleAfter();
+}
+
+void Buffer::subDataImplementationApple(const GLintptr offset, const GLsizeiptr size, const GLvoid* const data) {
+    textureWorkaroundAppleBefore();
+    subDataImplementationDefault(offset, size, data);
+    textureWorkaroundAppleAfter();
+}
+
+void* Buffer::mapImplementationApple(const MapAccess access) {
+    textureWorkaroundAppleBefore();
+    void* const out = mapImplementationDefault(access);
+    textureWorkaroundAppleAfter();
+    return out;
+}
+
+void* Buffer::mapRangeImplementationApple(const GLintptr offset, const GLsizeiptr length, const MapFlags access) {
+    textureWorkaroundAppleBefore();
+    void* const out = mapRangeImplementationDefault(offset, length, access);
+    textureWorkaroundAppleAfter();
+    return out;
+}
+
+bool Buffer::unmapImplementationApple() {
+    textureWorkaroundAppleBefore();
+    const bool out = unmapImplementationDefault();
+    textureWorkaroundAppleAfter();
+    return out;
+}
+#endif
+
 #ifndef DOXYGEN_GENERATING_OUTPUT
 Debug& operator<<(Debug& debug, const Buffer::TargetHint value) {
     debug << "GL::Buffer::TargetHint" << Debug::nospace;

src/Magnum/GL/Buffer.h

@@ -1214,43 +1214,69 @@ class MAGNUM_GL_EXPORT Buffer: public AbstractObject {
         void MAGNUM_GL_LOCAL getSubDataImplementationDSA(GLintptr offset, GLsizeiptr size, GLvoid* data);
         #endif
 
+        #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+        void MAGNUM_GL_LOCAL textureWorkaroundAppleBefore();
+        void MAGNUM_GL_LOCAL textureWorkaroundAppleAfter();
+        #endif
+
         void MAGNUM_GL_LOCAL dataImplementationDefault(GLsizeiptr size, const GLvoid* data, BufferUsage usage);
+        #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+        void MAGNUM_GL_LOCAL dataImplementationApple(GLsizeiptr size, const GLvoid* data, BufferUsage usage);
+        #endif
         #ifndef MAGNUM_TARGET_GLES
         void MAGNUM_GL_LOCAL dataImplementationDSA(GLsizeiptr size, const GLvoid* data, BufferUsage usage);
         #endif
 
         void MAGNUM_GL_LOCAL subDataImplementationDefault(GLintptr offset, GLsizeiptr size, const GLvoid* data);
+        #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+        void MAGNUM_GL_LOCAL subDataImplementationApple(GLintptr offset, GLsizeiptr size, const GLvoid* data);
+        #endif
         #ifndef MAGNUM_TARGET_GLES
         void MAGNUM_GL_LOCAL subDataImplementationDSA(GLintptr offset, GLsizeiptr size, const GLvoid* data);
         #endif
 
         void MAGNUM_GL_LOCAL invalidateImplementationNoOp();
+        /* No need for Apple-specific invalidateImplementation, as
+           GL_ARB_invalidate_subdata isn't supported */
         #ifndef MAGNUM_TARGET_GLES
         void MAGNUM_GL_LOCAL invalidateImplementationARB();
         #endif
 
         void MAGNUM_GL_LOCAL invalidateSubImplementationNoOp(GLintptr offset, GLsizeiptr length);
+        /* No need for Apple-specific invalidateSubImplementation, as
+           GL_ARB_invalidate_subdata isn't supported */
         #ifndef MAGNUM_TARGET_GLES
         void MAGNUM_GL_LOCAL invalidateSubImplementationARB(GLintptr offset, GLsizeiptr length);
         #endif
 
         #ifndef MAGNUM_TARGET_WEBGL
         void MAGNUM_GL_LOCAL * mapImplementationDefault(MapAccess access);
+        #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+        void MAGNUM_GL_LOCAL * mapImplementationApple(MapAccess access);
+        #endif
         #ifndef MAGNUM_TARGET_GLES
         void MAGNUM_GL_LOCAL * mapImplementationDSA(MapAccess access);
         #endif
 
         void MAGNUM_GL_LOCAL * mapRangeImplementationDefault(GLintptr offset, GLsizeiptr length, MapFlags access);
+        #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+        void MAGNUM_GL_LOCAL * mapRangeImplementationApple(GLintptr offset, GLsizeiptr length, MapFlags access);
+        #endif
         #ifndef MAGNUM_TARGET_GLES
         void MAGNUM_GL_LOCAL * mapRangeImplementationDSA(GLintptr offset, GLsizeiptr length, MapFlags access);
         #endif
 
         void MAGNUM_GL_LOCAL flushMappedRangeImplementationDefault(GLintptr offset, GLsizeiptr length);
+        /* No need for Apple-specific flushMappedRangeImplementation, as this
+           doesn't seem to hit the crashy code path */
         #ifndef MAGNUM_TARGET_GLES
         void MAGNUM_GL_LOCAL flushMappedRangeImplementationDSA(GLintptr offset, GLsizeiptr length);
         #endif
 
         bool MAGNUM_GL_LOCAL unmapImplementationDefault();
+        #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+        bool MAGNUM_GL_LOCAL unmapImplementationApple();
+        #endif
         #ifndef MAGNUM_TARGET_GLES
         bool MAGNUM_GL_LOCAL unmapImplementationDSA();
         #endif
@@ -1259,6 +1285,10 @@ class MAGNUM_GL_EXPORT Buffer: public AbstractObject {
         GLuint _id;
         TargetHint _targetHint;
         ObjectFlags _flags;
+        #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+        GLuint _bufferTexture{};
+        GLenum _bufferTextureFormat{};
+        #endif
 };
 
 #ifndef MAGNUM_TARGET_WEBGL
@@ -1275,21 +1305,37 @@ MAGNUM_GL_EXPORT Debug& operator<<(Debug& debug, Buffer::Target value);
 
 inline Buffer::Buffer(NoCreateT) noexcept: _id{0}, _targetHint{TargetHint::Array}, _flags{ObjectFlag::DeleteOnDestruction} {}
 
-inline Buffer::Buffer(Buffer&& other) noexcept: _id{other._id}, _targetHint{other._targetHint}, _flags{other._flags} {
+inline Buffer::Buffer(Buffer&& other) noexcept: _id{other._id}, _targetHint{other._targetHint}, _flags{other._flags}
+    #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+    , _bufferTexture{other._bufferTexture}, _bufferTextureFormat{other._bufferTextureFormat}
+    #endif
+{
     other._id = 0;
+    #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+    other._bufferTexture = 0;
+    other._bufferTextureFormat = 0;
+    #endif
 }
 
 inline Buffer& Buffer::operator=(Buffer&& other) noexcept {
     using std::swap;
     swap(_id, other._id);
     swap(_targetHint, other._targetHint);
     swap(_flags, other._flags);
+    #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+    swap(_bufferTexture, other._bufferTexture);
+    swap(_bufferTextureFormat, other._bufferTextureFormat);
+    #endif
     return *this;
 }
 
 inline GLuint Buffer::release() {
     const GLuint id = _id;
     _id = 0;
+    #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+    _bufferTexture = 0;
+    _bufferTextureFormat = 0;
+    #endif
     return id;
 }
 

src/Magnum/GL/BufferTexture.cpp

@@ -96,6 +96,21 @@ void BufferTexture::setBufferImplementationDefault(BufferTextureFormat internalF
     glTexBuffer(GL_TEXTURE_BUFFER, GLenum(internalFormat), buffer ? buffer->id() : 0);
 }
 
+#if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+void BufferTexture::setBufferImplementationApple(BufferTextureFormat internalFormat, Buffer* buffer) {
+    /* Reference this texture from the buffer so next time setData() is called
+       we can temporarily detach it. See apple-buffer-texture-detach-on-setdata
+       for more information. */
+    if(buffer) {
+        buffer->_bufferTexture = id();
+        buffer->_bufferTextureFormat = GLenum(internalFormat);
+    }
+
+    bindInternal();
+    glTexBuffer(GL_TEXTURE_BUFFER, GLenum(internalFormat), buffer ? buffer->id() : 0);
+}
+#endif
+
 #ifdef MAGNUM_TARGET_GLES
 void BufferTexture::setBufferImplementationEXT(BufferTextureFormat internalFormat, Buffer* buffer) {
     bindInternal();

src/Magnum/GL/BufferTexture.h

@@ -258,10 +258,16 @@ class MAGNUM_GL_EXPORT BufferTexture: public AbstractTexture {
 
     private:
         friend Implementation::TextureState;
+        #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+        friend Buffer;
+        #endif
 
         explicit BufferTexture(GLuint id, ObjectFlags flags): AbstractTexture{id, GL_TEXTURE_BUFFER, flags} {}
 
         void MAGNUM_GL_LOCAL setBufferImplementationDefault(BufferTextureFormat internalFormat, Buffer* buffer);
+        #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+        void MAGNUM_GL_LOCAL setBufferImplementationApple(BufferTextureFormat internalFormat, Buffer* buffer);
+        #endif
         #ifdef MAGNUM_TARGET_GLES
         void MAGNUM_GL_LOCAL setBufferImplementationEXT(BufferTextureFormat internalFormat, Buffer* buffer);
         #endif
@@ -270,6 +276,8 @@ class MAGNUM_GL_EXPORT BufferTexture: public AbstractTexture {
         #endif
 
         void MAGNUM_GL_LOCAL setBufferRangeImplementationDefault(BufferTextureFormat internalFormat, Buffer& buffer, GLintptr offset, GLsizeiptr size);
+        /* No need for Apple-specific setBufferRangeImplementation, as the
+           extension is not supported anyway */
         #ifdef MAGNUM_TARGET_GLES
         void MAGNUM_GL_LOCAL setBufferRangeImplementationEXT(BufferTextureFormat internalFormat, Buffer& buffer, GLintptr offset, GLsizeiptr size);
         #endif

src/Magnum/GL/Implementation/BufferState.cpp

@@ -177,6 +177,19 @@ BufferState::BufferState(Context& context, std::vector<std::string>& extensions)
         setTargetHintImplementation = &Buffer::setTargetHintImplementationDefault;
     }
 
+    #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+    if(!context.isDriverWorkaroundDisabled("apple-buffer-texture-detach-on-data-modify")) {
+        dataImplementation = &Buffer::dataImplementationApple;
+        subDataImplementation = &Buffer::subDataImplementationApple;
+        mapImplementation = &Buffer::mapImplementationApple;
+        mapRangeImplementation = &Buffer::mapRangeImplementationApple;
+        unmapImplementation = &Buffer::unmapImplementationApple;
+        /* No need for Apple-specific invalidate*Implementation, as the
+           extension isn't supported anyway */
+        CORRADE_INTERNAL_ASSERT(!context.isExtensionSupported<Extensions::ARB::invalidate_subdata>());
+    }
+    #endif
+
     #ifdef MAGNUM_TARGET_GLES
     static_cast<void>(context);
     static_cast<void>(extensions);

src/Magnum/GL/Implementation/TextureState.cpp

@@ -483,6 +483,15 @@ TextureState::TextureState(Context& context, std::vector<std::string>& extension
     }
     #endif
 
+    #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+    if(!context.isDriverWorkaroundDisabled("apple-buffer-texture-detach-on-data-modify")) {
+        setBufferImplementation = &BufferTexture::setBufferImplementationApple;
+        /* No need for Apple-specific setBufferRangeImplementation, as the
+           extension is not supported anyway */
+        CORRADE_INTERNAL_ASSERT(!context.isExtensionSupported<Extensions::ARB::texture_buffer_range>());
+    }
+    #endif
+
     /* Allocate texture bindings array to hold all possible texture units */
     glGetIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, &maxTextureUnits);
     CORRADE_INTERNAL_ASSERT(maxTextureUnits > 0);

src/Magnum/GL/Implementation/driverSpecific.cpp

@@ -38,6 +38,21 @@ namespace {
     /* Search the code for the following strings to see where they are implemented. */
     std::vector<std::string> KnownWorkarounds{
 /* [workarounds] */
+#if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+/* Calling glBufferData(), glMapBuffer(), glMapBufferRange() or glUnmapBuffer()
+   on a buffer that's attached to a GL_TEXTURE_BUFFER crashes in
+   gleUpdateCtxDirtyStateForBufStampChange deep inside Apple's GLengine. A
+   workaround is to remember if a buffer is attached to a buffer texture,
+   temporarily detaching it, calling given data-modifying API and then
+   attaching it back with the same parameters. Unfortunately we need to cache
+   also the internal texture format, as GL_TEXTURE_INTERNAL_FORMAT query is
+   broken for buffer textures as well, returning always GL_R8 (the
+   spec-mandated default). "Fortunately" macOS doesn't support
+   ARB_texture_buffer_range so we don't need to store also offset/size, only
+   texture ID and its internal format, wasting 8 bytes per Buffer instance. */
+"apple-buffer-texture-detach-on-data-modify",
+#endif
+
 #if defined(CORRADE_TARGET_ANDROID) && defined(MAGNUM_TARGET_GLES)
 /* glBeginQuery() with GL_TIME_ELAPSED causes a GL_OUT_OF_MEMORY error when
    running from the Android shell (through ADB). No such error happens in an