better fix for [2420565] Surface.lockCanvas() updates the dirty region too often

Change-Id: I83438b40effd21538f1c74396dc665254b9d5ab6

5 files changed, 44 insertions, 38 deletions

diff --git a/include/surfaceflinger/Surface.h b/include/surfaceflinger/Surface.h
index 68809a9..40dc467 100644
--- a/include/surfaceflinger/Surface.h
+++ b/include/surfaceflinger/Surface.h
@@ -250,7 +250,7 @@ private:
     sp<GraphicBuffer>           mLockedBuffer;
     sp<GraphicBuffer>           mPostedBuffer;
     mutable Region              mOldDirtyRegion;
-    bool                        mNeedFullUpdate;
+    bool                        mReserved;
 
     // query() must be called from dequeueBuffer() thread
     uint32_t                    mWidth;
diff --git a/libs/surfaceflinger/Layer.cpp b/libs/surfaceflinger/Layer.cpp
index 0a3254d..1a66970 100644
--- a/libs/surfaceflinger/Layer.cpp
+++ b/libs/surfaceflinger/Layer.cpp
@@ -540,9 +540,15 @@ void Layer::lockPageFlip(bool& recomputeVisibleRegions)
         mFlinger->signalEvent();
     }
 
-    if (!mPostedDirtyRegion.isEmpty()) {
-        reloadTexture( mPostedDirtyRegion );
-    }
+    /* a buffer was posted, so we need to call reloadTexture(), which
+     * will update our internal data structures (eg: EGLImageKHR or
+     * texture names). we need to do this even if mPostedDirtyRegion is
+     * empty -- it's orthogonal to the fact that a new buffer was posted,
+     * for instance, a degenerate case could be that the user did an empty
+     * update but repainted the buffer with appropriate content (after a
+     * resize for instance).
+     */
+    reloadTexture( mPostedDirtyRegion );
 }
 
 void Layer::unlockPageFlip(
diff --git a/libs/surfaceflinger/LayerBase.cpp b/libs/surfaceflinger/LayerBase.cpp
index 6dc8f10..7feccda 100644
--- a/libs/surfaceflinger/LayerBase.cpp
+++ b/libs/surfaceflinger/LayerBase.cpp
@@ -54,7 +54,7 @@ LayerBase::LayerBase(SurfaceFlinger* flinger, DisplayID display)
       mOrientation(0),
       mLeft(0), mTop(0),
       mTransactionFlags(0),
-      mPremultipliedAlpha(true), mDebug(false),
+      mPremultipliedAlpha(true), mName("unnamed"), mDebug(false),
       mInvalidate(0)
 {
     const DisplayHardware& hw(flinger->graphicPlane(0).displayHardware());
diff --git a/libs/surfaceflinger_client/SharedBufferStack.cpp b/libs/surfaceflinger_client/SharedBufferStack.cpp
index 881ffd4..f1aec8a 100644
--- a/libs/surfaceflinger_client/SharedBufferStack.cpp
+++ b/libs/surfaceflinger_client/SharedBufferStack.cpp
@@ -84,10 +84,14 @@ status_t SharedBufferStack::setDirtyRegion(int buffer, const Region& dirty)
     if (uint32_t(buffer) >= NUM_BUFFER_MAX)
         return BAD_INDEX;
 
-    // in the current implementation we only send a single rectangle
+    FlatRegion& reg(buffers[buffer].dirtyRegion);
+    if (dirty.isEmpty()) {
+        reg.count = 0;
+        return NO_ERROR;
+    }
+
     size_t count;
     Rect const* r = dirty.getArray(&count);
-    FlatRegion& reg(buffers[buffer].dirtyRegion);
     if (count > FlatRegion::NUM_RECT_MAX) {
         const Rect bounds(dirty.getBounds());
         reg.count = 1;
diff --git a/libs/surfaceflinger_client/Surface.cpp b/libs/surfaceflinger_client/Surface.cpp
index 39938e8..057d211 100644
--- a/libs/surfaceflinger_client/Surface.cpp
+++ b/libs/surfaceflinger_client/Surface.cpp
@@ -17,8 +17,6 @@
 #define LOG_TAG "Surface"
 
 #include <stdint.h>
-#include <unistd.h>
-#include <fcntl.h>
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -28,8 +26,6 @@
 #include <utils/CallStack.h>
 #include <utils/Log.h>
 
-#include <pixelflinger/pixelflinger.h>
-
 #include <binder/IPCThreadState.h>
 #include <binder/IMemory.h>
 
@@ -55,6 +51,8 @@ static status_t copyBlt(
         const sp<GraphicBuffer>& src, 
         const Region& reg)
 {
+    // src and dst with, height and format must be identical. no verification
+    // is done here.
     status_t err;
     uint8_t const * src_bits = NULL;
     err = src->lock(GRALLOC_USAGE_SW_READ_OFTEN, reg.bounds(), (void**)&src_bits);
@@ -67,7 +65,6 @@ static status_t copyBlt(
     Region::const_iterator head(reg.begin());
     Region::const_iterator tail(reg.end());
     if (head != tail && src_bits && dst_bits) {
-        // NOTE: dst and src must be the same format
         const size_t bpp = bytesPerPixel(src->format);
         const size_t dbpr = dst->stride * bpp;
         const size_t sbpr = src->stride * bpp;
@@ -354,7 +351,6 @@ void Surface::init()
     // be default we request a hardware surface
     mUsage = GRALLOC_USAGE_HW_RENDER;
     mConnected = 0;
-    mNeedFullUpdate = false;
 }
 
 Surface::~Surface()
@@ -734,37 +730,38 @@ status_t Surface::lock(SurfaceInfo* other, Region* dirtyIn, bool blocking)
         LOGE_IF(err, "lockBuffer (idx=%d) failed (%s)",
                 backBuffer->getIndex(), strerror(-err));
         if (err == NO_ERROR) {
-            // we handle copy-back here...
-
             const Rect bounds(backBuffer->width, backBuffer->height);
-            Region scratch(bounds);
+            const Region boundsRegion(bounds);
+            Region scratch(boundsRegion);
             Region& newDirtyRegion(dirtyIn ? *dirtyIn : scratch);
+            newDirtyRegion &= boundsRegion;
 
-            const Region copyback(mOldDirtyRegion.subtract(newDirtyRegion));
-            if (mNeedFullUpdate) {
-                mNeedFullUpdate = false;
-                Region uninitialized(bounds);
-                uninitialized.subtractSelf(copyback | newDirtyRegion);
-                // reset newDirtyRegion to bounds when a buffer is reallocated
-                // and we have nothing to copy back to it
-                if (!uninitialized.isEmpty())
-                    newDirtyRegion.set(bounds);
-            }
-            newDirtyRegion.andSelf(bounds);
-
+            // figure out if we can copy the frontbuffer back
             const sp<GraphicBuffer>& frontBuffer(mPostedBuffer);
-            if (frontBuffer != 0 &&
-                backBuffer->width  == frontBuffer->width && 
-                backBuffer->height == frontBuffer->height &&
-                !(mFlags & ISurfaceComposer::eDestroyBackbuffer)) 
-            {
-                if (!copyback.isEmpty()) {
-                    // copy front to back
+            const bool canCopyBack = (frontBuffer != 0 &&
+                    backBuffer->width  == frontBuffer->width &&
+                    backBuffer->height == frontBuffer->height &&
+                    backBuffer->format == frontBuffer->format &&
+                    !(mFlags & ISurfaceComposer::eDestroyBackbuffer));
+
+            // the dirty region we report to surfaceflinger is the one
+            // given by the user (as opposed to the one *we* return to the
+            // user).
+            mDirtyRegion = newDirtyRegion;
+
+            if (canCopyBack) {
+                // copy the area that is invalid and not repainted this round
+                const Region copyback(mOldDirtyRegion.subtract(newDirtyRegion));
+                if (!copyback.isEmpty())
                     copyBlt(backBuffer, frontBuffer, copyback);
-                }
+            } else {
+                // if we can't copy-back anything, modify the user's dirty
+                // region to make sure they redraw the whole buffer
+                newDirtyRegion = boundsRegion;
             }
 
-            mDirtyRegion = newDirtyRegion;
+            // keep track of the are of the buffer that is "clean"
+            // (ie: that will be redrawn)
             mOldDirtyRegion = newDirtyRegion;
 
             void* vaddr;
@@ -843,7 +840,6 @@ status_t Surface::getBufferLocked(int index, int usage)
             if (err == NO_ERROR) {
                 currentBuffer = buffer;
                 currentBuffer->setIndex(index);
-                mNeedFullUpdate = true;
             }
         } else {
             err = err<0 ? err : NO_MEMORY;