SurfaceFlinger : Ensure position changes are drawn with correct buffer size

If a single transaction has both positional and size changes, ensure we don't draw
any frames using the incorrect buffer size using the updated position. Wait for the correct
buffer size and then proceed.

Change-Id: I8e25f21f17e0936e66bb5053f85f8336c8464c7b

4 files changed, 136 insertions, 3 deletions

diff --git a/services/surfaceflinger/CleanSpec.mk b/services/surfaceflinger/CleanSpec.mk
new file mode 100644
index 0000000..c46eaeb
--- /dev/null
+++ b/services/surfaceflinger/CleanSpec.mk
@@ -0,0 +1,51 @@
+# Copyright (C) 2016 The CyanogenMod Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# If you don't need to do a full clean build but would like to touch
+# a file or delete some intermediate files, add a clean step to the end
+# of the list.  These steps will only be run once, if they haven't been
+# run before.
+#
+# E.g.:
+#     $(call add-clean-step, touch -c external/sqlite/sqlite3.h)
+#     $(call add-clean-step, rm -rf $(PRODUCT_OUT)/obj/STATIC_LIBRARIES/libz_intermediates)
+#
+# Always use "touch -c" and "rm -f" or "rm -rf" to gracefully deal with
+# files that are missing or have been moved.
+#
+# Use $(PRODUCT_OUT) to get to the "out/target/product/blah/" directory.
+# Use $(OUT_DIR) to refer to the "out" directory.
+#
+# If you need to re-do something that's already mentioned, just copy
+# the command and add it to the bottom of the list.  E.g., if a change
+# that you made last week required touching a file and a change you
+# made today requires touching the same file, just copy the old
+# touch step and add it to the end of the list.
+#
+# ************************************************
+# NEWER CLEAN STEPS MUST BE AT THE END OF THE LIST
+# ************************************************
+
+# For example:
+#$(call add-clean-step, rm -rf $(OUT_DIR)/target/common/obj/APPS/AndroidTests_intermediates)
+#$(call add-clean-step, rm -rf $(OUT_DIR)/target/common/obj/JAVA_LIBRARIES/core_intermediates)
+#$(call add-clean-step, find $(OUT_DIR) -type f -name "IGTalkSession*" -print0 | xargs -0 rm -f)
+#$(call add-clean-step, rm -rf $(PRODUCT_OUT)/data/*)
+
+# ************************************************
+# NEWER CLEAN STEPS MUST BE AT THE END OF THE LIST
+# ************************************************
+$(call add-clean-step, rm -rf $(PRODUCT_OUT)/obj/SHARED_LIBRARIES/libsurfaceflinger_intermediates)
+$(call add-clean-step, rm -rf $(PRODUCT_OUT)/obj/SHARED_LIBRARIES/libsurfaceflinger_ddmconnection_intermediates)
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 6dd8bad..c346a2f 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -104,9 +104,12 @@ Layer::Layer(SurfaceFlinger* flinger, const sp<Client>& client,
 
     mName = name;
 
+    mCurrentState.active.x = 0;
+    mCurrentState.active.y = 0;
     mCurrentState.active.w = w;
     mCurrentState.active.h = h;
     mCurrentState.active.crop.makeInvalid();
+    mCurrentState.active.isPositionPending = false;
     mCurrentState.z = 0;
     mCurrentState.alpha = 0xFF;
     mCurrentState.blur = 0xFF;
@@ -1027,6 +1030,17 @@ uint32_t Layer::doTransaction(uint32_t flags) {
     if (flags & eDontUpdateGeometryState)  {
     } else {
         Layer::State& editCurrentState(getCurrentState());
+        // If a position change was requested, and we have the correct
+        // buffer size, no need to delay, update state now.
+        if (editCurrentState.requested.isPositionPending) {
+            float requestedX = editCurrentState.requested.x;
+            float requestedY = editCurrentState.requested.y;
+            if (requestedX != editCurrentState.active.x ||
+                requestedY != editCurrentState.active.y) {
+                editCurrentState.requested.isPositionPending = false;
+                editCurrentState.transform.set(requestedX, requestedY);
+            }
+        }
         editCurrentState.active = c.requested;
     }
 
@@ -1064,10 +1078,15 @@ uint32_t Layer::setTransactionFlags(uint32_t flags) {
 }
 
 bool Layer::setPosition(float x, float y) {
-    if (mCurrentState.transform.tx() == x && mCurrentState.transform.ty() == y)
+    if ((mCurrentState.transform.tx() == x && mCurrentState.transform.ty() == y
+        && !mCurrentState.requested.isPositionPending) ||
+        (mCurrentState.requested.isPositionPending && mCurrentState.requested.x == x
+            && mCurrentState.requested.y == y))
         return false;
     mCurrentState.sequence++;
-    mCurrentState.transform.set(x, y);
+    mCurrentState.requested.x = x;
+    mCurrentState.requested.y = y;
+    mCurrentState.requested.isPositionPending = true;
     setTransactionFlags(eTransactionNeeded);
     return true;
 }
@@ -1290,6 +1309,19 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions)
                             (bufWidth == front.requested.w &&
                              bufHeight == front.requested.h))
                     {
+
+                        // If a position change was requested along with a resize.
+                        // Now that we have the correct buffer size, update the position as well.
+                        if (current.requested.isPositionPending) {
+                            float requestedX = current.requested.x;
+                            float requestedY = current.requested.y;
+                            if (requestedX != current.active.x || requestedY != current.active.y) {
+                                front.transform.set(requestedX, requestedY);
+                                current.transform.set(requestedX, requestedY);
+                                current.requested.isPositionPending = false;
+                            }
+                        }
+
                         // Here we pretend the transaction happened by updating the
                         // current and drawing states. Drawing state is only accessed
                         // in this thread, no need to have it locked
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 196ef3e..02d6f16 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -95,11 +95,15 @@ public:
     };
 
     struct Geometry {
+        float x;
+        float y;
         uint32_t w;
         uint32_t h;
+        bool isPositionPending;
         Rect crop;
         inline bool operator ==(const Geometry& rhs) const {
-            return (w == rhs.w && h == rhs.h && crop == rhs.crop);
+            return (w == rhs.w && h == rhs.h && crop == rhs.crop && x == rhs.x && y == rhs.y
+                && isPositionPending == rhs.isPositionPending);
         }
         inline bool operator !=(const Geometry& rhs) const {
             return !operator ==(rhs);
diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp
index dcde512..2ef2a50 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -249,4 +249,50 @@ TEST_F(LayerUpdateTest, LayerResizeWorks) {
     }
 }
 
+// Ensure that if we move and resize a surface in the same
+// transaction, we don't reposition the surface and draw
+// using the incorrect buffer size
+TEST_F(LayerUpdateTest, LayerMoveAndResizeWorks) {
+    sp<ScreenCapture> sc;
+    {
+        SCOPED_TRACE("before resize and reposition");
+        ScreenCapture::captureScreen(&sc);
+        sc->checkPixel(  0,  12,  63,  63, 195);
+        sc->checkPixel( 75,  75, 195,  63,  63);
+        sc->checkPixel(145, 145,  63,  63, 195);
+    }
+
+    ALOGD("resizing and repositioning");
+    SurfaceComposerClient::openGlobalTransaction();
+    ASSERT_EQ(NO_ERROR, mFGSurfaceControl->setPosition(64, 0));
+    ASSERT_EQ(NO_ERROR, mFGSurfaceControl->setSize(64, 128));
+    SurfaceComposerClient::closeGlobalTransaction(true);
+
+    ALOGD("resized and repositioned");
+    {
+        // This should not reflect the new size, position or color because SurfaceFlinger
+        // has not yet received a buffer of the correct size.
+        SCOPED_TRACE("after resize, before redraw");
+        ScreenCapture::captureScreen(&sc);
+        sc->checkPixel(  0,  12,  63,  63, 195);
+        sc->checkPixel( 75,  75, 195,  63,  63);
+        sc->checkPixel(145, 145,  63,  63, 195);
+    }
+
+    ALOGD("drawing");
+    fillSurfaceRGBA8(mFGSurfaceControl, 63, 195, 63);
+    waitForPostedBuffers();
+    ALOGD("drawn");
+    {
+        // This should reflect the new size, position and the new color.
+        SCOPED_TRACE("after redraw");
+        ScreenCapture::captureScreen(&sc);
+        sc->checkPixel( 64,  0, 63, 195, 63);
+        // This should pass to imply that we didn't have a frame where the
+        // surface was moved but not yet resized even though the operations
+        // were part of the same transaction
+        sc->checkPixel( 64, 75, 63, 195, 63);
+        sc->checkPixel(145, 145,  63, 63, 195);
+    }
+}
 }