Various fixes for wide color gamut rendering

This CL addresses multiple issues:
- A logic issue where the wide gamut color mode was not set at the right time
- The presence of scRGB surfaces was not detected
- The sRGB to Display P3 matrix was transposed
- The color matrix was applied in gamma space instead of linear space*
- The GPU code path was doing a division by w for each pixel when a
  color transform is set, which shouldn't be necessary (the code now
  checks that the matrix has the expected form)
- Incorrect comment
- Dead code in Description.cpp (mDirtyUniforms was never used)
- Screenshots were taken using the last render engine configuration,
  the configuration is now properly set every time

* This can in theory cause a discrepancy when we switch to/from hardware
  composer mode. I was not able to create a visible discrepancy in practice
  using Night Light, color blindness compensation modes and color inversion.
  More importantly, how/where the hardware composer applies the color
  transform is not specified: it could be gamma or linear space, before or
  after the hardware color LUT. We'll address this in a future CL if needed.
  In addition, this code assumes that fp16 surfaces are encoded in non-linear
  space (scRGB-nl instead of scRGB) but we do not have EGL/Vulkan extensions
  to specify this behavior. We need to address this as well

This CL also fixes potential divides by 0 in the GPU code path.

Bug: 29940137
Test: CtsUiRenderingTestsCases, CtsGraphicsTestCases

Change-Id: I9ae15850f8b9d48c39ebc2724ca3da202be9b008

9 files changed, 105 insertions, 33 deletions

diff --git a/include/gui/SurfaceComposerClient.h b/include/gui/SurfaceComposerClient.h
index ec310cf6f9..145c0597bd 100644
--- a/include/gui/SurfaceComposerClient.h
+++ b/include/gui/SurfaceComposerClient.h
@@ -271,6 +271,7 @@ public:
     uint32_t getStride() const;
     // size of allocated memory in bytes
     size_t getSize() const;
+    android_dataspace getDataSpace() const;
 };
 
 // ---------------------------------------------------------------------------
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 8c8384399c..7ae2672249 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -1080,5 +1080,9 @@ size_t ScreenshotClient::getSize() const {
     return mBuffer.stride * mBuffer.height * bytesPerPixel(mBuffer.format);
 }
 
+android_dataspace ScreenshotClient::getDataSpace() const {
+    return mBuffer.dataSpace;
+}
+
 // ----------------------------------------------------------------------------
 }; // namespace android
