DO NOT MERGE ANYWHERE: BufferQueue consumers: Add discardFreeBuffer method

This method releases all free buffers owned by the buffer queue,
in order to save memory (at the cost of potential future
reallocation of buffers).

Bug: 28695173
Change-Id: I458d10373e639e3144faf673af2ba01aca36e65a

diff --git a/include/gui/BufferQueueConsumer.h b/include/gui/BufferQueueConsumer.h
index b2daae4..3b681ee 100644 (file)
--- a/include/gui/BufferQueueConsumer.h
+++ b/include/gui/BufferQueueConsumer.h
@@ -136,6 +136,9 @@ public:
     // Retrieve the sideband buffer stream, if any.
     virtual sp<NativeHandle> getSidebandStream() const;
 
+    // See IGraphicBufferConsumer::discardFreeBuffers
+    virtual status_t discardFreeBuffers() override;
+
     // 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 0371db7..35c9170 100644 (file)
--- a/include/gui/BufferQueueCore.h
+++ b/include/gui/BufferQueueCore.h
@@ -124,6 +124,11 @@ private:
     // all slots, even if they're currently dequeued, queued, or acquired.
     void freeAllBuffersLocked();
 
+    // discardFreeBuffersLocked releases all currently-free buffers held by the
+    // queue, in order to reduce the memory consumption of the queue to the
+    // minimum possible without discarding data.
+    void discardFreeBuffersLocked();
+
     // If delta is positive, makes more slots available. If negative, takes
     // away slots. Returns false if the request can't be met.
     bool adjustAvailableSlotsLocked(int delta);

diff --git a/include/gui/ConsumerBase.h b/include/gui/ConsumerBase.h
index 9307a26..ba86478 100644 (file)
--- a/include/gui/ConsumerBase.h
+++ b/include/gui/ConsumerBase.h
@@ -85,6 +85,9 @@ public:
     // See IGraphicBufferConsumer::setDefaultBufferDataSpace
     status_t setDefaultBufferDataSpace(android_dataspace defaultDataSpace);
 
+    // See IGraphicBufferConsumer::discardFreeBuffers
+    status_t discardFreeBuffers();
+
 private:
     ConsumerBase(const ConsumerBase&);
     void operator=(const ConsumerBase&);

diff --git a/include/gui/IGraphicBufferConsumer.h b/include/gui/IGraphicBufferConsumer.h
index e983c16..f03d1ce 100644 (file)
--- a/include/gui/IGraphicBufferConsumer.h
+++ b/include/gui/IGraphicBufferConsumer.h
@@ -265,6 +265,11 @@ public:
     // Retrieve the sideband buffer stream, if any.
     virtual sp<NativeHandle> getSidebandStream() const = 0;
 
+    // discardFreeBuffers releases all currently-free buffers held by the queue,
+    // in order to reduce the memory consumption of the queue to the minimum
+    // possible without discarding data.
+    virtual status_t discardFreeBuffers() = 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 cbc8893..525938e 100644 (file)
--- a/libs/gui/BufferQueueConsumer.cpp
+++ b/libs/gui/BufferQueueConsumer.cpp
@@ -717,6 +717,12 @@ sp<NativeHandle> BufferQueueConsumer::getSidebandStream() const {
     return mCore->mSidebandStream;
 }
 
