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 /src/gui/painting/qcolortransform.cpp | |
parent | a0d8fb4ac3cb7bafdb39f340055eacee4f957513 (diff) |
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>
Diffstat (limited to 'src/gui/painting/qcolortransform.cpp')
-rw-r--r-- | src/gui/painting/qcolortransform.cpp | 21 |
1 files changed, 14 insertions, 7 deletions
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); |