GL: reworked apple-buffer-texture-unbind-on-buffer-modify workaround.

Much smaller, nicer and more robust.

doc/changelog.dox

@@ -65,10 +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
+-   A @cpp "apple-buffer-texture-unbind-on-buffer-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.
+    when a @ref GL::BufferTexture is bound. See @ref opengl-workarounds for
+    more information.
 
 @subsubsection changelog-latest-new-math Math library
 

src/Magnum/GL/Buffer.cpp

@@ -29,9 +29,6 @@
 #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"
@@ -41,6 +38,10 @@
 #endif
 #include "Magnum/GL/Implementation/MeshState.h"
 
+#if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
+#include "Magnum/GL/Implementation/TextureState.h"
+#endif
+
 namespace Magnum { namespace GL {
 
 #ifndef MAGNUM_TARGET_GLES
@@ -550,88 +551,51 @@ bool Buffer::unmapImplementationDSA() {
 #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. */
+/* See apple-buffer-texture-detach-on-data-modify for the gory details. */
 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);
+    /* Apple "fortunately" supports just 16 texture units, so this doesn't take
+       too long. */
+    Implementation::TextureState& textureState = *Context::current().state().texture;
+    for(GLint textureUnit = 0; textureUnit != GLint(textureState.bindings.size()); ++textureUnit) {
+        std::pair<GLenum, GLuint>& binding = textureState.bindings[textureUnit];
+        if(binding.first != GL_TEXTURE_BUFFER) continue;
+
+        /* Activate given texture unit if not already active, update state
+           tracker */
+        if(textureState.currentTextureUnit != textureUnit)
+            glActiveTexture(GL_TEXTURE0 + (textureState.currentTextureUnit = textureUnit));
+
+        /* Unbind the texture, reset state tracker */
+        glBindTexture(GL_TEXTURE_BUFFER, 0);
+        /* libstdc++ since GCC 6.3 can't handle just = {} (ambiguous overload
+           of operator=) */
+        binding = std::pair<GLenum, GLuint>{};
+    }
 }
 
 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;
+    return mapImplementationDefault(access);
 }
 
 void* Buffer::mapRangeImplementationApple(const GLintptr offset, const GLsizeiptr length, const MapFlags access) {
     textureWorkaroundAppleBefore();
-    void* const out = mapRangeImplementationDefault(offset, length, access);
-    textureWorkaroundAppleAfter();
-    return out;
+    return mapRangeImplementationDefault(offset, length, access);
 }
 
 bool Buffer::unmapImplementationApple() {
     textureWorkaroundAppleBefore();
-    const bool out = unmapImplementationDefault();
-    textureWorkaroundAppleAfter();
-    return out;
+    return unmapImplementationDefault();
 }
 #endif
 

src/Magnum/GL/Buffer.h