+status_t BufferQueueConsumer::discardFreeBuffers() {
+    Mutex::Autolock lock(mCore->mMutex);
+    mCore->discardFreeBuffersLocked();
+    return NO_ERROR;
+}
+
 void BufferQueueConsumer::dump(String8& result, const char* prefix) const {
     const IPCThreadState* ipc = IPCThreadState::self();
     const pid_t pid = ipc->getCallingPid();

diff --git a/libs/gui/BufferQueueCore.cpp b/libs/gui/BufferQueueCore.cpp
index 569b8f9..9cb9c62 100644 (file)
--- a/libs/gui/BufferQueueCore.cpp
+++ b/libs/gui/BufferQueueCore.cpp
@@ -244,6 +244,16 @@ void BufferQueueCore::freeAllBuffersLocked() {
     VALIDATE_CONSISTENCY();
 }
 
+void BufferQueueCore::discardFreeBuffersLocked() {
+    for (int s : mFreeBuffers) {
+        mFreeSlots.insert(s);
+        clearBufferSlotLocked(s);
+    }
+    mFreeBuffers.clear();
+
+    VALIDATE_CONSISTENCY();
+}
+
 bool BufferQueueCore::adjustAvailableSlotsLocked(int delta) {
     if (delta >= 0) {
         // If we're going to fail, do so before modifying anything

diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp
index a6a9712..c00526d 100644 (file)
--- a/libs/gui/ConsumerBase.cpp
+++ b/libs/gui/ConsumerBase.cpp
@@ -235,6 +235,15 @@ status_t ConsumerBase::setDefaultBufferDataSpace(
     return mConsumer->setDefaultBufferDataSpace(defaultDataSpace);
 }
 
+status_t ConsumerBase::discardFreeBuffers() {
+    Mutex::Autolock _l(mMutex);
+    if (mAbandoned) {
+        CB_LOGE("discardFreeBuffers: ConsumerBase is abandoned!");
+        return NO_INIT;
+    }
+    return mConsumer->discardFreeBuffers();
+}
+
 void ConsumerBase::dump(String8& result) const {
     dump(result, "");
 }

diff --git a/libs/gui/IGraphicBufferConsumer.cpp b/libs/gui/IGraphicBufferConsumer.cpp
index cb1ad35..af6800b 100644 (file)
--- a/libs/gui/IGraphicBufferConsumer.cpp
+++ b/libs/gui/IGraphicBufferConsumer.cpp
@@ -51,6 +51,7 @@ enum {
     SET_CONSUMER_USAGE_BITS,
     SET_TRANSFORM_HINT,
     GET_SIDEBAND_STREAM,
+    DISCARD_FREE_BUFFERS,
     DUMP,
 };
 
@@ -260,6 +261,21 @@ public:
         return stream;
     }
 
+    virtual status_t discardFreeBuffers() {
+        Parcel data, reply;
+        data.writeInterfaceToken(IGraphicBufferConsumer::getInterfaceDescriptor());
+        status_t error = remote()->transact(DISCARD_FREE_BUFFERS, data, &reply);
+        if (error != NO_ERROR) {
+            return error;
+        }
+        int32_t result = NO_ERROR;
+        error = reply.readInt32(&result);
+        if (error != NO_ERROR) {
+            return error;
+        }
+        return result;
+    }
+
     virtual void dump(String8& result, const char* prefix) const {
         Parcel data, reply;
         data.writeInterfaceToken(IGraphicBufferConsumer::getInterfaceDescriptor());
@@ -409,6 +425,12 @@ status_t BnGraphicBufferConsumer::onTransact(
             }
             return NO_ERROR;
         }
+        case DISCARD_FREE_BUFFERS: {
+            CHECK_INTERFACE(IGraphicBufferConsumer, data, reply);
+            status_t result = discardFreeBuffers();
+            status_t error = reply->writeInt32(result);
+            return error;
+        }
         case DUMP: {
             CHECK_INTERFACE(IGraphicBufferConsumer, data, reply);
             String8 result = data.readString8();

diff --git a/libs/gui/tests/BufferQueue_test.cpp b/libs/gui/tests/BufferQueue_test.cpp
index 85d63b4..721667c 100644 (file)
--- a/libs/gui/tests/BufferQueue_test.cpp
+++ b/libs/gui/tests/BufferQueue_test.cpp
@@ -850,4 +850,81 @@ TEST_F(BufferQueueTest, CanRetrieveLastQueuedBuffer) {
             returnedBuffer->getNativeBuffer()->handle);
 }
 
+TEST_F(BufferQueueTest, TestDiscardFreeBuffers) {
+    createBufferQueue();
+    sp<DummyConsumer> dc(new DummyConsumer);
+    ASSERT_EQ(OK, mConsumer->consumerConnect(dc, false));
+    IGraphicBufferProducer::QueueBufferOutput output;
+    ASSERT_EQ(OK, mProducer->connect(new DummyProducerListener,
+            NATIVE_WINDOW_API_CPU, false, &output));
+
+    int slot = BufferQueue::INVALID_BUFFER_SLOT;
+    sp<Fence> fence = Fence::NO_FENCE;
+    sp<GraphicBuffer> buffer = nullptr;
+    IGraphicBufferProducer::QueueBufferInput input(0ull, true,
+        HAL_DATASPACE_UNKNOWN, Rect::INVALID_RECT,
+        NATIVE_WINDOW_SCALING_MODE_FREEZE, 0, Fence::NO_FENCE);
+    BufferItem item{};
+
+    // Preallocate, dequeue, request, and cancel 4 buffers so we don't get
+    // BUFFER_NEEDS_REALLOCATION below
+    int slots[4] = {};
+    mProducer->setMaxDequeuedBufferCount(4);
+    for (size_t i = 0; i < 4; ++i) {
+        status_t result = mProducer->dequeueBuffer(&slots[i], &fence,
+                0, 0, 0, 0);
+        ASSERT_EQ(IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION, result);
+        ASSERT_EQ(OK, mProducer->requestBuffer(slots[i], &buffer));
+    }
+    for (size_t i = 0; i < 4; ++i) {
+        ASSERT_EQ(OK, mProducer->cancelBuffer(slots[i], Fence::NO_FENCE));
+    }
+
+    // Get buffers in all states: dequeued, filled, acquired, free
+
+    // Fill 3 buffers
+    ASSERT_EQ(OK, mProducer->dequeueBuffer(&slot, &fence, 0, 0, 0, 0));
+    ASSERT_EQ(OK, mProducer->queueBuffer(slot, input, &output));
+    ASSERT_EQ(OK, mProducer->dequeueBuffer(&slot, &fence, 0, 0, 0, 0));
+    ASSERT_EQ(OK, mProducer->queueBuffer(slot, input, &output));
+    ASSERT_EQ(OK, mProducer->dequeueBuffer(&slot, &fence, 0, 0, 0, 0));
+    ASSERT_EQ(OK, mProducer->queueBuffer(slot, input, &output));
+    // Dequeue 1 buffer
+    ASSERT_EQ(OK, mProducer->dequeueBuffer(&slot, &fence, 0, 0, 0, 0));
+
+    // Acquire and free 1 buffer
+    ASSERT_EQ(OK, mConsumer->acquireBuffer(&item, 0));
+    ASSERT_EQ(OK, mConsumer->releaseBuffer(item.mSlot, item.mFrameNumber,
+                    EGL_NO_DISPLAY, EGL_NO_SYNC_KHR, Fence::NO_FENCE));
+    // Acquire 1 buffer, leaving 1 filled buffer in queue
+    ASSERT_EQ(OK, mConsumer->acquireBuffer(&item, 0));
+
+    // Now discard the free buffers
+    ASSERT_EQ(OK, mConsumer->discardFreeBuffers());
+
+    // Check no free buffers in dump
+    String8 dumpString;
+    mConsumer->dump(dumpString, nullptr);
+
+    // Parse the dump to ensure that all buffer slots that are FREE also
+    // have a null GraphicBuffer
+    // Fragile - assumes the following format for the dump for a buffer entry:
+    // ":%p\][^:]*state=FREE" where %p is the buffer pointer in hex.
+    ssize_t idx = dumpString.find("state=FREE");
+    while (idx != -1) {
+        ssize_t bufferPtrIdx = idx - 1;
+        while (bufferPtrIdx > 0) {
+            if (dumpString[bufferPtrIdx] == ':') {
+                bufferPtrIdx++;
+                break;
+            }
+            bufferPtrIdx--;
+        }
+        ASSERT_GT(bufferPtrIdx, 0) << "Can't parse queue dump to validate";
+        ssize_t nullPtrIdx = dumpString.find("0x0]", bufferPtrIdx);
+        ASSERT_EQ(bufferPtrIdx, nullPtrIdx) << "Free buffer not discarded";
+        idx = dumpString.find("FREE", idx + 1);
+    }
+}
+
 } // namespace android