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

Commit 7bf247e2 authored by Robert Carr's avatar Robert Carr
Browse files

SurfaceFlinger: setGeometryAppliesWithResize crop latching fixes.

The same sort of thing we had with setPosition...not sure why I didn't
realize we would need the fixes here too! In particular we need to ensure
the following scenarios work:

1. Additional calls to set(Final)Crop while in the setGeometryAppliesWithResize
   state are eventually applied.
2. Additional calls to set(Final)Crop while in the setGeometryAppliesWithResize
   state are not immediately applied.
3. In LayerRejector.cpp we have to be sure we are not just latching a buffer
   at the old size, which we still allow. This is the correct time to latch
   the transparentRegion as it is content dependent, but doesn't represent
   a size changing.

The difference between this and the original CL which was reverted has to do with
point 3. The original CL tried to solve point 3 by moving the latching logic from
the LayerRejecter in to Layer::doTransaction. However, in general doTransaction
will not be called in between Latching the buffer and drawing the frame, so this
introduced errors. The new test "FinalCropLatchingBufferOldSize" encapsulates this.

Bug: 37621737
Bug: 37531386
Test: Included in Transaction_test.cpp
Change-Id: I14bd09d01ac6b85895caa1b707d6fa7dac962074
parent 3851225f
Loading
Loading
Loading
Loading
+36 −13
Original line number Original line Diff line number Diff line
@@ -103,7 +103,7 @@ Layer::Layer(SurfaceFlinger* flinger, const sp<Client>& client,
        mLastFrameNumberReceived(0),
        mLastFrameNumberReceived(0),
        mUpdateTexImageFailed(false),
        mUpdateTexImageFailed(false),
        mAutoRefresh(false),
        mAutoRefresh(false),
        mFreezePositionUpdates(false)
        mFreezeGeometryUpdates(false)
{
{
#ifdef USE_HWC2
#ifdef USE_HWC2
    ALOGV("Creating Layer %s", name.string());
    ALOGV("Creating Layer %s", name.string());
@@ -131,6 +131,8 @@ Layer::Layer(SurfaceFlinger* flinger, const sp<Client>& client,
    mCurrentState.active.transform.set(0, 0);
    mCurrentState.active.transform.set(0, 0);
    mCurrentState.crop.makeInvalid();
    mCurrentState.crop.makeInvalid();
    mCurrentState.finalCrop.makeInvalid();
    mCurrentState.finalCrop.makeInvalid();
    mCurrentState.requestedFinalCrop = mCurrentState.finalCrop;
    mCurrentState.requestedCrop = mCurrentState.crop;
    mCurrentState.z = 0;
    mCurrentState.z = 0;
#ifdef USE_HWC2
#ifdef USE_HWC2
    mCurrentState.alpha = 1.0f;
    mCurrentState.alpha = 1.0f;
@@ -1641,12 +1643,25 @@ uint32_t Layer::doTransaction(uint32_t flags) {
        }
        }
    }
    }


    // always set active to requested, unless we're asked not to
    // Here we apply various requested geometry states, depending on our
    // this is used by Layer, which special cases resizes.
    // latching configuration. See Layer.h for a detailed discussion of
    if (flags & eDontUpdateGeometryState)  {
    // how geometry latching is controlled.
    } else {
    if (!(flags & eDontUpdateGeometryState)) {
        Layer::State& editCurrentState(getCurrentState());
        Layer::State& editCurrentState(getCurrentState());
        if (mFreezePositionUpdates) {

        // If mFreezeGeometryUpdates is true we are in the setGeometryAppliesWithResize
        // mode, which causes attributes which normally latch regardless of scaling mode,
        // to be delayed. We copy the requested state to the active state making sure
        // to respect these rules (again see Layer.h for a detailed discussion).
        //
        // There is an awkward asymmetry in the handling of the crop states in the position
        // states, as can be seen below. Largely this arises from position and transform
        // being stored in the same data structure while having different latching rules.
        // b/38182305
        //
        // Careful that "c" and editCurrentState may not begin as equivalent due to
        // applyPendingStates in the presence of deferred transactions.
        if (mFreezeGeometryUpdates) {
            float tx = c.active.transform.tx();
            float tx = c.active.transform.tx();
            float ty = c.active.transform.ty();
            float ty = c.active.transform.ty();
            c.active = c.requested;
            c.active = c.requested;
@@ -1707,10 +1722,14 @@ bool Layer::setPosition(float x, float y, bool immediate) {
    // we want to apply the position portion of the transform matrix immediately,
    // we want to apply the position portion of the transform matrix immediately,
    // but still delay scaling when resizing a SCALING_MODE_FREEZE layer.
    // but still delay scaling when resizing a SCALING_MODE_FREEZE layer.
    mCurrentState.requested.transform.set(x, y);
    mCurrentState.requested.transform.set(x, y);
    if (immediate && !mFreezePositionUpdates) {
    if (immediate && !mFreezeGeometryUpdates) {
        // Here we directly update the active state
        // unlike other setters, because we store it within
        // the transform, but use different latching rules.
        // b/38182305
        mCurrentState.active.transform.set(x, y);
        mCurrentState.active.transform.set(x, y);
    }
    }
    mFreezePositionUpdates = mFreezePositionUpdates || !immediate;
    mFreezeGeometryUpdates = mFreezeGeometryUpdates || !immediate;


    mCurrentState.modified = true;
    mCurrentState.modified = true;
    setTransactionFlags(eTransactionNeeded);
    setTransactionFlags(eTransactionNeeded);
@@ -1833,26 +1852,30 @@ bool Layer::setFlags(uint8_t flags, uint8_t mask) {
}
}


bool Layer::setCrop(const Rect& crop, bool immediate) {
bool Layer::setCrop(const Rect& crop, bool immediate) {
    if (mCurrentState.crop == crop)
    if (mCurrentState.requestedCrop == crop)
        return false;
        return false;
    mCurrentState.sequence++;
    mCurrentState.sequence++;
    mCurrentState.requestedCrop = crop;
    mCurrentState.requestedCrop = crop;
    if (immediate) {
    if (immediate && !mFreezeGeometryUpdates) {
        mCurrentState.crop = crop;
        mCurrentState.crop = crop;
    }
    }
    mFreezeGeometryUpdates = mFreezeGeometryUpdates || !immediate;

    mCurrentState.modified = true;
    mCurrentState.modified = true;
    setTransactionFlags(eTransactionNeeded);
    setTransactionFlags(eTransactionNeeded);
    return true;
    return true;
}
}


bool Layer::setFinalCrop(const Rect& crop, bool immediate) {
bool Layer::setFinalCrop(const Rect& crop, bool immediate) {
    if (mCurrentState.finalCrop == crop)
    if (mCurrentState.requestedFinalCrop == crop)
        return false;
        return false;
    mCurrentState.sequence++;
    mCurrentState.sequence++;
    mCurrentState.requestedFinalCrop = crop;
    mCurrentState.requestedFinalCrop = crop;
    if (immediate) {
    if (immediate && !mFreezeGeometryUpdates) {
        mCurrentState.finalCrop = crop;
        mCurrentState.finalCrop = crop;
    }
    }
    mFreezeGeometryUpdates = mFreezeGeometryUpdates || !immediate;

    mCurrentState.modified = true;
    mCurrentState.modified = true;
    setTransactionFlags(eTransactionNeeded);
    setTransactionFlags(eTransactionNeeded);
    return true;
    return true;
@@ -2142,7 +2165,7 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions, nsecs_t latchTime)
    bool queuedBuffer = false;
    bool queuedBuffer = false;
    LayerRejecter r(mDrawingState, getCurrentState(), recomputeVisibleRegions,
    LayerRejecter r(mDrawingState, getCurrentState(), recomputeVisibleRegions,
                    getProducerStickyTransform() != 0, mName.string(),
                    getProducerStickyTransform() != 0, mName.string(),
                    mOverrideScalingMode, mFreezePositionUpdates);
                    mOverrideScalingMode, mFreezeGeometryUpdates);
    status_t updateResult = mSurfaceFlingerConsumer->updateTexImage(&r,
    status_t updateResult = mSurfaceFlingerConsumer->updateTexImage(&r,
            mFlinger->mPrimaryDispSync, &mAutoRefresh, &queuedBuffer,
            mFlinger->mPrimaryDispSync, &mAutoRefresh, &queuedBuffer,
            mLastFrameNumberReceived);
            mLastFrameNumberReceived);
+46 −7
Original line number Original line Diff line number Diff line
@@ -169,25 +169,64 @@ public:
    // the this layer's size and format
    // the this layer's size and format
    status_t setBuffers(uint32_t w, uint32_t h, PixelFormat format, uint32_t flags);
    status_t setBuffers(uint32_t w, uint32_t h, PixelFormat format, uint32_t flags);


    // modify current state
    // ------------------------------------------------------------------------
    // Geometry setting functions.
    //
    // The following group of functions are used to specify the layers
    // bounds, and the mapping of the texture on to those bounds. According
    // to various settings changes to them may apply immediately, or be delayed until
    // a pending resize is completed by the producer submitting a buffer. For example
    // if we were to change the buffer size, and update the matrix ahead of the
    // new buffer arriving, then we would be stretching the buffer to a different
    // aspect before and after the buffer arriving, which probably isn't what we wanted.
    //
    // The first set of geometry functions are controlled by the scaling mode, described
    // in window.h. The scaling mode may be set by the client, as it submits buffers.
    // This value may be overriden through SurfaceControl, with setOverrideScalingMode.
    //
    // Put simply, if our scaling mode is SCALING_MODE_FREEZE, then
    // matrix updates will not be applied while a resize is pending
    // and the size and transform will remain in their previous state
    // until a new buffer is submitted. If the scaling mode is another value
    // then the old-buffer will immediately be scaled to the pending size
    // and the new matrix will be immediately applied following this scaling
    // transformation.

    // Set the default buffer size for the assosciated Producer, in pixels. This is
    // also the rendered size of the layer prior to any transformations. Parent
    // or local matrix transformations will not affect the size of the buffer,
    // but may affect it's on-screen size or clipping.
    bool setSize(uint32_t w, uint32_t h);
    // Set a 2x2 transformation matrix on the layer. This transform
    // will be applied after parent transforms, but before any final
    // producer specified transform.
    bool setMatrix(const layer_state_t::matrix22_t& matrix);

    // This second set of geometry attributes are controlled by
    // setGeometryAppliesWithResize, and their default mode is to be
    // immediate. If setGeometryAppliesWithResize is specified
    // while a resize is pending, then update of these attributes will
    // be delayed until the resize completes.


    // These members of state (position, crop, and finalCrop)
    // setPosition operates in parent buffer space (pre parent-transform) or display
    // may be updated immediately or have the update delayed
    // space for top-level layers.
    // until a pending surface resize completes (if applicable).
    bool setPosition(float x, float y, bool immediate);
    bool setPosition(float x, float y, bool immediate);
    // Buffer space
    bool setCrop(const Rect& crop, bool immediate);
    bool setCrop(const Rect& crop, bool immediate);
    // Parent buffer space/display space
    bool setFinalCrop(const Rect& crop, bool immediate);
    bool setFinalCrop(const Rect& crop, bool immediate);


    // TODO(b/38182121): Could we eliminate the various latching modes by
    // using the layer hierarchy?
    // -----------------------------------------------------------------------
    bool setLayer(int32_t z);
    bool setLayer(int32_t z);
    bool setRelativeLayer(const sp<IBinder>& relativeToHandle, int32_t relativeZ);
    bool setRelativeLayer(const sp<IBinder>& relativeToHandle, int32_t relativeZ);


    bool setSize(uint32_t w, uint32_t h);
#ifdef USE_HWC2
#ifdef USE_HWC2
    bool setAlpha(float alpha);
    bool setAlpha(float alpha);
#else
#else
    bool setAlpha(uint8_t alpha);
    bool setAlpha(uint8_t alpha);
#endif
#endif
    bool setMatrix(const layer_state_t::matrix22_t& matrix);
    bool setTransparentRegionHint(const Region& transparent);
    bool setTransparentRegionHint(const Region& transparent);
    bool setFlags(uint8_t flags, uint8_t mask);
    bool setFlags(uint8_t flags, uint8_t mask);
    bool setLayerStack(uint32_t layerStack);
    bool setLayerStack(uint32_t layerStack);
@@ -754,7 +793,7 @@ private:
    bool mUpdateTexImageFailed; // This is only accessed on the main thread.
    bool mUpdateTexImageFailed; // This is only accessed on the main thread.


    bool mAutoRefresh;
    bool mAutoRefresh;
    bool mFreezePositionUpdates;
    bool mFreezeGeometryUpdates;


    // Child list about to be committed/used for editing.
    // Child list about to be committed/used for editing.
    LayerVector mCurrentChildren;
    LayerVector mCurrentChildren;
+18 −13
Original line number Original line Diff line number Diff line
@@ -37,7 +37,7 @@ LayerRejecter::LayerRejecter(Layer::State& front,
    mStickyTransformSet(stickySet),
    mStickyTransformSet(stickySet),
    mName(name),
    mName(name),
    mOverrideScalingMode(overrideScalingMode),
    mOverrideScalingMode(overrideScalingMode),
    mFreezePositionUpdates(freezePositionUpdates) {}
    mFreezeGeometryUpdates(freezePositionUpdates) {}


bool LayerRejecter::reject(const sp<GraphicBuffer>& buf, const BufferItem& item) {
bool LayerRejecter::reject(const sp<GraphicBuffer>& buf, const BufferItem& item) {
    if (buf == NULL) {
    if (buf == NULL) {
@@ -73,6 +73,19 @@ bool LayerRejecter::reject(const sp<GraphicBuffer>& buf, const BufferItem& item)


            // recompute visible region
            // recompute visible region
            mRecomputeVisibleRegions = true;
            mRecomputeVisibleRegions = true;

            mFreezeGeometryUpdates = false;

            if (mFront.crop != mFront.requestedCrop) {
                mFront.crop = mFront.requestedCrop;
                mCurrent.crop = mFront.requestedCrop;
                mRecomputeVisibleRegions = true;
            }
            if (mFront.finalCrop != mFront.requestedFinalCrop) {
                mFront.finalCrop = mFront.requestedFinalCrop;
                mCurrent.finalCrop = mFront.requestedFinalCrop;
                mRecomputeVisibleRegions = true;
            }
        }
        }


        ALOGD_IF(DEBUG_RESIZE,
        ALOGD_IF(DEBUG_RESIZE,
@@ -100,6 +113,10 @@ bool LayerRejecter::reject(const sp<GraphicBuffer>& buf, const BufferItem& item)
    // conservative, but that's fine, worst case we're doing
    // conservative, but that's fine, worst case we're doing
    // a bit of extra work), we latch the new one and we
    // a bit of extra work), we latch the new one and we
    // trigger a visible-region recompute.
    // trigger a visible-region recompute.
    //
    // We latch the transparent region here, instead of above where we latch
    // the rest of the geometry because it is only content but not necessarily
    // resize dependent.
    if (!mFront.activeTransparentRegion.isTriviallyEqual(mFront.requestedTransparentRegion)) {
    if (!mFront.activeTransparentRegion.isTriviallyEqual(mFront.requestedTransparentRegion)) {
        mFront.activeTransparentRegion = mFront.requestedTransparentRegion;
        mFront.activeTransparentRegion = mFront.requestedTransparentRegion;


@@ -115,18 +132,6 @@ bool LayerRejecter::reject(const sp<GraphicBuffer>& buf, const BufferItem& item)
        mRecomputeVisibleRegions = true;
        mRecomputeVisibleRegions = true;
    }
    }


    if (mFront.crop != mFront.requestedCrop) {
        mFront.crop = mFront.requestedCrop;
        mCurrent.crop = mFront.requestedCrop;
        mRecomputeVisibleRegions = true;
    }
    if (mFront.finalCrop != mFront.requestedFinalCrop) {
        mFront.finalCrop = mFront.requestedFinalCrop;
        mCurrent.finalCrop = mFront.requestedFinalCrop;
        mRecomputeVisibleRegions = true;
    }
    mFreezePositionUpdates = false;

    return false;
    return false;
}
}


+2 −2
Original line number Original line Diff line number Diff line
@@ -40,7 +40,7 @@ namespace android {
        bool mStickyTransformSet;
        bool mStickyTransformSet;
        const char *mName;
        const char *mName;
        int32_t mOverrideScalingMode;
        int32_t mOverrideScalingMode;
        bool &mFreezePositionUpdates;
        bool &mFreezeGeometryUpdates;
    };
    };
}  // namespace android
}  // namespace android


+74 −2
Original line number Original line Diff line number Diff line
@@ -33,7 +33,7 @@ namespace android {


// Fill an RGBA_8888 formatted surface with a single color.
// Fill an RGBA_8888 formatted surface with a single color.
static void fillSurfaceRGBA8(const sp<SurfaceControl>& sc,
static void fillSurfaceRGBA8(const sp<SurfaceControl>& sc,
        uint8_t r, uint8_t g, uint8_t b) {
        uint8_t r, uint8_t g, uint8_t b, bool unlock=true) {
    ANativeWindow_Buffer outBuffer;
    ANativeWindow_Buffer outBuffer;
    sp<Surface> s = sc->getSurface();
    sp<Surface> s = sc->getSurface();
    ASSERT_TRUE(s != NULL);
    ASSERT_TRUE(s != NULL);
@@ -48,8 +48,10 @@ static void fillSurfaceRGBA8(const sp<SurfaceControl>& sc,
            pixel[3] = 255;
            pixel[3] = 255;
        }
        }
    }
    }
    if (unlock) {
        ASSERT_EQ(NO_ERROR, s->unlockAndPost());
        ASSERT_EQ(NO_ERROR, s->unlockAndPost());
    }
    }
}


// A ScreenCapture is a screenshot from SurfaceFlinger that can be used to check
// A ScreenCapture is a screenshot from SurfaceFlinger that can be used to check
// individual pixel values for testing purposes.
// individual pixel values for testing purposes.
@@ -479,6 +481,17 @@ protected:
        sc->expectFGColor(127, 127);
        sc->expectFGColor(127, 127);
        sc->expectBGColor(128, 128);
        sc->expectBGColor(128, 128);
    }
    }

    void lockAndFillFGBuffer() {
        fillSurfaceRGBA8(mFGSurfaceControl, 195, 63, 63, false);
    }

    void unlockFGBuffer() {
        sp<Surface> s = mFGSurfaceControl->getSurface();
        ASSERT_EQ(NO_ERROR, s->unlockAndPost());
        waitForPostedBuffers();
    }

    void completeFGResize() {
    void completeFGResize() {
        fillSurfaceRGBA8(mFGSurfaceControl, 195, 63, 63);
        fillSurfaceRGBA8(mFGSurfaceControl, 195, 63, 63);
        waitForPostedBuffers();
        waitForPostedBuffers();
@@ -605,6 +618,65 @@ TEST_F(CropLatchingTest, FinalCropLatching) {
    EXPECT_CROPPED_STATE("after the resize finishes");
    EXPECT_CROPPED_STATE("after the resize finishes");
}
}


// In this test we ensure that setGeometryAppliesWithResize actually demands
// a buffer of the new size, and not just any size.
TEST_F(CropLatchingTest, FinalCropLatchingBufferOldSize) {
    EXPECT_INITIAL_STATE("before anything");
    // Normally the crop applies immediately even while a resize is pending.
    SurfaceComposerClient::openGlobalTransaction();
    mFGSurfaceControl->setSize(128, 128);
    mFGSurfaceControl->setFinalCrop(Rect(64, 64, 127, 127));
    SurfaceComposerClient::closeGlobalTransaction(true);

    EXPECT_CROPPED_STATE("after setting crop (without geometryAppliesWithResize)");

    restoreInitialState();

    // In order to prepare to submit a buffer at the wrong size, we acquire it prior to
    // initiating the resize.
    lockAndFillFGBuffer();

    SurfaceComposerClient::openGlobalTransaction();
    mFGSurfaceControl->setSize(128, 128);
    mFGSurfaceControl->setGeometryAppliesWithResize();
    mFGSurfaceControl->setFinalCrop(Rect(64, 64, 127, 127));
    SurfaceComposerClient::closeGlobalTransaction(true);

    EXPECT_INITIAL_STATE("after setting crop (with geometryAppliesWithResize)");

    // We now submit our old buffer, at the old size, and ensure it doesn't
    // trigger geometry latching.
    unlockFGBuffer();

    EXPECT_INITIAL_STATE("after unlocking FG buffer (with geometryAppliesWithResize)");

    completeFGResize();

    EXPECT_CROPPED_STATE("after the resize finishes");
}

TEST_F(CropLatchingTest, FinalCropLatchingRegressionForb37531386) {
    EXPECT_INITIAL_STATE("before anything");
    // In this scenario, we attempt to set the final crop a second time while the resize
    // is still pending, and ensure we are successful. Success meaning the second crop
    // is the one which eventually latches and not the first.
    SurfaceComposerClient::openGlobalTransaction();
    mFGSurfaceControl->setSize(128, 128);
    mFGSurfaceControl->setGeometryAppliesWithResize();
    mFGSurfaceControl->setFinalCrop(Rect(64, 64, 127, 127));
    SurfaceComposerClient::closeGlobalTransaction(true);

    SurfaceComposerClient::openGlobalTransaction();
    mFGSurfaceControl->setFinalCrop(Rect(0, 0, -1, -1));
    SurfaceComposerClient::closeGlobalTransaction(true);

    EXPECT_INITIAL_STATE("after setting crops with geometryAppliesWithResize");

    completeFGResize();

    EXPECT_INITIAL_STATE("after the resize finishes");
}

TEST_F(LayerUpdateTest, DeferredTransactionTest) {
TEST_F(LayerUpdateTest, DeferredTransactionTest) {
    sp<ScreenCapture> sc;
    sp<ScreenCapture> sc;
    {
    {