surfaceflinger: fix color tranform matrix races
The color transform matrices may be updated by the binder threads
while being used by the main thread. Protect the matrices with
mStateLock, compute the effective matrix when the individual
matrices are updated, and copy the effective matrix to mDrawingState
during commitTransaction.
This commit fixes the race and moves the matrix computation out of
the hot path.
Bug: 79210409
Test: night light, color correction, boosted
Change-Id: Ibdb756b7b66345ffcef3c665652e20b050865f6d
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index f736c8c..01f855b 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -244,7 +244,6 @@
mPrimaryDispSync("PrimaryDispSync"),
mPrimaryHWVsyncEnabled(false),
mHWVsyncAvailable(false),
- mHasColorMatrix(false),
mHasPoweredOff(false),
mNumLayers(0),
mVrFlingerRequestsDisplay(false),
@@ -723,10 +722,13 @@
}
void SurfaceFlinger::readPersistentProperties() {
+ Mutex::Autolock _l(mStateLock);
+
char value[PROPERTY_VALUE_MAX];
property_get("persist.sys.sf.color_saturation", value, "1.0");
mGlobalSaturationFactor = atof(value);
+ updateColorMatrixLocked();
ALOGV("Saturation is set to %.2f", mGlobalSaturationFactor);
property_get("persist.sys.sf.native_mode", value, "0");
@@ -1859,22 +1861,6 @@
}
}
-mat4 SurfaceFlinger::computeSaturationMatrix() const {
- if (mGlobalSaturationFactor == 1.0f) {
- return mat4();
- }
-
- // Rec.709 luma coefficients
- float3 luminance{0.213f, 0.715f, 0.072f};
- luminance *= 1.0f - mGlobalSaturationFactor;
- return mat4(
- vec4{luminance.r + mGlobalSaturationFactor, luminance.r, luminance.r, 0.0f},
- vec4{luminance.g, luminance.g + mGlobalSaturationFactor, luminance.g, 0.0f},
- vec4{luminance.b, luminance.b, luminance.b + mGlobalSaturationFactor, 0.0f},
- vec4{0.0f, 0.0f, 0.0f, 1.0f}
- );
-}
-
// Returns a dataspace that fits all visible layers. The returned dataspace
// can only be one of
//
@@ -2000,9 +1986,6 @@
}
}
-
- mat4 colorMatrix = mColorMatrix * computeSaturationMatrix() * mDaltonizer();
-
// Set the per-frame data
for (size_t displayId = 0; displayId < mDisplays.size(); ++displayId) {
auto& displayDevice = mDisplays[displayId];
@@ -2011,9 +1994,9 @@
if (hwcId < 0) {
continue;
}
- if (colorMatrix != mPreviousColorMatrix) {
- displayDevice->setColorTransform(colorMatrix);
- status_t result = getBE().mHwc->setColorTransform(hwcId, colorMatrix);
+ if (mDrawingState.colorMatrixChanged) {
+ displayDevice->setColorTransform(mDrawingState.colorMatrix);
+ status_t result = getBE().mHwc->setColorTransform(hwcId, mDrawingState.colorMatrix);
ALOGE_IF(result != NO_ERROR, "Failed to set color transform on "
"display %zd: %d", displayId, result);
}
@@ -2046,7 +2029,7 @@
}
}
- mPreviousColorMatrix = colorMatrix;
+ mDrawingState.colorMatrixChanged = false;
for (size_t displayId = 0; displayId < mDisplays.size(); ++displayId) {
auto& displayDevice = mDisplays[displayId];
@@ -2635,6 +2618,9 @@
mAnimCompositionPending = mAnimTransactionPending;
mDrawingState = mCurrentState;
+ // clear the "changed" flags in current state
+ mCurrentState.colorMatrixChanged = false;
+
mDrawingState.traverseInZOrder([](Layer* layer) {
layer->commitChildList();
});
@@ -2887,9 +2873,8 @@
mat4 legacySrgbSaturationMatrix = mLegacySrgbSaturationMatrix;
const bool applyColorMatrix = !hasDeviceComposition && !skipClientColorTransform;
if (applyColorMatrix) {
- mat4 colorMatrix = mColorMatrix * computeSaturationMatrix() * mDaltonizer();
- oldColorMatrix = getRenderEngine().setupColorTransform(colorMatrix);
- legacySrgbSaturationMatrix = colorMatrix * legacySrgbSaturationMatrix;
+ oldColorMatrix = getRenderEngine().setupColorTransform(mDrawingState.colorMatrix);
+ legacySrgbSaturationMatrix = mDrawingState.colorMatrix * legacySrgbSaturationMatrix;
}
if (hasClientComposition) {
@@ -4349,6 +4334,30 @@
return true;
}
+void SurfaceFlinger::updateColorMatrixLocked() {
+ mat4 colorMatrix;
+ if (mGlobalSaturationFactor != 1.0f) {
+ // Rec.709 luma coefficients
+ float3 luminance{0.213f, 0.715f, 0.072f};
+ luminance *= 1.0f - mGlobalSaturationFactor;
+ mat4 saturationMatrix = mat4(
+ vec4{luminance.r + mGlobalSaturationFactor, luminance.r, luminance.r, 0.0f},
+ vec4{luminance.g, luminance.g + mGlobalSaturationFactor, luminance.g, 0.0f},
+ vec4{luminance.b, luminance.b, luminance.b + mGlobalSaturationFactor, 0.0f},
+ vec4{0.0f, 0.0f, 0.0f, 1.0f}
+ );
+ colorMatrix = mClientColorMatrix * saturationMatrix * mDaltonizer();
+ } else {
+ colorMatrix = mClientColorMatrix * mDaltonizer();
+ }
+
+ if (mCurrentState.colorMatrix != colorMatrix) {
+ mCurrentState.colorMatrix = colorMatrix;
+ mCurrentState.colorMatrixChanged = true;
+ setTransactionFlags(eTransactionNeeded);
+ }
+}
+
status_t SurfaceFlinger::CheckTransactCodeCredentials(uint32_t code) {
switch (code) {
case CREATE_CONNECTION:
@@ -4483,6 +4492,7 @@
return NO_ERROR;
}
case 1014: {
+ Mutex::Autolock _l(mStateLock);
// daltonize
n = data.readInt32();
switch (n % 10) {
@@ -4504,33 +4514,33 @@
} else {
mDaltonizer.setMode(ColorBlindnessMode::Simulation);
}
- invalidateHwcGeometry();
- repaintEverything();
+
+ updateColorMatrixLocked();
return NO_ERROR;
}
case 1015: {
+ Mutex::Autolock _l(mStateLock);
// apply a color matrix
n = data.readInt32();
if (n) {
// color matrix is sent as a column-major mat4 matrix
for (size_t i = 0 ; i < 4; i++) {
for (size_t j = 0; j < 4; j++) {
- mColorMatrix[i][j] = data.readFloat();
+ mClientColorMatrix[i][j] = data.readFloat();
}
}
} else {
- mColorMatrix = mat4();
+ mClientColorMatrix = mat4();
}
// Check that supplied matrix's last row is {0,0,0,1} so we can avoid
// the division by w in the fragment shader
- float4 lastRow(transpose(mColorMatrix)[3]);
+ float4 lastRow(transpose(mClientColorMatrix)[3]);
if (any(greaterThan(abs(lastRow - float4{0, 0, 0, 1}), float4{1e-4f}))) {
ALOGE("The color transform's last row must be (0, 0, 0, 1)");
}
- invalidateHwcGeometry();
- repaintEverything();
+ updateColorMatrixLocked();
return NO_ERROR;
}
// This is an experimental interface
@@ -4573,10 +4583,10 @@
return NO_ERROR;
}
case 1022: { // Set saturation boost
+ Mutex::Autolock _l(mStateLock);
mGlobalSaturationFactor = std::max(0.0f, std::min(data.readFloat(), 2.0f));
- invalidateHwcGeometry();
- repaintEverything();
+ updateColorMatrixLocked();
return NO_ERROR;
}
case 1023: { // Set native mode