From 812ed0644f8f8f71ca403f4e5793f0dbc1fcf9b2 Mon Sep 17 00:00:00 2001 From: Dan Stoza Date: Tue, 2 Jun 2015 15:45:22 -0700 Subject: [PATCH] libgui: Add generation numbers to BufferQueue This change allows producers to set a generation number on a BufferQueue. This number will be embedded in any new GraphicBuffers created in that BufferQueue, and attempts to attach buffers which have a different generation number will fail. It also plumbs the setGenerationNumber method through Surface, with the additional effect that any buffers attached to the Surface after setting a new generation number will automatically be updated with the new number (as opposed to failing, as would happen on through IGBP). Bug: 20923096 Change-Id: I32bf726b035f99c3e5834beaf76afb9f01adcbc2 --- include/gui/BufferQueueCore.h | 5 +++ include/gui/BufferQueueProducer.h | 3 ++ include/gui/IGraphicBufferConsumer.h | 3 +- include/gui/IGraphicBufferProducer.h | 14 ++++++- include/gui/Surface.h | 9 ++++ include/ui/GraphicBuffer.h | 10 +++++ libs/gui/BufferQueueConsumer.cpp | 7 ++++ libs/gui/BufferQueueCore.cpp | 3 +- libs/gui/BufferQueueProducer.cpp | 17 ++++++++ libs/gui/IGraphicBufferProducer.cpp | 19 +++++++++ libs/gui/Surface.cpp | 14 ++++++- libs/gui/tests/BufferQueue_test.cpp | 42 +++++++++++++++++++ libs/gui/tests/Surface_test.cpp | 33 +++++++++++++++ libs/ui/GraphicBuffer.cpp | 25 ++++++----- .../DisplayHardware/VirtualDisplaySurface.cpp | 5 +++ .../DisplayHardware/VirtualDisplaySurface.h | 1 + services/surfaceflinger/MonitoredProducer.cpp | 4 ++ services/surfaceflinger/MonitoredProducer.h | 1 + 18 files changed, 199 insertions(+), 16 deletions(-) diff --git a/include/gui/BufferQueueCore.h b/include/gui/BufferQueueCore.h index d7686ec2d..99134ea50 100644 --- a/include/gui/BufferQueueCore.h +++ b/include/gui/BufferQueueCore.h @@ -275,6 +275,11 @@ private: // buffer as the number of frames that have elapsed since it was last queued uint64_t mBufferAge; + // mGenerationNumber stores the current generation number of the attached + // producer. Any attempt to attach a buffer with a different generation + // number will fail. + uint32_t mGenerationNumber; + }; // class BufferQueueCore } // namespace android diff --git a/include/gui/BufferQueueProducer.h b/include/gui/BufferQueueProducer.h index ed660fbf6..afa7eb180 100644 --- a/include/gui/BufferQueueProducer.h +++ b/include/gui/BufferQueueProducer.h @@ -175,6 +175,9 @@ public: // See IGraphicBufferProducer::allowAllocation virtual status_t allowAllocation(bool allow); + // See IGraphicBufferProducer::setGenerationNumber + virtual status_t setGenerationNumber(uint32_t generationNumber); + private: // This is required by the IBinder::DeathRecipient interface virtual void binderDied(const wp& who); diff --git a/include/gui/IGraphicBufferConsumer.h b/include/gui/IGraphicBufferConsumer.h index 6363a3a4f..60ec9cc0e 100644 --- a/include/gui/IGraphicBufferConsumer.h +++ b/include/gui/IGraphicBufferConsumer.h @@ -110,7 +110,8 @@ public: // will be deallocated as stale. // // Return of a value other than NO_ERROR means an error has occurred: - // * BAD_VALUE - outSlot or buffer were NULL + // * BAD_VALUE - outSlot or buffer were NULL, or the generation number of + // the buffer did not match the buffer queue. // * INVALID_OPERATION - cannot attach the buffer because it would cause too // many buffers to be acquired. // * NO_MEMORY - no free slots available diff --git a/include/gui/IGraphicBufferProducer.h b/include/gui/IGraphicBufferProducer.h index 5c50b2b39..4ca4cd50d 100644 --- a/include/gui/IGraphicBufferProducer.h +++ b/include/gui/IGraphicBufferProducer.h @@ -218,8 +218,9 @@ public: // // Return of a negative value means an error has occurred: // * NO_INIT - the buffer queue has been abandoned. - // * BAD_VALUE - outSlot or buffer were NULL or invalid combination of - // async mode and buffer count override. + // * BAD_VALUE - outSlot or buffer were NULL, invalid combination of + // async mode and buffer count override, or the generation + // number of the buffer did not match the buffer queue. // * INVALID_OPERATION - cannot attach the buffer because it would cause // too many buffers to be dequeued, either because // the producer already has a single buffer dequeued @@ -470,6 +471,15 @@ public: // eligible slot is available, dequeueBuffer will block or return an error // as usual. virtual status_t allowAllocation(bool allow) = 0; + + // Sets the current generation number of the BufferQueue. + // + // This generation number will be inserted into any buffers allocated by the + // BufferQueue, and any attempts to attach a buffer with a different + // generation number will fail. Buffers already in the queue are not + // affected and will retain their current generation number. The generation + // number defaults to 0. + virtual status_t setGenerationNumber(uint32_t generationNumber) = 0; }; // ---------------------------------------------------------------------------- diff --git a/include/gui/Surface.h b/include/gui/Surface.h index fd6d48c2a..261b07c8f 100644 --- a/include/gui/Surface.h +++ b/include/gui/Surface.h @@ -101,6 +101,11 @@ public: */ void allocateBuffers(); + /* Sets the generation number on the IGraphicBufferProducer and updates the + * generation number on any buffers attached to the Surface after this call. + * See IGBP::setGenerationNumber for more information. */ + status_t setGenerationNumber(uint32_t generationNumber); + protected: virtual ~Surface(); @@ -305,6 +310,10 @@ private: // When a non-CPU producer is attached, this reflects the surface damage // (the change since the previous frame) passed in by the producer. Region mDirtyRegion; + + // Stores the current generation number. See setGenerationNumber and + // IGraphicBufferProducer::setGenerationNumber for more information. + uint32_t mGenerationNumber; }; }; // namespace android diff --git a/include/ui/GraphicBuffer.h b/include/ui/GraphicBuffer.h index f91d1920c..3da720ff3 100644 --- a/include/ui/GraphicBuffer.h +++ b/include/ui/GraphicBuffer.h @@ -94,6 +94,11 @@ public: Rect getBounds() const { return Rect(width, height); } uint64_t getId() const { return mId; } + uint32_t getGenerationNumber() const { return mGenerationNumber; } + void setGenerationNumber(uint32_t generation) { + mGenerationNumber = generation; + } + status_t reallocate(uint32_t inWidth, uint32_t inHeight, PixelFormat inFormat, uint32_t inUsage); @@ -166,6 +171,11 @@ private: sp mWrappedBuffer; uint64_t mId; + + // Stores the generation number of this buffer. If this number does not + // match the BufferQueue's internal generation number (set through + // IGBP::setGenerationNumber), attempts to attach the buffer will fail. + uint32_t mGenerationNumber; }; }; // namespace android diff --git a/libs/gui/BufferQueueConsumer.cpp b/libs/gui/BufferQueueConsumer.cpp index 4174676c7..ae796b14d 100644 --- a/libs/gui/BufferQueueConsumer.cpp +++ b/libs/gui/BufferQueueConsumer.cpp @@ -248,6 +248,13 @@ status_t BufferQueueConsumer::attachBuffer(int* outSlot, return INVALID_OPERATION; } + if (buffer->getGenerationNumber() != mCore->mGenerationNumber) { + BQ_LOGE("attachBuffer: generation number mismatch [buffer %u] " + "[queue %u]", buffer->getGenerationNumber(), + mCore->mGenerationNumber); + return BAD_VALUE; + } + // Find a free slot to put the buffer into int found = BufferQueueCore::INVALID_BUFFER_SLOT; if (!mCore->mFreeSlots.empty()) { diff --git a/libs/gui/BufferQueueCore.cpp b/libs/gui/BufferQueueCore.cpp index e78464420..851a39615 100644 --- a/libs/gui/BufferQueueCore.cpp +++ b/libs/gui/BufferQueueCore.cpp @@ -71,7 +71,8 @@ BufferQueueCore::BufferQueueCore(const sp& allocator) : mIsAllocating(false), mIsAllocatingCondition(), mAllowAllocation(true), - mBufferAge(0) + mBufferAge(0), + mGenerationNumber(0) { if (allocator == NULL) { sp composer(ComposerService::getComposerService()); diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index e318484d4..73d4261d5 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -383,6 +383,7 @@ status_t BufferQueueProducer::dequeueBuffer(int *outSlot, return NO_INIT; } + graphicBuffer->setGenerationNumber(mCore->mGenerationNumber); mSlots[*outSlot].mGraphicBuffer = graphicBuffer; } // Autolock scope } @@ -498,6 +499,13 @@ status_t BufferQueueProducer::attachBuffer(int* outSlot, Mutex::Autolock lock(mCore->mMutex); mCore->waitWhileAllocatingLocked(); + if (buffer->getGenerationNumber() != mCore->mGenerationNumber) { + BQ_LOGE("attachBuffer: generation number mismatch [buffer %u] " + "[queue %u]", buffer->getGenerationNumber(), + mCore->mGenerationNumber); + return BAD_VALUE; + } + status_t returnFlags = NO_ERROR; int found; // TODO: Should we provide an async flag to attachBuffer? It seems @@ -1072,6 +1080,15 @@ status_t BufferQueueProducer::allowAllocation(bool allow) { return NO_ERROR; } +status_t BufferQueueProducer::setGenerationNumber(uint32_t generationNumber) { + ATRACE_CALL(); + BQ_LOGV("setGenerationNumber: %u", generationNumber); + + Mutex::Autolock lock(mCore->mMutex); + mCore->mGenerationNumber = generationNumber; + return NO_ERROR; +} + void BufferQueueProducer::binderDied(const wp& /* who */) { // If we're here, it means that a producer we were connected to died. // We're guaranteed that we are still connected to it because we remove diff --git a/libs/gui/IGraphicBufferProducer.cpp b/libs/gui/IGraphicBufferProducer.cpp index 7093ffaa1..cfe726bc0 100644 --- a/libs/gui/IGraphicBufferProducer.cpp +++ b/libs/gui/IGraphicBufferProducer.cpp @@ -47,6 +47,7 @@ enum { SET_SIDEBAND_STREAM, ALLOCATE_BUFFERS, ALLOW_ALLOCATION, + SET_GENERATION_NUMBER, }; class BpGraphicBufferProducer : public BpInterface @@ -284,6 +285,17 @@ public: result = reply.readInt32(); return result; } + + virtual status_t setGenerationNumber(uint32_t generationNumber) { + Parcel data, reply; + data.writeInterfaceToken(IGraphicBufferProducer::getInterfaceDescriptor()); + data.writeUint32(generationNumber); + status_t result = remote()->transact(SET_GENERATION_NUMBER, data, &reply); + if (result == NO_ERROR) { + result = reply.readInt32(); + } + return result; + } }; // Out-of-line virtual method definition to trigger vtable emission in this @@ -448,6 +460,13 @@ status_t BnGraphicBufferProducer::onTransact( reply->writeInt32(result); return NO_ERROR; } + case SET_GENERATION_NUMBER: { + CHECK_INTERFACE(IGraphicBufferProducer, data, reply); + uint32_t generationNumber = data.readUint32(); + status_t result = setGenerationNumber(generationNumber); + reply->writeInt32(result); + return NO_ERROR; + } } return BBinder::onTransact(code, data, reply, flags); } diff --git a/libs/gui/Surface.cpp b/libs/gui/Surface.cpp index 04ac0f438..aeb56e002 100644 --- a/libs/gui/Surface.cpp +++ b/libs/gui/Surface.cpp @@ -42,7 +42,8 @@ namespace android { Surface::Surface( const sp& bufferProducer, bool controlledByApp) - : mGraphicBufferProducer(bufferProducer) + : mGraphicBufferProducer(bufferProducer), + mGenerationNumber(0) { // Initialize the ANativeWindow function pointers. ANativeWindow::setSwapInterval = hook_setSwapInterval; @@ -102,6 +103,14 @@ void Surface::allocateBuffers() { reqHeight, mReqFormat, mReqUsage); } +status_t Surface::setGenerationNumber(uint32_t generation) { + status_t result = mGraphicBufferProducer->setGenerationNumber(generation); + if (result == NO_ERROR) { + mGenerationNumber = generation; + } + return result; +} + int Surface::hook_setSwapInterval(ANativeWindow* window, int interval) { Surface* c = getSelf(window); return c->setSwapInterval(interval); @@ -698,11 +707,14 @@ int Surface::attachBuffer(ANativeWindowBuffer* buffer) Mutex::Autolock lock(mMutex); sp graphicBuffer(static_cast(buffer)); + uint32_t priorGeneration = graphicBuffer->mGenerationNumber; + graphicBuffer->mGenerationNumber = mGenerationNumber; int32_t attachedSlot = -1; status_t result = mGraphicBufferProducer->attachBuffer( &attachedSlot, graphicBuffer); if (result != NO_ERROR) { ALOGE("attachBuffer: IGraphicBufferProducer call failed (%d)", result); + graphicBuffer->mGenerationNumber = priorGeneration; return result; } mSlots[attachedSlot].buffer = graphicBuffer; diff --git a/libs/gui/tests/BufferQueue_test.cpp b/libs/gui/tests/BufferQueue_test.cpp index 1584fef07..3d1139d4f 100644 --- a/libs/gui/tests/BufferQueue_test.cpp +++ b/libs/gui/tests/BufferQueue_test.cpp @@ -402,4 +402,46 @@ TEST_F(BufferQueueTest, TestDisallowingAllocation) { WIDTH * 2, HEIGHT * 2, 0, GRALLOC_USAGE_SW_WRITE_OFTEN)); } +TEST_F(BufferQueueTest, TestGenerationNumbers) { + createBufferQueue(); + sp dc(new DummyConsumer); + ASSERT_EQ(OK, mConsumer->consumerConnect(dc, true)); + IGraphicBufferProducer::QueueBufferOutput output; + ASSERT_EQ(OK, mProducer->connect(new DummyProducerListener, + NATIVE_WINDOW_API_CPU, true, &output)); + + ASSERT_EQ(OK, mProducer->setGenerationNumber(1)); + + // Get one buffer to play with + int slot; + sp fence; + ASSERT_EQ(IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION, + mProducer->dequeueBuffer(&slot, &fence, false, 0, 0, 0, 0)); + + sp buffer; + ASSERT_EQ(OK, mProducer->requestBuffer(slot, &buffer)); + + // Ensure that the generation number we set propagates to allocated buffers + ASSERT_EQ(1U, buffer->getGenerationNumber()); + + ASSERT_EQ(OK, mProducer->detachBuffer(slot)); + + ASSERT_EQ(OK, mProducer->setGenerationNumber(2)); + + // These should fail, since we've changed the generation number on the queue + int outSlot; + ASSERT_EQ(BAD_VALUE, mProducer->attachBuffer(&outSlot, buffer)); + ASSERT_EQ(BAD_VALUE, mConsumer->attachBuffer(&outSlot, buffer)); + + buffer->setGenerationNumber(2); + + // This should succeed now that we've changed the buffer's generation number + ASSERT_EQ(OK, mProducer->attachBuffer(&outSlot, buffer)); + + ASSERT_EQ(OK, mProducer->detachBuffer(outSlot)); + + // This should also succeed with the new generation number + ASSERT_EQ(OK, mConsumer->attachBuffer(&outSlot, buffer)); +} + } // namespace android diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index 4f878244b..cf0043dc6 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -177,4 +177,37 @@ TEST_F(SurfaceTest, QueryDefaultBuffersDataSpace) { ASSERT_EQ(TEST_DATASPACE, dataSpace); } +TEST_F(SurfaceTest, SettingGenerationNumber) { + sp producer; + sp consumer; + BufferQueue::createBufferQueue(&producer, &consumer); + sp cpuConsumer = new CpuConsumer(consumer, 1); + sp surface = new Surface(producer); + sp window(surface); + + // Allocate a buffer with a generation number of 0 + ANativeWindowBuffer* buffer; + int fenceFd; + ASSERT_EQ(NO_ERROR, window->dequeueBuffer(window.get(), &buffer, &fenceFd)); + ASSERT_EQ(NO_ERROR, window->cancelBuffer(window.get(), buffer, fenceFd)); + + // Detach the buffer and check its generation number + sp graphicBuffer; + sp fence; + ASSERT_EQ(NO_ERROR, surface->detachNextBuffer(&graphicBuffer, &fence)); + ASSERT_EQ(0U, graphicBuffer->getGenerationNumber()); + + ASSERT_EQ(NO_ERROR, surface->setGenerationNumber(1)); + buffer = static_cast(graphicBuffer.get()); + + // This should change the generation number of the GraphicBuffer + ASSERT_EQ(NO_ERROR, surface->attachBuffer(buffer)); + + // Check that the new generation number sticks with the buffer + ASSERT_EQ(NO_ERROR, window->cancelBuffer(window.get(), buffer, -1)); + ASSERT_EQ(NO_ERROR, window->dequeueBuffer(window.get(), &buffer, &fenceFd)); + graphicBuffer = static_cast(buffer); + ASSERT_EQ(1U, graphicBuffer->getGenerationNumber()); +} + } diff --git a/libs/ui/GraphicBuffer.cpp b/libs/ui/GraphicBuffer.cpp index 6a42a2295..e55db30f8 100644 --- a/libs/ui/GraphicBuffer.cpp +++ b/libs/ui/GraphicBuffer.cpp @@ -278,7 +278,7 @@ status_t GraphicBuffer::unlockAsync(int *fenceFd) } size_t GraphicBuffer::getFlattenedSize() const { - return static_cast(10 + (handle ? handle->numInts : 0)) * sizeof(int); + return static_cast(11 + (handle ? handle->numInts : 0)) * sizeof(int); } size_t GraphicBuffer::getFdCount() const { @@ -301,15 +301,16 @@ status_t GraphicBuffer::flatten(void*& buffer, size_t& size, int*& fds, size_t& buf[5] = usage; buf[6] = static_cast(mId >> 32); buf[7] = static_cast(mId & 0xFFFFFFFFull); - buf[8] = 0; + buf[8] = static_cast(mGenerationNumber); buf[9] = 0; + buf[10] = 0; if (handle) { - buf[8] = handle->numFds; - buf[9] = handle->numInts; + buf[9] = handle->numFds; + buf[10] = handle->numInts; memcpy(fds, handle->data, static_cast(handle->numFds) * sizeof(int)); - memcpy(&buf[10], handle->data + handle->numFds, + memcpy(&buf[11], handle->data + handle->numFds, static_cast(handle->numInts) * sizeof(int)); } @@ -325,20 +326,20 @@ status_t GraphicBuffer::flatten(void*& buffer, size_t& size, int*& fds, size_t& status_t GraphicBuffer::unflatten( void const*& buffer, size_t& size, int const*& fds, size_t& count) { - if (size < 8*sizeof(int)) return NO_MEMORY; + if (size < 11 * sizeof(int)) return NO_MEMORY; int const* buf = static_cast(buffer); if (buf[0] != 'GBFR') return BAD_TYPE; - const size_t numFds = static_cast(buf[8]); - const size_t numInts = static_cast(buf[9]); + const size_t numFds = static_cast(buf[9]); + const size_t numInts = static_cast(buf[10]); // Limit the maxNumber to be relatively small. The number of fds or ints // should not come close to this number, and the number itself was simply // chosen to be high enough to not cause issues and low enough to prevent // overflow problems. const size_t maxNumber = 4096; - if (numFds >= maxNumber || numInts >= (maxNumber - 10)) { + if (numFds >= maxNumber || numInts >= (maxNumber - 11)) { width = height = stride = format = usage = 0; handle = NULL; ALOGE("unflatten: numFds or numInts is too large: %zd, %zd", @@ -346,7 +347,7 @@ status_t GraphicBuffer::unflatten( return BAD_VALUE; } - const size_t sizeNeeded = (10 + numInts) * sizeof(int); + const size_t sizeNeeded = (11 + numInts) * sizeof(int); if (size < sizeNeeded) return NO_MEMORY; size_t fdCountNeeded = numFds; @@ -372,7 +373,7 @@ status_t GraphicBuffer::unflatten( return NO_MEMORY; } memcpy(h->data, fds, numFds * sizeof(int)); - memcpy(h->data + numFds, &buf[10], numInts * sizeof(int)); + memcpy(h->data + numFds, &buf[11], numInts * sizeof(int)); handle = h; } else { width = height = stride = format = usage = 0; @@ -382,6 +383,8 @@ status_t GraphicBuffer::unflatten( mId = static_cast(buf[6]) << 32; mId |= static_cast(buf[7]); + mGenerationNumber = static_cast(buf[8]); + mOwner = ownHandle; if (handle != 0) { diff --git a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp index 11cbdc6d9..ee8d41ceb 100644 --- a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp +++ b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.cpp @@ -530,6 +530,11 @@ status_t VirtualDisplaySurface::allowAllocation(bool /* allow */) { return INVALID_OPERATION; } +status_t VirtualDisplaySurface::setGenerationNumber(uint32_t /* generation */) { + ALOGE("setGenerationNumber not supported on VirtualDisplaySurface"); + return INVALID_OPERATION; +} + void VirtualDisplaySurface::updateQueueBufferOutput( const QueueBufferOutput& qbo) { uint32_t w, h, transformHint, numPendingBuffers; diff --git a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.h b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.h index 97af9809a..ff022908f 100644 --- a/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.h +++ b/services/surfaceflinger/DisplayHardware/VirtualDisplaySurface.h @@ -116,6 +116,7 @@ private: virtual void allocateBuffers(bool async, uint32_t width, uint32_t height, PixelFormat format, uint32_t usage); virtual status_t allowAllocation(bool allow); + virtual status_t setGenerationNumber(uint32_t generationNumber); // // Utility methods diff --git a/services/surfaceflinger/MonitoredProducer.cpp b/services/surfaceflinger/MonitoredProducer.cpp index 9fb555b47..6f25bf539 100644 --- a/services/surfaceflinger/MonitoredProducer.cpp +++ b/services/surfaceflinger/MonitoredProducer.cpp @@ -114,6 +114,10 @@ status_t MonitoredProducer::allowAllocation(bool allow) { return mProducer->allowAllocation(allow); } +status_t MonitoredProducer::setGenerationNumber(uint32_t generationNumber) { + return mProducer->setGenerationNumber(generationNumber); +} + IBinder* MonitoredProducer::onAsBinder() { return IInterface::asBinder(mProducer).get(); } diff --git a/services/surfaceflinger/MonitoredProducer.h b/services/surfaceflinger/MonitoredProducer.h index b2f8293e6..ff691d826 100644 --- a/services/surfaceflinger/MonitoredProducer.h +++ b/services/surfaceflinger/MonitoredProducer.h @@ -54,6 +54,7 @@ public: virtual void allocateBuffers(bool async, uint32_t width, uint32_t height, PixelFormat format, uint32_t usage); virtual status_t allowAllocation(bool allow); + virtual status_t setGenerationNumber(uint32_t generationNumber); virtual IBinder* onAsBinder(); private: