Allow SurfaceFlinger to treat 170M as sRGB.

Introduce a debug sysprop, defaulted to false, to allow for rewriting
the transfer function to sRGB when set to true.

This is due to several considerations:

1. SurfaceFlinger has not color managed SMPTE 170M properly ever since
   color management was introduced in Android O, and was only fixed in
   Android 13. This means that some camera -> encoding -> storage ->
   playback flows may be incorrectly calibrated on some devices since
   SMPTE 170M was reinterpreted as sRGB for a long time.
2. BT. 1886 recommends a reference EOTF of gamma 2.4 with tuneable
   parameters to approximate a CRT on a non-CRT display. Because the
   framework doesn't support BT. 1886 and it's too late to add support,
   casting as sRGB is probably okay for now and matches old behavior.
3. Typical Rec. 709 content is graded assuming a dim surround, but phone
   displays are used in a wide range of viewing environments, which
   means that viewing Rec. 709 content in a bright environment may
   appear to have lower contrast. Decoding as sRGB will push the dark
   codes into lower luminance levels, which will improve contrast in
   those scenarios. Note that it's better to adjust contrast based on
   the ambient viewing environment, but again it's too late for Android
   13 to improve the color pipeline in the GPU.

Bug: 229442032
Test: Photos playback after recording a video
Change-Id: I64fc8f2ea77f8e595333de36fb9da2979d8316ca

13 files changed, 79 insertions, 0 deletions

diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h
index 5846e674f5..db2fd1b500 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/Output.h
@@ -267,6 +267,9 @@ public:
     // Enables predicting composition strategy to run client composition earlier
     virtual void setPredictCompositionStrategy(bool) = 0;
 
+    // Enables overriding the 170M trasnfer function as sRGB
+    virtual void setTreat170mAsSrgb(bool) = 0;
+
 protected:
     virtual void setDisplayColorProfile(std::unique_ptr<DisplayColorProfile>) = 0;
     virtual void setRenderSurface(std::unique_ptr<RenderSurface>) = 0;
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h
index 0feb9f7fb7..31c51e6a8d 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/Output.h
@@ -108,6 +108,7 @@ public:
     void cacheClientCompositionRequests(uint32_t) override;
     bool canPredictCompositionStrategy(const CompositionRefreshArgs&) override;
     void setPredictCompositionStrategy(bool) override;
+    void setTreat170mAsSrgb(bool) override;
 
     // Testing
     const ReleasedLayers& getReleasedLayersForTest() const;
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/OutputCompositionState.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/OutputCompositionState.h
index 61be983480..c65d467b0d 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/OutputCompositionState.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/OutputCompositionState.h
@@ -162,6 +162,8 @@ struct OutputCompositionState {
     CompositionStrategyPredictionState strategyPrediction =
             CompositionStrategyPredictionState::DISABLED;
 
+    bool treat170mAsSrgb = false;
+
     // Debugging
     void dump(std::string& result) const;
 };
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h
index fa86076b44..cb9fbad8dd 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/mock/Output.h
@@ -132,6 +132,7 @@ public:
     MOCK_METHOD1(cacheClientCompositionRequests, void(uint32_t));
     MOCK_METHOD1(canPredictCompositionStrategy, bool(const CompositionRefreshArgs&));
     MOCK_METHOD1(setPredictCompositionStrategy, void(bool));
+    MOCK_METHOD1(setTreat170mAsSrgb, void(bool));
 };
 
 } // namespace android::compositionengine::mock
diff --git a/services/surfaceflinger/CompositionEngine/src/Output.cpp b/services/surfaceflinger/CompositionEngine/src/Output.cpp
index ec8673177f..4c30f99610 100644
--- a/services/surfaceflinger/CompositionEngine/src/Output.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/Output.cpp
@@ -1488,6 +1488,10 @@ void Output::setPredictCompositionStrategy(bool predict) {
     }
 }
 
