ProCamera: Fix waitForFrameBuffer not handling multiple outstanding frames

If the CpuConsumer triggered multiple onFrameAvailable callbacks in between
a single waitForFrameBuffer call, the old code would only handle 1 callback.

This meant on two subsequent waitForFrameBuffer calls the second would always
timeout when two buffers were already available to be unlocked.

Bug: 8238112
Change-Id: Ibefca35005ac5c408e5ada97ec4a4344a9e3e497

3 files changed, 102 insertions, 13 deletions

diff --git a/camera/ProCamera.cpp b/camera/ProCamera.cpp
index d4a9556..7c66d62 100644
--- a/camera/ProCamera.cpp
+++ b/camera/ProCamera.cpp
@@ -432,20 +432,21 @@ void ProCamera::onFrameAvailable(int streamId) {
     // Unblock waitForFrame(id) callers
     {
         Mutex::Autolock al(mWaitMutex);
-        getStreamInfo(streamId).frameReady = true;
+        getStreamInfo(streamId).frameReady++;
         mWaitCondition.broadcast();
     }
 }
 
-status_t ProCamera::waitForFrameBuffer(int streamId) {
+int ProCamera::waitForFrameBuffer(int streamId) {
     status_t stat = BAD_VALUE;
     Mutex::Autolock al(mWaitMutex);
 
     StreamInfo& si = getStreamInfo(streamId);
 
-    if (si.frameReady) {
-        si.frameReady = false;
-        return OK;
+    if (si.frameReady > 0) {
+        int numFrames = si.frameReady;
+        si.frameReady = 0;
+        return numFrames;
     } else {
         while (true) {
             stat = mWaitCondition.waitRelative(mWaitMutex,
@@ -456,9 +457,10 @@ status_t ProCamera::waitForFrameBuffer(int streamId) {
                 return stat;
             }
 
-            if (si.frameReady) {
-                si.frameReady = false;
-                return OK;
+            if (si.frameReady > 0) {
+                int numFrames = si.frameReady;
+                si.frameReady = 0;
+                return numFrames;
             }
             // else it was some other stream that got unblocked
         }
@@ -467,6 +469,29 @@ status_t ProCamera::waitForFrameBuffer(int streamId) {
     return stat;
 }
 
+int ProCamera::dropFrameBuffer(int streamId, int count) {
+    StreamInfo& si = getStreamInfo(streamId);
+
+    if (!si.cpuStream) {
+        return BAD_VALUE;
+    } else if (count < 0) {
+        return BAD_VALUE;
+    }
+
+    int numDropped = 0;
+    for (int i = 0; i < count; ++i) {
+        CpuConsumer::LockedBuffer buffer;
+        if (si.cpuConsumer->lockNextBuffer(&buffer) != OK) {
+            break;
+        }
+
+        si.cpuConsumer->unlockBuffer(buffer);
+        numDropped++;
+    }
+
+    return numDropped;
+}
+
 status_t ProCamera::waitForFrameMetadata() {
     status_t stat = BAD_VALUE;
     Mutex::Autolock al(mWaitMutex);
diff --git a/camera/tests/ProCameraTests.cpp b/camera/tests/ProCameraTests.cpp
index 33c9179..f93e5cd 100644
--- a/camera/tests/ProCameraTests.cpp
+++ b/camera/tests/ProCameraTests.cpp
@@ -55,6 +55,8 @@ namespace client {
 #define TEST_CPU_FRAME_COUNT 2
 #define TEST_CPU_HEAP_COUNT 5
 
+#define TEST_FRAME_PROCESSING_DELAY_US 200000 // 200 ms
+
 #if TEST_DEBUGGING
 #define dout std::cerr
 #else
@@ -888,7 +890,7 @@ TEST_F(ProCameraTest, WaitForSingleStreamBuffer) {
 
     // Consume a couple of results
     for (int i = 0; i < TEST_CPU_FRAME_COUNT; ++i) {
-        EXPECT_OK(mCamera->waitForFrameBuffer(streamId));
+        EXPECT_EQ(1, mCamera->waitForFrameBuffer(streamId));
 
         CpuConsumer::LockedBuffer buf;
         EXPECT_OK(consumer->lockNextBuffer(&buf));
@@ -942,7 +944,7 @@ TEST_F(ProCameraTest, WaitForDualStreamBuffer) {
 
         // Get the buffers
 
-        EXPECT_OK(mCamera->waitForFrameBuffer(depthStreamId));
+        EXPECT_EQ(1, mCamera->waitForFrameBuffer(depthStreamId));
 
         /**
           * Guaranteed to be able to consume the depth frame,
@@ -978,7 +980,58 @@ TEST_F(ProCameraTest, WaitForDualStreamBuffer) {
     EXPECT_OK(mCamera->exclusiveUnlock());
 }
 
+TEST_F(ProCameraTest, WaitForSingleStreamBufferAndDropFrames) {
+    if (HasFatalFailure()) {
+        return;
+    }
+
+    const int NUM_REQUESTS = 20 * TEST_CPU_FRAME_COUNT;
+
+    int streamId = -1;
+    sp<CpuConsumer> consumer;
+    EXPECT_OK(mCamera->createStreamCpu(/*width*/1280, /*height*/960,
+                  TEST_FORMAT_MAIN, TEST_CPU_HEAP_COUNT, &consumer, &streamId));
+    EXPECT_NE(-1, streamId);
+
+    EXPECT_OK(mCamera->exclusiveTryLock());
+
+    uint8_t streams[] = { streamId };
+    ASSERT_NO_FATAL_FAILURE(createSubmitRequestForStreams(streams, /*count*/1,
+                                                     /*requests*/NUM_REQUESTS));
+
+    // Consume a couple of results
+    for (int i = 0; i < NUM_REQUESTS; ++i) {
+        // Process at 10fps, stream is at 15fps.
+        // This means we will definitely fill up the buffer queue with
+        // extra buffers and need to drop them.
+        usleep(TEST_FRAME_PROCESSING_DELAY_US);
+
+        int numFrames;
+        EXPECT_TRUE((numFrames = mCamera->waitForFrameBuffer(streamId)) > 0);
+
+        // Drop all but the newest framebuffer
+        EXPECT_EQ(numFrames-1, mCamera->dropFrameBuffer(streamId, numFrames-1));
+
+        dout << "Dropped " << (numFrames - 1) << " frames" << std::endl;
+
+        // Skip the counter ahead, don't try to consume these frames again
+        i += numFrames-1;
+
+        // "Consume" the buffer
+        CpuConsumer::LockedBuffer buf;
+        EXPECT_OK(consumer->lockNextBuffer(&buf));
+
+        dout << "Buffer synchronously received on streamId = " << streamId <<
+                ", dataPtr = " << (void*)buf.data <<
+                ", timestamp = " << buf.timestamp << std::endl;
+
+        EXPECT_OK(consumer->unlockBuffer(buf));
+    }
 
+    // Done: clean up
+    EXPECT_OK(mCamera->deleteStream(streamId));
+    EXPECT_OK(mCamera->exclusiveUnlock());
+}
 
 
 
diff --git a/include/camera/ProCamera.h b/include/camera/ProCamera.h
index f813c1c..cd2772c 100644
--- a/include/camera/ProCamera.h
+++ b/include/camera/ProCamera.h
@@ -196,9 +196,12 @@ public:
     // Blocks until a frame is available (CPU streams only)
     // - Obtain the frame data by calling CpuConsumer::lockNextBuffer
     // - Release the frame data after use with CpuConsumer::unlockBuffer
+    // Return value:
+    // - >0 - number of frames available to be locked
+    // - <0 - error (refer to error codes)
     // Error codes:
     // -ETIMEDOUT if it took too long to get a frame
-    status_t waitForFrameBuffer(int streamId);
+    int waitForFrameBuffer(int streamId);
 
     // Blocks until a metadata result is available
     // - Obtain the metadata by calling consumeFrameMetadata()
@@ -211,6 +214,14 @@ public:
     // - Use waitForFrameMetadata to sync until new data is available.
     CameraMetadata consumeFrameMetadata();
 
+    // Convenience method to drop frame buffers (CPU streams only)
+    // Return values:
+    //  >=0 - number of frames dropped (up to count)
+    //  <0  - error code
+    // Error codes:
+    //   BAD_VALUE - invalid streamId or count passed
+    int dropFrameBuffer(int streamId, int count);
+
     sp<IProCameraUser>         remote();
 
 protected:
@@ -286,7 +297,7 @@ private:
         StreamInfo(int streamId) {
             this->streamID = streamId;
             cpuStream = false;
-            frameReady = false;
+            frameReady = 0;
         }
 
         StreamInfo() {
@@ -299,7 +310,7 @@ private:
         sp<CpuConsumer> cpuConsumer;
         sp<ProFrameListener> frameAvailableListener;
         sp<Surface> stc;
-        bool frameReady;
+        int frameReady;
     };
 
     Condition mWaitCondition;