OSDN Git Service

SurfaceFlinger: setGeometryAppliesWithResize crop latching fixes.
authorRobert Carr <racarr@google.com>
Tue, 9 May 2017 19:16:54 +0000 (12:16 -0700)
committerRobert Carr <racarr@google.com>
Mon, 15 May 2017 20:02:19 +0000 (13:02 -0700)
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. When we latch the buffer completing the resize...current hasn't been swapped
   to drawing...which means our location in LayerRejector.cpp was the wrong
   place to update the crop. This raises questions about whether the
   Transparent region latching works.

Bug: 37531386
Test: Included in Transaction_test.
Change-Id: I5140d44fd5e591a4afe5bddc201db45f7bcb5674

services/surfaceflinger/Layer.cpp
services/surfaceflinger/Layer.h
services/surfaceflinger/LayerRejecter.cpp
services/surfaceflinger/LayerRejecter.h
services/surfaceflinger/tests/Transaction_test.cpp

index 06a0765..50d8998 100644 (file)
@@ -103,7 +103,7 @@ Layer::Layer(SurfaceFlinger* flinger, const sp<Client>& client,
         mLastFrameNumberReceived(0),
         mUpdateTexImageFailed(false),
         mAutoRefresh(false),
-        mFreezePositionUpdates(false)
+        mFreezeGeometryUpdates(false)
 {
 #ifdef USE_HWC2
     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.crop.makeInvalid();
     mCurrentState.finalCrop.makeInvalid();
+    mCurrentState.requestedFinalCrop = mCurrentState.finalCrop;
+    mCurrentState.requestedCrop = mCurrentState.crop;
     mCurrentState.z = 0;
 #ifdef USE_HWC2
     mCurrentState.alpha = 1.0f;
@@ -1636,12 +1638,25 @@ uint32_t Layer::doTransaction(uint32_t flags) {
         }
     }
 
-    // always set active to requested, unless we're asked not to
-    // this is used by Layer, which special cases resizes.
-    if (flags & eDontUpdateGeometryState)  {
-    } else {
+    // 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)) {
         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 ty = c.active.transform.ty();
             c.active = c.requested;
@@ -1650,6 +1665,14 @@ 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;
+            }
         }
     }
 
@@ -1702,10 +1725,14 @@ 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 && !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);
     }
-    mFreezePositionUpdates = mFreezePositionUpdates || !immediate;
+    mFreezeGeometryUpdates = mFreezeGeometryUpdates || !immediate;
 
     mCurrentState.modified = true;
     setTransactionFlags(eTransactionNeeded);
@@ -1828,26 +1855,30 @@ bool Layer::setFlags(uint8_t flags, uint8_t mask) {
 }
 
 bool Layer::setCrop(const Rect& crop, bool immediate) {
-    if (mCurrentState.crop == crop)
+    if (mCurrentState.requestedCrop == crop)
         return false;
     mCurrentState.sequence++;
     mCurrentState.requestedCrop = crop;
-    if (immediate) {
+    if (immediate && !mFreezeGeometryUpdates) {
         mCurrentState.crop = crop;
     }
+    mFreezeGeometryUpdates = mFreezeGeometryUpdates || !immediate;
+
     mCurrentState.modified = true;
     setTransactionFlags(eTransactionNeeded);
     return true;
 }
 
 bool Layer::setFinalCrop(const Rect& crop, bool immediate) {
-    if (mCurrentState.finalCrop == crop)
+    if (mCurrentState.requestedFinalCrop == crop)
         return false;
     mCurrentState.sequence++;
     mCurrentState.requestedFinalCrop = crop;
-    if (immediate) {
+    if (immediate && !mFreezeGeometryUpdates) {
         mCurrentState.finalCrop = crop;
     }
+    mFreezeGeometryUpdates = mFreezeGeometryUpdates || !immediate;
+
     mCurrentState.modified = true;
     setTransactionFlags(eTransactionNeeded);
     return true;
@@ -2137,7 +2168,7 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions, nsecs_t latchTime)
     bool queuedBuffer = false;
     LayerRejecter r(mDrawingState, getCurrentState(), recomputeVisibleRegions,
                     getProducerStickyTransform() != 0, mName.string(),
-                    mOverrideScalingMode, mFreezePositionUpdates);
+                    mOverrideScalingMode, mFreezeGeometryUpdates);
     status_t updateResult = mSurfaceFlingerConsumer->updateTexImage(&r,
             mFlinger->mPrimaryDispSync, &mAutoRefresh, &queuedBuffer,
             mLastFrameNumberReceived);
index 9f45435..513ddff 100644 (file)
@@ -169,25 +169,64 @@ public:
     // the this layer's size and format
     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);
 
-    // 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).
+    // 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.
     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);
@@ -754,7 +793,7 @@ private:
     bool mUpdateTexImageFailed; // This is only accessed on the main thread.
 
     bool mAutoRefresh;
-    bool mFreezePositionUpdates;
+    bool mFreezeGeometryUpdates;
 
     // Child list about to be committed/used for editing.
     LayerVector mCurrentChildren;
index 0b302eb..b2424d0 100644 (file)
@@ -37,7 +37,7 @@ LayerRejecter::LayerRejecter(Layer::State& front,
     mStickyTransformSet(stickySet),
     mName(name),
     mOverrideScalingMode(overrideScalingMode),
-    mFreezePositionUpdates(freezePositionUpdates) {}
+    mFreezeGeometryUpdates(freezePositionUpdates) {}
 
 bool LayerRejecter::reject(const sp<GraphicBuffer>& buf, const BufferItem& item) {
     if (buf == NULL) {
@@ -115,17 +115,7 @@ bool LayerRejecter::reject(const sp<GraphicBuffer>& buf, const BufferItem& item)
         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;
+    mFreezeGeometryUpdates = false;
 
     return false;
 }
index c2a9483..828cd8b 100644 (file)
@@ -40,8 +40,8 @@ namespace android {
         bool mStickyTransformSet;
         const char *mName;
         int32_t mOverrideScalingMode;
-        bool &mFreezePositionUpdates;
+        bool &mFreezeGeometryUpdates;
     };
 }  // namespace android
 
-#endif  // ANDROID_LAYER_REJECTER_H
\ No newline at end of file
+#endif  // ANDROID_LAYER_REJECTER_H
index 1b17e55..eaf082d 100644 (file)
@@ -605,6 +605,27 @@ 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;
     {