From b1f16725cc7c872f01ba291f9d3b26ecb718e4a9 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Mon, 7 Feb 2022 13:03:44 -0800 Subject: [PATCH] Provide dimming ratio instead of white point nits in composer There are several reasons for limiting the notion of white point nits in the composer interface: 1. Some KMS apis exposed by drivers only expose a notion of a per-plane brightness. While these are non-mobile drivers, e.g., nouveau, this does indicate that white point is not directly going to be understood by typical hardware 2. Changing the brightness without requiring a frame update introduces implicit state in composer. If the brightness and white point nits for a set of SDR layers are 200 nits, and the brightness changes to 205 nits to respond to ambient conditions, then composer must not dim the layers, and in fact DisplayManager will tell SurfaceFlinger that the SDR white point will be 205 nits. But SurfaceFlinger will not tell composer that the SDR white point changed as that would otherwise introduce a re-composition cycle, meaning that HW Composer must track somehow that the layer white point changed without a corresponding change on the layer data structure, which is confusing. 3. It's poorly defined what the dimming ratio should be if SurfaceFlinger provides the following inputs: Layer A has a white point of 200 nits, Layer B has a white point of 400 nits, and display brightness is 300 nits. Current implementations may clamp the brightness of Layer B to be 300 nits and dim layer A by 2/3s, but there is an equally valid interpretation which is just dim Layer A to be 50% of Layer B's brightness. 4. The problem indicated by (2) and (3) suggests that layer white point is really an up-stack concept, that SurfaceFlinger can be aware of for properly computing the dimming ratios it can send to composer, but the composer hal shouldn't really be speaking in terms of nits. Note that this patch does not yet change the interface for ClientTargetWithNits, which may be done in a follow-up patch. Bug: 217961164 Test: builds, boots Change-Id: I4a1b4e8c300d22599a5683bd44b7b8afa9a29425 --- .../{Luminance.aidl => LayerBrightness.aidl} | 4 +-- .../graphics/composer3/LayerCommand.aidl | 2 +- .../graphics/composer3/DisplayCommand.aidl | 10 +------- .../{Luminance.aidl => LayerBrightness.aidl} | 8 +++--- .../graphics/composer3/LayerCommand.aidl | 12 ++++----- .../VtsHalGraphicsComposer3_ReadbackTest.cpp | 4 ++- .../VtsHalGraphicsComposer3_TargetTest.cpp | 25 +++++++++++++------ .../functional/composer-vts/ReadbackVts.cpp | 2 +- .../composer-vts/include/ReadbackVts.h | 4 ++- .../graphics/composer3/ComposerClientWriter.h | 7 +++--- 10 files changed, 43 insertions(+), 35 deletions(-) rename graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/{Luminance.aidl => LayerBrightness.aidl} (97%) rename graphics/composer/aidl/android/hardware/graphics/composer3/{Luminance.aidl => LayerBrightness.aidl} (76%) diff --git a/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/Luminance.aidl b/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/LayerBrightness.aidl similarity index 97% rename from graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/Luminance.aidl rename to graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/LayerBrightness.aidl index adb49a81b4..a726cc12bf 100644 --- a/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/Luminance.aidl +++ b/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/LayerBrightness.aidl @@ -33,6 +33,6 @@ package android.hardware.graphics.composer3; @VintfStability -parcelable Luminance { - float nits; +parcelable LayerBrightness { + float brightness; } diff --git a/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/LayerCommand.aidl b/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/LayerCommand.aidl index c1c01172c1..0c5fac9640 100644 --- a/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/LayerCommand.aidl +++ b/graphics/composer/aidl/aidl_api/android.hardware.graphics.composer3/current/android/hardware/graphics/composer3/LayerCommand.aidl @@ -50,7 +50,7 @@ parcelable LayerCommand { @nullable android.hardware.graphics.common.Rect[] visibleRegion; @nullable android.hardware.graphics.composer3.ZOrder z; @nullable float[] colorTransform; - @nullable android.hardware.graphics.composer3.Luminance whitePointNits; + @nullable android.hardware.graphics.composer3.LayerBrightness brightness; @nullable android.hardware.graphics.composer3.PerFrameMetadata[] perFrameMetadata; @nullable android.hardware.graphics.composer3.PerFrameMetadataBlob[] perFrameMetadataBlob; @nullable android.hardware.graphics.common.Rect[] blockingRegion; diff --git a/graphics/composer/aidl/android/hardware/graphics/composer3/DisplayCommand.aidl b/graphics/composer/aidl/android/hardware/graphics/composer3/DisplayCommand.aidl index f1ce1a7dad..b6df147df8 100644 --- a/graphics/composer/aidl/android/hardware/graphics/composer3/DisplayCommand.aidl +++ b/graphics/composer/aidl/android/hardware/graphics/composer3/DisplayCommand.aidl @@ -77,15 +77,7 @@ parcelable DisplayCommand { * the display brightness, for example when internally switching the display between multiple * power modes to achieve higher luminance. In those cases, the underlying display panel's real * brightness may not be applied atomically; however, layer dimming when mixing HDR and SDR - * content must be synchronized. - * - * As an illustrative example: suppose two layers have white - * points of 200 nits and 1000 nits respectively, the old display luminance is 200 nits, and the - * new display luminance is 1000 nits. If the new display luminance takes two frames to apply, - * then: In the first frame, there must not be any relative dimming of layers (treat both layers - * as 200 nits as the maximum luminance of the display is 200 nits). In the second frame, there - * dimming should be applied to ensure that the first layer does not become perceptually - * brighter during the transition. + * content must be synchronized to ensure that there is no user-perceptable flicker. * * The display luminance must be updated by this command even if there is not pending validate * or present command. diff --git a/graphics/composer/aidl/android/hardware/graphics/composer3/Luminance.aidl b/graphics/composer/aidl/android/hardware/graphics/composer3/LayerBrightness.aidl similarity index 76% rename from graphics/composer/aidl/android/hardware/graphics/composer3/Luminance.aidl rename to graphics/composer/aidl/android/hardware/graphics/composer3/LayerBrightness.aidl index 5b1c1b40fe..146e0127df 100644 --- a/graphics/composer/aidl/android/hardware/graphics/composer3/Luminance.aidl +++ b/graphics/composer/aidl/android/hardware/graphics/composer3/LayerBrightness.aidl @@ -17,10 +17,10 @@ package android.hardware.graphics.composer3; @VintfStability -parcelable Luminance { +parcelable LayerBrightness { /** - * Photometric measure of luminous intensity per unit area of light. - * Units are nits, or cd/m^2. + * Brightness of the current layer, ranging from 0 to 1, where 0 is the minimum brightness of + * the display, and 1 is the current brightness of the display. */ - float nits; + float brightness; } diff --git a/graphics/composer/aidl/android/hardware/graphics/composer3/LayerCommand.aidl b/graphics/composer/aidl/android/hardware/graphics/composer3/LayerCommand.aidl index 6f6894fcdd..f3b67a99b3 100644 --- a/graphics/composer/aidl/android/hardware/graphics/composer3/LayerCommand.aidl +++ b/graphics/composer/aidl/android/hardware/graphics/composer3/LayerCommand.aidl @@ -22,7 +22,7 @@ import android.hardware.graphics.common.Point; import android.hardware.graphics.common.Rect; import android.hardware.graphics.composer3.Buffer; import android.hardware.graphics.composer3.Color; -import android.hardware.graphics.composer3.Luminance; +import android.hardware.graphics.composer3.LayerBrightness; import android.hardware.graphics.composer3.ParcelableBlendMode; import android.hardware.graphics.composer3.ParcelableComposition; import android.hardware.graphics.composer3.ParcelableDataspace; @@ -221,12 +221,12 @@ parcelable LayerCommand { @nullable float[] colorTransform; /** - * Sets the desired white point for the layer. This is intended to be used when presenting - * an SDR layer alongside HDR content. The HDR content will be presented at the display - * brightness in nits, and accordingly SDR content shall be dimmed to the desired white point - * provided. + * Sets the desired brightness for the layer. This is intended to be used for instance when + * presenting an SDR layer alongside HDR content. The HDR content will be presented at the + * display brightness in nits, and accordingly SDR content shall be dimmed according to the + * provided brightness ratio. */ - @nullable Luminance whitePointNits; + @nullable LayerBrightness brightness; /** * Sets the PerFrameMetadata for the display. This metadata must be used diff --git a/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/VtsHalGraphicsComposer3_ReadbackTest.cpp b/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/VtsHalGraphicsComposer3_ReadbackTest.cpp index 45a8f6f359..a179f1bcbf 100644 --- a/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/VtsHalGraphicsComposer3_ReadbackTest.cpp +++ b/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/VtsHalGraphicsComposer3_ReadbackTest.cpp @@ -965,7 +965,7 @@ TEST_P(GraphicsCompositionTest, SetLayerZOrder) { } } -TEST_P(GraphicsCompositionTest, SetLayerWhitePointDims) { +TEST_P(GraphicsCompositionTest, SetLayerBrightnessDims) { const auto& [status, capabilities] = mComposerClient->getDisplayCapabilities(getPrimaryDisplayId()); ASSERT_TRUE(status.isOk()); @@ -1013,6 +1013,7 @@ TEST_P(GraphicsCompositionTest, SetLayerWhitePointDims) { redLayer->setColor(RED); redLayer->setDisplayFrame(redRect); redLayer->setWhitePointNits(maxBrightnessNits); + redLayer->setBrightness(1.f); const auto dimmerRedLayer = std::make_shared(mComposerClient, getPrimaryDisplayId()); @@ -1022,6 +1023,7 @@ TEST_P(GraphicsCompositionTest, SetLayerWhitePointDims) { // kick into GPU composition to apply dithering when the dimming ratio is high. static constexpr float kDimmingRatio = 0.9f; dimmerRedLayer->setWhitePointNits(maxBrightnessNits * kDimmingRatio); + dimmerRedLayer->setBrightness(kDimmingRatio); const std::vector> layers = {redLayer, dimmerRedLayer}; std::vector expectedColors( diff --git a/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/VtsHalGraphicsComposer3_TargetTest.cpp b/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/VtsHalGraphicsComposer3_TargetTest.cpp index 17ec8854e9..8d4bc11950 100644 --- a/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/VtsHalGraphicsComposer3_TargetTest.cpp +++ b/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/VtsHalGraphicsComposer3_TargetTest.cpp @@ -1798,26 +1798,37 @@ TEST_P(GraphicsComposerAidlCommandTest, SetLayerPerFrameMetadata) { EXPECT_TRUE(mComposerClient->destroyLayer(getPrimaryDisplayId(), layer).isOk()); } -TEST_P(GraphicsComposerAidlCommandTest, SetLayerWhitePointNits) { +TEST_P(GraphicsComposerAidlCommandTest, setLayerBrightness) { const auto& [layerStatus, layer] = mComposerClient->createLayer(getPrimaryDisplayId(), kBufferSlotCount); - EXPECT_TRUE(layerStatus.isOk()); - mWriter.setLayerWhitePointNits(getPrimaryDisplayId(), layer, /*whitePointNits*/ 200.f); + mWriter.setLayerBrightness(getPrimaryDisplayId(), layer, 0.2f); execute(); ASSERT_TRUE(mReader.takeErrors().empty()); - mWriter.setLayerWhitePointNits(getPrimaryDisplayId(), layer, /*whitePointNits*/ 1000.f); + mWriter.setLayerBrightness(getPrimaryDisplayId(), layer, 1.f); execute(); ASSERT_TRUE(mReader.takeErrors().empty()); - mWriter.setLayerWhitePointNits(getPrimaryDisplayId(), layer, /*whitePointNits*/ 0.f); + mWriter.setLayerBrightness(getPrimaryDisplayId(), layer, 0.f); execute(); ASSERT_TRUE(mReader.takeErrors().empty()); - mWriter.setLayerWhitePointNits(getPrimaryDisplayId(), layer, /*whitePointNits*/ -1.f); + mWriter.setLayerBrightness(getPrimaryDisplayId(), layer, -1.f); execute(); - ASSERT_TRUE(mReader.takeErrors().empty()); + { + const auto errors = mReader.takeErrors(); + ASSERT_EQ(1, errors.size()); + EXPECT_EQ(IComposerClient::EX_BAD_PARAMETER, errors[0].errorCode); + } + + mWriter.setLayerBrightness(getPrimaryDisplayId(), layer, std::nanf("")); + execute(); + { + const auto errors = mReader.takeErrors(); + ASSERT_EQ(1, errors.size()); + EXPECT_EQ(IComposerClient::EX_BAD_PARAMETER, errors[0].errorCode); + } } TEST_P(GraphicsComposerAidlCommandTest, SetActiveConfigWithConstraints) { diff --git a/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/composer-vts/ReadbackVts.cpp b/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/composer-vts/ReadbackVts.cpp index 1aca76f3c8..f163e09fb7 100644 --- a/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/composer-vts/ReadbackVts.cpp +++ b/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/composer-vts/ReadbackVts.cpp @@ -34,7 +34,7 @@ void TestLayer::write(ComposerClientWriter& writer) { writer.setLayerTransform(mDisplay, mLayer, mTransform); writer.setLayerPlaneAlpha(mDisplay, mLayer, mAlpha); writer.setLayerBlendMode(mDisplay, mLayer, mBlendMode); - writer.setLayerWhitePointNits(mDisplay, mLayer, mWhitePointNits); + writer.setLayerBrightness(mDisplay, mLayer, mBrightness); } std::string ReadbackHelper::getColorModeString(ColorMode mode) { diff --git a/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/composer-vts/include/ReadbackVts.h b/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/composer-vts/include/ReadbackVts.h index 7135dcae77..ce30db0704 100644 --- a/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/composer-vts/include/ReadbackVts.h +++ b/graphics/composer/aidl/android/hardware/graphics/composer3/vts/functional/composer-vts/include/ReadbackVts.h @@ -67,6 +67,7 @@ class TestLayer { void setSourceCrop(FRect crop) { mSourceCrop = crop; } void setZOrder(uint32_t z) { mZOrder = z; } void setWhitePointNits(float whitePointNits) { mWhitePointNits = whitePointNits; } + void setBrightness(float brightness) { mBrightness = brightness; } void setSurfaceDamage(std::vector surfaceDamage) { mSurfaceDamage = std::move(surfaceDamage); @@ -84,12 +85,13 @@ class TestLayer { int64_t getLayer() const { return mLayer; } - float getWhitePointNits() const { return mWhitePointNits; } + float getBrightness() const { return mBrightness; } protected: int64_t mDisplay; int64_t mLayer; Rect mDisplayFrame = {0, 0, 0, 0}; + float mBrightness = 1.f; float mWhitePointNits = -1.f; std::vector mSurfaceDamage; Transform mTransform = static_cast(0); diff --git a/graphics/composer/aidl/include/android/hardware/graphics/composer3/ComposerClientWriter.h b/graphics/composer/aidl/include/android/hardware/graphics/composer3/ComposerClientWriter.h index a04b9824c4..ae17c51ea2 100644 --- a/graphics/composer/aidl/include/android/hardware/graphics/composer3/ComposerClientWriter.h +++ b/graphics/composer/aidl/include/android/hardware/graphics/composer3/ComposerClientWriter.h @@ -30,7 +30,7 @@ #include #include #include -#include +#include #include #include @@ -209,8 +209,9 @@ class ComposerClientWriter { .perFrameMetadataBlob.emplace(metadata.begin(), metadata.end()); } - void setLayerWhitePointNits(int64_t display, int64_t layer, float whitePointNits) { - getLayerCommand(display, layer).whitePointNits.emplace(Luminance{.nits = whitePointNits}); + void setLayerBrightness(int64_t display, int64_t layer, float brightness) { + getLayerCommand(display, layer) + .brightness.emplace(LayerBrightness{.brightness = brightness}); } void setLayerBlockingRegion(int64_t display, int64_t layer, const std::vector& blocking) {