From a4650a50a0b35e9e4342d6600b6eb24fd94bb8e5 Mon Sep 17 00:00:00 2001 From: Dan Stoza Date: Tue, 12 May 2015 12:56:16 -0700 Subject: [PATCH] Fix PTS handling for buffer replacement This changes the way that SurfaceFlinger's shadow buffer management works such that instead of tracking the size of the shadow queue in the BufferQueue, SF tracks the last frame number it has seen, and passes that into the acquireBuffer call. BufferQueueConsumer then ensures that it never returns a buffer newer than that frame number, even if that means that it must return PRESENT_LATER for an otherwise valid buffer. Change-Id: I3fcb45f683ed660c3f18a8b85ae1f8a962ba6f0e --- include/gui/BufferQueueConsumer.h | 5 +- include/gui/BufferQueueCore.h | 6 -- include/gui/ConsumerBase.h | 3 +- include/gui/GLConsumer.h | 3 +- include/gui/IGraphicBufferConsumer.h | 14 +++-- libs/gui/BufferQueueConsumer.cpp | 42 +++++-------- libs/gui/BufferQueueCore.cpp | 4 +- libs/gui/ConsumerBase.cpp | 4 +- libs/gui/GLConsumer.cpp | 5 +- libs/gui/IGraphicBufferConsumer.cpp | 25 ++------ services/surfaceflinger/Layer.cpp | 61 ++++++++++++++----- services/surfaceflinger/Layer.h | 2 + .../surfaceflinger/SurfaceFlingerConsumer.cpp | 14 ++--- .../surfaceflinger/SurfaceFlingerConsumer.h | 9 ++- 14 files changed, 98 insertions(+), 99 deletions(-) diff --git a/include/gui/BufferQueueConsumer.h b/include/gui/BufferQueueConsumer.h index 0f4261310..cde302f8a 100644 --- a/include/gui/BufferQueueConsumer.h +++ b/include/gui/BufferQueueConsumer.h @@ -47,7 +47,7 @@ public: // returned. The presentation time is in nanoseconds, and the time base // is CLOCK_MONOTONIC. virtual status_t acquireBuffer(BufferItem* outBuffer, - nsecs_t expectedPresent); + nsecs_t expectedPresent, uint64_t maxFrameNumber = 0) override; // See IGraphicBufferConsumer::detachBuffer virtual status_t detachBuffer(int slot); @@ -148,9 +148,6 @@ 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 a22015c1a..d7686ec2d 100644 --- a/include/gui/BufferQueueCore.h +++ b/include/gui/BufferQueueCore.h @@ -275,12 +275,6 @@ private: // 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/ConsumerBase.h b/include/gui/ConsumerBase.h index 5be3ffb8d..f46bf014f 100644 --- a/include/gui/ConsumerBase.h +++ b/include/gui/ConsumerBase.h @@ -152,7 +152,8 @@ protected: // initialization that must take place the first time a buffer is assigned // to a slot. If it is overridden the derived class's implementation must // call ConsumerBase::acquireBufferLocked. - virtual status_t acquireBufferLocked(BufferItem *item, nsecs_t presentWhen); + virtual status_t acquireBufferLocked(BufferItem *item, nsecs_t presentWhen, + uint64_t maxFrameNumber = 0); // releaseBufferLocked relinquishes control over a buffer, returning that // control to the BufferQueue. diff --git a/include/gui/GLConsumer.h b/include/gui/GLConsumer.h index 491258046..c35c7be06 100644 --- a/include/gui/GLConsumer.h +++ b/include/gui/GLConsumer.h @@ -241,7 +241,8 @@ protected: // acquireBufferLocked overrides the ConsumerBase method to update the // mEglSlots array in addition to the ConsumerBase behavior. - virtual status_t acquireBufferLocked(BufferItem *item, nsecs_t presentWhen); + virtual status_t acquireBufferLocked(BufferItem *item, nsecs_t presentWhen, + uint64_t maxFrameNumber = 0) override; // releaseBufferLocked overrides the ConsumerBase method to update the // mEglSlots array in addition to the ConsumerBase. diff --git a/include/gui/IGraphicBufferConsumer.h b/include/gui/IGraphicBufferConsumer.h index 0be09a283..6363a3a4f 100644 --- a/include/gui/IGraphicBufferConsumer.h +++ b/include/gui/IGraphicBufferConsumer.h @@ -69,6 +69,12 @@ public: // returned. The presentation time is in nanoseconds, and the time base // is CLOCK_MONOTONIC. // + // If maxFrameNumber is non-zero, it indicates that acquireBuffer should + // only return a buffer with a frame number less than or equal to + // maxFrameNumber. If no such frame is available (such as when a buffer has + // been replaced but the consumer has not received the onFrameReplaced + // callback), then PRESENT_LATER will be returned. + // // Return of NO_ERROR means the operation completed as normal. // // Return of a positive value means the operation could not be completed @@ -78,7 +84,8 @@ public: // // Return of a negative value means an error has occurred: // * INVALID_OPERATION - too many buffers have been acquired - virtual status_t acquireBuffer(BufferItem* buffer, nsecs_t presentWhen) = 0; + virtual status_t acquireBuffer(BufferItem* buffer, nsecs_t presentWhen, + uint64_t maxFrameNumber = 0) = 0; // detachBuffer attempts to remove all ownership of the buffer in the given // slot from the buffer queue. If this call succeeds, the slot will be @@ -248,11 +255,6 @@ 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 336ddb667..4174676c7 100644 --- a/libs/gui/BufferQueueConsumer.cpp +++ b/libs/gui/BufferQueueConsumer.cpp @@ -36,7 +36,7 @@ BufferQueueConsumer::BufferQueueConsumer(const sp& core) : BufferQueueConsumer::~BufferQueueConsumer() {} status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer, - nsecs_t expectedPresent) { + nsecs_t expectedPresent, uint64_t maxFrameNumber) { ATRACE_CALL(); Mutex::Autolock lock(mCore->mMutex); @@ -89,17 +89,12 @@ 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"); + const BufferItem& bufferItem(mCore->mQueue[1]); + + // If dropping entry[0] would leave us with a buffer that the + // consumer is not yet ready for, don't drop it. + if (maxFrameNumber && bufferItem.mFrameNumber > maxFrameNumber) { break; } @@ -112,7 +107,6 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer, // // We may want to add an additional criterion: don't drop the // earlier buffer if entry[1]'s fence hasn't signaled yet. - const BufferItem& bufferItem(mCore->mQueue[1]); nsecs_t desiredPresent = bufferItem.mTimestamp; if (desiredPresent < expectedPresent - MAX_REASONABLE_NSEC || desiredPresent > expectedPresent) { @@ -137,18 +131,22 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer, } mCore->mQueue.erase(front); front = mCore->mQueue.begin(); - --droppableBuffers; } - // See if the front buffer is due + // See if the front buffer is ready to be acquired nsecs_t desiredPresent = front->mTimestamp; - if (desiredPresent > expectedPresent && - desiredPresent < expectedPresent + MAX_REASONABLE_NSEC) { + bool bufferIsDue = desiredPresent <= expectedPresent || + desiredPresent > expectedPresent + MAX_REASONABLE_NSEC; + bool consumerIsReady = maxFrameNumber > 0 ? + front->mFrameNumber <= maxFrameNumber : true; + if (!bufferIsDue || !consumerIsReady) { BQ_LOGV("acquireBuffer: defer desire=%" PRId64 " expect=%" PRId64 - " (%" PRId64 ") now=%" PRId64, + " (%" PRId64 ") now=%" PRId64 " frame=%" PRIu64 + " consumer=%" PRIu64, desiredPresent, expectedPresent, desiredPresent - expectedPresent, - systemTime(CLOCK_MONOTONIC)); + systemTime(CLOCK_MONOTONIC), + front->mFrameNumber, maxFrameNumber); return PRESENT_LATER; } @@ -553,14 +551,6 @@ 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 37171edd1..e78464420 100644 --- a/libs/gui/BufferQueueCore.cpp +++ b/libs/gui/BufferQueueCore.cpp @@ -71,9 +71,7 @@ BufferQueueCore::BufferQueueCore(const sp& allocator) : mIsAllocating(false), mIsAllocatingCondition(), mAllowAllocation(true), - mBufferAge(0), - mConsumerHasShadowQueue(false), - mConsumerShadowQueueSize(0) + mBufferAge(0) { if (allocator == NULL) { sp composer(ComposerService::getComposerService()); diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp index aa9443acf..072ab44bc 100644 --- a/libs/gui/ConsumerBase.cpp +++ b/libs/gui/ConsumerBase.cpp @@ -211,8 +211,8 @@ void ConsumerBase::dumpLocked(String8& result, const char* prefix) const { } status_t ConsumerBase::acquireBufferLocked(BufferItem *item, - nsecs_t presentWhen) { - status_t err = mConsumer->acquireBuffer(item, presentWhen); + nsecs_t presentWhen, uint64_t maxFrameNumber) { + status_t err = mConsumer->acquireBuffer(item, presentWhen, maxFrameNumber); if (err != NO_ERROR) { return err; } diff --git a/libs/gui/GLConsumer.cpp b/libs/gui/GLConsumer.cpp index 96c0841bd..9fcac2dbe 100644 --- a/libs/gui/GLConsumer.cpp +++ b/libs/gui/GLConsumer.cpp @@ -344,8 +344,9 @@ sp GLConsumer::getDebugTexImageBuffer() { } status_t GLConsumer::acquireBufferLocked(BufferItem *item, - nsecs_t presentWhen) { - status_t err = ConsumerBase::acquireBufferLocked(item, presentWhen); + nsecs_t presentWhen, uint64_t maxFrameNumber) { + status_t err = ConsumerBase::acquireBufferLocked(item, presentWhen, + maxFrameNumber); if (err != NO_ERROR) { return err; } diff --git a/libs/gui/IGraphicBufferConsumer.cpp b/libs/gui/IGraphicBufferConsumer.cpp index 480dfb646..b86f4c5d2 100644 --- a/libs/gui/IGraphicBufferConsumer.cpp +++ b/libs/gui/IGraphicBufferConsumer.cpp @@ -52,7 +52,6 @@ enum { SET_CONSUMER_USAGE_BITS, SET_TRANSFORM_HINT, GET_SIDEBAND_STREAM, - SET_SHADOW_QUEUE_SIZE, DUMP, }; @@ -67,10 +66,12 @@ public: virtual ~BpGraphicBufferConsumer(); - virtual status_t acquireBuffer(BufferItem *buffer, nsecs_t presentWhen) { + virtual status_t acquireBuffer(BufferItem *buffer, nsecs_t presentWhen, + uint64_t maxFrameNumber) { Parcel data, reply; data.writeInterfaceToken(IGraphicBufferConsumer::getInterfaceDescriptor()); data.writeInt64(presentWhen); + data.writeUint64(maxFrameNumber); status_t result = remote()->transact(ACQUIRE_BUFFER, data, &reply); if (result != NO_ERROR) { return result; @@ -270,17 +271,6 @@ 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()); @@ -307,7 +297,8 @@ status_t BnGraphicBufferConsumer::onTransact( CHECK_INTERFACE(IGraphicBufferConsumer, data, reply); BufferItem item; int64_t presentWhen = data.readInt64(); - status_t result = acquireBuffer(&item, presentWhen); + uint64_t maxFrameNumber = data.readUint64(); + status_t result = acquireBuffer(&item, presentWhen, maxFrameNumber); status_t err = reply->write(item); if (err) return err; reply->writeInt32(result); @@ -435,12 +426,6 @@ 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 7b104c3df..2b733e905 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -80,7 +80,11 @@ Layer::Layer(SurfaceFlinger* flinger, const sp& client, mProtectedByApp(false), mHasSurface(false), mClientRef(client), - mPotentialCursor(false) + mPotentialCursor(false), + mQueueItemLock(), + mQueueItemCondition(), + mQueueItems(), + mLastFrameNumberReceived(0) { mCurrentCrop.makeInvalid(); mFlinger->getRenderEngine().genTextures(1, &mTextureName); @@ -127,10 +131,6 @@ 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); @@ -167,9 +167,28 @@ void Layer::onFrameAvailable(const BufferItem& item) { // Add this buffer from our internal queue tracker { // Autolock scope Mutex::Autolock lock(mQueueItemLock); + + // Reset the frame number tracker when we receive the first buffer after + // a frame number reset + if (item.mFrameNumber == 1) { + mLastFrameNumberReceived = 0; + } + + // Ensure that callbacks are handled in order + while (item.mFrameNumber != mLastFrameNumberReceived + 1) { + status_t result = mQueueItemCondition.waitRelative(mQueueItemLock, + ms2ns(500)); + if (result != NO_ERROR) { + ALOGE("[%s] Timed out waiting on callback", mName.string()); + } + } + mQueueItems.push_back(item); - mSurfaceFlingerConsumer->setShadowQueueSize(mQueueItems.size()); android_atomic_inc(&mQueuedFrames); + + // Wake up any pending callbacks + mLastFrameNumberReceived = item.mFrameNumber; + mQueueItemCondition.broadcast(); } mFlinger->signalLayerUpdate(); @@ -177,11 +196,25 @@ void Layer::onFrameAvailable(const BufferItem& item) { void Layer::onFrameReplaced(const BufferItem& item) { Mutex::Autolock lock(mQueueItemLock); + + // Ensure that callbacks are handled in order + while (item.mFrameNumber != mLastFrameNumberReceived + 1) { + status_t result = mQueueItemCondition.waitRelative(mQueueItemLock, + ms2ns(500)); + if (result != NO_ERROR) { + ALOGE("[%s] Timed out waiting on callback", mName.string()); + } + } + if (mQueueItems.empty()) { ALOGE("Can't replace a frame on an empty queue"); return; } mQueueItems.editItemAt(0) = item; + + // Wake up any pending callbacks + mLastFrameNumberReceived = item.mFrameNumber; + mQueueItemCondition.broadcast(); } void Layer::onSidebandStreamChanged() { @@ -1261,8 +1294,14 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions) Reject r(mDrawingState, getCurrentState(), recomputeVisibleRegions, getProducerStickyTransform() != 0); + uint64_t maxFrameNumber = 0; + { + Mutex::Autolock lock(mQueueItemLock); + maxFrameNumber = mLastFrameNumberReceived; + } + status_t updateResult = mSurfaceFlingerConsumer->updateTexImage(&r, - mFlinger->mPrimaryDispSync); + mFlinger->mPrimaryDispSync, maxFrameNumber); if (updateResult == BufferQueue::PRESENT_LATER) { // Producer doesn't want buffer to be displayed yet. Signal a // layer update so we check again at the next opportunity. @@ -1272,12 +1311,7 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions) // 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; } @@ -1294,10 +1328,7 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions) 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()); } diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 0b4c640c3..947da854c 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -416,7 +416,9 @@ private: // Local copy of the queued contents of the incoming BufferQueue mutable Mutex mQueueItemLock; + Condition mQueueItemCondition; Vector mQueueItems; + uint64_t mLastFrameNumberReceived; }; // --------------------------------------------------------------------------- diff --git a/services/surfaceflinger/SurfaceFlingerConsumer.cpp b/services/surfaceflinger/SurfaceFlingerConsumer.cpp index a9a295882..ed1f31b55 100644 --- a/services/surfaceflinger/SurfaceFlingerConsumer.cpp +++ b/services/surfaceflinger/SurfaceFlingerConsumer.cpp @@ -32,7 +32,7 @@ namespace android { // --------------------------------------------------------------------------- status_t SurfaceFlingerConsumer::updateTexImage(BufferRejecter* rejecter, - const DispSync& dispSync) + const DispSync& dispSync, uint64_t maxFrameNumber) { ATRACE_CALL(); ALOGV("updateTexImage"); @@ -54,7 +54,8 @@ status_t SurfaceFlingerConsumer::updateTexImage(BufferRejecter* rejecter, // Acquire the next buffer. // In asynchronous mode the list is guaranteed to be one buffer // deep, while in synchronous mode we use the oldest buffer. - err = acquireBufferLocked(&item, computeExpectedPresent(dispSync)); + err = acquireBufferLocked(&item, computeExpectedPresent(dispSync), + maxFrameNumber); if (err != NO_ERROR) { if (err == BufferQueue::NO_BUFFER_AVAILABLE) { err = NO_ERROR; @@ -104,8 +105,9 @@ status_t SurfaceFlingerConsumer::bindTextureImage() } status_t SurfaceFlingerConsumer::acquireBufferLocked(BufferItem* item, - nsecs_t presentWhen) { - status_t result = GLConsumer::acquireBufferLocked(item, presentWhen); + nsecs_t presentWhen, uint64_t maxFrameNumber) { + status_t result = GLConsumer::acquireBufferLocked(item, presentWhen, + maxFrameNumber); if (result == NO_ERROR) { mTransformToDisplayInverse = item->mTransformToDisplayInverse; mSurfaceDamage = item->mSurfaceDamage; @@ -125,10 +127,6 @@ 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 a90a8b931..779e5b77b 100644 --- a/services/surfaceflinger/SurfaceFlingerConsumer.h +++ b/services/surfaceflinger/SurfaceFlingerConsumer.h @@ -49,13 +49,15 @@ public: virtual ~BufferRejecter() { } }; - virtual status_t acquireBufferLocked(BufferItem *item, nsecs_t presentWhen); + virtual status_t acquireBufferLocked(BufferItem *item, nsecs_t presentWhen, + uint64_t maxFrameNumber = 0) override; // This version of updateTexImage() takes a functor that may be used to // reject the newly acquired buffer. Unlike the GLConsumer version, // this does not guarantee that the buffer has been bound to the GL // texture. - status_t updateTexImage(BufferRejecter* rejecter, const DispSync& dispSync); + status_t updateTexImage(BufferRejecter* rejecter, const DispSync& dispSync, + uint64_t maxFrameNumber = 0); // See GLConsumer::bindTextureImageLocked(). status_t bindTextureImage(); @@ -70,9 +72,6 @@ public: sp getSidebandStream() const; - // See IGraphicBufferConsumer::setShadowQueueSize - void setShadowQueueSize(size_t size); - nsecs_t computeExpectedPresent(const DispSync& dispSync); private: