From 245e4d78c5fb304fe153c36303ec69bf8a907f65 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 21 Apr 2010 15:24:11 -0700 Subject: [PATCH] better fix for [2420565] Surface.lockCanvas() updates the dirty region too often Change-Id: I83438b40effd21538f1c74396dc665254b9d5ab6 --- include/surfaceflinger/Surface.h | 2 +- libs/surfaceflinger/Layer.cpp | 12 +++- libs/surfaceflinger/LayerBase.cpp | 2 +- .../SharedBufferStack.cpp | 8 ++- libs/surfaceflinger_client/Surface.cpp | 58 +++++++++---------- 5 files changed, 44 insertions(+), 38 deletions(-) diff --git a/include/surfaceflinger/Surface.h b/include/surfaceflinger/Surface.h index 68809a999..40dc467fb 100644 --- a/include/surfaceflinger/Surface.h +++ b/include/surfaceflinger/Surface.h @@ -250,7 +250,7 @@ private: sp mLockedBuffer; sp 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 0a3254dec..1a6697021 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 6dc8f1097..7feccdac4 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 881ffd44e..f1aec8ab7 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 39938e8a8..057d211c5 100644 --- a/libs/surfaceflinger_client/Surface.cpp +++ b/libs/surfaceflinger_client/Surface.cpp @@ -17,8 +17,6 @@ #define LOG_TAG "Surface" #include -#include -#include #include #include #include @@ -28,8 +26,6 @@ #include #include -#include - #include #include @@ -55,6 +51,8 @@ static status_t copyBlt( const sp& 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& 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;