diff options
author | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2019-05-15 16:45:57 +0200 |
---|---|---|
committer | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2019-05-16 11:09:04 +0000 |
commit | db525e6e9d90350cb9778e745f43271aca4cb4e6 (patch) | |
tree | fa7858dccad407f1f88ca2aadad89a01c8a61ae8 | |
parent | Fix mingw pkgconfig file and dependency naming (diff) | |
download | qtbase-db525e6e9d90350cb9778e745f43271aca4cb4e6.tar.xz qtbase-db525e6e9d90350cb9778e745f43271aca4cb4e6.zip |
Fix race in colorspace LUT generation
The old code did not prevent concurrent writes to the LUTs by separate
threads, each finding lutsGenerated to be false.
Let's consider whether the change is safe now: the storeRelease(1)
comes before the QMutex::unlock(), but since it is release semantics
no writes may be ordered past it. We have two releases, and their
order doesn't matter, since nothing else happens in-between.
Could we use a normal relaxed store? No, because the unlock() of the
mutex only synchronizes with the lock() of the same mutex, which
doesn't happen if the loadAcquire() succeeds. For loadAcquire() to
happen-before a write to the luts, we need a storeRelease() on the
atomic.
So, everything is correct, and minimal.
But maybe, to save the next reader from having to do the same mental
exercise again, add a manual locker.unlock() in front of the
storeRelease()?
Again no: that opens a gap where the luts are already generated on T0,
and the mutex unlocked, but the atomic not set. If another thread T1
gets to execute the function, it will enter the critical section, then
writing new values to the LUT. Meanwhile, the T0 sets generate to true
and a T2 enters the function, sees the final write from T0 and starts
using the luts -> data race with the writes concurrently done by T1.
Change-Id: Id278812a74b6e326e3ddf0dbcbb94b34766aa52e
Reviewed-by: Marc Mutz <marc.mutz@kdab.com>
-rw-r--r-- | src/gui/painting/qcolorspace.cpp | 2 | ||||
-rw-r--r-- | src/gui/painting/qcolorspace_p.h | 23 | ||||
-rw-r--r-- | src/gui/painting/qcolortransform.cpp | 21 |
3 files changed, 36 insertions, 10 deletions
diff --git a/src/gui/painting/qcolorspace.cpp b/src/gui/painting/qcolorspace.cpp index c98c31fe05..0f37ea19a0 100644 --- a/src/gui/painting/qcolorspace.cpp +++ b/src/gui/painting/qcolorspace.cpp @@ -53,6 +53,8 @@ QT_BEGIN_NAMESPACE +QBasicMutex QColorSpacePrivate::s_lutWriteLock; + QColorSpacePrimaries::QColorSpacePrimaries(QColorSpace::Gamut gamut) { switch (gamut) { diff --git a/src/gui/painting/qcolorspace_p.h b/src/gui/painting/qcolorspace_p.h index a6ab2ab5cd..a49c46f195 100644 --- a/src/gui/painting/qcolorspace_p.h +++ b/src/gui/painting/qcolorspace_p.h @@ -56,8 +56,9 @@ #include "qcolortrc_p.h" #include "qcolortrclut_p.h" -#include <QtCore/qshareddata.h> +#include <QtCore/qmutex.h> #include <QtCore/qpoint.h> +#include <QtCore/qshareddata.h> QT_BEGIN_NAMESPACE @@ -112,8 +113,24 @@ public: QString description; QByteArray iccProfile; - mutable QSharedPointer<QColorTrcLut> lut[3]; - mutable QAtomicInt lutsGenerated; + static QBasicMutex s_lutWriteLock; + struct LUT { + LUT() = default; + ~LUT() = default; + LUT(const LUT &other) + { + if (other.generated.loadAcquire()) { + table[0] = other.table[0]; + table[1] = other.table[1]; + table[2] = other.table[2]; + generated.store(1); + } + } + QSharedPointer<QColorTrcLut> &operator[](int i) { return table[i]; } + const QSharedPointer<QColorTrcLut> &operator[](int i) const { return table[i]; } + QSharedPointer<QColorTrcLut> table[3]; + QAtomicInt generated; + } mutable lut; }; QT_END_NAMESPACE diff --git a/src/gui/painting/qcolortransform.cpp b/src/gui/painting/qcolortransform.cpp index c723e12f8a..2f81449693 100644 --- a/src/gui/painting/qcolortransform.cpp +++ b/src/gui/painting/qcolortransform.cpp @@ -68,8 +68,12 @@ QColorTrcLut *lutFromTrc(const QColorTrc &trc) void QColorTransformPrivate::updateLutsIn() const { - if (colorSpaceIn->lutsGenerated.loadAcquire()) + if (colorSpaceIn->lut.generated.loadAcquire()) return; + QMutexLocker lock(&QColorSpacePrivate::s_lutWriteLock); + if (colorSpaceIn->lut.generated.load()) + return; + for (int i = 0; i < 3; ++i) { if (!colorSpaceIn->trc[i].isValid()) return; @@ -84,12 +88,15 @@ void QColorTransformPrivate::updateLutsIn() const colorSpaceIn->lut[i].reset(lutFromTrc(colorSpaceIn->trc[i])); } - colorSpaceIn->lutsGenerated.storeRelease(1); + colorSpaceIn->lut.generated.storeRelease(1); } void QColorTransformPrivate::updateLutsOut() const { - if (colorSpaceOut->lutsGenerated.loadAcquire()) + if (colorSpaceOut->lut.generated.loadAcquire()) + return; + QMutexLocker lock(&QColorSpacePrivate::s_lutWriteLock); + if (colorSpaceOut->lut.generated.load()) return; for (int i = 0; i < 3; ++i) { if (!colorSpaceOut->trc[i].isValid()) @@ -105,7 +112,7 @@ void QColorTransformPrivate::updateLutsOut() const colorSpaceOut->lut[i].reset(lutFromTrc(colorSpaceOut->trc[i])); } - colorSpaceOut->lutsGenerated.storeRelease(1); + colorSpaceOut->lut.generated.storeRelease(1); } /*! @@ -150,7 +157,7 @@ QRgb QColorTransform::map(const QRgb &argb) const c.x = std::max(0.0f, std::min(1.0f, c.x)); c.y = std::max(0.0f, std::min(1.0f, c.y)); c.z = std::max(0.0f, std::min(1.0f, c.z)); - if (d->colorSpaceOut->lutsGenerated.loadAcquire()) { + if (d->colorSpaceOut->lut.generated.loadAcquire()) { c.x = d->colorSpaceOut->lut[0]->fromLinear(c.x); c.y = d->colorSpaceOut->lut[1]->fromLinear(c.y); c.z = d->colorSpaceOut->lut[2]->fromLinear(c.z); @@ -182,7 +189,7 @@ QRgba64 QColorTransform::map(const QRgba64 &rgba64) const c.x = std::max(0.0f, std::min(1.0f, c.x)); c.y = std::max(0.0f, std::min(1.0f, c.y)); c.z = std::max(0.0f, std::min(1.0f, c.z)); - if (d->colorSpaceOut->lutsGenerated.loadAcquire()) { + if (d->colorSpaceOut->lut.generated.loadAcquire()) { c.x = d->colorSpaceOut->lut[0]->fromLinear(c.x); c.y = d->colorSpaceOut->lut[1]->fromLinear(c.y); c.z = d->colorSpaceOut->lut[2]->fromLinear(c.z); @@ -221,7 +228,7 @@ QColor QColorTransform::map(const QColor &color) const c = d->colorMatrix.map(c); bool inGamut = c.x >= 0.0f && c.x <= 1.0f && c.y >= 0.0f && c.y <= 1.0f && c.z >= 0.0f && c.z <= 1.0f; if (inGamut) { - if (d_ptr->colorSpaceOut->lutsGenerated.loadAcquire()) { + if (d_ptr->colorSpaceOut->lut.generated.loadAcquire()) { c.x = d->colorSpaceOut->lut[0]->fromLinear(c.x); c.y = d->colorSpaceOut->lut[1]->fromLinear(c.y); c.z = d->colorSpaceOut->lut[2]->fromLinear(c.z); |