From 7cf6c57648c67bea42a54c561dc3c1616a305756 Mon Sep 17 00:00:00 2001 From: Laszlo Agocs Date: Wed, 1 Mar 2023 13:51:58 +0100 Subject: [PATCH] Avoid incomplete textures in GL paint engine ...which can happen if the OpenGL implementation reuses IDs so that a glGenTextures results in the same ID that was used by a texture destroyed by glDeleteTextures just before. In this case the lastTextureUsed tracking needs to be aware and invalidate, otherwise the newly created texture is assumed to be already existing (the ID is the same as before after all), and so operations such as setting the minification filter is skipped (which is fatal with OpenGL given that we do not use mipmaps). This is exercised in practice by the drawPixmapFragments test introduced in the previous patch. On Intel graphics it would work, but it does not render correctly with NVIDIA due to how the driver's internal ID handling works. Pick-to: 6.5.0 6.5 6.4 6.2 Change-Id: Ic2771fe9a2dec7da4aa3804d8498dd89e3c18415 Reviewed-by: Eirik Aavitsland --- src/opengl/qopenglpaintengine.cpp | 38 ++++++++++++++++++++++-------- src/opengl/qopenglpaintengine_p.h | 2 +- src/opengl/qopengltexturecache.cpp | 35 ++++++++++++++++----------- src/opengl/qopengltexturecache_p.h | 22 +++++++++++++---- 4 files changed, 67 insertions(+), 30 deletions(-) diff --git a/src/opengl/qopenglpaintengine.cpp b/src/opengl/qopenglpaintengine.cpp index f78ad265515..972e2763709 100644 --- a/src/opengl/qopenglpaintengine.cpp +++ b/src/opengl/qopenglpaintengine.cpp @@ -160,10 +160,14 @@ template void QOpenGL2PaintEngineExPrivate::updateTexture(GLenum textureUnit, const T &texture, GLenum wrapMode, GLenum filterMode, TextureUpdateMode updateMode) { static const GLenum target = GL_TEXTURE_2D; + bool newTextureCreated = false; activateTextureUnit(textureUnit); - GLuint textureId = bindTexture(texture); + GLuint textureId = bindTexture(texture, &newTextureCreated); + + if (newTextureCreated) + lastTextureUsed = GLuint(-1); if (updateMode == UpdateIfNeeded && textureId == lastTextureUsed) return; @@ -192,8 +196,11 @@ void QOpenGL2PaintEngineExPrivate::activateTextureUnit(GLenum textureUnit) } template<> -GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const GLuint &textureId) +GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const GLuint &textureId, bool *newTextureCreated) { + if (newTextureCreated) + *newTextureCreated = false; + if (textureId != lastTextureUsed) funcs.glBindTexture(GL_TEXTURE_2D, textureId); @@ -201,19 +208,25 @@ GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const GLuint &textureId) } template<> -GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const QImage &image) +GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const QImage &image, bool *newTextureCreated) { - return QOpenGLTextureCache::cacheForContext(ctx)->bindTexture(ctx, image); + QOpenGLTextureCache::BindResult result = QOpenGLTextureCache::cacheForContext(ctx)->bindTexture(ctx, image); + if (newTextureCreated) + *newTextureCreated = result.flags.testFlag(QOpenGLTextureCache::BindResultFlag::NewTexture); + return result.id; } template<> -GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const QPixmap &pixmap) +GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const QPixmap &pixmap, bool *newTextureCreated) { - return QOpenGLTextureCache::cacheForContext(ctx)->bindTexture(ctx, pixmap); + QOpenGLTextureCache::BindResult result = QOpenGLTextureCache::cacheForContext(ctx)->bindTexture(ctx, pixmap); + if (newTextureCreated) + *newTextureCreated = result.flags.testFlag(QOpenGLTextureCache::BindResultFlag::NewTexture); + return result.id; } template<> -GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const QGradient &gradient) +GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const QGradient &gradient, bool *newTextureCreated) { // We apply global opacity in the fragment shaders, so we always pass 1.0 // for opacity to the cache. @@ -223,7 +236,7 @@ GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const QGradient &gradient) // hasn't been cached yet, but will otherwise return an unbound texture id. To // be sure that the texture is bound, we unfortunately have to bind again, // which results in the initial generation of the texture doing two binds. - return bindTexture(textureId); + return bindTexture(textureId, newTextureCreated); } struct ImageWithBindOptions @@ -233,9 +246,14 @@ struct ImageWithBindOptions }; template<> -GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const ImageWithBindOptions &imageWithOptions) +GLuint QOpenGL2PaintEngineExPrivate::bindTexture(const ImageWithBindOptions &imageWithOptions, bool *newTextureCreated) { - return QOpenGLTextureCache::cacheForContext(ctx)->bindTexture(ctx, imageWithOptions.image, imageWithOptions.options); + QOpenGLTextureCache::BindResult result = QOpenGLTextureCache::cacheForContext(ctx)->bindTexture(ctx, + imageWithOptions.image, + imageWithOptions.options); + if (newTextureCreated) + *newTextureCreated = result.flags.testFlag(QOpenGLTextureCache::BindResultFlag::NewTexture); + return result.id; } inline static bool isPowerOfTwo(int x) diff --git a/src/opengl/qopenglpaintengine_p.h b/src/opengl/qopenglpaintengine_p.h index 5d2bbbeb116..40ed7fe6099 100644 --- a/src/opengl/qopenglpaintengine_p.h +++ b/src/opengl/qopenglpaintengine_p.h @@ -177,7 +177,7 @@ public: template void updateTexture(GLenum textureUnit, const T &texture, GLenum wrapMode, GLenum filterMode, TextureUpdateMode updateMode = UpdateIfNeeded); template - GLuint bindTexture(const T &texture); + GLuint bindTexture(const T &texture, bool *newTextureCreated); void activateTextureUnit(GLenum textureUnit); void resetGLState(); diff --git a/src/opengl/qopengltexturecache.cpp b/src/opengl/qopengltexturecache.cpp index 88bd280f7a3..64583c03a97 100644 --- a/src/opengl/qopengltexturecache.cpp +++ b/src/opengl/qopengltexturecache.cpp @@ -81,10 +81,12 @@ QOpenGLTextureCache::~QOpenGLTextureCache() { } -GLuint QOpenGLTextureCache::bindTexture(QOpenGLContext *context, const QPixmap &pixmap, QOpenGLTextureUploader::BindOptions options) +QOpenGLTextureCache::BindResult QOpenGLTextureCache::bindTexture(QOpenGLContext *context, + const QPixmap &pixmap, + QOpenGLTextureUploader::BindOptions options) { if (pixmap.isNull()) - return 0; + return { 0, {} }; QMutexLocker locker(&m_mutex); qint64 key = pixmap.cacheKey(); @@ -93,21 +95,23 @@ GLuint QOpenGLTextureCache::bindTexture(QOpenGLContext *context, const QPixmap & QOpenGLCachedTexture *entry = m_cache.object(key); if (entry && entry->options() == options) { context->functions()->glBindTexture(GL_TEXTURE_2D, entry->id()); - return entry->id(); + return { entry->id(), {} }; } } - GLuint id = bindTexture(context, key, pixmap.toImage(), options); - if (id > 0) + BindResult result = bindTexture(context, key, pixmap.toImage(), options); + if (result.id > 0) QImagePixmapCleanupHooks::enableCleanupHooks(pixmap); - return id; + return result; } -GLuint QOpenGLTextureCache::bindTexture(QOpenGLContext *context, const QImage &image, QOpenGLTextureUploader::BindOptions options) +QOpenGLTextureCache::BindResult QOpenGLTextureCache::bindTexture(QOpenGLContext *context, + const QImage &image, + QOpenGLTextureUploader::BindOptions options) { if (image.isNull()) - return 0; + return { 0, {} }; QMutexLocker locker(&m_mutex); qint64 key = image.cacheKey(); @@ -116,7 +120,7 @@ GLuint QOpenGLTextureCache::bindTexture(QOpenGLContext *context, const QImage &i QOpenGLCachedTexture *entry = m_cache.object(key); if (entry && entry->options() == options) { context->functions()->glBindTexture(GL_TEXTURE_2D, entry->id()); - return entry->id(); + return { entry->id(), {} }; } } @@ -124,17 +128,20 @@ GLuint QOpenGLTextureCache::bindTexture(QOpenGLContext *context, const QImage &i if (!context->functions()->hasOpenGLFeature(QOpenGLFunctions::NPOTTextures)) options |= QOpenGLTextureUploader::PowerOfTwoBindOption; - GLuint id = bindTexture(context, key, img, options); - if (id > 0) + BindResult result = bindTexture(context, key, img, options); + if (result.id > 0) QImagePixmapCleanupHooks::enableCleanupHooks(image); - return id; + return result; } Q_TRACE_POINT(qtopengl, QOpenGLTextureCache_bindTexture_entry, QOpenGLContext *context, qint64 key, const unsigned char *image, int options); Q_TRACE_POINT(qtopengl, QOpenGLTextureCache_bindTexture_exit); -GLuint QOpenGLTextureCache::bindTexture(QOpenGLContext *context, qint64 key, const QImage &image, QOpenGLTextureUploader::BindOptions options) +QOpenGLTextureCache::BindResult QOpenGLTextureCache::bindTexture(QOpenGLContext *context, + qint64 key, + const QImage &image, + QOpenGLTextureUploader::BindOptions options) { Q_TRACE_SCOPE(QOpenGLTextureCache_bindTexture, context, key, image.bits(), options); @@ -147,7 +154,7 @@ GLuint QOpenGLTextureCache::bindTexture(QOpenGLContext *context, qint64 key, con m_cache.insert(key, new QOpenGLCachedTexture(id, options, context), cost / 1024); - return id; + return { id, BindResultFlag::NewTexture }; } void QOpenGLTextureCache::invalidate(qint64 key) diff --git a/src/opengl/qopengltexturecache_p.h b/src/opengl/qopengltexturecache_p.h index afc12f0b383..b70520aa1c8 100644 --- a/src/opengl/qopengltexturecache_p.h +++ b/src/opengl/qopengltexturecache_p.h @@ -35,10 +35,20 @@ public: QOpenGLTextureCache(QOpenGLContext *); ~QOpenGLTextureCache(); - GLuint bindTexture(QOpenGLContext *context, const QPixmap &pixmap, - QOpenGLTextureUploader::BindOptions options = QOpenGLTextureUploader::PremultipliedAlphaBindOption); - GLuint bindTexture(QOpenGLContext *context, const QImage &image, - QOpenGLTextureUploader::BindOptions options = QOpenGLTextureUploader::PremultipliedAlphaBindOption); + enum class BindResultFlag : quint8 { + NewTexture = 0x01 + }; + Q_DECLARE_FLAGS(BindResultFlags, BindResultFlag) + + struct BindResult { + GLuint id; + BindResultFlags flags; + }; + + BindResult bindTexture(QOpenGLContext *context, const QPixmap &pixmap, + QOpenGLTextureUploader::BindOptions options = QOpenGLTextureUploader::PremultipliedAlphaBindOption); + BindResult bindTexture(QOpenGLContext *context, const QImage &image, + QOpenGLTextureUploader::BindOptions options = QOpenGLTextureUploader::PremultipliedAlphaBindOption); void invalidate(qint64 key); @@ -46,12 +56,14 @@ public: void freeResource(QOpenGLContext *ctx) override; private: - GLuint bindTexture(QOpenGLContext *context, qint64 key, const QImage &image, QOpenGLTextureUploader::BindOptions options); + BindResult bindTexture(QOpenGLContext *context, qint64 key, const QImage &image, QOpenGLTextureUploader::BindOptions options); QMutex m_mutex; QCache m_cache; }; +Q_DECLARE_OPERATORS_FOR_FLAGS(QOpenGLTextureCache::BindResultFlags) + class QOpenGLCachedTexture { public: