From 8f6f4053fcd7893511a748df35e9cda881d390bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 22 Nov 2023 17:15:33 +0100 Subject: [PATCH] GL: properly track buffer binding affected by base/range bind calls. The test added in previous commit now passes. Besides the behavior being different for single- and multi-bind calls, an additional "interesting" behavior is that the glBindBufferBase() / glBindBufferRange() apparently also creates the GL object, if not already (in contrast to the multi-bind APIs which *require* the GL objects to be created before). --- doc/changelog.dox | 4 ++ src/Magnum/GL/Buffer.cpp | 37 +++++++++++++++ src/Magnum/GL/Test/BufferGLTest.cpp | 70 +++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+) diff --git a/doc/changelog.dox b/doc/changelog.dox index ad1f9b5e0..b0e1d9273 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -1017,6 +1017,10 @@ See also: @subsection changelog-latest-bugfixes Bug fixes +- The state tracker didn't correctly recognize the "base" / "range" + @ref GL::Buffer::bind() call as affecting also the regular binding point, + leading to wrong buffer object being used for data upload etc. in certain + cases on platforms without DSA extensions. - @ref GL::Context move constructor was not marked @cpp noexcept @ce by accident and it was also not really moving everything properly, especially when delayed creation was done on the moved-to object diff --git a/src/Magnum/GL/Buffer.cpp b/src/Magnum/GL/Buffer.cpp index 2d4d3c39c..b426315a9 100644 --- a/src/Magnum/GL/Buffer.cpp +++ b/src/Magnum/GL/Buffer.cpp @@ -302,11 +302,44 @@ auto Buffer::bindSomewhereInternal(const TargetHint hint) -> TargetHint { #ifndef MAGNUM_TARGET_GLES2 Buffer& Buffer::bind(const Target target, const UnsignedInt index, const GLintptr offset, const GLsizeiptr size) { + /* glBindBufferBase() and glBindBufferRange() bind the buffer to the + "regular" binding target as a side effect: + https://registry.khronos.org/OpenGL-Refpages/gl4/html/glBindBufferBase.xhtml + So update the state tracker to be aware of that. Apart from saving a + needless rebind in some cases, this also prevents an inverse case where + it would think a buffer is bound and won't call glBindBuffer() for it, + causing for example a (DSA-less) data upload to happen to some entirely + different buffer. In comparison, the multi-bind APIs don't have this + side effect: + https://registry.khronos.org/OpenGL-Refpages/gl4/html/glBindBuffersBase.xhtml + + See BufferGLTest::bindBaseRangeUpdateRegularBinding() for a + corresponding test case. */ + Context::current().state().buffer.bindings[Implementation::BufferState::indexForTarget( + /* The Target enum is a subset of TargetHint and the values match, so + no expensive translation is necessary */ + TargetHint(UnsignedInt(target)))] = _id; + /* As the "regular" binding target is a side effect, I assume it also + creates the object internally if not already, equivalently to + glBindBuffer() and to what the createIfNotAlready() call does, and + satisfying the internal assertion there which in turn expects that if a + buffer is in the bindings state tracker array, it also has the Created + flag set. */ + _flags |= ObjectFlag::Created; + glBindBufferRange(GLenum(target), index, _id, offset, size); return *this; } Buffer& Buffer::bind(const Target target, const UnsignedInt index) { + /* Same as in bind(Target, UnsignedInt, GLintptr, GLsizeiptr) above */ + Context::current().state().buffer.bindings[Implementation::BufferState::indexForTarget( + /* The Target enum is a subset of TargetHint and the values match, so + no expensive translation is necessary */ + TargetHint(UnsignedInt(target)))] = _id; + /* Ditto */ + _flags |= ObjectFlag::Created; + glBindBufferBase(GLenum(target), index, _id); return *this; } @@ -405,6 +438,10 @@ void Buffer::bindImplementationMulti(const Target target, const GLuint firstInde } } + /* Unlike Buffer::bind(Target, UnsignedInt) this doesn't affect the regular + binding points: + https://registry.khronos.org/OpenGL-Refpages/gl4/html/glBindBuffersBase.xhtml + See the comment in that function for details. */ glBindBuffersBase(GLenum(target), firstIndex, buffers.size(), ids); } #endif diff --git a/src/Magnum/GL/Test/BufferGLTest.cpp b/src/Magnum/GL/Test/BufferGLTest.cpp index 6d8d9db4c..a1127bfad 100644 --- a/src/Magnum/GL/Test/BufferGLTest.cpp +++ b/src/Magnum/GL/Test/BufferGLTest.cpp @@ -57,6 +57,9 @@ struct BufferGLTest: OpenGLTester { void bindBase(); void bindRange(); void bindBaseRangeUpdateRegularBinding(); + #ifndef MAGNUM_TARGET_WEBGL + void bindBaseRangeCreatesObject(); + #endif #endif #ifndef MAGNUM_TARGET_GLES @@ -87,6 +90,16 @@ const struct { {"bind ranges", true, true}, }; +#ifndef MAGNUM_TARGET_WEBGL +const struct { + const char* name; + bool multi; +} BindBaseRangeCreatesObjectData[]{ + {"bind base", false}, + {"bind bases", true}, +}; +#endif + BufferGLTest::BufferGLTest() { addTests({&BufferGLTest::construct, &BufferGLTest::constructFromData, @@ -106,6 +119,11 @@ BufferGLTest::BufferGLTest() { #ifndef MAGNUM_TARGET_GLES2 addInstancedTests({&BufferGLTest::bindBaseRangeUpdateRegularBinding}, Containers::arraySize(BindBaseRangeUpdateRegularBindingData)); + + #ifndef MAGNUM_TARGET_WEBGL + addInstancedTests({&BufferGLTest::bindBaseRangeCreatesObject}, + Containers::arraySize(BindBaseRangeCreatesObjectData)); + #endif #endif addTests({ @@ -333,6 +351,58 @@ void BufferGLTest::bindBaseRangeUpdateRegularBinding() { MAGNUM_VERIFY_NO_GL_ERROR(); } + +#ifndef MAGNUM_TARGET_WEBGL +void BufferGLTest::bindBaseRangeCreatesObject() { + auto&& data = BindBaseRangeCreatesObjectData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + #ifndef MAGNUM_TARGET_GLES + if(!Context::current().isExtensionSupported()) + CORRADE_SKIP(Extensions::ARB::uniform_buffer_object::string() << "is not supported."); + if(Context::current().isExtensionSupported()) + CORRADE_SKIP(Extensions::ARB::direct_state_access::string() << "is supported, can't test."); + #endif + if(!Context::current().isExtensionSupported()) + CORRADE_SKIP(Extensions::KHR::debug::string() << "is not supported."); + + Buffer buffer; + + /* The glGenBuffers() API doesn't actually create a buffer object, creation + only happens on the first glBindBuffer(). The DSA glCreateBuffer() API + combines the two, and then some DSA APIs that take just an object ID + such as glObjectLabel() require the object to be created. + + As the glBindBufferBase() / glBindBufferRange() binds the buffer to the + regular binding point as a side effect, the implementation assumes it + also performs the creation, and so sets the ObjectFlag::Created flag. To + verify that, the glObjectLabel() call should then work without a GL + error. + + On the other hand, the multi-bind APIs *don't* bind the buffer to the + regular binding point, but conversely require the objects to be created. + So for these, the multi-bind is actually internally preceded by an + explicit glBindBuffer() that creates the buffer if not already. Calling + the multi-bind variant here just to be sure it all works as intended. + + Also, only the "base" binding APIs are tested here, the range APIs fail + on an error because size of 0 is not an allowed value. The + implementation and ObjectFlag::Created flag setting however behaves the + same for both for consistency, even though it's impossible to test. */ + + if(data.multi) + Buffer::bind(Buffer::Target::Uniform, 0, {&buffer}); + else + buffer.bind(Buffer::Target::Uniform, 0); + + MAGNUM_VERIFY_NO_GL_ERROR(); + + buffer.setLabel("hello"); + + MAGNUM_VERIFY_NO_GL_ERROR(); + CORRADE_COMPARE(buffer.label(), "hello"); +} +#endif #endif #ifndef MAGNUM_TARGET_GLES