From 2e36f2283f48ab764b496490c73a132acf21df3a Mon Sep 17 00:00:00 2001 From: Dan Stoza Date: Tue, 28 Apr 2015 14:42:06 -0700 Subject: [PATCH] SurfaceFlinger: Fix PTS on stale buffers SurfaceFlinger's (Layer's) shadow copy of the BufferQueue queue was getting out of sync for a few reasons. This change fixes these by doing the following: - Adds a check to re-synchronize the shadow copy every time we successfully acquire a buffer by first dropping stale buffers before removing the current buffer. - Avoids trying to perform updates for buffers which have been rejected (for incorrect dimensions) by SurfaceFlinger. - Adds IGraphicBufferConsumer::setShadowQueueSize, which allows the consumer to notify the BufferQueue that it is maintaining a shadow copy of the queue and prevents it from dropping so many buffers during acquireBuffer that it ends up returning a buffer for which the consumer has not yet received an onFrameAvailable call. Bug: 20096136 Change-Id: I78d0738428005fc19b3be85cc8f1db498043612f --- include/gui/BufferQueueConsumer.h | 3 ++ include/gui/BufferQueueCore.h | 7 ++++ include/gui/IGraphicBufferConsumer.h | 5 +++ libs/gui/BufferQueueConsumer.cpp | 22 ++++++++++++ libs/gui/BufferQueueCore.cpp | 4 ++- libs/gui/IGraphicBufferConsumer.cpp | 18 ++++++++++ services/surfaceflinger/Layer.cpp | 34 +++++++++++++++++-- .../surfaceflinger/SurfaceFlingerConsumer.cpp | 6 +++- .../surfaceflinger/SurfaceFlingerConsumer.h | 5 +++ 9 files changed, 100 insertions(+), 4 deletions(-) diff --git a/include/gui/BufferQueueConsumer.h b/include/gui/BufferQueueConsumer.h index 9c91fc736..0f4261310 100644 --- a/include/gui/BufferQueueConsumer.h +++ b/include/gui/BufferQueueConsumer.h @@ -148,6 +148,9 @@ public: // Retrieve the sideband buffer stream, if any. virtual sp getSidebandStream() const; + // See IGraphicBufferConsumer::setShadowQueueSize + virtual void setShadowQueueSize(size_t size); + // dump our state in a String virtual void dump(String8& result, const char* prefix) const; diff --git a/include/gui/BufferQueueCore.h b/include/gui/BufferQueueCore.h index e3624b71d..a22015c1a 100644 --- a/include/gui/BufferQueueCore.h +++ b/include/gui/BufferQueueCore.h @@ -274,6 +274,13 @@ private: // mBufferAge tracks the age of the contents of the most recently dequeued // buffer as the number of frames that have elapsed since it was last queued uint64_t mBufferAge; + + // mConsumerHasShadowQueue determines if acquireBuffer should be more + // cautious about dropping buffers so that it always returns a buffer that + // is represented in the consumer's shadow queue. + bool mConsumerHasShadowQueue; + size_t mConsumerShadowQueueSize; + }; // class BufferQueueCore } // namespace android diff --git a/include/gui/IGraphicBufferConsumer.h b/include/gui/IGraphicBufferConsumer.h index 8f31b55e5..0be09a283 100644 --- a/include/gui/IGraphicBufferConsumer.h +++ b/include/gui/IGraphicBufferConsumer.h @@ -248,6 +248,11 @@ public: // Retrieve the sideband buffer stream, if any. virtual sp getSidebandStream() const = 0; + // setShadowQueueSize notifies the BufferQueue that the consumer is + // shadowing its queue and allows it to limit the number of buffers it is + // permitted to drop during acquire so as to not get out of sync. + virtual void setShadowQueueSize(size_t size) = 0; + // dump state into a string virtual void dump(String8& result, const char* prefix) const = 0; diff --git a/libs/gui/BufferQueueConsumer.cpp b/libs/gui/BufferQueueConsumer.cpp index c7d5e0032..2deef0e77 100644 --- a/libs/gui/BufferQueueConsumer.cpp +++ b/libs/gui/BufferQueueConsumer.cpp @@ -89,7 +89,20 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer, // the timestamps are being auto-generated by Surface. If the app isn't // generating timestamps explicitly, it probably doesn't want frames to // be discarded based on them. + // + // If the consumer is shadowing our queue, we also make sure that we + // don't drop so many buffers that the consumer hasn't received the + // onFrameAvailable callback for the buffer it acquires. That is, we + // want the buffer we return to be in the consumer's shadow queue. + size_t droppableBuffers = mCore->mConsumerShadowQueueSize > 1 ? + mCore->mConsumerShadowQueueSize - 1 : 0; while (mCore->mQueue.size() > 1 && !mCore->mQueue[0].mIsAutoTimestamp) { + if (mCore->mConsumerHasShadowQueue && droppableBuffers == 0) { + BQ_LOGV("acquireBuffer: no droppable buffers in consumer's" + " shadow queue, continuing"); + break; + } + // If entry[1] is timely, drop entry[0] (and repeat). We apply an // additional criterion here: we only drop the earlier buffer if our // desiredPresent falls within +/- 1 second of the expected present. @@ -124,6 +137,7 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer, } mCore->mQueue.erase(front); front = mCore->mQueue.begin(); + --droppableBuffers; } // See if the front buffer is due @@ -537,6 +551,14 @@ sp BufferQueueConsumer::getSidebandStream() const { return mCore->mSidebandStream; } +void BufferQueueConsumer::setShadowQueueSize(size_t size) { + ATRACE_CALL(); + BQ_LOGV("setShadowQueueSize: %zu", size); + Mutex::Autolock lock(mCore->mMutex); + mCore->mConsumerHasShadowQueue = true; + mCore->mConsumerShadowQueueSize = size; +} + void BufferQueueConsumer::dump(String8& result, const char* prefix) const { mCore->dump(result, prefix); } diff --git a/libs/gui/BufferQueueCore.cpp b/libs/gui/BufferQueueCore.cpp index 887f2cbf0..d0f7afad4 100644 --- a/libs/gui/BufferQueueCore.cpp +++ b/libs/gui/BufferQueueCore.cpp @@ -71,7 +71,9 @@ BufferQueueCore::BufferQueueCore(const sp& allocator) : mIsAllocating(false), mIsAllocatingCondition(), mAllowAllocation(true), - mBufferAge(0) + mBufferAge(0), + mConsumerHasShadowQueue(false), + mConsumerShadowQueueSize(0) { if (allocator == NULL) { sp composer(ComposerService::getComposerService()); diff --git a/libs/gui/IGraphicBufferConsumer.cpp b/libs/gui/IGraphicBufferConsumer.cpp index 6658ab11d..480dfb646 100644 --- a/libs/gui/IGraphicBufferConsumer.cpp +++ b/libs/gui/IGraphicBufferConsumer.cpp @@ -52,6 +52,7 @@ enum { SET_CONSUMER_USAGE_BITS, SET_TRANSFORM_HINT, GET_SIDEBAND_STREAM, + SET_SHADOW_QUEUE_SIZE, DUMP, }; @@ -269,6 +270,17 @@ public: return stream; } + virtual void setShadowQueueSize(size_t size) { + Parcel data, reply; + data.writeInterfaceToken(IGraphicBufferConsumer::getInterfaceDescriptor()); + data.writeInt64(static_cast(size)); + status_t result = remote()->transact(SET_SHADOW_QUEUE_SIZE, data, &reply); + if (result != NO_ERROR) { + ALOGE("setShadowQueueSize failed (%d)", result); + return; + } + } + virtual void dump(String8& result, const char* prefix) const { Parcel data, reply; data.writeInterfaceToken(IGraphicBufferConsumer::getInterfaceDescriptor()); @@ -423,6 +435,12 @@ status_t BnGraphicBufferConsumer::onTransact( } return NO_ERROR; } + case SET_SHADOW_QUEUE_SIZE: { + CHECK_INTERFACE(IGraphicBufferConsumer, data, reply); + size_t size = static_cast(data.readInt64()); + setShadowQueueSize(size); + return NO_ERROR; + } case DUMP: { CHECK_INTERFACE(IGraphicBufferConsumer, data, reply); String8 result = data.readString8(); diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 2944c6323..9fb94dd3d 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -127,6 +127,10 @@ void Layer::onFirstRef() { mSurfaceFlingerConsumer->setContentsChangedListener(this); mSurfaceFlingerConsumer->setName(mName); + // Set the shadow queue size to 0 to notify the BufferQueue that we are + // shadowing it + mSurfaceFlingerConsumer->setShadowQueueSize(0); + #ifdef TARGET_DISABLE_TRIPLE_BUFFERING #warning "disabling triple buffering" mSurfaceFlingerConsumer->setDefaultMaxBufferCount(2); @@ -164,9 +168,10 @@ void Layer::onFrameAvailable(const BufferItem& item) { { // Autolock scope Mutex::Autolock lock(mQueueItemLock); mQueueItems.push_back(item); + mSurfaceFlingerConsumer->setShadowQueueSize(mQueueItems.size()); + android_atomic_inc(&mQueuedFrames); } - android_atomic_inc(&mQueuedFrames); mFlinger->signalLayerUpdate(); } @@ -1259,14 +1264,39 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions) // layer update so we check again at the next opportunity. mFlinger->signalLayerUpdate(); return outDirtyRegion; + } else if (updateResult == SurfaceFlingerConsumer::BUFFER_REJECTED) { + // If the buffer has been rejected, remove it from the shadow queue + // and return early + Mutex::Autolock lock(mQueueItemLock); + + // Update the BufferQueue with the new shadow queue size after + // dropping this item + mQueueItems.removeAt(0); + mSurfaceFlingerConsumer->setShadowQueueSize(mQueueItems.size()); + + android_atomic_dec(&mQueuedFrames); + return outDirtyRegion; } - // Remove this buffer from our internal queue tracker { // Autolock scope + auto currentFrameNumber = mSurfaceFlingerConsumer->getFrameNumber(); + Mutex::Autolock lock(mQueueItemLock); + + // Remove any stale buffers that have been dropped during + // updateTexImage + while (mQueueItems[0].mFrameNumber != currentFrameNumber) { + mQueueItems.removeAt(0); + android_atomic_dec(&mQueuedFrames); + } + + // Update the BufferQueue with our new shadow queue size, since we + // have removed at least one item mQueueItems.removeAt(0); + mSurfaceFlingerConsumer->setShadowQueueSize(mQueueItems.size()); } + // Decrement the queued-frames count. Signal another event if we // have more frames pending. if (android_atomic_dec(&mQueuedFrames) > 1) { diff --git a/services/surfaceflinger/SurfaceFlingerConsumer.cpp b/services/surfaceflinger/SurfaceFlingerConsumer.cpp index 19c497a5d..a9a295882 100644 --- a/services/surfaceflinger/SurfaceFlingerConsumer.cpp +++ b/services/surfaceflinger/SurfaceFlingerConsumer.cpp @@ -74,7 +74,7 @@ status_t SurfaceFlingerConsumer::updateTexImage(BufferRejecter* rejecter, int buf = item.mBuf; if (rejecter && rejecter->reject(mSlots[buf].mGraphicBuffer, item)) { releaseBufferLocked(buf, mSlots[buf].mGraphicBuffer, EGL_NO_SYNC_KHR); - return NO_ERROR; + return BUFFER_REJECTED; } // Release the previous buffer. @@ -125,6 +125,10 @@ sp SurfaceFlingerConsumer::getSidebandStream() const { return mConsumer->getSidebandStream(); } +void SurfaceFlingerConsumer::setShadowQueueSize(size_t size) { + mConsumer->setShadowQueueSize(size); +} + // We need to determine the time when a buffer acquired now will be // displayed. This can be calculated: // time when previous buffer's actual-present fence was signaled diff --git a/services/surfaceflinger/SurfaceFlingerConsumer.h b/services/surfaceflinger/SurfaceFlingerConsumer.h index 1aaba1884..a90a8b931 100644 --- a/services/surfaceflinger/SurfaceFlingerConsumer.h +++ b/services/surfaceflinger/SurfaceFlingerConsumer.h @@ -28,6 +28,8 @@ namespace android { */ class SurfaceFlingerConsumer : public GLConsumer { public: + static const status_t BUFFER_REJECTED = UNKNOWN_ERROR + 8; + struct ContentsChangedListener: public FrameAvailableListener { virtual void onSidebandStreamChanged() = 0; }; @@ -68,6 +70,9 @@ public: sp getSidebandStream() const; + // See IGraphicBufferConsumer::setShadowQueueSize + void setShadowQueueSize(size_t size); + nsecs_t computeExpectedPresent(const DispSync& dispSync); private: