From 95b320c17f4a0a505dd9da2362b0b7cc09ac64d8 Mon Sep 17 00:00:00 2001 From: Tom Cooksey Date: Fri, 12 Feb 2010 10:41:50 +0100 Subject: Fix several bugs with GL texture cache Reviewed-By: Trond Autotest: tst_QGL::qglContextDefaultBindTexture --- src/gui/image/qimagepixmapcleanuphooks.cpp | 21 +++++++++++--- src/gui/image/qimagepixmapcleanuphooks_p.h | 3 ++ src/opengl/qgl.cpp | 44 ++++++++++++++---------------- src/opengl/qgl_p.h | 4 +-- tests/auto/qgl/tst_qgl.cpp | 24 ++++++++++------ 5 files changed, 59 insertions(+), 37 deletions(-) diff --git a/src/gui/image/qimagepixmapcleanuphooks.cpp b/src/gui/image/qimagepixmapcleanuphooks.cpp index ace4bb6..517fcb0 100644 --- a/src/gui/image/qimagepixmapcleanuphooks.cpp +++ b/src/gui/image/qimagepixmapcleanuphooks.cpp @@ -122,19 +122,32 @@ void QImagePixmapCleanupHooks::executeImageHooks(qint64 key) qt_image_cleanup_hook_64(key); } -void QImagePixmapCleanupHooks::enableCleanupHooks(const QPixmap &pixmap) -{ - enableCleanupHooks(const_cast(pixmap).data_ptr().data()); -} void QImagePixmapCleanupHooks::enableCleanupHooks(QPixmapData *pixmapData) { pixmapData->is_cached = true; } +void QImagePixmapCleanupHooks::enableCleanupHooks(const QPixmap &pixmap) +{ + enableCleanupHooks(const_cast(pixmap).data_ptr().data()); +} + void QImagePixmapCleanupHooks::enableCleanupHooks(const QImage &image) { const_cast(image).data_ptr()->is_cached = true; } +bool QImagePixmapCleanupHooks::isImageCached(const QImage &image) +{ + return const_cast(image).data_ptr()->is_cached; +} + +bool QImagePixmapCleanupHooks::isPixmapCached(const QPixmap &pixmap) +{ + return const_cast(pixmap).data_ptr().data()->is_cached; +} + + + QT_END_NAMESPACE diff --git a/src/gui/image/qimagepixmapcleanuphooks_p.h b/src/gui/image/qimagepixmapcleanuphooks_p.h index 88dd3a6..eae11f4 100644 --- a/src/gui/image/qimagepixmapcleanuphooks_p.h +++ b/src/gui/image/qimagepixmapcleanuphooks_p.h @@ -72,6 +72,9 @@ public: static void enableCleanupHooks(const QPixmap &pixmap); static void enableCleanupHooks(QPixmapData *pixmapData); + static bool isImageCached(const QImage &image); + static bool isPixmapCached(const QPixmap &pixmap); + // Gets called when a pixmap data is about to be modified: void addPixmapDataModificationHook(_qt_pixmap_cleanup_hook_pmd); diff --git a/src/opengl/qgl.cpp b/src/opengl/qgl.cpp index 0a89412..22b0602 100644 --- a/src/opengl/qgl.cpp +++ b/src/opengl/qgl.cpp @@ -1594,18 +1594,18 @@ QGLTextureCache::QGLTextureCache() Q_ASSERT(qt_gl_texture_cache == 0); qt_gl_texture_cache = this; - QImagePixmapCleanupHooks::instance()->addPixmapDataModificationHook(cleanupTextures); + QImagePixmapCleanupHooks::instance()->addPixmapDataModificationHook(cleanupTexturesForPixampData); QImagePixmapCleanupHooks::instance()->addPixmapDataDestructionHook(cleanupBeforePixmapDestruction); - QImagePixmapCleanupHooks::instance()->addImageHook(imageCleanupHook); + QImagePixmapCleanupHooks::instance()->addImageHook(cleanupTexturesForCacheKey); } QGLTextureCache::~QGLTextureCache() { qt_gl_texture_cache = 0; - QImagePixmapCleanupHooks::instance()->removePixmapDataModificationHook(cleanupTextures); + QImagePixmapCleanupHooks::instance()->removePixmapDataModificationHook(cleanupTexturesForPixampData); QImagePixmapCleanupHooks::instance()->removePixmapDataDestructionHook(cleanupBeforePixmapDestruction); - QImagePixmapCleanupHooks::instance()->removeImageHook(imageCleanupHook); + QImagePixmapCleanupHooks::instance()->removeImageHook(cleanupTexturesForCacheKey); } void QGLTextureCache::insert(QGLContext* ctx, qint64 key, QGLTexture* texture, int cost) @@ -1661,32 +1661,25 @@ QGLTextureCache* QGLTextureCache::instance() a hook that removes textures from the cache when a pixmap/image is deref'ed */ -void QGLTextureCache::imageCleanupHook(qint64 cacheKey) +void QGLTextureCache::cleanupTexturesForCacheKey(qint64 cacheKey) { // ### remove when the GL texture cache becomes thread-safe - if (qApp->thread() != QThread::currentThread()) - return; - QGLTexture *texture = instance()->getTexture(cacheKey); - if (texture && texture->options & QGLContext::MemoryManagedBindOption) + if (qApp->thread() == QThread::currentThread()) { instance()->remove(cacheKey); + Q_ASSERT(instance()->getTexture(cacheKey) == 0); + } } -void QGLTextureCache::cleanupTextures(QPixmapData* pmd) +void QGLTextureCache::cleanupTexturesForPixampData(QPixmapData* pmd) { - // ### remove when the GL texture cache becomes thread-safe - if (qApp->thread() == QThread::currentThread()) { - const qint64 cacheKey = pmd->cacheKey(); - QGLTexture *texture = instance()->getTexture(cacheKey); - if (texture && texture->options & QGLContext::MemoryManagedBindOption) - instance()->remove(cacheKey); - } + cleanupTexturesForCacheKey(pmd->cacheKey()); } void QGLTextureCache::cleanupBeforePixmapDestruction(QPixmapData* pmd) { // Remove any bound textures first: - cleanupTextures(pmd); + cleanupTexturesForPixampData(pmd); #if defined(Q_WS_X11) if (pmd->classId() == QPixmapData::X11Class) { @@ -2087,8 +2080,9 @@ QGLTexture *QGLContextPrivate::bindTexture(const QImage &image, GLenum target, G // NOTE: bindTexture(const QImage&, GLenum, GLint, const qint64, bool) should never return null Q_ASSERT(texture); - if (texture->id > 0) - QImagePixmapCleanupHooks::enableCleanupHooks(image); + // Enable the cleanup hooks for this image so that the texture cache entry is removed when the + // image gets deleted: + QImagePixmapCleanupHooks::enableCleanupHooks(image); return texture; } @@ -2142,6 +2136,7 @@ QGLTexture* QGLContextPrivate::bindTexture(const QImage &image, GLenum target, G int tx_h = qt_next_power_of_two(image.height()); QImage img = image; + if (!(QGLExtensions::glExtensions() & QGLExtensions::NPOTTextures) && !(QGLFormat::openGLVersionFlags() & QGLFormat::OpenGL_ES_Version_2_0) && (target == GL_TEXTURE_2D && (tx_w != image.width() || tx_h != image.height()))) @@ -2310,6 +2305,7 @@ QGLTexture* QGLContextPrivate::bindTexture(const QImage &image, GLenum target, G int cost = img.width()*img.height()*4/1024; QGLTexture *texture = new QGLTexture(q, tx_id, target, options); QGLTextureCache::instance()->insert(q, key, texture, cost); + return texture; } @@ -2426,7 +2422,7 @@ GLuint QGLContext::bindTexture(const QImage &image, GLenum target, GLint format) return 0; Q_D(QGLContext); - QGLTexture *texture = d->bindTexture(image, target, format, false, DefaultBindOption); + QGLTexture *texture = d->bindTexture(image, target, format, DefaultBindOption); return texture->id; } @@ -2566,11 +2562,13 @@ void QGLContext::deleteTexture(GLuint id) for (int i = 0; i < ddsKeys.size(); ++i) { GLuint texture = dds_cache->value(ddsKeys.at(i)); if (id == texture) { - glDeleteTextures(1, &texture); dds_cache->remove(ddsKeys.at(i)); - return; + break; } } + + // Finally, actually delete the texture ID + glDeleteTextures(1, &id); } #ifdef Q_MAC_COMPAT_GL_FUNCTIONS diff --git a/src/opengl/qgl_p.h b/src/opengl/qgl_p.h index da0887e..0d5a54a 100644 --- a/src/opengl/qgl_p.h +++ b/src/opengl/qgl_p.h @@ -540,8 +540,8 @@ public: static QGLTextureCache *instance(); static void deleteIfEmpty(); - static void imageCleanupHook(qint64 cacheKey); - static void cleanupTextures(QPixmapData* pixmap); + static void cleanupTexturesForCacheKey(qint64 cacheKey); + static void cleanupTexturesForPixampData(QPixmapData* pixmap); static void cleanupBeforePixmapDestruction(QPixmapData* pixmap); private: diff --git a/tests/auto/qgl/tst_qgl.cpp b/tests/auto/qgl/tst_qgl.cpp index d37d727..2983af3 100644 --- a/tests/auto/qgl/tst_qgl.cpp +++ b/tests/auto/qgl/tst_qgl.cpp @@ -57,6 +57,8 @@ #ifdef QT_BUILD_INTERNAL #include #include +#include +#include #endif //TESTED_CLASS= @@ -1947,7 +1949,6 @@ void tst_QGL::qglContextDefaultBindTexture() #ifdef QT_BUILD_INTERNAL QGLWidget w; w.makeCurrent(); - QGLContext *ctx = const_cast(w.context()); QImage *boundImage = new QImage(256, 256, QImage::Format_RGB32); @@ -1955,29 +1956,36 @@ void tst_QGL::qglContextDefaultBindTexture() QPixmap *boundPixmap = new QPixmap(256, 256); boundPixmap->fill(Qt::red); - // Check that calling QGLContext::bindTexture with default args adds textures to cache int startCacheItemCount = QGLTextureCache::instance()->size(); + GLuint boundImageTextureId = ctx->bindTexture(*boundImage); GLuint boundPixmapTextureId = ctx->bindTexture(*boundPixmap); + + // Make sure the image & pixmap have been added to the cache: QCOMPARE(QGLTextureCache::instance()->size(), startCacheItemCount+2); + // Make sure the image & pixmap have the is_cached flag set: + QVERIFY(QImagePixmapCleanupHooks::isImageCached(*boundImage)); + QVERIFY(QImagePixmapCleanupHooks::isPixmapCached(*boundPixmap)); + // Make sure the texture IDs returned are valid: QCOMPARE((bool)glIsTexture(boundImageTextureId), GL_TRUE); QCOMPARE((bool)glIsTexture(boundPixmapTextureId), GL_TRUE); - // Make sure the textures are still there after we delete the image/pixmap: + // Make sure the textures are still valid after we delete the image/pixmap: + // Also check that although the textures are left intact, the cache entries are removed: delete boundImage; boundImage = 0; + QCOMPARE((bool)glIsTexture(boundImageTextureId), GL_TRUE); + QCOMPARE(QGLTextureCache::instance()->size(), startCacheItemCount+1); delete boundPixmap; boundPixmap = 0; - QCOMPARE(QGLTextureCache::instance()->size(), startCacheItemCount+2); + QCOMPARE((bool)glIsTexture(boundPixmapTextureId), GL_TRUE); + QCOMPARE(QGLTextureCache::instance()->size(), startCacheItemCount); - // Make sure the textures are deleted from the cache after calling QGLContext::deleteTexture() + // Finally, make sure QGLContext::deleteTexture deletes the texture IDs: ctx->deleteTexture(boundImageTextureId); ctx->deleteTexture(boundPixmapTextureId); - QCOMPARE(QGLTextureCache::instance()->size(), startCacheItemCount); - - // Finally, make sure QGLContext::deleteTexture also deleted the texture IDs: QCOMPARE((bool)glIsTexture(boundImageTextureId), GL_FALSE); QCOMPARE((bool)glIsTexture(boundPixmapTextureId), GL_FALSE); #endif -- cgit v0.12