From 24cc971b1f43c61aa157228f6ea9916007ffb3cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 28 Jan 2020 20:51:10 +0100 Subject: [PATCH] GL: reworked apple-buffer-texture-unbind-on-buffer-modify workaround. Much smaller, nicer and more robust. --- doc/changelog.dox | 6 +- src/Magnum/GL/Buffer.cpp | 86 +++++----------- src/Magnum/GL/Buffer.h | 22 +---- src/Magnum/GL/BufferTexture.cpp | 15 --- src/Magnum/GL/BufferTexture.h | 8 -- src/Magnum/GL/Implementation/BufferState.cpp | 2 +- src/Magnum/GL/Implementation/TextureState.cpp | 9 -- .../GL/Implementation/driverSpecific.cpp | 32 +++--- src/Magnum/GL/Test/BufferTextureGLTest.cpp | 97 +------------------ 9 files changed, 52 insertions(+), 225 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index f70a9a8c9..069f2c44e 100644 --- a/doc/changelog.dox +++ b/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 diff --git a/src/Magnum/GL/Buffer.cpp b/src/Magnum/GL/Buffer.cpp index 062b137b0..10c8e04f5 100644 --- a/src/Magnum/GL/Buffer.cpp +++ b/src/Magnum/GL/Buffer.cpp @@ -29,9 +29,6 @@ #include #include -#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; + /* 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& 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{}; } - - /* 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(¤tBufferBinding)); - 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()); - - /* 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; + 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 diff --git a/src/Magnum/GL/Buffer.h b/src/Magnum/GL/Buffer.h index d5b8fdbef..3fc6bab3b 100644 --- a/src/Magnum/GL/Buffer.h +++ b/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; } diff --git a/src/Magnum/GL/BufferTexture.cpp b/src/Magnum/GL/BufferTexture.cpp index 7b77b7c55..6df6c1398 100644 --- a/src/Magnum/GL/BufferTexture.cpp +++ b/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(); diff --git a/src/Magnum/GL/BufferTexture.h b/src/Magnum/GL/BufferTexture.h index fa227e9a4..0fb2b90bd 100644 --- a/src/Magnum/GL/BufferTexture.h +++ b/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 diff --git a/src/Magnum/GL/Implementation/BufferState.cpp b/src/Magnum/GL/Implementation/BufferState.cpp index 300ff803a..7168b5ceb 100644 --- a/src/Magnum/GL/Implementation/BufferState.cpp +++ b/src/Magnum/GL/Implementation/BufferState.cpp @@ -178,7 +178,7 @@ BufferState::BufferState(Context& context, std::vector& 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; diff --git a/src/Magnum/GL/Implementation/TextureState.cpp b/src/Magnum/GL/Implementation/TextureState.cpp index 5abf5c923..c21565fc7 100644 --- a/src/Magnum/GL/Implementation/TextureState.cpp +++ b/src/Magnum/GL/Implementation/TextureState.cpp @@ -483,15 +483,6 @@ TextureState::TextureState(Context& context, std::vector& 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()); - } - #endif - /* Allocate texture bindings array to hold all possible texture units */ glGetIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, &maxTextureUnits); CORRADE_INTERNAL_ASSERT(maxTextureUnits > 0); diff --git a/src/Magnum/GL/Implementation/driverSpecific.cpp b/src/Magnum/GL/Implementation/driverSpecific.cpp index 30f30de13..03c7dde19 100644 --- a/src/Magnum/GL/Implementation/driverSpecific.cpp +++ b/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) diff --git a/src/Magnum/GL/Test/BufferTextureGLTest.cpp b/src/Magnum/GL/Test/BufferTextureGLTest.cpp index 000a33c1f..a4119ca72 100644 --- a/src/Magnum/GL/Test/BufferTextureGLTest.cpp +++ b/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()) - CORRADE_SKIP(Extensions::ARB::texture_buffer_object::string() + std::string(" is not supported.")); - #else - if(!Context::current().isExtensionSupported()) - 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({ - 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({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({ - 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()) - CORRADE_SKIP(Extensions::ARB::texture_buffer_object::string() + std::string(" is not supported.")); - - BufferTexture texture; - Buffer buffer; - buffer.setData({ - 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({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()) - CORRADE_SKIP(Extensions::ARB::texture_buffer_object::string() + std::string(" is not supported.")); - - Buffer buffer; - buffer.setData({ - 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({0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f}); - MAGNUM_VERIFY_NO_GL_ERROR(); -} #endif }}}}