+void Output::setTreat170mAsSrgb(bool enable) {
+    editState().treat170mAsSrgb = enable;
+}
+
 bool Output::canPredictCompositionStrategy(const CompositionRefreshArgs& refreshArgs) {
     if (!getState().isEnabled || !mHwComposerAsyncWorker) {
         ALOGV("canPredictCompositionStrategy disabled");
diff --git a/services/surfaceflinger/CompositionEngine/src/OutputCompositionState.cpp b/services/surfaceflinger/CompositionEngine/src/OutputCompositionState.cpp
index 3b85e3b5b0..948c0c90bf 100644
--- a/services/surfaceflinger/CompositionEngine/src/OutputCompositionState.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/OutputCompositionState.cpp
@@ -66,6 +66,9 @@ void OutputCompositionState::dump(std::string& out) const {
     out.append("\n   ");
     dumpVal(out, "compositionStrategyPredictionState", ftl::enum_string(strategyPrediction));
 
+    out.append("\n   ");
+    dumpVal(out, "treate170mAsSrgb", treat170mAsSrgb);
+
     out += '\n';
 }
 
diff --git a/services/surfaceflinger/CompositionEngine/src/OutputLayer.cpp b/services/surfaceflinger/CompositionEngine/src/OutputLayer.cpp
index 3289d55870..61c53df38d 100644
--- a/services/surfaceflinger/CompositionEngine/src/OutputLayer.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/OutputLayer.cpp
@@ -322,6 +322,17 @@ void OutputLayer::updateCompositionState(
             ? outputState.targetDataspace
             : layerFEState->dataspace;
 
+    // Override the dataspace transfer from 170M to sRGB if the device configuration requests this.
+    // We do this here instead of in buffer info so that dumpsys can still report layers that are
+    // using the 170M transfer. Also we only do this if the colorspace is not agnostic for the
+    // layer, in case the color profile uses a 170M transfer function.
+    if (outputState.treat170mAsSrgb && !layerFEState->isColorspaceAgnostic &&
+        (state.dataspace & HAL_DATASPACE_TRANSFER_MASK) == HAL_DATASPACE_TRANSFER_SMPTE_170M) {
+        state.dataspace = static_cast<ui::Dataspace>(
+                (state.dataspace & HAL_DATASPACE_STANDARD_MASK) |
+                (state.dataspace & HAL_DATASPACE_RANGE_MASK) | HAL_DATASPACE_TRANSFER_SRGB);
+    }
+
     // For hdr content, treat the white point as the display brightness - HDR content should not be
     // boosted or dimmed.
     // If the layer explicitly requests to disable dimming, then don't dim either.
diff --git a/services/surfaceflinger/CompositionEngine/tests/OutputLayerTest.cpp b/services/surfaceflinger/CompositionEngine/tests/OutputLayerTest.cpp
index ceee48c1ef..ca4f39729a 100644
--- a/services/surfaceflinger/CompositionEngine/tests/OutputLayerTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/OutputLayerTest.cpp
@@ -657,6 +657,23 @@ TEST_F(OutputLayerUpdateCompositionStateTest, setsOutputLayerColorspaceCorrectly
     EXPECT_EQ(ui::Dataspace::V0_SCRGB, mOutputLayer.getState().dataspace);
 }
 
+TEST_F(OutputLayerUpdateCompositionStateTest, setsOutputLayerColorspaceWith170mReplacement) {
+    mLayerFEState.dataspace = ui::Dataspace::TRANSFER_SMPTE_170M;
+    mOutputState.targetDataspace = ui::Dataspace::V0_SCRGB;
+    mOutputState.treat170mAsSrgb = false;
+    mLayerFEState.isColorspaceAgnostic = false;
+
+    mOutputLayer.updateCompositionState(false, false, ui::Transform::RotationFlags::ROT_0);
+
+    EXPECT_EQ(ui::Dataspace::TRANSFER_SMPTE_170M, mOutputLayer.getState().dataspace);
+
+    // Rewrite SMPTE 170M as sRGB
+    mOutputState.treat170mAsSrgb = true;
+    mOutputLayer.updateCompositionState(false, false, ui::Transform::RotationFlags::ROT_0);
+
+    EXPECT_EQ(ui::Dataspace::TRANSFER_SRGB, mOutputLayer.getState().dataspace);
+}
+
 TEST_F(OutputLayerUpdateCompositionStateTest, setsWhitePointNitsAndDimmingRatioCorrectly) {
     mOutputState.sdrWhitePointNits = 200.f;
     mOutputState.displayBrightnessNits = 800.f;
diff --git a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
index 42c8b37710..3a3c91e518 100644
--- a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
@@ -248,6 +248,20 @@ TEST_F(OutputTest, setCompositionEnabledSetsDisabledAndDirtiesEntireOutput) {
 }
 
 /*
+ * Output::setTreat170mAsSrgb()
+ */
+
+TEST_F(OutputTest, setTreat170mAsSrgb) {
+    EXPECT_FALSE(mOutput->getState().treat170mAsSrgb);
+
+    mOutput->setTreat170mAsSrgb(true);
+    EXPECT_TRUE(mOutput->getState().treat170mAsSrgb);
+
+    mOutput->setTreat170mAsSrgb(false);
+    EXPECT_FALSE(mOutput->getState().treat170mAsSrgb);
+}
+
+/*
  * Output::setLayerCachingEnabled()
  */
 
diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp
index 52529d6b04..a915b615d9 100644
--- a/services/surfaceflinger/DisplayDevice.cpp
+++ b/services/surfaceflinger/DisplayDevice.cpp
@@ -89,6 +89,7 @@ DisplayDevice::DisplayDevice(DisplayDeviceCreationArgs& args)
     }
 
     mCompositionDisplay->setPredictCompositionStrategy(mFlinger->mPredictCompositionStrategy);
+    mCompositionDisplay->setTreat170mAsSrgb(mFlinger->mTreat170mAsSrgb);
     mCompositionDisplay->createDisplayColorProfile(
             compositionengine::DisplayColorProfileCreationArgsBuilder()
                     .setHasWideColorGamut(args.hasWideColorGamut)
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 3a92ca49a2..e1eec8b97e 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -47,6 +47,7 @@
 #include <stdint.h>
 #include <stdlib.h>
 #include <sys/types.h>
+#include <system/graphics-base-v1.0.h>
 #include <ui/DataspaceUtils.h>
 #include <ui/DebugUtils.h>
 #include <ui/GraphicBuffer.h>
@@ -600,6 +601,18 @@ std::optional<compositionengine::LayerFE::LayerSettings> Layer::prepareClientCom
     layerSettings.alpha = alpha;
     layerSettings.sourceDataspace = getDataSpace();
 
+    // Override the dataspace transfer from 170M to sRGB if the device configuration requests this.
+    // We do this here instead of in buffer info so that dumpsys can still report layers that are
+    // using the 170M transfer.
+    if (mFlinger->mTreat170mAsSrgb &&
+        (layerSettings.sourceDataspace & HAL_DATASPACE_TRANSFER_MASK) ==
+                HAL_DATASPACE_TRANSFER_SMPTE_170M) {
+        layerSettings.sourceDataspace = static_cast<ui::Dataspace>(
+                (layerSettings.sourceDataspace & HAL_DATASPACE_STANDARD_MASK) |
+                (layerSettings.sourceDataspace & HAL_DATASPACE_RANGE_MASK) |
+                HAL_DATASPACE_TRANSFER_SRGB);
+    }
+
     layerSettings.whitePointNits = targetSettings.whitePointNits;
     switch (targetSettings.blurSetting) {
         case LayerFE::ClientCompositionTargetSettings::BlurSetting::Enabled:
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index d39176be14..65c1c97efb 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -419,6 +419,9 @@ SurfaceFlinger::SurfaceFlinger(Factory& factory) : SurfaceFlinger(factory, SkipI
     property_get("debug.sf.predict_hwc_composition_strategy", value, "1");
     mPredictCompositionStrategy = atoi(value);
 
+    property_get("debug.sf.treat_170m_as_sRGB", value, "0");
+    mTreat170mAsSrgb = atoi(value);
+
     // We should be reading 'persist.sys.sf.color_saturation' here
     // but since /data may be encrypted, we need to wait until after vold
     // comes online to attempt to read the property. The property is
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 74e0407215..c70e1749ff 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -349,6 +349,12 @@ public:
     // run parallel to the hwc validateDisplay call and re-run if the predition is incorrect.
     bool mPredictCompositionStrategy = false;
 
+    // If true, then any layer with a SMPTE 170M transfer function is decoded using the sRGB
+    // transfer instead. This is mainly to preserve legacy behavior, where implementations treated
+    // SMPTE 170M as sRGB prior to color management being implemented, and now implementations rely
+    // on this behavior to increase contrast for some media sources.
+    bool mTreat170mAsSrgb = false;
+
 protected:
     // We're reference counted, never destroy SurfaceFlinger directly
     virtual ~SurfaceFlinger();