Browse Source

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).
pull/168/head
Vladimír Vondruš 2 years ago
parent
commit
8f6f4053fc
  1. 4
      doc/changelog.dox
  2. 37
      src/Magnum/GL/Buffer.cpp
  3. 70
      src/Magnum/GL/Test/BufferGLTest.cpp

4
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

37
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

70
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<Extensions::ARB::uniform_buffer_object>())
CORRADE_SKIP(Extensions::ARB::uniform_buffer_object::string() << "is not supported.");
if(Context::current().isExtensionSupported<Extensions::ARB::direct_state_access>())
CORRADE_SKIP(Extensions::ARB::direct_state_access::string() << "is supported, can't test.");
#endif
if(!Context::current().isExtensionSupported<Extensions::KHR::debug>())
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

Loading…
Cancel
Save