@@ -1285,10 +1285,6 @@ 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
@@ -1305,16 +1301,8 @@ 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}
-    #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
-    , _bufferTexture{other._bufferTexture}, _bufferTextureFormat{other._bufferTextureFormat}
-    #endif
-{
+inline Buffer::Buffer(Buffer&& other) noexcept: _id{other._id}, _targetHint{other._targetHint}, _flags{other._flags} {
     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 {
@@ -1322,20 +1310,12 @@ inline Buffer& Buffer::operator=(Buffer&& other) noexcept {
     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,21 +96,6 @@ 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,16 +258,10 @@ 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
@@ -276,8 +270,6 @@ 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

@@ -178,7 +178,7 @@ BufferState::BufferState(Context& context, std::vector<std::string>& extensions)
     }
 
     #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS)
-    if(!context.isDriverWorkaroundDisabled("apple-buffer-texture-detach-on-data-modify")) {
+    if(!context.isDriverWorkaroundDisabled("apple-buffer-texture-unbind-on-buffer-modify")) {
         dataImplementation = &Buffer::dataImplementationApple;
         subDataImplementation = &Buffer::subDataImplementationApple;
         mapImplementation = &Buffer::mapImplementationApple;

src/Magnum/GL/Implementation/TextureState.cpp

@@ -483,15 +483,6 @@ 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

@@ -40,17 +40,27 @@ namespace {
 /* [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",
+   on ANY buffer when ANY buffer is attached to a currently bound
+   GL_TEXTURE_BUFFER crashes in gleUpdateCtxDirtyStateForBufStampChange deep
+   inside Apple's GLengine. This can be worked around by unbinding all buffer
+   textures before attempting to do such operation.
+
+   A previous iteration of this workaround was 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 also had to cache 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 didn't need to store also
+   offset/size, only texture ID and its internal format, wasting 8 bytes per
+   Buffer instance. HOWEVER, then we discovered this is not enough and also
+   completely unrelated buffers suffer from the same crash. Fixing that
+   properly in a similar manner would mean going through all live buffer
+   texture instances and temporarily detaching their buffer when doing *any*
+   data modification on *any* buffer, which would have extreme perf
+   implications. So FORTUNATELY unbinding the textures worked around this too,
+   and is a much nicer workaround after all. */
+"apple-buffer-texture-unbind-on-buffer-modify",
 #endif
 
 #if defined(CORRADE_TARGET_ANDROID) && defined(MAGNUM_TARGET_GLES)

src/Magnum/GL/Test/BufferTextureGLTest.cpp

@@ -57,9 +57,6 @@ struct BufferTextureGLTest: OpenGLTester {
     void appleSetBufferQueryData();
     void appleSetBufferMap();
     void appleSetBufferMapRange();
-    void appleSetBufferDataMoved();
-    void appleSetBufferDataBufferDetached();
-    void appleSetBufferDataTextureDeleted();
     #endif
 };
 
@@ -81,10 +78,7 @@ BufferTextureGLTest::BufferTextureGLTest() {
               &BufferTextureGLTest::appleSetUnrelatedBufferData,
               &BufferTextureGLTest::appleSetBufferQueryData,
               &BufferTextureGLTest::appleSetBufferMap,
-              &BufferTextureGLTest::appleSetBufferMapRange,
-              &BufferTextureGLTest::appleSetBufferDataMoved,
-              &BufferTextureGLTest::appleSetBufferDataBufferDetached,
-              &BufferTextureGLTest::appleSetBufferDataTextureDeleted
+              &BufferTextureGLTest::appleSetBufferMapRange
               #endif
               });
 }
@@ -461,95 +455,6 @@ void BufferTextureGLTest::appleSetBufferMapRange() {
 
     MAGNUM_VERIFY_NO_GL_ERROR();
 }
-
-void BufferTextureGLTest::appleSetBufferDataMoved() {
-    #ifndef MAGNUM_TARGET_GLES
-    if(!Context::current().isExtensionSupported<Extensions::ARB::texture_buffer_object>())
-        CORRADE_SKIP(Extensions::ARB::texture_buffer_object::string() + std::string(" is not supported."));
-    #else
-    if(!Context::current().isExtensionSupported<Extensions::EXT::texture_buffer>())
-        CORRADE_SKIP(Extensions::EXT::texture_buffer::string() + std::string(" is not supported."));
-    #endif
-
-    BufferTexture texture;
-    Buffer a;
-    texture.setBuffer(BufferTextureFormat::RG8UI, a);
-
-    MAGNUM_VERIFY_NO_GL_ERROR();
-
-    if(Context::current().isVersionSupported(Version::GLES310))
-        CORRADE_COMPARE(texture.size(), 0);
-
-    /* Verify that the texture relation info survives moving the buffer */
-
-    a.setData<UnsignedByte>({
-        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
-        0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f
-    });
-    MAGNUM_VERIFY_NO_GL_ERROR();
-    CORRADE_COMPARE(texture.size(), 8);
-
-    Buffer b{std::move(a)};
-    b.setData<UnsignedByte>({0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f});
-    MAGNUM_VERIFY_NO_GL_ERROR();
-    CORRADE_COMPARE(texture.size(), 4);
-
-    Buffer c;
-    c = std::move(b);
-    c.setData<UnsignedByte>({
-        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
-        0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f
-    });
-    MAGNUM_VERIFY_NO_GL_ERROR();
-    CORRADE_COMPARE(texture.size(), 8);
-}
-
-void BufferTextureGLTest::appleSetBufferDataBufferDetached() {
-    if(!Context::current().isExtensionSupported<Extensions::ARB::texture_buffer_object>())
-        CORRADE_SKIP(Extensions::ARB::texture_buffer_object::string() + std::string(" is not supported."));
-
-    BufferTexture texture;
-    Buffer buffer;
-    buffer.setData<UnsignedByte>({
-        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
-        0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f
-    });
-    texture.setBuffer(BufferTextureFormat::RG8UI, buffer);
-    MAGNUM_VERIFY_NO_GL_ERROR();
-    CORRADE_COMPARE(texture.size(), 8);
-
-    texture.resetBuffer();
-    CORRADE_COMPARE(texture.size(), 0);
-
-    /* The buffer is no longer attached to the texture, so it should not
-       attempt to attach itself again */
-    buffer.setData<UnsignedByte>({0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f});
-    MAGNUM_VERIFY_NO_GL_ERROR();
-    CORRADE_COMPARE(texture.size(), 0);
-}
-
-void BufferTextureGLTest::appleSetBufferDataTextureDeleted() {
-    if(!Context::current().isExtensionSupported<Extensions::ARB::texture_buffer_object>())
-        CORRADE_SKIP(Extensions::ARB::texture_buffer_object::string() + std::string(" is not supported."));
-
-    Buffer buffer;
-    buffer.setData<UnsignedByte>({
-        0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
-        0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f
-    });
-
-    {
-        BufferTexture texture;
-        texture.setBuffer(BufferTextureFormat::RG8UI, buffer);
-        MAGNUM_VERIFY_NO_GL_ERROR();
-        CORRADE_COMPARE(texture.size(), 8);
-    }
-
-    /* The texture no longer exists, so the buffer should not attempt to
-       access it */
-    buffer.setData<UnsignedByte>({0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f});
-    MAGNUM_VERIFY_NO_GL_ERROR();
-}
 #endif
 
 }}}}