Merge "Revert "SurfaceFlinger: setGeometryAppliesWithResize crop latching fixes."" into oc-dev

5 files changed, 34 insertions, 115 deletions

diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 50d8998f65..06a0765719 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -103,7 +103,7 @@ Layer::Layer(SurfaceFlinger* flinger, const sp<Client>& client,
         mLastFrameNumberReceived(0),
         mUpdateTexImageFailed(false),
         mAutoRefresh(false),
-        mFreezeGeometryUpdates(false)
+        mFreezePositionUpdates(false)
 {
 #ifdef USE_HWC2
     ALOGV("Creating Layer %s", name.string());
@@ -131,8 +131,6 @@ Layer::Layer(SurfaceFlinger* flinger, const sp<Client>& client,
     mCurrentState.active.transform.set(0, 0);
     mCurrentState.crop.makeInvalid();
     mCurrentState.finalCrop.makeInvalid();
-    mCurrentState.requestedFinalCrop = mCurrentState.finalCrop;
-    mCurrentState.requestedCrop = mCurrentState.crop;
     mCurrentState.z = 0;
 #ifdef USE_HWC2
     mCurrentState.alpha = 1.0f;
@@ -1638,25 +1636,12 @@ uint32_t Layer::doTransaction(uint32_t flags) {
         }
     }
 
-    // Here we apply various requested geometry states, depending on our
-    // latching configuration. See Layer.h for a detailed discussion of
-    // how geometry latching is controlled.
-    if (!(flags & eDontUpdateGeometryState)) {
+    // always set active to requested, unless we're asked not to
+    // this is used by Layer, which special cases resizes.
+    if (flags & eDontUpdateGeometryState)  {
+    } else {
         Layer::State& editCurrentState(getCurrentState());
-
-        // 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) {
+        if (mFreezePositionUpdates) {
             float tx = c.active.transform.tx();
             float ty = c.active.transform.ty();
             c.active = c.requested;
@@ -1665,14 +1650,6 @@ uint32_t Layer::doTransaction(uint32_t flags) {
         } else {
             editCurrentState.active = editCurrentState.requested;
             c.active = c.requested;
-            if (c.crop != c.requestedCrop ||
-                    c.finalCrop != c.requestedFinalCrop) {
-                c.sequence++;
-                c.crop = c.requestedCrop;
-                c.finalCrop = c.requestedFinalCrop;
-                editCurrentState.crop = c.crop;
-                editCurrentState.finalCrop = c.finalCrop;
-            }
         }
     }
 
@@ -1725,14 +1702,10 @@ bool Layer::setPosition(float x, float y, bool immediate) {
     // we want to apply the position portion of the transform matrix immediately,
     // but still delay scaling when resizing a SCALING_MODE_FREEZE layer.
     mCurrentState.requested.transform.set(x, y);
-    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
+    if (immediate && !mFreezePositionUpdates) {
         mCurrentState.active.transform.set(x, y);
     }
-    mFreezeGeometryUpdates = mFreezeGeometryUpdates || !immediate;
+    mFreezePositionUpdates = mFreezePositionUpdates || !immediate;
 
     mCurrentState.modified = true;
     setTransactionFlags(eTransactionNeeded);
@@ -1855,30 +1828,26 @@ bool Layer::setFlags(uint8_t flags, uint8_t mask) {
 }
 
 bool Layer::setCrop(const Rect& crop, bool immediate) {
-    if (mCurrentState.requestedCrop == crop)
+    if (mCurrentState.crop == crop)
         return false;
     mCurrentState.sequence++;
     mCurrentState.requestedCrop = crop;
-    if (immediate && !mFreezeGeometryUpdates) {
+    if (immediate) {
         mCurrentState.crop = crop;
     }
-    mFreezeGeometryUpdates = mFreezeGeometryUpdates || !immediate;
-
     mCurrentState.modified = true;
     setTransactionFlags(eTransactionNeeded);
     return true;
 }
 
 bool Layer::setFinalCrop(const Rect& crop, bool immediate) {
-    if (mCurrentState.requestedFinalCrop == crop)
+    if (mCurrentState.finalCrop == crop)
         return false;
     mCurrentState.sequence++;
     mCurrentState.requestedFinalCrop = crop;
-    if (immediate && !mFreezeGeometryUpdates) {
+    if (immediate) {
         mCurrentState.finalCrop = crop;
     }
-    mFreezeGeometryUpdates = mFreezeGeometryUpdates || !immediate;
-
     mCurrentState.modified = true;
     setTransactionFlags(eTransactionNeeded);
     return true;
@@ -2168,7 +2137,7 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions, nsecs_t latchTime)
     bool queuedBuffer = false;
     LayerRejecter r(mDrawingState, getCurrentState(), recomputeVisibleRegions,
                     getProducerStickyTransform() != 0, mName.string(),
-                    mOverrideScalingMode, mFreezeGeometryUpdates);
+                    mOverrideScalingMode, mFreezePositionUpdates);
     status_t updateResult = mSurfaceFlingerConsumer->updateTexImage(&r,
             mFlinger->mPrimaryDispSync, &mAutoRefresh, &queuedBuffer,
             mLastFrameNumberReceived);
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 513ddff62e..9f45435327 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -169,64 +169,25 @@ public:
     // the this layer's size and format
     status_t setBuffers(uint32_t w, uint32_t h, PixelFormat format, uint32_t flags);
 
-    // ------------------------------------------------------------------------
-    // 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);
+    // modify current state
 
-    // 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.
-    
-    // setPosition operates in parent buffer space (pre parent-transform) or display
-    // space for top-level layers.
+    // These members of state (position, crop, and finalCrop)
+    // may be updated immediately or have the update delayed
+    // until a pending surface resize completes (if applicable).
     bool setPosition(float x, float y, bool immediate);
-    // Buffer space
     bool setCrop(const Rect& crop, bool immediate);
-    // Parent buffer space/display space
     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 setRelativeLayer(const sp<IBinder>& relativeToHandle, int32_t relativeZ);
 
+    bool setSize(uint32_t w, uint32_t h);
 #ifdef USE_HWC2
     bool setAlpha(float alpha);
 #else
     bool setAlpha(uint8_t alpha);
 #endif
+    bool setMatrix(const layer_state_t::matrix22_t& matrix);
     bool setTransparentRegionHint(const Region& transparent);
     bool setFlags(uint8_t flags, uint8_t mask);
     bool setLayerStack(uint32_t layerStack);
@@ -793,7 +754,7 @@ private:
     bool mUpdateTexImageFailed; // This is only accessed on the main thread.
 
     bool mAutoRefresh;
-    bool mFreezeGeometryUpdates;
+    bool mFreezePositionUpdates;
 
     // Child list about to be committed/used for editing.
     LayerVector mCurrentChildren;
diff --git a/services/surfaceflinger/LayerRejecter.cpp b/services/surfaceflinger/LayerRejecter.cpp
index b2424d0ed2..0b302ebe03 100644
--- a/services/surfaceflinger/LayerRejecter.cpp
+++ b/services/surfaceflinger/LayerRejecter.cpp
@@ -37,7 +37,7 @@ LayerRejecter::LayerRejecter(Layer::State& front,
     mStickyTransformSet(stickySet),
     mName(name),
     mOverrideScalingMode(overrideScalingMode),
-    mFreezeGeometryUpdates(freezePositionUpdates) {}
+    mFreezePositionUpdates(freezePositionUpdates) {}
 
 bool LayerRejecter::reject(const sp<GraphicBuffer>& buf, const BufferItem& item) {
     if (buf == NULL) {
@@ -115,7 +115,17 @@ bool LayerRejecter::reject(const sp<GraphicBuffer>& buf, const BufferItem& item)
         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;
+    }
+    mFreezePositionUpdates = false;
 
     return false;
 }
diff --git a/services/surfaceflinger/LayerRejecter.h b/services/surfaceflinger/LayerRejecter.h
index 828cd8b96e..c2a948317d 100644
--- a/services/surfaceflinger/LayerRejecter.h
+++ b/services/surfaceflinger/LayerRejecter.h
@@ -40,8 +40,8 @@ namespace android {
         bool mStickyTransformSet;
         const char *mName;
         int32_t mOverrideScalingMode;
-        bool &mFreezeGeometryUpdates;
+        bool &mFreezePositionUpdates;
     };
 }  // namespace android
 
-#endif  // ANDROID_LAYER_REJECTER_H
+#endif  // ANDROID_LAYER_REJECTER_H
\ No newline at end of file
diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp
index eaf082d483..1b17e55b30 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -605,27 +605,6 @@ TEST_F(CropLatchingTest, FinalCropLatching) {
     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.
-    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) {
     sp<ScreenCapture> sc;
     {