Merge changes from topic "color-auto-legacy-srgb" into pi-dev

* changes:
  surfaceflinger: no re-saturation when in SRGB mode
  surfaceflinger: switch RE color matrices lazily
  surfaceflinger: remove hwcId < 0 path in doComposeSurfaces
  surfaceflinger: re-saturate all legacy dataspaces

8 files changed, 94 insertions, 87 deletions

diff --git a/services/surfaceflinger/BufferLayer.cpp b/services/surfaceflinger/BufferLayer.cpp
index 4c3844e130..6feec53257 100644
--- a/services/surfaceflinger/BufferLayer.cpp
+++ b/services/surfaceflinger/BufferLayer.cpp
@@ -528,11 +528,9 @@ Region BufferLayer::latchBuffer(bool& recomputeVisibleRegions, nsecs_t latchTime
         recomputeVisibleRegions = true;
     }
 
-    // Dataspace::V0_SRGB and Dataspace::V0_SRGB_LINEAR are not legacy
-    // data space, however since framework doesn't distinguish them out of
-    // legacy SRGB, we have to treat them as the same for now.
-    // UNKNOWN is treated as legacy SRGB when the connected api is EGL.
     ui::Dataspace dataSpace = mConsumer->getCurrentDataSpace();
+    // treat modern dataspaces as legacy dataspaces whenever possible, until
+    // we can trust the buffer producers
     switch (dataSpace) {
         case ui::Dataspace::V0_SRGB:
             dataSpace = ui::Dataspace::SRGB;
@@ -540,10 +538,17 @@ Region BufferLayer::latchBuffer(bool& recomputeVisibleRegions, nsecs_t latchTime
         case ui::Dataspace::V0_SRGB_LINEAR:
             dataSpace = ui::Dataspace::SRGB_LINEAR;
             break;
-        case ui::Dataspace::UNKNOWN:
-            if (mConsumer->getCurrentApi() == NATIVE_WINDOW_API_EGL) {
-                dataSpace = ui::Dataspace::SRGB;
-            }
+        case ui::Dataspace::V0_JFIF:
+            dataSpace = ui::Dataspace::JFIF;
+            break;
+        case ui::Dataspace::V0_BT601_625:
+            dataSpace = ui::Dataspace::BT601_625;
+            break;
+        case ui::Dataspace::V0_BT601_525:
+            dataSpace = ui::Dataspace::BT601_525;
+            break;
+        case ui::Dataspace::V0_BT709:
+            dataSpace = ui::Dataspace::BT709;
             break;
         default:
             break;
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 2802fc7b21..9043234132 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -1650,9 +1650,10 @@ bool Layer::detachChildren() {
     return true;
 }
 
-bool Layer::isLegacySrgbDataSpace() const {
-    return mDrawingState.dataSpace == ui::Dataspace::SRGB ||
-        mDrawingState.dataSpace == ui::Dataspace::SRGB_LINEAR;
+bool Layer::isLegacyDataSpace() const {
+    // return true when no higher bits are set
+    return !(mDrawingState.dataSpace & (ui::Dataspace::STANDARD_MASK |
+                ui::Dataspace::TRANSFER_MASK | ui::Dataspace::RANGE_MASK));
 }
 
 void Layer::setParent(const sp<Layer>& layer) {
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 0b15b67062..632efbe23e 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -303,8 +303,8 @@ public:
     // desaturated in order to match what they appears like visually.
     // With color management, these contents will appear desaturated, thus
     // needed to be saturated so that they match what they are designed for
-    // visually. When returns true, legacy SRGB data space is passed to HWC.
-    bool isLegacySrgbDataSpace() const;
+    // visually.
+    bool isLegacyDataSpace() const;
 
     // If we have received a new buffer this frame, we will pass its surface
     // damage down to hardware composer. Otherwise, we must send a region with
diff --git a/services/surfaceflinger/RenderEngine/GLES20RenderEngine.cpp b/services/surfaceflinger/RenderEngine/GLES20RenderEngine.cpp
index 08cd5b0012..64095ddfb8 100644
--- a/services/surfaceflinger/RenderEngine/GLES20RenderEngine.cpp
+++ b/services/surfaceflinger/RenderEngine/GLES20RenderEngine.cpp
@@ -259,10 +259,8 @@ void GLES20RenderEngine::setupLayerBlackedOut() {
     mState.setTexture(texture);
 }
 
-mat4 GLES20RenderEngine::setupColorTransform(const mat4& colorTransform) {
-    mat4 oldTransform = mState.getColorMatrix();
+void GLES20RenderEngine::setupColorTransform(const mat4& colorTransform) {
     mState.setColorMatrix(colorTransform);
-    return oldTransform;
 }
 
 void GLES20RenderEngine::disableTexturing() {
diff --git a/services/surfaceflinger/RenderEngine/GLES20RenderEngine.h b/services/surfaceflinger/RenderEngine/GLES20RenderEngine.h
index 9acd79bf71..c9e402de8c 100644
--- a/services/surfaceflinger/RenderEngine/GLES20RenderEngine.h
+++ b/services/surfaceflinger/RenderEngine/GLES20RenderEngine.h
@@ -80,7 +80,7 @@ protected:
     virtual void setupLayerTexturing(const Texture& texture);
     virtual void setupLayerBlackedOut();
     virtual void setupFillWithColor(float r, float g, float b, float a);
-    virtual mat4 setupColorTransform(const mat4& colorTransform);
+    virtual void setupColorTransform(const mat4& colorTransform);
     virtual void disableTexturing();
     virtual void disableBlending();
 
diff --git a/services/surfaceflinger/RenderEngine/RenderEngine.h b/services/surfaceflinger/RenderEngine/RenderEngine.h
index df9e6a74a2..d559464eb5 100644
--- a/services/surfaceflinger/RenderEngine/RenderEngine.h
+++ b/services/surfaceflinger/RenderEngine/RenderEngine.h
@@ -112,7 +112,7 @@ public:
     virtual void setupLayerBlackedOut() = 0;
     virtual void setupFillWithColor(float r, float g, float b, float a) = 0;
 
-    virtual mat4 setupColorTransform(const mat4& /* colorTransform */) = 0;
+    virtual void setupColorTransform(const mat4& /* colorTransform */) = 0;
 
     virtual void disableTexturing() = 0;
     virtual void disableBlending() = 0;
@@ -224,7 +224,7 @@ public:
 
     void checkErrors() const override;
 
-    mat4 setupColorTransform(const mat4& /* colorTransform */) override { return mat4(); }
+    void setupColorTransform(const mat4& /* colorTransform */) override {}
 
     // internal to RenderEngine
     EGLDisplay getEGLDisplay() const;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 2e8a27606a..be70244b79 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1872,13 +1872,13 @@ void SurfaceFlinger::rebuildLayerStacks() {
 // Returns a dataspace that fits all visible layers.  The returned dataspace
 // can only be one of
 //
-//  - Dataspace::V0_SRGB
+//  - Dataspace::SRGB (use legacy dataspace and let HWC saturate when colors are enhanced)
 //  - Dataspace::DISPLAY_P3
 //  - Dataspace::V0_SCRGB_LINEAR
 // TODO(b/73825729) Add BT2020 data space.
 ui::Dataspace SurfaceFlinger::getBestDataspace(
         const sp<const DisplayDevice>& displayDevice) const {
-    Dataspace bestDataspace = Dataspace::V0_SRGB;
+    Dataspace bestDataspace = Dataspace::SRGB;
     for (const auto& layer : displayDevice->getVisibleLayersSortedByZ()) {
         switch (layer->getDataSpace()) {
             case Dataspace::V0_SCRGB:
@@ -1929,7 +1929,7 @@ void SurfaceFlinger::pickColorMode(const sp<DisplayDevice>& displayDevice,
             break;
         default:
             *outMode = ColorMode::SRGB;
-            *outDataSpace = Dataspace::V0_SRGB;
+            *outDataSpace = Dataspace::SRGB;
             break;
     }
 }
@@ -2872,18 +2872,13 @@ bool SurfaceFlinger::doComposeSurfaces(const sp<const DisplayDevice>& displayDev
     const DisplayRenderArea renderArea(displayDevice);
     const auto hwcId = displayDevice->getHwcDisplayId();
     const bool hasClientComposition = getBE().mHwc->hasClientComposition(hwcId);
-    const bool hasDeviceComposition = getBE().mHwc->hasDeviceComposition(hwcId);
-    const bool skipClientColorTransform = getBE().mHwc->hasCapability(
-        HWC2::Capability::SkipClientColorTransform);
     ATRACE_INT("hasClientComposition", hasClientComposition);
 
-    mat4 oldColorMatrix;
-    mat4 legacySrgbSaturationMatrix = mLegacySrgbSaturationMatrix;
-    const bool applyColorMatrix = !hasDeviceComposition && !skipClientColorTransform;
-    if (applyColorMatrix) {
-        oldColorMatrix = getRenderEngine().setupColorTransform(mDrawingState.colorMatrix);
-        legacySrgbSaturationMatrix = mDrawingState.colorMatrix * legacySrgbSaturationMatrix;
-    }
+    bool applyColorMatrix = false;
+    bool applyLegacyColorMatrix = false;
+    mat4 colorMatrix;
+    mat4 legacyColorMatrix;
+    const mat4* currentColorMatrix = nullptr;
 
     if (hasClientComposition) {
         ALOGV("hasClientComposition");
@@ -2896,6 +2891,26 @@ bool SurfaceFlinger::doComposeSurfaces(const sp<const DisplayDevice>& displayDev
         getBE().mRenderEngine->setDisplayMaxLuminance(
                 displayDevice->getHdrCapabilities().getDesiredMaxLuminance());
 
+        const bool hasDeviceComposition = getBE().mHwc->hasDeviceComposition(hwcId);
+        const bool skipClientColorTransform = getBE().mHwc->hasCapability(
+            HWC2::Capability::SkipClientColorTransform);
+
+        applyColorMatrix = !hasDeviceComposition && !skipClientColorTransform;
+        if (applyColorMatrix) {
+            colorMatrix = mDrawingState.colorMatrix;
+        }
+
+        applyLegacyColorMatrix = (mDisplayColorSetting == DisplayColorSetting::ENHANCED &&
+                outputDataspace != Dataspace::UNKNOWN &&
+                outputDataspace != Dataspace::SRGB);
+        if (applyLegacyColorMatrix) {
+            if (applyColorMatrix) {
+                legacyColorMatrix = colorMatrix * mLegacySrgbSaturationMatrix;
+            } else {
+                legacyColorMatrix = mLegacySrgbSaturationMatrix;
+            }
+        }
+
         if (!displayDevice->makeCurrent()) {
             ALOGW("DisplayDevice::makeCurrent failed. Aborting surface composition for display %s",
                   displayDevice->getDisplayName().string());
@@ -2957,69 +2972,57 @@ bool SurfaceFlinger::doComposeSurfaces(const sp<const DisplayDevice>& displayDev
 
     ALOGV("Rendering client layers");
     const Transform& displayTransform = displayDevice->getTransform();
-    if (hwcId >= 0) {
-        // we're using h/w composer
-        bool firstLayer = true;
-        for (auto& layer : displayDevice->getVisibleLayersSortedByZ()) {
-            const Region clip(bounds.intersect(
-                    displayTransform.transform(layer->visibleRegion)));
-            ALOGV("Layer: %s", layer->getName().string());
-            ALOGV("  Composition type: %s",
-                    to_string(layer->getCompositionType(hwcId)).c_str());
-            if (!clip.isEmpty()) {
-                switch (layer->getCompositionType(hwcId)) {
-                    case HWC2::Composition::Cursor:
-                    case HWC2::Composition::Device:
-                    case HWC2::Composition::Sideband:
-                    case HWC2::Composition::SolidColor: {
-                        const Layer::State& state(layer->getDrawingState());
-                        if (layer->getClearClientTarget(hwcId) && !firstLayer &&
-                                layer->isOpaque(state) && (state.color.a == 1.0f)
-                                && hasClientComposition) {
-                            // never clear the very first layer since we're
-                            // guaranteed the FB is already cleared
-                            layer->clearWithOpenGL(renderArea);
-                        }
-                        break;
+    bool firstLayer = true;
+    for (auto& layer : displayDevice->getVisibleLayersSortedByZ()) {
+        const Region clip(bounds.intersect(
+                displayTransform.transform(layer->visibleRegion)));
+        ALOGV("Layer: %s", layer->getName().string());
+        ALOGV("  Composition type: %s",
+                to_string(layer->getCompositionType(hwcId)).c_str());
+        if (!clip.isEmpty()) {
+            switch (layer->getCompositionType(hwcId)) {
+                case HWC2::Composition::Cursor:
+                case HWC2::Composition::Device:
+                case HWC2::Composition::Sideband:
+                case HWC2::Composition::SolidColor: {
+                    const Layer::State& state(layer->getDrawingState());
+                    if (layer->getClearClientTarget(hwcId) && !firstLayer &&
+                            layer->isOpaque(state) && (state.color.a == 1.0f)
+                            && hasClientComposition) {
+                        // never clear the very first layer since we're
+                        // guaranteed the FB is already cleared
+                        layer->clearWithOpenGL(renderArea);
                     }
-                    case HWC2::Composition::Client: {
-                        // Only apply saturation matrix layer that is legacy SRGB dataspace
-                        // when auto color mode is on.
-                        bool restore = false;
-                        mat4 savedMatrix;
-                        if (mDisplayColorSetting == DisplayColorSetting::ENHANCED &&
-                            layer->isLegacySrgbDataSpace()) {
-                            savedMatrix =
-                                getRenderEngine().setupColorTransform(legacySrgbSaturationMatrix);
-                            restore = true;
+                    break;
+                }
+                case HWC2::Composition::Client: {
+                    // switch color matrices lazily
+                    if (layer->isLegacyDataSpace()) {
+                        if (applyLegacyColorMatrix && currentColorMatrix != &legacyColorMatrix) {
+                            getRenderEngine().setupColorTransform(legacyColorMatrix);
+                            currentColorMatrix = &legacyColorMatrix;
                         }
-                        layer->draw(renderArea, clip);
-                        if (restore) {
-                            getRenderEngine().setupColorTransform(savedMatrix);
+                    } else {
+                        if (applyColorMatrix && currentColorMatrix != &colorMatrix) {
+                            getRenderEngine().setupColorTransform(colorMatrix);
+                            currentColorMatrix = &colorMatrix;
                         }
-                        break;
                     }
-                    default:
-                        break;
+
+                    layer->draw(renderArea, clip);
+                    break;
                 }
-            } else {
-                ALOGV("  Skipping for empty clip");
-            }
-            firstLayer = false;
-        }
-    } else {
-        // we're not using h/w composer
-        for (auto& layer : displayDevice->getVisibleLayersSortedByZ()) {
-            const Region clip(bounds.intersect(
-                    displayTransform.transform(layer->visibleRegion)));
-            if (!clip.isEmpty()) {
-                layer->draw(renderArea, clip);
+                default:
+                    break;
             }
+        } else {
+            ALOGV("  Skipping for empty clip");
         }
+        firstLayer = false;
     }
 
-    if (applyColorMatrix) {
-        getRenderEngine().setupColorTransform(oldColorMatrix);
+    if (applyColorMatrix || applyLegacyColorMatrix) {
+        getRenderEngine().setupColorTransform(mat4());
     }
 
     // disable scissor at the end of the frame
diff --git a/services/surfaceflinger/tests/unittests/mock/RenderEngine/MockRenderEngine.h b/services/surfaceflinger/tests/unittests/mock/RenderEngine/MockRenderEngine.h
index 29cd2d545a..93769a53e4 100644
--- a/services/surfaceflinger/tests/unittests/mock/RenderEngine/MockRenderEngine.h
+++ b/services/surfaceflinger/tests/unittests/mock/RenderEngine/MockRenderEngine.h
@@ -60,7 +60,7 @@ public:
     MOCK_METHOD1(setupLayerTexturing, void(const Texture&));
     MOCK_METHOD0(setupLayerBlackedOut, void());
     MOCK_METHOD4(setupFillWithColor, void(float, float, float, float));
-    MOCK_METHOD1(setupColorTransform, mat4(const mat4&));
+    MOCK_METHOD1(setupColorTransform, void(const mat4&));
     MOCK_METHOD0(disableTexturing, void());
     MOCK_METHOD0(disableBlending, void());
     MOCK_METHOD1(setSourceY410BT2020, void(bool));