From 36f51e3116ebe484a6cdba006660816e2b828e02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 26 Jun 2020 12:42:39 +0200 Subject: [PATCH] GL: finalize the apple-buffer-texture-unbind-on-buffer-modify workaround. Followup to 24cc971b1f43c61aa157228f6ea9916007ffb3cd, covering the remaining case. --- src/Magnum/GL/AbstractTexture.cpp | 19 ++++++++++++++----- src/Magnum/GL/AbstractTexture.h | 3 +++ src/Magnum/GL/Buffer.cpp | 18 +++++++++++++----- src/Magnum/GL/Implementation/TextureState.cpp | 10 ++++++++++ src/Magnum/GL/Implementation/TextureState.h | 7 +++++++ 5 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/Magnum/GL/AbstractTexture.cpp b/src/Magnum/GL/AbstractTexture.cpp index 7c848855b7..052a9de004 100644 --- a/src/Magnum/GL/AbstractTexture.cpp +++ b/src/Magnum/GL/AbstractTexture.cpp @@ -371,6 +371,14 @@ void AbstractTexture::bindImplementationDSAIntelWindows(const GLint textureUnit) else bindImplementationDSA(textureUnit); } #endif + +#ifdef CORRADE_TARGET_APPLE +void AbstractTexture::bindImplementationAppleBufferTextureWorkaround(const GLint textureUnit) { + bindImplementationDefault(textureUnit); + if(_target == GL_TEXTURE_BUFFER) + Context::current().state().texture->bufferTextureBound.set(textureUnit, true); +} +#endif #endif #ifndef MAGNUM_TARGET_GLES2 @@ -526,13 +534,14 @@ void AbstractTexture::bindInternal() { if(textureState.currentTextureUnit != internalTextureUnit) glActiveTexture(GL_TEXTURE0 + (textureState.currentTextureUnit = internalTextureUnit)); - /* Bind the texture to internal unit if not already, update state tracker */ + /* If already bound in given texture unit, nothing to do */ if(textureState.bindings[internalTextureUnit].second == _id) return; - textureState.bindings[internalTextureUnit] = {_target, _id}; - /* Binding the texture finally creates it */ - _flags |= ObjectFlag::Created; - glBindTexture(_target, _id); + /* Update state tracker, bind the texture to the unit. Not directly calling + glBindTexture() here because we may need to include various + platform-specific workarounds (Apple, Intel Windpws) */ + textureState.bindings[internalTextureUnit] = {_target, _id}; + (this->*textureState.bindImplementation)(internalTextureUnit); } #if !defined(MAGNUM_TARGET_GLES) || defined(MAGNUM_TARGET_GLES2) diff --git a/src/Magnum/GL/AbstractTexture.h b/src/Magnum/GL/AbstractTexture.h index 9a064d138a..d381ad2197 100644 --- a/src/Magnum/GL/AbstractTexture.h +++ b/src/Magnum/GL/AbstractTexture.h @@ -551,6 +551,9 @@ class MAGNUM_GL_EXPORT AbstractTexture: public AbstractObject { #ifdef CORRADE_TARGET_WINDOWS void MAGNUM_GL_LOCAL bindImplementationDSAIntelWindows(GLint textureUnit); #endif + #ifdef CORRADE_TARGET_APPLE + void MAGNUM_GL_LOCAL bindImplementationAppleBufferTextureWorkaround(GLint textureUnit); + #endif #endif void MAGNUM_GL_LOCAL parameterImplementationDefault(GLenum parameter, GLint value); diff --git a/src/Magnum/GL/Buffer.cpp b/src/Magnum/GL/Buffer.cpp index 60c89f72bb..5fa02c244b 100644 --- a/src/Magnum/GL/Buffer.cpp +++ b/src/Magnum/GL/Buffer.cpp @@ -553,12 +553,19 @@ bool Buffer::unmapImplementationDSA() { #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS) /* See apple-buffer-texture-detach-on-data-modify for the gory details. */ void Buffer::textureWorkaroundAppleBefore() { - /* Apple "fortunately" supports just 16 texture units, so this doesn't take - too long. */ + /* My Mac Mini reports 80 texture units, which means this thing can have a + pretty significant overhead. Skipping the whole thing if no buffer + texture is known to be bound. */ Implementation::TextureState& textureState = *Context::current().state().texture; + if(textureState.bufferTextureBound.none()) return; for(GLint textureUnit = 0; textureUnit != GLint(textureState.bindings.size()); ++textureUnit) { - std::pair& binding = textureState.bindings[textureUnit]; - if(binding.first != GL_TEXTURE_BUFFER) continue; + /* Checking just + textureState.bindings[textureUnit].first != GL_TEXTURE_BUFFER + isn't enough, as for GL allows to bind different texture types under + the same texture unit. Magnum's state tracker ignores that (as it + would mean having to maintain a state cache of 128 units times 12 + targets) and so this state is tracked separately. */ + if(!textureState.bufferTextureBound[textureUnit]) continue; /* Activate given texture unit if not already active, update state tracker */ @@ -569,7 +576,8 @@ void Buffer::textureWorkaroundAppleBefore() { glBindTexture(GL_TEXTURE_BUFFER, 0); /* libstdc++ since GCC 6.3 can't handle just = {} (ambiguous overload of operator=) */ - binding = std::pair{}; + textureState.bindings[textureUnit] = std::pair{}; + textureState.bufferTextureBound.set(textureUnit, false); } } diff --git a/src/Magnum/GL/Implementation/TextureState.cpp b/src/Magnum/GL/Implementation/TextureState.cpp index 549e336cbc..11a4d4952f 100644 --- a/src/Magnum/GL/Implementation/TextureState.cpp +++ b/src/Magnum/GL/Implementation/TextureState.cpp @@ -488,6 +488,16 @@ TextureState::TextureState(Context& context, std::vector& extension CORRADE_INTERNAL_ASSERT(maxTextureUnits > 0); bindings = Containers::Array>{Containers::ValueInit, std::size_t(maxTextureUnits)}; + #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS) + if(!context.isDriverWorkaroundDisabled("apple-buffer-texture-unbind-on-buffer-modify")) { + CORRADE_INTERNAL_ASSERT(std::size_t(maxTextureUnits) <= decltype(bufferTextureBound)::Size); + /* Assume ARB_multi_bind is not supported, otherwise we'd need to + implement the workaround also for bindMultiImplementation */ + CORRADE_INTERNAL_ASSERT(!context.isExtensionSupported()); + bindImplementation = &AbstractTexture::bindImplementationAppleBufferTextureWorkaround; + } + #endif + #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) /* Allocate image bindings array to hold all possible image units */ #ifndef MAGNUM_TARGET_GLES diff --git a/src/Magnum/GL/Implementation/TextureState.h b/src/Magnum/GL/Implementation/TextureState.h index 7f96a5dc21..07f473efc0 100644 --- a/src/Magnum/GL/Implementation/TextureState.h +++ b/src/Magnum/GL/Implementation/TextureState.h @@ -46,6 +46,10 @@ #endif #endif +#if defined(CORRADE_TARGET_APPLE) && !defined(MAGNUM_TARGET_GLES) +#include "Magnum/Math/BoolVector.h" +#endif + namespace Magnum { namespace GL { namespace Implementation { struct TextureState { @@ -153,6 +157,9 @@ struct TextureState { #endif Containers::Array> bindings; + #if defined(CORRADE_TARGET_APPLE) && !defined(CORRADE_TARGET_IOS) + Math::BoolVector<80> bufferTextureBound; + #endif #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) /* Texture object ID, level, layered, layer, access */ Containers::Array> imageBindings;