Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 28f320b4 authored by Chia-I Wu's avatar Chia-I Wu
Browse files

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
parent a296b0c2

services/surfaceflinger/SurfaceFlinger.cpp

+46 −36
Original line number Diff line number Diff line
@@ -244,7 +244,6 @@ SurfaceFlinger::SurfaceFlinger(SurfaceFlinger::SkipInitializationTag) 
        mPrimaryDispSync("PrimaryDispSync"),

        mPrimaryHWVsyncEnabled(false),

        mHWVsyncAvailable(false),

        mHasColorMatrix(false),

        mHasPoweredOff(false),

        mNumLayers(0),

        mVrFlingerRequestsDisplay(false),
@@ -723,10 +722,13 @@ void SurfaceFlinger::init() { 
}



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 @@ void SurfaceFlinger::rebuildLayerStacks() { 
    }

}



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 @@ void SurfaceFlinger::setUpHWComposer() { 
        }

    }





    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 @@ void SurfaceFlinger::setUpHWComposer() { 
        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 @@ void SurfaceFlinger::setUpHWComposer() { 
        }

    }



    mPreviousColorMatrix = colorMatrix;

    mDrawingState.colorMatrixChanged = false;



    for (size_t displayId = 0; displayId < mDisplays.size(); ++displayId) {

        auto& displayDevice = mDisplays[displayId];
@@ -2635,6 +2618,9 @@ void SurfaceFlinger::commitTransaction() 
    mAnimCompositionPending = mAnimTransactionPending;



    mDrawingState = mCurrentState;

    // clear the "changed" flags in current state

    mCurrentState.colorMatrixChanged = false;



    mDrawingState.traverseInZOrder([](Layer* layer) {

        layer->commitChildList();

    });
@@ -2887,9 +2873,8 @@ bool SurfaceFlinger::doComposeSurfaces(const sp<const DisplayDevice>& displayDev 
    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 @@ bool SurfaceFlinger::startDdmConnection() 
    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 @@ status_t SurfaceFlinger::onTransact( 
                return NO_ERROR;

            }

            case 1014: {

                Mutex::Autolock _l(mStateLock);

                // daltonize

                n = data.readInt32();

                switch (n % 10) {
@@ -4504,33 +4514,33 @@ status_t SurfaceFlinger::onTransact( 
                } 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 @@ status_t SurfaceFlinger::onTransact( 
                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

services/surfaceflinger/SurfaceFlinger.h

+14 −10
Original line number Diff line number Diff line
@@ -372,6 +372,10 @@ private: 
            // always uses the Drawing StateSet.

            layersSortedByZ = other.layersSortedByZ;

            displays = other.displays;

            colorMatrixChanged = other.colorMatrixChanged;

            if (colorMatrixChanged) {

                colorMatrix = other.colorMatrix;

            }

            return *this;

        }
@@ -379,6 +383,9 @@ private: 
        LayerVector layersSortedByZ;

        DefaultKeyedVector< wp<IBinder>, DisplayDeviceState> displays;



        bool colorMatrixChanged = true;

        mat4 colorMatrix;



        void traverseInZOrder(const LayerVector::Visitor& visitor) const;

        void traverseInReverseZOrder(const LayerVector::Visitor& visitor) const;

    };
@@ -652,8 +659,6 @@ private: 
                       ui::ColorMode* outMode,

                       ui::Dataspace* outDataSpace) const;



    mat4 computeSaturationMatrix() const;



    void setUpHWComposer();

    void doComposition();

    void doDebugFlashRegions();
@@ -740,6 +745,8 @@ private: 
    // Check to see if we should handoff to vr flinger.

    void updateVrFlinger();



    void updateColorMatrixLocked();



    /* ------------------------------------------------------------------------

     * Attributes

     */
@@ -753,6 +760,11 @@ private: 
    bool mAnimTransactionPending;

    SortedVector< sp<Layer> > mLayersPendingRemoval;



    // global color transform states

    Daltonizer mDaltonizer;

    float mGlobalSaturationFactor = 1.0f;

    mat4 mClientColorMatrix;



    // Can't be unordered_set because wp<> isn't hashable

    std::set<wp<IBinder>> mGraphicBufferProducerList;

    size_t mMaxGraphicBufferProducerListSize = MAX_LAYERS;
@@ -842,12 +854,6 @@ private: 


    bool mInjectVSyncs;



    Daltonizer mDaltonizer;



    mat4 mPreviousColorMatrix;

    mat4 mColorMatrix;

    bool mHasColorMatrix;



    // Static screen stats

    bool mHasPoweredOff;
@@ -865,8 +871,6 @@ private: 
    DisplayColorSetting mDisplayColorSetting = DisplayColorSetting::MANAGED;

    // Applied on sRGB layers when the render intent is non-colorimetric.

    mat4 mLegacySrgbSaturationMatrix;

    // Applied globally.

    float mGlobalSaturationFactor = 1.0f;

    bool mBuiltinDisplaySupportsEnhance = false;



    using CreateBufferQueueFunction =