From ae6dc7d6df12b3a5631d20b968823e23f6d7f1f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tor=20Arne=20Vestb=C3=B8?= Date: Fri, 10 Dec 2021 17:53:25 +0100 Subject: [PATCH] Fix qt_scrollRectInImage when scrolling outside of the image We were clipping the source rect to the image, both pre and post scrolling, but did not apply the same logic to the target position. By computing the target position based on the already clipped source rect we ensure that the target position is also correct. This was causing valgrind warnings on Linux, and crashes on Windows, when trying to test the lower level QBackingStore::scroll() function. The reason we were not seeing this in practice was that QWidget does its own sanitation and clipping of the arguments before passing them on. As a drive-by, fix the access of image to use constBits instead of a manual cast, and rename variables to better reflect their use. Pick-to: 6.3 6.2 5.15 Change-Id: Ibc190c2ef825e634956758f612a018f642f4202b Reviewed-by: Qt CI Bot Reviewed-by: Eirik Aavitsland --- src/gui/painting/qbackingstore.cpp | 26 +++++++------ .../qbackingstore/tst_qbackingstore.cpp | 39 +++++++++++++++++++ 2 files changed, 53 insertions(+), 12 deletions(-) diff --git a/src/gui/painting/qbackingstore.cpp b/src/gui/painting/qbackingstore.cpp index f76b9864fc..762938e764 100644 --- a/src/gui/painting/qbackingstore.cpp +++ b/src/gui/painting/qbackingstore.cpp @@ -298,32 +298,34 @@ bool QBackingStore::hasStaticContents() const void Q_GUI_EXPORT qt_scrollRectInImage(QImage &img, const QRect &rect, const QPoint &offset) { // make sure we don't detach - uchar *mem = const_cast(const_cast(img).bits()); + uchar *mem = const_cast(img.constBits()); qsizetype lineskip = img.bytesPerLine(); int depth = img.depth() >> 3; const QRect imageRect(0, 0, img.width(), img.height()); - const QRect r = rect & imageRect & imageRect.translated(-offset); - const QPoint p = rect.topLeft() + offset; - - if (r.isEmpty()) + const QRect sourceRect = rect.intersected(imageRect).intersected(imageRect.translated(-offset)); + if (sourceRect.isEmpty()) return; + const QRect destRect = sourceRect.translated(offset); + Q_ASSERT_X(imageRect.contains(destRect), "qt_scrollRectInImage", + "The sourceRect should already account for clipping, both pre and post scroll"); + const uchar *src; uchar *dest; - if (r.top() < p.y()) { - src = mem + r.bottom() * lineskip + r.left() * depth; - dest = mem + (p.y() + r.height() - 1) * lineskip + p.x() * depth; + if (sourceRect.top() < destRect.top()) { + src = mem + sourceRect.bottom() * lineskip + sourceRect.left() * depth; + dest = mem + (destRect.top() + sourceRect.height() - 1) * lineskip + destRect.left() * depth; lineskip = -lineskip; } else { - src = mem + r.top() * lineskip + r.left() * depth; - dest = mem + p.y() * lineskip + p.x() * depth; + src = mem + sourceRect.top() * lineskip + sourceRect.left() * depth; + dest = mem + destRect.top() * lineskip + destRect.left() * depth; } - const int w = r.width(); - int h = r.height(); + const int w = sourceRect.width(); + int h = sourceRect.height(); const int bytes = w * depth; // overlapping segments? diff --git a/tests/auto/gui/kernel/qbackingstore/tst_qbackingstore.cpp b/tests/auto/gui/kernel/qbackingstore/tst_qbackingstore.cpp index 5349119b6d..14cb58315b 100644 --- a/tests/auto/gui/kernel/qbackingstore/tst_qbackingstore.cpp +++ b/tests/auto/gui/kernel/qbackingstore/tst_qbackingstore.cpp @@ -43,8 +43,47 @@ class tst_QBackingStore : public QObject private slots: void flush(); + + void scrollRectInImage_data(); + void scrollRectInImage(); }; +void tst_QBackingStore::scrollRectInImage_data() +{ + QTest::addColumn("rect"); + QTest::addColumn("offset"); + + QTest::newRow("empty rect") << QRect() << QPoint(); + QTest::newRow("rect outside image") << QRect(-100, -100, 1000, 1000) << QPoint(10, 10); + QTest::newRow("scroll outside positive") << QRect(10, 10, 10, 10) << QPoint(1000, 1000); + QTest::newRow("scroll outside negative") << QRect(10, 10, 10, 10) << QPoint(-1000, -1000); + + QTest::newRow("sub-rect positive scroll") << QRect(100, 100, 50, 50) << QPoint(10, 10); + QTest::newRow("sub-rect negative scroll") << QRect(100, 100, 50, 50) << QPoint(-10, -10); + + QTest::newRow("positive vertical only") << QRect(100, 100, 50, 50) << QPoint(0, 10); + QTest::newRow("negative vertical only") << QRect(100, 100, 50, 50) << QPoint(0, -10); + QTest::newRow("positive horizontal only") << QRect(100, 100, 50, 50) << QPoint(10, 0); + QTest::newRow("negative horizontal only") << QRect(100, 100, 50, 50) << QPoint(-10, 0); + + QTest::newRow("whole rect positive") << QRect(0, 0, 250, 250) << QPoint(10, 10); + QTest::newRow("whole rect negative") << QRect(0, 0, 250, 250) << QPoint(-10, -10); +} + +QT_BEGIN_NAMESPACE +Q_GUI_EXPORT void qt_scrollRectInImage(QImage &, const QRect &, const QPoint &); +QT_END_NAMESPACE + +void tst_QBackingStore::scrollRectInImage() +{ + QImage test(250, 250, QImage::Format_ARGB32_Premultiplied); + + QFETCH(QRect, rect); + QFETCH(QPoint, offset); + + qt_scrollRectInImage(test, rect, offset); +} + class Window : public QWindow { public: