From cb2a108d79b09ec6ea91106e7f33826dceac4a42 Mon Sep 17 00:00:00 2001 From: Charles Yin Date: Wed, 27 Oct 2010 12:05:52 +1000 Subject: Fix QTBUG-14132 oracle (xe) stored procedures with bind variables get errors Strange Oracle bug: some Oracle servers crash the server process with non-zero error handle (mostly for 10g). So pass null error handle pointer to OCIAttrSet when setting OCI_ATTR_CHARSET_FORM to bypass this bug. Change-Id: Ifcf65994a22783e69a89bebe1adb882cdf36bdbb Task-number:QTBUG-14132 Reviewed-by: Michael Goddard --- src/sql/drivers/oci/qsql_oci.cpp | 106 +++++++++++++-------------------- tests/auto/qsqlquery/tst_qsqlquery.cpp | 21 +++++++ 2 files changed, 63 insertions(+), 64 deletions(-) diff --git a/src/sql/drivers/oci/qsql_oci.cpp b/src/sql/drivers/oci/qsql_oci.cpp index e11cf75..3f30ea5 100644 --- a/src/sql/drivers/oci/qsql_oci.cpp +++ b/src/sql/drivers/oci/qsql_oci.cpp @@ -93,11 +93,13 @@ enum { QOCIEncoding = 2002 }; // AL16UTF16LE enum { QOCIEncoding = 2000 }; // AL16UTF16 #endif +#ifdef OCI_ATTR_CHARSET_FORM // Always set the OCI_ATTR_CHARSET_FORM to SQLCS_NCHAR is safe // because Oracle server will deal with the implicit Conversion // Between CHAR and NCHAR. // see: http://download.oracle.com/docs/cd/A91202_01/901_doc/appdev.901/a89857/oci05bnd.htm#422705 static const ub1 qOraCharsetForm = SQLCS_NCHAR; +#endif #if defined (OCI_UTF16ID) static const ub2 qOraCharset = OCI_UTF16ID; @@ -117,6 +119,8 @@ static // for some reason, Sun CC can't use qOraWarning when it's declared stati void qOraWarning(const char* msg, OCIError *err); static QSqlError qMakeError(const QString& errString, QSqlError::ErrorType type, OCIError *err); + + class QOCIRowId: public QSharedData { public: @@ -164,7 +168,6 @@ struct QOCIResultPrivate int serverVersion; int prefetchRows, prefetchMem; - void setCharset(OCIBind* hbnd); void setStatementAttributes(); int bindValue(OCIStmt *sql, OCIBind **hbnd, OCIError *err, int pos, const QVariant &val, dvoid *indPtr, ub2 *tmpSize, QList &tmpStorage); @@ -176,6 +179,41 @@ struct QOCIResultPrivate { return q->bindValueType(i) & QSql::Out; } inline bool isBinaryValue(int i) const { return q->bindValueType(i) & QSql::Binary; } + + void setCharset(dvoid* handle, ub4 type) const + { + int r = 0; + Q_ASSERT(handle); + +#ifdef OCI_ATTR_CHARSET_FORM + r = OCIAttrSet(handle, + type, + // this const cast is safe since OCI doesn't touch + // the charset. + const_cast(static_cast(&qOraCharsetForm)), + 0, + OCI_ATTR_CHARSET_FORM, + //Strange Oracle bug: some Oracle servers crash the server process with non-zero error handle (mostly for 10g). + //So ignore the error message here. + 0); + #ifdef QOCI_DEBUG + if (r != 0) + qWarning("QOCIResultPrivate::setCharset: Couldn't set OCI_ATTR_CHARSET_FORM."); + #endif +#endif + + r = OCIAttrSet(handle, + type, + // this const cast is safe since OCI doesn't touch + // the charset. + const_cast(static_cast(&qOraCharset)), + 0, + OCI_ATTR_CHARSET_ID, + err); + if (r != 0) + qOraWarning("QOCIResultPrivate::setCharsetI Couldn't set OCI_ATTR_CHARSET_ID: ", err); + + } }; void QOCIResultPrivate::setStatementAttributes() @@ -208,36 +246,6 @@ void QOCIResultPrivate::setStatementAttributes() } } -void QOCIResultPrivate::setCharset(OCIBind* hbnd) -{ - int r = 0; - - Q_ASSERT(hbnd); - - r = OCIAttrSet(hbnd, - OCI_HTYPE_BIND, - // this const cast is safe since OCI doesn't touch - // the charset. - const_cast(static_cast(&qOraCharsetForm)), - 0, - OCI_ATTR_CHARSET_FORM, - err); - if (r != 0) - qOraWarning("QOCIResultPrivate::setCharset: Couldn't set OCI_ATTR_CHARSET_FORM: ", err); - - r = OCIAttrSet(hbnd, - OCI_HTYPE_BIND, - // this const cast is safe since OCI doesn't touch - // the charset. - const_cast(static_cast(&qOraCharset)), - 0, - OCI_ATTR_CHARSET_ID, - err); - if (r != 0) - qOraWarning("QOCIResultPrivate::setCharset: Couldn't set OCI_ATTR_CHARSET_ID: ", err); - -} - int QOCIResultPrivate::bindValue(OCIStmt *sql, OCIBind **hbnd, OCIError *err, int pos, const QVariant &val, dvoid *indPtr, ub2 *tmpSize, QList &tmpStorage) { @@ -325,7 +333,7 @@ int QOCIResultPrivate::bindValue(OCIStmt *sql, OCIBind **hbnd, OCIError *err, in (s.length() + 1) * sizeof(QChar), SQLT_STR, indPtr, 0, 0, 0, 0, OCI_DEFAULT); if (r == OCI_SUCCESS) - setCharset(*hbnd); + setCharset(*hbnd, OCI_HTYPE_BIND); break; } } // fall through for OUT values @@ -349,7 +357,7 @@ int QOCIResultPrivate::bindValue(OCIStmt *sql, OCIBind **hbnd, OCIError *err, in SQLT_STR, indPtr, 0, 0, 0, 0, OCI_DEFAULT); } if (r == OCI_SUCCESS) - setCharset(*hbnd); + setCharset(*hbnd, OCI_HTYPE_BIND); tmpStorage.append(ba); break; } // default case @@ -688,7 +696,6 @@ class QOCICols public: QOCICols(int size, QOCIResultPrivate* dp); ~QOCICols(); - void setCharset(OCIDefine* dfn); int readPiecewise(QVector &values, int index = 0); int readLOBs(QVector &values, int index = 0); int fieldFromDefine(OCIDefine* d); @@ -890,7 +897,7 @@ QOCICols::QOCICols(int size, QOCIResultPrivate* dp) &(fieldInf[idx].ind), 0, 0, OCI_DEFAULT); if (r == 0) - setCharset(dfn); + d->setCharset(dfn, OCI_HTYPE_DEFINE); } break; default: @@ -950,35 +957,6 @@ OCILobLocator **QOCICols::createLobLocator(int position, OCIEnv* env) return &lob; } -void QOCICols::setCharset(OCIDefine* dfn) -{ - int r = 0; - - Q_ASSERT(dfn); - - r = OCIAttrSet(dfn, - OCI_HTYPE_DEFINE, - // this const cast is safe since OCI doesn't touch - // the charset. - const_cast(static_cast(&qOraCharsetForm)), - 0, - OCI_ATTR_CHARSET_FORM, - d->err); - if (r != 0) - qOraWarning("QOCIResultPrivate::setCharset: Couldn't set OCI_ATTR_CHARSET_FORM: ", d->err); - - r = OCIAttrSet(dfn, - OCI_HTYPE_DEFINE, - // this const cast is safe since OCI doesn't touch - // the charset. - const_cast(static_cast(&qOraCharset)), - 0, - OCI_ATTR_CHARSET_ID, - d->err); - if (r != 0) - qOraWarning("QOCICols::setCharset: Couldn't set OCI_ATTR_CHARSET_ID: ", d->err); -} - int QOCICols::readPiecewise(QVector &values, int index) { OCIDefine* dfn; diff --git a/tests/auto/qsqlquery/tst_qsqlquery.cpp b/tests/auto/qsqlquery/tst_qsqlquery.cpp index c7a61a5..7c93c7a 100644 --- a/tests/auto/qsqlquery/tst_qsqlquery.cpp +++ b/tests/auto/qsqlquery/tst_qsqlquery.cpp @@ -209,6 +209,8 @@ private slots: void QTBUG_6852(); void QTBUG_5765_data() { generic_data("QMYSQL"); } void QTBUG_5765(); + void QTBUG_14132_data() { generic_data("QOCI"); } + void QTBUG_14132(); void sqlite_constraint_data() { generic_data("QSQLITE"); } void sqlite_constraint(); @@ -2933,6 +2935,25 @@ void tst_QSqlQuery::QTBUG_551() QCOMPARE(res_outLst[2].toString(), QLatin1String("3. Value is 2")); } +void tst_QSqlQuery::QTBUG_14132() +{ + QFETCH( QString, dbName ); + QSqlDatabase db = QSqlDatabase::database( dbName ); + CHECK_DATABASE( db ); + QSqlQuery q(db); + const QString procedureName(qTableName("procedure", __FILE__)); + QVERIFY_SQL(q, exec("CREATE OR REPLACE PROCEDURE "+ procedureName + " (outStr OUT varchar2) \n\ + is \n\ + begin \n\ + outStr := 'OUTSTRING'; \n\ + end;")); + QString outValue = "XXXXXXXXX"; + q.prepare("CALL "+procedureName+"(?)"); + q.addBindValue(outValue, QSql::Out); + QVERIFY_SQL(q, exec()); + QCOMPARE(outValue, QLatin1String("OUTSTRING")); +} + void tst_QSqlQuery::QTBUG_5251() { QFETCH( QString, dbName ); -- cgit v0.12 From c76f9f15d1c604cdf3d68d2147a4a06d227871fa Mon Sep 17 00:00:00 2001 From: Eskil Abrahamsen Blomfeldt Date: Thu, 28 Oct 2010 09:27:24 +0200 Subject: Fix possible crash in QStaticText and QDeclarativeTextLayout The QStaticTextItem held an uncounted reference to QFontEngine. The pointer would dangle in some cases where there was no font object referencing the engine and the cache was cleaned out (e.g. when a new application font is added.) Properly count the reference, and also add reference counting to userData to make it harder to shoot yourself in the foot, since the QStaticTextItem class is now being used in different places, Task-number: QTBUG-14446 Reviewed-by: Martin Jones --- .../graphicsitems/qdeclarativetextlayout.cpp | 4 +- src/gui/painting/qpaintengine_raster.cpp | 2 +- src/gui/painting/qpainter.cpp | 4 +- src/gui/text/qstatictext.cpp | 22 ++++++++- src/gui/text/qstatictext_p.h | 54 ++++++++++++++++++---- .../gl2paintengineex/qpaintengineex_opengl2.cpp | 24 +++++----- src/opengl/qpaintengine_opengl.cpp | 8 ++-- src/openvg/qpaintengine_vg.cpp | 2 +- 8 files changed, 88 insertions(+), 32 deletions(-) diff --git a/src/declarative/graphicsitems/qdeclarativetextlayout.cpp b/src/declarative/graphicsitems/qdeclarativetextlayout.cpp index 89a2158..635ceab 100644 --- a/src/declarative/graphicsitems/qdeclarativetextlayout.cpp +++ b/src/declarative/graphicsitems/qdeclarativetextlayout.cpp @@ -94,7 +94,7 @@ class DrawTextItemRecorder: public QPaintEngine if (!m_inertText->items.isEmpty()) { QStaticTextItem &last = m_inertText->items[m_inertText->items.count() - 1]; - if (last.fontEngine == ti.fontEngine && last.font == ti.font() && + if (last.fontEngine() == ti.fontEngine && last.font == ti.font() && (!m_dirtyPen || last.color == state->pen().color())) { needFreshCurrentItem = false; @@ -107,7 +107,7 @@ class DrawTextItemRecorder: public QPaintEngine if (needFreshCurrentItem) { QStaticTextItem currentItem; - currentItem.fontEngine = ti.fontEngine; + currentItem.setFontEngine(ti.fontEngine); currentItem.font = ti.font(); currentItem.charOffset = charOffset; currentItem.numChars = ti.num_chars; diff --git a/src/gui/painting/qpaintengine_raster.cpp b/src/gui/painting/qpaintengine_raster.cpp index 8f9d8ba..d4b044b 100644 --- a/src/gui/painting/qpaintengine_raster.cpp +++ b/src/gui/painting/qpaintengine_raster.cpp @@ -3300,7 +3300,7 @@ void QRasterPaintEngine::drawStaticTextItem(QStaticTextItem *textItem) ensureState(); drawCachedGlyphs(textItem->numGlyphs, textItem->glyphs, textItem->glyphPositions, - textItem->fontEngine); + textItem->fontEngine()); } /*! diff --git a/src/gui/painting/qpainter.cpp b/src/gui/painting/qpainter.cpp index 7fed7e4..ab9707d 100644 --- a/src/gui/painting/qpainter.cpp +++ b/src/gui/painting/qpainter.cpp @@ -5746,7 +5746,7 @@ void QPainterPrivate::drawGlyphs(const quint32 *glyphArray, const QPointF *posit QStaticTextItem staticTextItem; staticTextItem.color = state->pen.color(); staticTextItem.font = state->font; - staticTextItem.fontEngine = fontEngine; + staticTextItem.setFontEngine(fontEngine); staticTextItem.numGlyphs = glyphCount; staticTextItem.glyphs = reinterpret_cast(const_cast(glyphArray)); staticTextItem.glyphPositions = positions.data(); @@ -5938,7 +5938,7 @@ void QPainter::drawStaticText(const QPointF &topLeftPosition, const QStaticText d->extended->drawStaticTextItem(item); drawDecorationForGlyphs(this, item->glyphs, item->glyphPositions, - item->numGlyphs, item->fontEngine, staticText_d->font, + item->numGlyphs, item->fontEngine(), staticText_d->font, QTextCharFormat()); } if (currentColor != oldPen.color()) diff --git a/src/gui/text/qstatictext.cpp b/src/gui/text/qstatictext.cpp index 9506006..edf248a 100644 --- a/src/gui/text/qstatictext.cpp +++ b/src/gui/text/qstatictext.cpp @@ -445,7 +445,7 @@ namespace { const QTextItemInt &ti = static_cast(textItem); QStaticTextItem currentItem; - currentItem.fontEngine = ti.fontEngine; + currentItem.setFontEngine(ti.fontEngine); currentItem.font = ti.font(); currentItem.charOffset = m_chars.size(); currentItem.numChars = ti.num_chars; @@ -713,4 +713,24 @@ void QStaticTextPrivate::init() needsRelayout = false; } +QStaticTextItem::~QStaticTextItem() +{ + if (m_userData != 0 && !m_userData->ref.deref()) + delete m_userData; + if (!m_fontEngine->ref.deref()) + delete m_fontEngine; +} + +void QStaticTextItem::setFontEngine(QFontEngine *fe) +{ + if (m_fontEngine != 0) { + if (!m_fontEngine->ref.deref()) + delete m_fontEngine; + } + + m_fontEngine = fe; + if (m_fontEngine != 0) + m_fontEngine->ref.ref(); +} + QT_END_NAMESPACE diff --git a/src/gui/text/qstatictext_p.h b/src/gui/text/qstatictext_p.h index cb60626..87ef0d5 100644 --- a/src/gui/text/qstatictext_p.h +++ b/src/gui/text/qstatictext_p.h @@ -68,27 +68,60 @@ public: OpenGLUserData }; - QStaticTextUserData(Type t) : type(t) {} + QStaticTextUserData(Type t) : type(t) { ref = 0; } virtual ~QStaticTextUserData() {} + QAtomicInt ref; Type type; }; class Q_GUI_EXPORT QStaticTextItem { public: - QStaticTextItem() : chars(0), numChars(0), fontEngine(0), userData(0), - useBackendOptimizations(false), userDataNeedsUpdate(0) {} - ~QStaticTextItem() { delete userData; } + QStaticTextItem() : chars(0), numChars(0), useBackendOptimizations(false), + userDataNeedsUpdate(0), m_fontEngine(0), m_userData(0) {} + + QStaticTextItem(const QStaticTextItem &other) + { + operator=(other); + } + + void operator=(const QStaticTextItem &other) + { + glyphPositions = other.glyphPositions; + glyphs = other.glyphs; + chars = other.chars; + numGlyphs = other.numGlyphs; + numChars = other.numChars; + font = other.font; + color = other.color; + useBackendOptimizations = other.useBackendOptimizations; + userDataNeedsUpdate = other.userDataNeedsUpdate; + + m_fontEngine = 0; + m_userData = 0; + setUserData(other.userData()); + setFontEngine(other.fontEngine()); + } + + ~QStaticTextItem(); void setUserData(QStaticTextUserData *newUserData) { - if (userData == newUserData) + if (m_userData == newUserData) return; - delete userData; - userData = newUserData; + if (m_userData != 0 && !m_userData->ref.deref()) + delete m_userData; + + m_userData = newUserData; + if (m_userData != 0) + m_userData->ref.ref(); } + QStaticTextUserData *userData() const { return m_userData; } + + void setFontEngine(QFontEngine *fe); + QFontEngine *fontEngine() const { return m_fontEngine; } union { QFixedPoint *glyphPositions; // 8 bytes per glyph @@ -108,14 +141,17 @@ public: // 12 bytes for pointers int numGlyphs; // 4 bytes per item int numChars; // 4 bytes per item - QFontEngine *fontEngine; // 4 bytes per item QFont font; // 8 bytes per item QColor color; // 10 bytes per item - QStaticTextUserData *userData; // 8 bytes per item char useBackendOptimizations : 1; // 1 byte per item char userDataNeedsUpdate : 1; // // ================ // 51 bytes per item + +private: // Needs special handling in setters, so private to avoid abuse + QFontEngine *m_fontEngine; // 4 bytes per item + QStaticTextUserData *m_userData; // 8 bytes per item + }; class QStaticText; diff --git a/src/opengl/gl2paintengineex/qpaintengineex_opengl2.cpp b/src/opengl/gl2paintengineex/qpaintengineex_opengl2.cpp index 84c7fed..1dcb773 100644 --- a/src/opengl/gl2paintengineex/qpaintengineex_opengl2.cpp +++ b/src/opengl/gl2paintengineex/qpaintengineex_opengl2.cpp @@ -1350,8 +1350,8 @@ void QGL2PaintEngineEx::drawStaticTextItem(QStaticTextItem *textItem) ensureActive(); - QFontEngineGlyphCache::Type glyphType = textItem->fontEngine->glyphFormat >= 0 - ? QFontEngineGlyphCache::Type(textItem->fontEngine->glyphFormat) + QFontEngineGlyphCache::Type glyphType = textItem->fontEngine()->glyphFormat >= 0 + ? QFontEngineGlyphCache::Type(textItem->fontEngine()->glyphFormat) : d->glyphCacheType; if (glyphType == QFontEngineGlyphCache::Raster_RGBMask) { if (d->device->alphaRequested() || state()->matrix.type() > QTransform::TxTranslate @@ -1430,7 +1430,7 @@ void QGL2PaintEngineEx::drawTextItem(const QPointF &p, const QTextItem &textItem { QStaticTextItem staticTextItem; staticTextItem.chars = const_cast(ti.chars); - staticTextItem.fontEngine = ti.fontEngine; + staticTextItem.setFontEngine(ti.fontEngine); staticTextItem.glyphs = glyphs.data(); staticTextItem.numChars = ti.num_chars; staticTextItem.numGlyphs = glyphs.size(); @@ -1475,18 +1475,18 @@ void QGL2PaintEngineExPrivate::drawCachedGlyphs(QFontEngineGlyphCache::Type glyp QOpenGL2PaintEngineState *s = q->state(); QGLTextureGlyphCache *cache = - (QGLTextureGlyphCache *) staticTextItem->fontEngine->glyphCache(ctx, glyphType, QTransform()); + (QGLTextureGlyphCache *) staticTextItem->fontEngine()->glyphCache(ctx, glyphType, QTransform()); if (!cache || cache->cacheType() != glyphType) { cache = new QGLTextureGlyphCache(ctx, glyphType, QTransform()); - staticTextItem->fontEngine->setGlyphCache(ctx, cache); + staticTextItem->fontEngine()->setGlyphCache(ctx, cache); } bool recreateVertexArrays = false; if (staticTextItem->userDataNeedsUpdate) recreateVertexArrays = true; - else if (staticTextItem->userData == 0) + else if (staticTextItem->userData() == 0) recreateVertexArrays = true; - else if (staticTextItem->userData->type != QStaticTextUserData::OpenGLUserData) + else if (staticTextItem->userData()->type != QStaticTextUserData::OpenGLUserData) recreateVertexArrays = true; // We only need to update the cache with new glyphs if we are actually going to recreate the vertex arrays. @@ -1494,8 +1494,8 @@ void QGL2PaintEngineExPrivate::drawCachedGlyphs(QFontEngineGlyphCache::Type glyp // cache so this text is performed before we test if the cache size has changed. if (recreateVertexArrays) { cache->setPaintEnginePrivate(this); - cache->populate(staticTextItem->fontEngine, staticTextItem->numGlyphs, staticTextItem->glyphs, - staticTextItem->glyphPositions); + cache->populate(staticTextItem->fontEngine(), staticTextItem->numGlyphs, + staticTextItem->glyphs, staticTextItem->glyphPositions); } if (cache->width() == 0 || cache->height() == 0) @@ -1515,14 +1515,14 @@ void QGL2PaintEngineExPrivate::drawCachedGlyphs(QFontEngineGlyphCache::Type glyp if (staticTextItem->useBackendOptimizations) { QOpenGLStaticTextUserData *userData = 0; - if (staticTextItem->userData == 0 - || staticTextItem->userData->type != QStaticTextUserData::OpenGLUserData) { + if (staticTextItem->userData() == 0 + || staticTextItem->userData()->type != QStaticTextUserData::OpenGLUserData) { userData = new QOpenGLStaticTextUserData(); staticTextItem->setUserData(userData); } else { - userData = static_cast(staticTextItem->userData); + userData = static_cast(staticTextItem->userData()); } // Use cache if backend optimizations is turned on diff --git a/src/opengl/qpaintengine_opengl.cpp b/src/opengl/qpaintengine_opengl.cpp index 74b6b9a..a04d930 100644 --- a/src/opengl/qpaintengine_opengl.cpp +++ b/src/opengl/qpaintengine_opengl.cpp @@ -4918,7 +4918,7 @@ void QOpenGLPaintEngine::drawStaticTextItem(QStaticTextItem *textItem) d->flushDrawQueue(); // make sure the glyphs we want to draw are in the cache - qt_glyph_cache()->cacheGlyphs(d->device->context(), textItem->fontEngine, textItem->glyphs, + qt_glyph_cache()->cacheGlyphs(d->device->context(), textItem->fontEngine(), textItem->glyphs, textItem->numGlyphs); d->setGradientOps(Qt::SolidPattern, QRectF()); // turns off gradient ops @@ -4941,13 +4941,13 @@ void QOpenGLPaintEngine::drawStaticTextItem(QStaticTextItem *textItem) glEnableClientState(GL_VERTEX_ARRAY); glEnableClientState(GL_TEXTURE_COORD_ARRAY); - bool antialias = !(textItem->fontEngine->fontDef.styleStrategy & QFont::NoAntialias) + bool antialias = !(textItem->fontEngine()->fontDef.styleStrategy & QFont::NoAntialias) && (d->matrix.type() > QTransform::TxTranslate); glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, antialias ? GL_LINEAR : GL_NEAREST); glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, antialias ? GL_LINEAR : GL_NEAREST); for (int i=0; i< textItem->numGlyphs; ++i) { - QGLGlyphCoord *g = qt_glyph_cache()->lookup(textItem->fontEngine, textItem->glyphs[i]); + QGLGlyphCoord *g = qt_glyph_cache()->lookup(textItem->fontEngine(), textItem->glyphs[i]); // we don't cache glyphs with no width/height if (!g) @@ -5003,7 +5003,7 @@ void QOpenGLPaintEngine::drawTextItem(const QPointF &p, const QTextItem &textIte { QStaticTextItem staticTextItem; staticTextItem.chars = const_cast(ti.chars); - staticTextItem.fontEngine = ti.fontEngine; + staticTextItem.setFontEngine(ti.fontEngine); staticTextItem.glyphs = glyphs.data(); staticTextItem.numChars = ti.num_chars; staticTextItem.numGlyphs = glyphs.size(); diff --git a/src/openvg/qpaintengine_vg.cpp b/src/openvg/qpaintengine_vg.cpp index ce9d11a..f8788b0 100644 --- a/src/openvg/qpaintengine_vg.cpp +++ b/src/openvg/qpaintengine_vg.cpp @@ -3407,7 +3407,7 @@ void QVGPaintEngine::drawTextItem(const QPointF &p, const QTextItem &textItem) void QVGPaintEngine::drawStaticTextItem(QStaticTextItem *textItem) { - drawCachedGlyphs(textItem->numGlyphs, textItem->glyphs, textItem->font, textItem->fontEngine, + drawCachedGlyphs(textItem->numGlyphs, textItem->glyphs, textItem->font, textItem->fontEngine(), QPointF(0, 0), textItem->glyphPositions); } -- cgit v0.12 From c4d715260bd0ed5f3b6d38a63a2659715342c90a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20R=C3=B8dal?= Date: Fri, 29 Oct 2010 13:37:43 +0200 Subject: Prevented threading related crash in OpenGL module. If the last shallow copy of a QImage which is cached in the QGLTextureCache is destroyed in a thread at the same time as the QGLContext which the texture was initialized in is active in a different thread, the QImage thread incorrectly tries to make the context active in two threads at once. To prevent this from happening it's best to always do the texture clean-up in the main thread. Task-number: QT-4238 Reviewed-by: Eskil Abrahamsen Blomfeldt --- src/opengl/qgl.cpp | 7 ++- src/opengl/qgl_p.h | 68 +++++++++++++++++--------- tests/auto/qgl/tst_qgl.cpp | 118 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 24 deletions(-) diff --git a/src/opengl/qgl.cpp b/src/opengl/qgl.cpp index dbd295f..84cfa97 100644 --- a/src/opengl/qgl.cpp +++ b/src/opengl/qgl.cpp @@ -127,7 +127,12 @@ Q_GLOBAL_STATIC(QGLDefaultOverlayFormat, defaultOverlayFormatInstance) Q_GLOBAL_STATIC(QGLSignalProxy, theSignalProxy) QGLSignalProxy *QGLSignalProxy::instance() { - return theSignalProxy(); + QGLSignalProxy *proxy = theSignalProxy(); + if (proxy && proxy->thread() != qApp->thread()) { + if (proxy->thread() == QThread::currentThread()) + proxy->moveToThread(qApp->thread()); + } + return proxy; } diff --git a/src/opengl/qgl_p.h b/src/opengl/qgl_p.h index f86c77f..18b2765 100644 --- a/src/opengl/qgl_p.h +++ b/src/opengl/qgl_p.h @@ -434,20 +434,6 @@ public: static void setCurrentContext(QGLContext *context); }; -// ### make QGLContext a QObject in 5.0 and remove the proxy stuff -class Q_OPENGL_EXPORT QGLSignalProxy : public QObject -{ - Q_OBJECT -public: - QGLSignalProxy() : QObject() {} - void emitAboutToDestroyContext(const QGLContext *context) { - emit aboutToDestroyContext(context); - } - static QGLSignalProxy *instance(); -Q_SIGNALS: - void aboutToDestroyContext(const QGLContext *context); -}; - Q_DECLARE_OPERATORS_FOR_FLAGS(QGLExtensions::Extensions) // Temporarily make a context current if not already current or @@ -490,6 +476,48 @@ private: QGLContext *m_ctx; }; +// ### make QGLContext a QObject in 5.0 and remove the proxy stuff +class Q_OPENGL_EXPORT QGLSignalProxy : public QObject +{ + Q_OBJECT +public: + QGLSignalProxy() : QObject() { + qRegisterMetaType("GLuint"); + connect(this, SIGNAL(freeTexture(QGLContext *, QPixmapData *, GLuint)), + this, SLOT(freeTexture_slot(QGLContext *, QPixmapData *, GLuint))); + } + void emitAboutToDestroyContext(const QGLContext *context) { + emit aboutToDestroyContext(context); + } + void emitFreeTexture(QGLContext *context, QPixmapData *boundPixmap, GLuint id) { + emit freeTexture(context, boundPixmap, id); + } + static QGLSignalProxy *instance(); +Q_SIGNALS: + void aboutToDestroyContext(const QGLContext *context); + void freeTexture(QGLContext *context, QPixmapData *boundPixmap, GLuint id); + +private slots: + void freeTexture_slot(QGLContext *context, QPixmapData *boundPixmap, GLuint id) { +#if defined(Q_WS_X11) + if (boundPixmap) { + QGLContext *oldContext = const_cast(QGLContext::currentContext()); + context->makeCurrent(); + // Although glXReleaseTexImage is a glX call, it must be called while there + // is a current context - the context the pixmap was bound to a texture in. + // Otherwise the release doesn't do anything and you get BadDrawable errors + // when you come to delete the context. + QGLContextPrivate::unbindPixmapFromTexture(boundPixmap); + glDeleteTextures(1, &id); + oldContext->makeCurrent(); + return; + } +#endif + QGLShareContextScope scope(context); + glDeleteTextures(1, &id); + } +}; + class QGLTexture { public: QGLTexture(QGLContext *ctx = 0, GLuint tx_id = 0, GLenum tx_target = GL_TEXTURE_2D, @@ -506,16 +534,10 @@ public: ~QGLTexture() { if (options & QGLContext::MemoryManagedBindOption) { Q_ASSERT(context); - QGLShareContextScope scope(context); -#if defined(Q_WS_X11) - // Although glXReleaseTexImage is a glX call, it must be called while there - // is a current context - the context the pixmap was bound to a texture in. - // Otherwise the release doesn't do anything and you get BadDrawable errors - // when you come to delete the context. - if (boundPixmap) - QGLContextPrivate::unbindPixmapFromTexture(boundPixmap); +#if !defined(Q_WS_X11) + QPixmapData *boundPixmap = 0; #endif - glDeleteTextures(1, &id); + QGLSignalProxy::instance()->emitFreeTexture(context, boundPixmap, id); } } diff --git a/tests/auto/qgl/tst_qgl.cpp b/tests/auto/qgl/tst_qgl.cpp index e411508..4220b45 100644 --- a/tests/auto/qgl/tst_qgl.cpp +++ b/tests/auto/qgl/tst_qgl.cpp @@ -96,6 +96,7 @@ private slots: void shareRegister(); void qglContextDefaultBindTexture(); void textureCleanup(); + void threadImages(); }; tst_QGL::tst_QGL() @@ -2253,6 +2254,123 @@ void tst_QGL::textureCleanup() #endif } +namespace ThreadImages { + +class Producer : public QObject +{ + Q_OBJECT +public: + Producer() + { + startTimer(20); + + QThread *thread = new QThread; + thread->start(); + + connect(this, SIGNAL(destroyed()), thread, SLOT(quit())); + + moveToThread(thread); + connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater())); + } + +signals: + void imageReady(const QImage &image); + +protected: + void timerEvent(QTimerEvent *) + { + QImage image(256, 256, QImage::Format_RGB32); + QLinearGradient g(0, 0, 0, 256); + g.setColorAt(0, QColor(255, 180, 180)); + g.setColorAt(1, Qt::white); + g.setSpread(QGradient::ReflectSpread); + + QBrush brush(g); + brush.setTransform(QTransform::fromTranslate(0, delta)); + delta += 10; + + QPainter p(&image); + p.fillRect(image.rect(), brush); + + if (images.size() > 10) + images.removeFirst(); + + images.append(image); + + emit imageReady(image); + } + +private: + QList images; + int delta; +}; + + +class DisplayWidget : public QGLWidget +{ + Q_OBJECT +public: + DisplayWidget(QWidget *parent) : QGLWidget(parent) {} + void paintEvent(QPaintEvent *) + { + QPainter p(this); + p.drawImage(rect(), m_image); + } + +public slots: + void setImage(const QImage &image) + { + m_image = image; + update(); + } + +private: + QImage m_image; +}; + +class Widget : public QWidget +{ + Q_OBJECT +public: + Widget() + : iterations(0) + , display(0) + { + startTimer(400); + } + + int iterations; + +protected: + void timerEvent(QTimerEvent *) + { + ++iterations; + + delete display; + display = new DisplayWidget(this); + connect(&producer, SIGNAL(imageReady(const QImage &)), display, SLOT(setImage(const QImage &))); + + display->setGeometry(rect()); + display->show(); + } + +private: + Producer producer; + DisplayWidget *display; +}; + +} + +void tst_QGL::threadImages() +{ + ThreadImages::Widget *widget = new ThreadImages::Widget; + widget->show(); + + while (widget->iterations <= 5) { + qApp->processEvents(); + } +} + class tst_QGLDummy : public QObject { Q_OBJECT -- cgit v0.12 From fb94cee1a10c7bfb8e73dd0a094ae71fe96bf3e7 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Fri, 29 Oct 2010 14:06:28 +0200 Subject: Fix gcc bug in qReallocAligned the line faked.pptr[-1] = real.ptr; was ignored by the GCC 4.5. (optimized out) Workaround by using proper reinterpret_cast casting instead of union casting. Fixes crash in tst_Collections::alignment The crash would occurs for any use of qReallocAligned with alignement > sizeof(void*) (fortunately this is not often called) Reviewed-by: thiago --- src/corelib/global/qmalloc.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/corelib/global/qmalloc.cpp b/src/corelib/global/qmalloc.cpp index 090998c..028a0a5 100644 --- a/src/corelib/global/qmalloc.cpp +++ b/src/corelib/global/qmalloc.cpp @@ -90,8 +90,6 @@ void *qReallocAligned(void *oldptr, size_t newsize, size_t oldsize, size_t align return newptr + 1; } - union { void *ptr; void **pptr; quintptr n; } real, faked; - // qMalloc returns pointers aligned at least at sizeof(size_t) boundaries // but usually more (8- or 16-byte boundaries). // So we overallocate by alignment-sizeof(size_t) bytes, so we're guaranteed to find a @@ -100,19 +98,21 @@ void *qReallocAligned(void *oldptr, size_t newsize, size_t oldsize, size_t align // However, we need to store the actual pointer, so we need to allocate actually size + // alignment anyway. - real.ptr = qRealloc(actualptr, newsize + alignment); - if (!real.ptr) + void *real = qRealloc(actualptr, newsize + alignment); + if (!real) return 0; - faked.n = real.n + alignment; - faked.n &= ~(alignment - 1); + quintptr faked = reinterpret_cast(real) + alignment; + faked &= ~(alignment - 1); + + void **faked_ptr = reinterpret_cast(faked); // now save the value of the real pointer at faked-sizeof(void*) // by construction, alignment > sizeof(void*) and is a power of 2, so // faked-sizeof(void*) is properly aligned for a pointer - faked.pptr[-1] = real.ptr; + faked_ptr[-1] = real; - return faked.ptr; + return faked_ptr; } void qFreeAligned(void *ptr) -- cgit v0.12 From 1fff0c3c764500c755c615f9eb8dc90433406a10 Mon Sep 17 00:00:00 2001 From: Michael Dominic K Date: Fri, 29 Oct 2010 16:01:55 +0200 Subject: 32bit => 16bit conversion has 4byte-aligned output. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge-request: 2499 Reviewed-by: Samuel Rødal --- src/plugins/graphicssystems/meego/dithering.cpp | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/plugins/graphicssystems/meego/dithering.cpp b/src/plugins/graphicssystems/meego/dithering.cpp index b50826c..c0b4b53 100644 --- a/src/plugins/graphicssystems/meego/dithering.cpp +++ b/src/plugins/graphicssystems/meego/dithering.cpp @@ -76,8 +76,13 @@ // Converts incoming RGB32 (QImage::Format_RGB32) to RGB565. Returns the newly allocated data. unsigned short* convertRGB32_to_RGB565(const unsigned char *in, int width, int height, int stride) { + // Output line stride. Alligned to 4 bytes. + int alignedWidth = width; + if (alignedWidth % 2 > 0) + alignedWidth++; + // Will store output - unsigned short *out = (unsigned short *) malloc(width * height * 2); + unsigned short *out = (unsigned short *) malloc(alignedWidth * height * 2); // Lookup tables for the 8bit => 6bit and 8bit => 5bit conversion unsigned char lookup_8bit_to_5bit[256]; @@ -174,7 +179,7 @@ unsigned short* convertRGB32_to_RGB565(const unsigned char *in, int width, int h } // Write the newly produced pixel - PUT_565(out, x, y, width, component[2], component[1], component[0]); + PUT_565(out, x, y, alignedWidth, component[2], component[1], component[0]); } } @@ -183,10 +188,16 @@ unsigned short* convertRGB32_to_RGB565(const unsigned char *in, int width, int h // Converts incoming RGBA32 (QImage::Format_ARGB32_Premultiplied) to RGB565. Returns the newly allocated data. // This function is similar (yet different) to the _565 variant but it makes sense to duplicate it here for simplicity. +// The output has each scan line aligned to 4 bytes (as expected by GL by default). unsigned short* convertARGB32_to_RGBA4444(const unsigned char *in, int width, int height, int stride) { + // Output line stride. Alligned to 4 bytes. + int alignedWidth = width; + if (alignedWidth % 2 > 0) + alignedWidth++; + // Will store output - unsigned short *out = (unsigned short *) malloc(width * height * 2); + unsigned short *out = (unsigned short *) malloc(alignedWidth * 2 * height); // Lookup tables for the 8bit => 4bit conversion unsigned char lookup_8bit_to_4bit[256]; @@ -269,7 +280,7 @@ unsigned short* convertARGB32_to_RGBA4444(const unsigned char *in, int width, in } // Write the newly produced pixel - PUT_4444(out, x, y, width, component[0], component[1], component[2], component[3]); + PUT_4444(out, x, y, alignedWidth, component[0], component[1], component[2], component[3]); } } -- cgit v0.12