From 69e8fd3359a72230d377513b0314e64b5b83b712 Mon Sep 17 00:00:00 2001 From: Rhys Weatherley Date: Thu, 1 Oct 2009 13:02:49 +1000 Subject: Make QGLFramebufferObject crash-proof if QGLContext destroyed first Sometimes it isn't possible to arrange for the QGLFramebufferObject to be destroyed before the QGLContext group in which it was created. Especially during application shutdown or in applications with multiple shared contexts. This change modifies QGLFramebufferObject to use QGLSharedResourceGuard, which ensures that when the last QGLContext in a sharing group is destroyed, any remaining FBO's will revert to !isValid(). It is now safe to destroy the context before the FBO, or the FBO before the context. Unit test included. Reviewed-by: Sarah Smith --- src/opengl/qglframebufferobject.cpp | 58 ++++++++++++++++++++++--------------- src/opengl/qglframebufferobject_p.h | 7 +++-- tests/auto/qgl/tst_qgl.cpp | 28 ++++++++++++++++++ 3 files changed, 67 insertions(+), 26 deletions(-) diff --git a/src/opengl/qglframebufferobject.cpp b/src/opengl/qglframebufferobject.cpp index c728902..3e54b35 100644 --- a/src/opengl/qglframebufferobject.cpp +++ b/src/opengl/qglframebufferobject.cpp @@ -64,7 +64,8 @@ QT_BEGIN_NAMESPACE extern QImage qt_gl_read_framebuffer(const QSize&, bool, bool); -#define QGL_FUNC_CONTEXT QGLContextGroup *ctx = d_ptr->ctx; +#define QGL_FUNC_CONTEXT const QGLContext *ctx = d_ptr->fbo_guard.context(); +#define QGL_FUNCP_CONTEXT const QGLContext *ctx = fbo_guard.context(); #ifndef QT_NO_DEBUG #define QT_RESET_GLERROR() \ @@ -317,7 +318,7 @@ void QGLFBOGLPaintDevice::setFBO(QGLFramebufferObject* f, QGLFramebufferObject::Attachment attachment) { fbo = f; - m_thisFBO = fbo->d_func()->fbo; // This shouldn't be needed + m_thisFBO = fbo->d_func()->fbo(); // This shouldn't be needed // The context that the fbo was created in may not have depth // and stencil buffers, but the fbo itself might. @@ -334,7 +335,7 @@ void QGLFBOGLPaintDevice::ensureActiveTarget() { QGLContext* ctx = const_cast(QGLContext::currentContext()); Q_ASSERT(ctx); - const GLuint fboId = fbo->d_func()->fbo; + const GLuint fboId = fbo->d_func()->fbo(); if (ctx->d_func()->current_fbo != fboId) { ctx->d_func()->current_fbo = fboId; glBindFramebuffer(GL_FRAMEBUFFER_EXT, fboId); @@ -359,6 +360,9 @@ void QGLFBOGLPaintDevice::endPaint() bool QGLFramebufferObjectPrivate::checkFramebufferStatus() const { + QGL_FUNCP_CONTEXT; + if (!ctx) + return false; // Context no longer exists. GLenum status = glCheckFramebufferStatus(GL_FRAMEBUFFER_EXT); switch(status) { case GL_NO_ERROR: @@ -405,11 +409,11 @@ void QGLFramebufferObjectPrivate::init(QGLFramebufferObject *q, const QSize &sz, QGLFramebufferObject::Attachment attachment, GLenum texture_target, GLenum internal_format, GLint samples) { - QGLContext *currentContext = const_cast(QGLContext::currentContext()); - ctx = QGLContextPrivate::contextGroup(currentContext); + QGLContext *ctx = const_cast(QGLContext::currentContext()); + fbo_guard.setContext(ctx); bool ext_detected = (QGLExtensions::glExtensions & QGLExtensions::FramebufferObject); - if (!ext_detected || (ext_detected && !qt_resolve_framebufferobject_extensions(currentContext))) + if (!ext_detected || (ext_detected && !qt_resolve_framebufferobject_extensions(ctx))) return; size = sz; @@ -417,8 +421,10 @@ void QGLFramebufferObjectPrivate::init(QGLFramebufferObject *q, const QSize &sz, // texture dimensions QT_RESET_GLERROR(); // reset error state + GLuint fbo = 0; glGenFramebuffers(1, &fbo); glBindFramebuffer(GL_FRAMEBUFFER_EXT, fbo); + fbo_guard.setId(fbo); glDevice.setFBO(q, attachment); @@ -535,13 +541,14 @@ void QGLFramebufferObjectPrivate::init(QGLFramebufferObject *q, const QSize &sz, fbo_attachment = QGLFramebufferObject::NoAttachment; } - glBindFramebuffer(GL_FRAMEBUFFER_EXT, currentContext->d_ptr->current_fbo); + glBindFramebuffer(GL_FRAMEBUFFER_EXT, ctx->d_ptr->current_fbo); if (!valid) { if (color_buffer) glDeleteRenderbuffers(1, &color_buffer); else glDeleteTextures(1, &texture); glDeleteFramebuffers(1, &fbo); + fbo_guard.setId(0); } QT_CHECK_GLERROR(); @@ -810,19 +817,15 @@ QGLFramebufferObject::~QGLFramebufferObject() delete d->engine; - if (isValid()) { - const QGLContext *oldContext = QGLContext::currentContext(); - bool switchContext = !oldContext || QGLContextPrivate::contextGroup(oldContext) != ctx; - if (switchContext) - const_cast(ctx->context())->makeCurrent(); + if (isValid() && ctx) { + QGLShareContextScope scope(ctx); glDeleteTextures(1, &d->texture); if (d->color_buffer) glDeleteRenderbuffers(1, &d->color_buffer); if (d->depth_stencil_buffer) glDeleteRenderbuffers(1, &d->depth_stencil_buffer); - glDeleteFramebuffers(1, &d->fbo); - if (oldContext && switchContext) - const_cast(oldContext)->makeCurrent(); + GLuint fbo = d->fbo(); + glDeleteFramebuffers(1, &fbo); } } @@ -838,11 +841,16 @@ QGLFramebufferObject::~QGLFramebufferObject() The non-power of two limitation does not apply if the OpenGL version is 2.0 or higher, or if the GL_ARB_texture_non_power_of_two extension is present. + + The framebuffer can also become invalid if the QGLContext that + the framebuffer was created within is destroyed and there are + no other shared contexts that can take over ownership of the + framebuffer. */ bool QGLFramebufferObject::isValid() const { Q_D(const QGLFramebufferObject); - return d->valid; + return d->valid && d->fbo_guard.context(); } /*! @@ -867,15 +875,17 @@ bool QGLFramebufferObject::bind() return false; Q_D(QGLFramebufferObject); QGL_FUNC_CONTEXT; - glBindFramebuffer(GL_FRAMEBUFFER_EXT, d->fbo); + if (!ctx) + return false; // Context no longer exists. + glBindFramebuffer(GL_FRAMEBUFFER_EXT, d->fbo()); d->valid = d->checkFramebufferStatus(); const QGLContext *context = QGLContext::currentContext(); if (d->valid && context) { - Q_ASSERT(QGLContextPrivate::contextGroup(context) == ctx); + Q_ASSERT(QGLContextPrivate::contextGroup(context) == QGLContextPrivate::contextGroup(ctx)); // Save the previous setting to automatically restore in release(). - if (context->d_ptr->current_fbo != d->fbo) { + if (context->d_ptr->current_fbo != d->fbo()) { d->previous_fbo = context->d_ptr->current_fbo; - context->d_ptr->current_fbo = d->fbo; + context->d_ptr->current_fbo = d->fbo(); } } return d->valid; @@ -900,10 +910,12 @@ bool QGLFramebufferObject::release() return false; Q_D(QGLFramebufferObject); QGL_FUNC_CONTEXT; + if (!ctx) + return false; // Context no longer exists. const QGLContext *context = QGLContext::currentContext(); if (context) { - Q_ASSERT(QGLContextPrivate::contextGroup(context) == ctx); + Q_ASSERT(QGLContextPrivate::contextGroup(context) == QGLContextPrivate::contextGroup(ctx)); // Restore the previous setting for stacked framebuffer objects. if (d->previous_fbo != context->d_ptr->current_fbo) { context->d_ptr->current_fbo = d->previous_fbo; @@ -1144,7 +1156,7 @@ int QGLFramebufferObject::metric(PaintDeviceMetric metric) const GLuint QGLFramebufferObject::handle() const { Q_D(const QGLFramebufferObject); - return d->fbo; + return d->fbo(); } /*! \fn int QGLFramebufferObject::devType() const @@ -1175,7 +1187,7 @@ QGLFramebufferObject::Attachment QGLFramebufferObject::attachment() const bool QGLFramebufferObject::isBound() const { Q_D(const QGLFramebufferObject); - return QGLContext::currentContext()->d_ptr->current_fbo == d->fbo; + return QGLContext::currentContext()->d_ptr->current_fbo == d->fbo(); } /*! diff --git a/src/opengl/qglframebufferobject_p.h b/src/opengl/qglframebufferobject_p.h index f80209d..055a752 100644 --- a/src/opengl/qglframebufferobject_p.h +++ b/src/opengl/qglframebufferobject_p.h @@ -127,15 +127,15 @@ private: class QGLFramebufferObjectPrivate { public: - QGLFramebufferObjectPrivate() : depth_stencil_buffer(0), valid(false), ctx(0), previous_fbo(0), engine(0) {} + QGLFramebufferObjectPrivate() : fbo_guard(0), depth_stencil_buffer(0), valid(false), previous_fbo(0), engine(0) {} ~QGLFramebufferObjectPrivate() {} void init(QGLFramebufferObject *q, const QSize& sz, QGLFramebufferObject::Attachment attachment, GLenum internal_format, GLenum texture_target, GLint samples = 0); bool checkFramebufferStatus() const; + QGLSharedResourceGuard fbo_guard; GLuint texture; - GLuint fbo; GLuint depth_stencil_buffer; GLuint color_buffer; GLenum target; @@ -143,10 +143,11 @@ public: QGLFramebufferObjectFormat format; uint valid : 1; QGLFramebufferObject::Attachment fbo_attachment; - QGLContextGroup *ctx; // for Windows extension ptrs GLuint previous_fbo; mutable QPaintEngine *engine; QGLFBOGLPaintDevice glDevice; + + inline GLuint fbo() const { return fbo_guard.id(); } }; diff --git a/tests/auto/qgl/tst_qgl.cpp b/tests/auto/qgl/tst_qgl.cpp index 999d119..8027e9b 100644 --- a/tests/auto/qgl/tst_qgl.cpp +++ b/tests/auto/qgl/tst_qgl.cpp @@ -84,6 +84,7 @@ private slots: void testDontCrashOnDanglingResources(); void replaceClipping(); void clipTest(); + void destroyFBOAfterContext(); }; tst_QGL::tst_QGL() @@ -1720,6 +1721,33 @@ void tst_QGL::clipTest() QCOMPARE(widgetFB, reference); } +void tst_QGL::destroyFBOAfterContext() +{ + if (!QGLFramebufferObject::hasOpenGLFramebufferObjects()) + QSKIP("QGLFramebufferObject not supported on this platform", SkipSingle); + + QGLWidget *glw = new QGLWidget(); + glw->makeCurrent(); + + // No multisample with combined depth/stencil attachment: + QGLFramebufferObjectFormat fboFormat; + fboFormat.setAttachment(QGLFramebufferObject::CombinedDepthStencil); + + // Don't complicate things by using NPOT: + QGLFramebufferObject *fbo = new QGLFramebufferObject(256, 128, fboFormat); + + // The handle should be valid until the context is destroyed. + QVERIFY(fbo->handle() != 0); + QVERIFY(fbo->isValid()); + + delete glw; + + // The handle should now be zero. + QVERIFY(fbo->handle() == 0); + QVERIFY(!fbo->isValid()); + + delete fbo; +} QTEST_MAIN(tst_QGL) #include "tst_qgl.moc" -- cgit v0.12