diff --git a/libs/math/include/math/TMatHelpers.h b/libs/math/include/math/TMatHelpers.h
index 5cb725d10a..1423adea2c 100644
--- a/libs/math/include/math/TMatHelpers.h
+++ b/libs/math/include/math/TMatHelpers.h
@@ -342,9 +342,9 @@ TQuaternion<typename MATRIX::value_type> extractQuat(const MATRIX& mat) {
 template <typename MATRIX>
 String8 asString(const MATRIX& m) {
     String8 s;
-    for (size_t c = 0; c < MATRIX::col_size(); c++) {
+    for (size_t c = 0; c < MATRIX::COL_SIZE; c++) {
         s.append("|  ");
-        for (size_t r = 0; r < MATRIX::row_size(); r++) {
+        for (size_t r = 0; r < MATRIX::ROW_SIZE; r++) {
             s.appendFormat("%7.2f  ", m[r][c]);
         }
         s.append("|\n");
diff --git a/services/surfaceflinger/RenderEngine/Description.cpp b/services/surfaceflinger/RenderEngine/Description.cpp
index 0dab872682..effd3191c8 100644
--- a/services/surfaceflinger/RenderEngine/Description.cpp
+++ b/services/surfaceflinger/RenderEngine/Description.cpp
@@ -26,8 +26,7 @@
 
 namespace android {
 
-Description::Description() :
-    mUniformsDirty(true) {
+Description::Description() {
     mPlaneAlpha = 1.0f;
     mPremultipliedAlpha = false;
     mOpaque = true;
@@ -41,28 +40,20 @@ Description::~Description() {
 }
 
 void Description::setPlaneAlpha(GLclampf planeAlpha) {
-    if (planeAlpha != mPlaneAlpha) {
-        mUniformsDirty = true;
-        mPlaneAlpha = planeAlpha;
-    }
+    mPlaneAlpha = planeAlpha;
 }
 
 void Description::setPremultipliedAlpha(bool premultipliedAlpha) {
-    if (premultipliedAlpha != mPremultipliedAlpha) {
-        mPremultipliedAlpha = premultipliedAlpha;
-    }
+    mPremultipliedAlpha = premultipliedAlpha;
 }
 
 void Description::setOpaque(bool opaque) {
-    if (opaque != mOpaque) {
-        mOpaque = opaque;
-    }
+    mOpaque = opaque;
 }
 
 void Description::setTexture(const Texture& texture) {
     mTexture = texture;
     mTextureEnabled = true;
-    mUniformsDirty = true;
 }
 
 void Description::disableTexture() {
@@ -74,12 +65,10 @@ void Description::setColor(GLclampf red, GLclampf green, GLclampf blue, GLclampf
     mColor[1] = green;
     mColor[2] = blue;
     mColor[3] = alpha;
-    mUniformsDirty = true;
 }
 
 void Description::setProjectionMatrix(const mat4& mtx) {
     mProjectionMatrix = mtx;
-    mUniformsDirty = true;
 }
 
 void Description::setColorMatrix(const mat4& mtx) {
@@ -92,5 +81,8 @@ const mat4& Description::getColorMatrix() const {
     return mColorMatrix;
 }
 
+void Description::setWideGamut(bool wideGamut) {
+    mIsWideGamut = wideGamut;
+}
 
 } /* namespace android */
diff --git a/services/surfaceflinger/RenderEngine/Description.h b/services/surfaceflinger/RenderEngine/Description.h
index 8a3447c5a1..3beffdf9e1 100644
--- a/services/surfaceflinger/RenderEngine/Description.h
+++ b/services/surfaceflinger/RenderEngine/Description.h
@@ -54,6 +54,8 @@ class Description {
     bool mColorMatrixEnabled;
     mat4 mColorMatrix;
 
+    bool mIsWideGamut;
+
 public:
     Description();
     ~Description();
@@ -67,9 +69,7 @@ public:
     void setProjectionMatrix(const mat4& mtx);
     void setColorMatrix(const mat4& mtx);
     const mat4& getColorMatrix() const;
-
-private:
-    bool mUniformsDirty;
+    void setWideGamut(bool wideGamut);
 };
 
 } /* namespace android */
diff --git a/services/surfaceflinger/RenderEngine/GLES20RenderEngine.cpp b/services/surfaceflinger/RenderEngine/GLES20RenderEngine.cpp
index a1ee29477e..37a530b33a 100644
--- a/services/surfaceflinger/RenderEngine/GLES20RenderEngine.cpp
+++ b/services/surfaceflinger/RenderEngine/GLES20RenderEngine.cpp
@@ -139,11 +139,10 @@ GLES20RenderEngine::GLES20RenderEngine(uint32_t featureFlags) :
         // Display-P3 only.
         mat3 srgbToP3 = ColorSpaceConnector(ColorSpace::sRGB(), ColorSpace::DisplayP3()).getTransform();
 
-        // color transform needs to be transposed and expanded to 4x4
-        // to be what the shader wants
+        // color transform needs to be expanded to 4x4 to be what the shader wants
         // mat has an initializer that expands mat3 to mat4, but
         // not an assignment operator
-        mat4 gamutTransform(transpose(srgbToP3));
+        mat4 gamutTransform(srgbToP3);
         mSrgbToDisplayP3 = gamutTransform;
     }
 #endif
@@ -386,6 +385,7 @@ void GLES20RenderEngine::drawMesh(const Mesh& mesh) {
         Description wideColorState = mState;
         if (mDataSpace != HAL_DATASPACE_DISPLAY_P3) {
             wideColorState.setColorMatrix(mState.getColorMatrix() * mSrgbToDisplayP3);
+            wideColorState.setWideGamut(true);
             ALOGV("drawMesh: gamut transform applied");
         }
         ProgramCache::getInstance().useProgram(wideColorState);
diff --git a/services/surfaceflinger/RenderEngine/ProgramCache.cpp b/services/surfaceflinger/RenderEngine/ProgramCache.cpp
index ba11259ac2..06b225299c 100644
--- a/services/surfaceflinger/RenderEngine/ProgramCache.cpp
+++ b/services/surfaceflinger/RenderEngine/ProgramCache.cpp
@@ -129,7 +129,9 @@ ProgramCache::Key ProgramCache::computeKey(const Description& description) {
     .set(Key::OPACITY_MASK,
             description.mOpaque ? Key::OPACITY_OPAQUE : Key::OPACITY_TRANSLUCENT)
     .set(Key::COLOR_MATRIX_MASK,
-            description.mColorMatrixEnabled ? Key::COLOR_MATRIX_ON :  Key::COLOR_MATRIX_OFF);
+            description.mColorMatrixEnabled ? Key::COLOR_MATRIX_ON :  Key::COLOR_MATRIX_OFF)
+    .set(Key::WIDE_GAMUT_MASK,
+            description.mIsWideGamut ? Key::WIDE_GAMUT_ON : Key::WIDE_GAMUT_OFF);
     return needs;
 }
 
@@ -175,6 +177,50 @@ String8 ProgramCache::generateFragmentShader(const Key& needs) {
     if (needs.hasColorMatrix()) {
         fs << "uniform mat4 colorMatrix;";
     }
+    if (needs.hasColorMatrix()) {
+        // When in wide gamut mode, the color matrix will contain a color space
+        // conversion matrix that needs to be applied in linear space
+        // When not in wide gamut, we can simply no-op the transfer functions
+        // and let the shader compiler get rid of them
+        if (needs.isWideGamut()) {
+            fs << R"__SHADER__(
+                  float OETF_sRGB(const float linear) {
+                      return linear <= 0.0031308 ?
+                              linear * 12.92 : (pow(linear, 1.0 / 2.4) * 1.055) - 0.055;
+                  }
+
+                  vec3 OETF_sRGB(const vec3 linear) {
+                      return vec3(OETF_sRGB(linear.r), OETF_sRGB(linear.g), OETF_sRGB(linear.b));
+                  }
+
+                  vec3 OETF_scRGB(const vec3 linear) {
+                      return sign(linear.rgb) * OETF_sRGB(abs(linear.rgb));
+                  }
+
+                  float EOTF_sRGB(float srgb) {
+                      return srgb <= 0.04045 ? srgb / 12.92 : pow((srgb + 0.055) / 1.055, 2.4);
+                  }
+
+                  vec3 EOTF_sRGB(const vec3 srgb) {
+                      return vec3(EOTF_sRGB(srgb.r), EOTF_sRGB(srgb.g), EOTF_sRGB(srgb.b));
+                  }
+
+                  vec3 EOTF_scRGB(const vec3 srgb) {
+                      return sign(srgb.rgb) * EOTF_sRGB(abs(srgb.rgb));
+                  }
+            )__SHADER__";
+        } else {
+            fs << R"__SHADER__(
+                  vec3 OETF_scRGB(const vec3 linear) {
+                      return linear;
+                  }
+
+                  vec3 EOTF_scRGB(const vec3 srgb) {
+                      return srgb;
+                  }
+            )__SHADER__";
+        }
+    }
     fs << "void main(void) {" << indent;
     if (needs.isTexturing()) {
         fs << "gl_FragColor = texture2D(sampler, outTexCoords);";
@@ -197,13 +243,15 @@ String8 ProgramCache::generateFragmentShader(const Key& needs) {
     if (needs.hasColorMatrix()) {
         if (!needs.isOpaque() && needs.isPremultiplied()) {
             // un-premultiply if needed before linearization
-            fs << "gl_FragColor.rgb = gl_FragColor.rgb/gl_FragColor.a;";
+            // avoid divide by 0 by adding 0.5/256 to the alpha channel
+            fs << "gl_FragColor.rgb = gl_FragColor.rgb / (gl_FragColor.a + 0.0019);";
         }
-        fs << "vec4 transformed = colorMatrix * vec4(gl_FragColor.rgb, 1);";
-        fs << "gl_FragColor.rgb = transformed.rgb/transformed.a;";
+        fs << "vec4 transformed = colorMatrix * vec4(EOTF_scRGB(gl_FragColor.rgb), 1);";
+        // We assume the last row is always {0,0,0,1} and we skip the division by w
+        fs << "gl_FragColor.rgb = OETF_scRGB(transformed.rgb);";
         if (!needs.isOpaque() && needs.isPremultiplied()) {
             // and re-premultiply if needed after gamma correction
-            fs << "gl_FragColor.rgb = gl_FragColor.rgb*gl_FragColor.a;";
+            fs << "gl_FragColor.rgb = gl_FragColor.rgb * (gl_FragColor.a + 0.0019);";
         }
     }
 
diff --git a/services/surfaceflinger/RenderEngine/ProgramCache.h b/services/surfaceflinger/RenderEngine/ProgramCache.h
index 1fa53d3379..5b0fbcd153 100644
--- a/services/surfaceflinger/RenderEngine/ProgramCache.h
+++ b/services/surfaceflinger/RenderEngine/ProgramCache.h
@@ -69,6 +69,10 @@ public:
             COLOR_MATRIX_OFF        =       0x00000000,
             COLOR_MATRIX_ON         =       0x00000020,
             COLOR_MATRIX_MASK       =       0x00000020,
+
+            WIDE_GAMUT_OFF          =       0x00000000,
+            WIDE_GAMUT_ON           =       0x00000040,
+            WIDE_GAMUT_MASK         =       0x00000040,
         };
 
         inline Key() : mKey(0) { }
@@ -97,10 +101,13 @@ public:
         inline bool hasColorMatrix() const {
             return (mKey & COLOR_MATRIX_MASK) == COLOR_MATRIX_ON;
         }
+        inline bool isWideGamut() const {
+            return (mKey & WIDE_GAMUT_MASK) == WIDE_GAMUT_ON;
+        }
 
         // this is the definition of a friend function -- not a method of class Needs
         friend inline int strictly_order_type(const Key& lhs, const Key& rhs) {
-            return  (lhs.mKey < rhs.mKey) ? 1 : 0;
+            return (lhs.mKey < rhs.mKey) ? 1 : 0;
         }
     };
 
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 7392006c6f..beefc5066b 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1705,6 +1705,13 @@ android_dataspace SurfaceFlinger::bestTargetDataSpace(android_dataspace a, andro
     if (a == HAL_DATASPACE_DISPLAY_P3 || b == HAL_DATASPACE_DISPLAY_P3) {
         return HAL_DATASPACE_DISPLAY_P3;
     }
+    if (a == HAL_DATASPACE_V0_SCRGB_LINEAR || b == HAL_DATASPACE_V0_SCRGB_LINEAR) {
+        return HAL_DATASPACE_DISPLAY_P3;
+    }
+    if (a == HAL_DATASPACE_V0_SCRGB || b == HAL_DATASPACE_V0_SCRGB) {
+        return HAL_DATASPACE_DISPLAY_P3;
+    }
+
     return HAL_DATASPACE_V0_SRGB;
 }
 
@@ -2508,8 +2515,8 @@ bool SurfaceFlinger::doComposeSurfaces(
         ALOGV("hasClientComposition");
 
 #ifdef USE_HWC2
-        mRenderEngine->setColorMode(displayDevice->getActiveColorMode());
         mRenderEngine->setWideColor(displayDevice->getWideColorSupport());
+        mRenderEngine->setColorMode(displayDevice->getActiveColorMode());
 #endif
         if (!displayDevice->makeCurrent(mEGLDisplay, mEGLContext)) {
             ALOGW("DisplayDevice::makeCurrent failed. Aborting surface composition for display %s",
@@ -3893,9 +3900,7 @@ status_t SurfaceFlinger::onTransact(
                 // apply a color matrix
                 n = data.readInt32();
                 if (n) {
-                    // color matrix is sent as mat3 matrix followed by vec3
-                    // offset, then packed into a mat4 where the last row is
-                    // the offset and extra values are 0
+                    // color matrix is sent as a row-major mat4 matrix
                     for (size_t i = 0 ; i < 4; i++) {
                         for (size_t j = 0; j < 4; j++) {
                             mColorMatrix[i][j] = data.readFloat();
@@ -3904,6 +3909,14 @@ status_t SurfaceFlinger::onTransact(
                 } else {
                     mColorMatrix = 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]);
+                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();
                 return NO_ERROR;
@@ -4210,6 +4223,11 @@ void SurfaceFlinger::renderScreenImplLocked(
         ALOGE("Invalid crop rect: b = %d (> %d)", sourceCrop.bottom, hw_h);
     }
 
+#ifdef USE_HWC2
+     engine.setWideColor(hw->getWideColorSupport());
+     engine.setColorMode(hw->getActiveColorMode());
+#endif
+
     // make sure to clear all GL error flags
     engine.checkErrors();
 
@@ -4313,6 +4331,8 @@ status_t SurfaceFlinger::captureScreenImplLocked(
         err |= native_window_set_scaling_mode(window, NATIVE_WINDOW_SCALING_MODE_SCALE_TO_WINDOW);
         err |= native_window_set_buffers_format(window, HAL_PIXEL_FORMAT_RGBA_8888);
         err |= native_window_set_usage(window, usage);
+        err |= native_window_set_buffers_data_space(window, hw->getWideColorSupport()
+                ? HAL_DATASPACE_DISPLAY_P3 : HAL_DATASPACE_V0_SRGB);
 
         if (err == NO_ERROR) {
             ANativeWindowBuffer* buffer;