From 0d4b66789b844c4d4199794caa7667e6a1c82557 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 17 Aug 2014 15:58:43 +0200 Subject: [PATCH] Ensure that the framebuffer object is properly created before using it. --- src/Magnum/AbstractFramebuffer.cpp | 19 +++++++++++++++++++ src/Magnum/AbstractFramebuffer.h | 3 +++ src/Magnum/DefaultFramebuffer.cpp | 5 ++++- src/Magnum/Framebuffer.cpp | 9 ++++++++- src/Magnum/Framebuffer.h | 4 +++- src/Magnum/Test/FramebufferGLTest.cpp | 12 ------------ 6 files changed, 37 insertions(+), 15 deletions(-) diff --git a/src/Magnum/AbstractFramebuffer.cpp b/src/Magnum/AbstractFramebuffer.cpp index 2bf1f5b97..03b5ab92c 100644 --- a/src/Magnum/AbstractFramebuffer.cpp +++ b/src/Magnum/AbstractFramebuffer.cpp @@ -82,6 +82,17 @@ Int AbstractFramebuffer::maxDualSourceDrawBuffers() { } #endif +void AbstractFramebuffer::createIfNotAlready() { + if(_created) return; + + /* glGen*() does not create the object, just reserves the name. Some + commands (such as glObjectLabel()) operate with IDs directly and they + require the object to be created. Binding the framebuffer finally + creates it. Also all EXT DSA functions implicitly create it. */ + bindInternal(); + CORRADE_INTERNAL_ASSERT(_created); +} + void AbstractFramebuffer::bind(FramebufferTarget target) { bindInternal(target); setViewportInternal(); @@ -102,6 +113,8 @@ void AbstractFramebuffer::bindInternal(FramebufferTarget target) { state->readBinding = state->drawBinding = _id; } else CORRADE_ASSERT_UNREACHABLE(); + /* Binding the framebuffer finally creates it */ + _created = true; glBindFramebuffer(GLenum(target), _id); } @@ -119,6 +132,8 @@ FramebufferTarget AbstractFramebuffer::bindInternal() { /* Or bind it, if not already */ state->readBinding = _id; + /* Binding the framebuffer finally creates it */ + _created = true; #ifndef MAGNUM_TARGET_GLES2 glBindFramebuffer(GLenum(FramebufferTarget::Read), _id); return FramebufferTarget::Read; @@ -256,6 +271,7 @@ GLenum AbstractFramebuffer::checkStatusImplementationDefault(const FramebufferTa #ifndef MAGNUM_TARGET_GLES GLenum AbstractFramebuffer::checkStatusImplementationDSA(const FramebufferTarget target) { + _created = true; return glCheckNamedFramebufferStatusEXT(_id, GLenum(target)); } #endif @@ -280,6 +296,7 @@ void AbstractFramebuffer::drawBuffersImplementationDefault(GLsizei count, const #ifndef MAGNUM_TARGET_GLES void AbstractFramebuffer::drawBuffersImplementationDSA(GLsizei count, const GLenum* buffers) { + _created = true; glFramebufferDrawBuffersEXT(_id, count, buffers); } #endif @@ -305,6 +322,7 @@ void AbstractFramebuffer::drawBufferImplementationDefault(GLenum buffer) { #ifndef MAGNUM_TARGET_GLES void AbstractFramebuffer::drawBufferImplementationDSA(GLenum buffer) { + _created = true; glFramebufferDrawBufferEXT(_id, buffer); } #endif @@ -328,6 +346,7 @@ void AbstractFramebuffer::readBufferImplementationDefault(GLenum buffer) { #ifndef MAGNUM_TARGET_GLES void AbstractFramebuffer::readBufferImplementationDSA(GLenum buffer) { + _created = true; glFramebufferReadBufferEXT(_id, buffer); } #endif diff --git a/src/Magnum/AbstractFramebuffer.h b/src/Magnum/AbstractFramebuffer.h index c3c11c26e..dea83a065 100644 --- a/src/Magnum/AbstractFramebuffer.h +++ b/src/Magnum/AbstractFramebuffer.h @@ -329,11 +329,14 @@ class MAGNUM_EXPORT AbstractFramebuffer { explicit AbstractFramebuffer() = default; ~AbstractFramebuffer() = default; + void MAGNUM_LOCAL createIfNotAlready(); + void MAGNUM_LOCAL bindInternal(FramebufferTarget target); FramebufferTarget MAGNUM_LOCAL bindInternal(); void MAGNUM_LOCAL setViewportInternal(); GLuint _id; + bool _created; /* see createIfNotAlready() for details */ Range2Di _viewport; private: diff --git a/src/Magnum/DefaultFramebuffer.cpp b/src/Magnum/DefaultFramebuffer.cpp index 9c9f4991b..034f37cc7 100644 --- a/src/Magnum/DefaultFramebuffer.cpp +++ b/src/Magnum/DefaultFramebuffer.cpp @@ -36,7 +36,10 @@ namespace Magnum { DefaultFramebuffer defaultFramebuffer; -DefaultFramebuffer::DefaultFramebuffer() { _id = 0; } +DefaultFramebuffer::DefaultFramebuffer() { + _id = 0; + _created = true; +} DefaultFramebuffer::Status DefaultFramebuffer::checkStatus(const FramebufferTarget target) { return Status((this->*Context::current()->state().framebuffer->checkStatusImplementation)(target)); diff --git a/src/Magnum/Framebuffer.cpp b/src/Magnum/Framebuffer.cpp index fefb79bfa..a1357979f 100644 --- a/src/Magnum/Framebuffer.cpp +++ b/src/Magnum/Framebuffer.cpp @@ -81,6 +81,7 @@ Int Framebuffer::maxColorAttachments() { Framebuffer::Framebuffer(const Range2Di& viewport) { _viewport = viewport; + _created = false; glGenFramebuffers(1, &_id); CORRADE_INTERNAL_ASSERT(_id != Implementation::State::DisengagedBinding); @@ -108,11 +109,13 @@ Framebuffer::~Framebuffer() { glDeleteFramebuffers(1, &_id); } -std::string Framebuffer::label() const { +std::string Framebuffer::label() { + createIfNotAlready(); return Context::current()->state().debug->getLabelImplementation(GL_FRAMEBUFFER, _id); } Framebuffer& Framebuffer::setLabelInternal(const Containers::ArrayReference label) { + createIfNotAlready(); Context::current()->state().debug->labelImplementation(GL_FRAMEBUFFER, _id, label); return *this; } @@ -255,6 +258,7 @@ void Framebuffer::renderbufferImplementationDefault(BufferAttachment attachment, #ifndef MAGNUM_TARGET_GLES void Framebuffer::renderbufferImplementationDSA(BufferAttachment attachment, Renderbuffer& renderbuffer) { + _created = true; glNamedFramebufferRenderbufferEXT(_id, GLenum(attachment), GL_RENDERBUFFER, renderbuffer.id()); } @@ -263,6 +267,7 @@ void Framebuffer::texture1DImplementationDefault(BufferAttachment attachment, GL } void Framebuffer::texture1DImplementationDSA(BufferAttachment attachment, GLuint textureId, GLint mipLevel) { + _created = true; glNamedFramebufferTexture1DEXT(_id, GLenum(attachment), GL_TEXTURE_1D, textureId, mipLevel); } #endif @@ -273,6 +278,7 @@ void Framebuffer::texture2DImplementationDefault(BufferAttachment attachment, GL #ifndef MAGNUM_TARGET_GLES void Framebuffer::texture2DImplementationDSA(BufferAttachment attachment, GLenum textureTarget, GLuint textureId, GLint mipLevel) { + _created = true; glNamedFramebufferTexture2DEXT(_id, GLenum(attachment), textureTarget, textureId, mipLevel); } #endif @@ -293,6 +299,7 @@ void Framebuffer::textureLayerImplementationDefault(BufferAttachment attachment, #ifndef MAGNUM_TARGET_GLES void Framebuffer::textureLayerImplementationDSA(BufferAttachment attachment, GLuint textureId, GLint mipLevel, GLint layer) { + _created = true; glNamedFramebufferTextureLayerEXT(_id, GLenum(attachment), textureId, mipLevel, layer); } #endif diff --git a/src/Magnum/Framebuffer.h b/src/Magnum/Framebuffer.h index e5d7b76da..c264aa0bf 100644 --- a/src/Magnum/Framebuffer.h +++ b/src/Magnum/Framebuffer.h @@ -341,7 +341,7 @@ class MAGNUM_EXPORT Framebuffer: public AbstractFramebuffer, public AbstractObje * @fn_gl_extension2{GetObjectLabel,EXT,debug_label} with * @def_gl{FRAMEBUFFER} */ - std::string label() const; + std::string label(); /** * @brief Set framebuffer label @@ -664,6 +664,7 @@ Debug MAGNUM_EXPORT operator<<(Debug debug, Framebuffer::Status value); inline Framebuffer::Framebuffer(Framebuffer&& other) noexcept { _id = other._id; _viewport = other._viewport; + _created = other._created; other._id = 0; other._viewport = {}; } @@ -671,6 +672,7 @@ inline Framebuffer::Framebuffer(Framebuffer&& other) noexcept { inline Framebuffer& Framebuffer::operator=(Framebuffer&& other) noexcept { std::swap(_id, other._id); std::swap(_viewport, other._viewport); + std::swap(_created, other._created); return *this; } diff --git a/src/Magnum/Test/FramebufferGLTest.cpp b/src/Magnum/Test/FramebufferGLTest.cpp index e4c897ae1..93970d89f 100644 --- a/src/Magnum/Test/FramebufferGLTest.cpp +++ b/src/Magnum/Test/FramebufferGLTest.cpp @@ -215,19 +215,7 @@ void FramebufferGLTest::label() { !Context::current()->isExtensionSupported()) CORRADE_SKIP("Required extension is not available"); - { - /** @todo Is this even legal optimization? */ - CORRADE_EXPECT_FAIL("The object must be used at least once before setting/querying label."); - CORRADE_VERIFY(false); - } - Renderbuffer renderbuffer; - #ifndef MAGNUM_TARGET_GLES2 - renderbuffer.setStorage(RenderbufferFormat::RGBA8, {128, 128}); - #else - renderbuffer.setStorage(RenderbufferFormat::RGBA4, {128, 128}); - #endif Framebuffer framebuffer({{}, Vector2i(32)}); - framebuffer.attachRenderbuffer(Framebuffer::BufferAttachment(Framebuffer::BufferAttachment::Depth), renderbuffer); CORRADE_COMPARE(framebuffer.label(), ""); MAGNUM_VERIFY_NO_ERROR();