Merge "[RenderEngine] add an ImageManager thread" into qt-r1-dev

11 files changed, 360 insertions, 28 deletions

diff --git a/libs/renderengine/Android.bp b/libs/renderengine/Android.bp
index 36211ca733..cc252d67ae 100644
--- a/libs/renderengine/Android.bp
+++ b/libs/renderengine/Android.bp
@@ -26,6 +26,7 @@ cc_defaults {
         "libgui",
         "liblog",
         "libnativewindow",
+        "libprocessgroup",
         "libsync",
         "libui",
         "libutils",
@@ -51,6 +52,7 @@ filegroup {
         "gl/GLExtensions.cpp",
         "gl/GLFramebuffer.cpp",
         "gl/GLImage.cpp",
+        "gl/ImageManager.cpp",
         "gl/Program.cpp",
         "gl/ProgramCache.cpp",
     ],
diff --git a/libs/renderengine/gl/GLESRenderEngine.cpp b/libs/renderengine/gl/GLESRenderEngine.cpp
index 7caaef35be..d2a7525113 100644
--- a/libs/renderengine/gl/GLESRenderEngine.cpp
+++ b/libs/renderengine/gl/GLESRenderEngine.cpp
@@ -19,9 +19,8 @@
 #define LOG_TAG "RenderEngine"
 #define ATRACE_TAG ATRACE_TAG_GRAPHICS
 
-#include "GLESRenderEngine.h"
-
-#include <math.h>
+#include <sched.h>
+#include <cmath>
 #include <fstream>
 #include <sstream>
 #include <unordered_set>
@@ -43,6 +42,7 @@
 #include <ui/Region.h>
 #include <utils/KeyedVector.h>
 #include <utils/Trace.h>
+#include "GLESRenderEngine.h"
 #include "GLExtensions.h"
 #include "GLFramebuffer.h"
 #include "GLImage.h"
@@ -423,10 +423,13 @@ GLESRenderEngine::GLESRenderEngine(uint32_t featureFlags, EGLDisplay display, EG
         mTraceGpuCompletion = true;
         mFlushTracer = std::make_unique<FlushTracer>(this);
     }
+    mImageManager = std::make_unique<ImageManager>(this);
     mDrawingBuffer = createFramebuffer();
 }
 
 GLESRenderEngine::~GLESRenderEngine() {
+    // Destroy the image manager first.
+    mImageManager = nullptr;
     std::lock_guard<std::mutex> lock(mRenderingMutex);
     unbindFrameBuffer(mDrawingBuffer.get());
     mDrawingBuffer = nullptr;
@@ -619,19 +622,41 @@ void GLESRenderEngine::bindExternalTextureImage(uint32_t texName, const Image& i
 status_t GLESRenderEngine::bindExternalTextureBuffer(uint32_t texName,
                                                      const sp<GraphicBuffer>& buffer,
                                                      const sp<Fence>& bufferFence) {
+    if (buffer == nullptr) {
+        return BAD_VALUE;
+    }
+
     ATRACE_CALL();
-    status_t cacheResult = cacheExternalTextureBuffer(buffer);
 
-    if (cacheResult != NO_ERROR) {
-        return cacheResult;
+    bool found = false;
+    {
+        std::lock_guard<std::mutex> lock(mRenderingMutex);
+        auto cachedImage = mImageCache.find(buffer->getId());
+        found = (cachedImage != mImageCache.end());
     }
 
+    // If we couldn't find the image in the cache at this time, then either
+    // SurfaceFlinger messed up registering the buffer ahead of time or we got
+    // backed up creating other EGLImages.
+    if (!found) {
+        status_t cacheResult = mImageManager->cache(buffer);
+        if (cacheResult != NO_ERROR) {
+            return cacheResult;
+        }
+    }
+
+    // Whether or not we needed to cache, re-check mImageCache to make sure that
+    // there's an EGLImage. The current threading model guarantees that we don't
+    // destroy a cached image until it's really not needed anymore (i.e. this
+    // function should not be called), so the only possibility is that something
+    // terrible went wrong and we should just bind something and move on.
     {
         std::lock_guard<std::mutex> lock(mRenderingMutex);
         auto cachedImage = mImageCache.find(buffer->getId());
 
         if (cachedImage == mImageCache.end()) {
             // We failed creating the image if we got here, so bail out.
+            ALOGE("Failed to create an EGLImage when rendering");
             bindExternalTextureImage(texName, *createImage());
             return NO_INIT;
         }
@@ -663,7 +688,18 @@ status_t GLESRenderEngine::bindExternalTextureBuffer(uint32_t texName,
     return NO_ERROR;
 }
 
-status_t GLESRenderEngine::cacheExternalTextureBuffer(const sp<GraphicBuffer>& buffer) {
+void GLESRenderEngine::cacheExternalTextureBuffer(const sp<GraphicBuffer>& buffer) {
+    mImageManager->cacheAsync(buffer, nullptr);
+}
+
+std::shared_ptr<ImageManager::Barrier> GLESRenderEngine::cacheExternalTextureBufferForTesting(
+        const sp<GraphicBuffer>& buffer) {
+    auto barrier = std::make_shared<ImageManager::Barrier>();
+    mImageManager->cacheAsync(buffer, barrier);
+    return barrier;
+}
+
+status_t GLESRenderEngine::cacheExternalTextureBufferInternal(const sp<GraphicBuffer>& buffer) {
     if (buffer == nullptr) {
         return BAD_VALUE;
     }
@@ -703,12 +739,30 @@ status_t GLESRenderEngine::cacheExternalTextureBuffer(const sp<GraphicBuffer>& b
 }
 
 void GLESRenderEngine::unbindExternalTextureBuffer(uint64_t bufferId) {
-    std::lock_guard<std::mutex> lock(mRenderingMutex);
-    const auto& cachedImage = mImageCache.find(bufferId);
-    if (cachedImage != mImageCache.end()) {
-        ALOGV("Destroying image for buffer: %" PRIu64, bufferId);
-        mImageCache.erase(bufferId);
-        return;
+    mImageManager->releaseAsync(bufferId, nullptr);
+}
+
+std::shared_ptr<ImageManager::Barrier> GLESRenderEngine::unbindExternalTextureBufferForTesting(
+        uint64_t bufferId) {
+    auto barrier = std::make_shared<ImageManager::Barrier>();
+    mImageManager->releaseAsync(bufferId, barrier);
+    return barrier;
+}
+
+void GLESRenderEngine::unbindExternalTextureBufferInternal(uint64_t bufferId) {
+    std::unique_ptr<Image> image;
+    {
+        std::lock_guard<std::mutex> lock(mRenderingMutex);
+        const auto& cachedImage = mImageCache.find(bufferId);
+
+        if (cachedImage != mImageCache.end()) {
+            ALOGV("Destroying image for buffer: %" PRIu64, bufferId);
+            // Move the buffer out of cache first, so that we can destroy
+            // without holding the cache's lock.
+            image = std::move(cachedImage->second);
+            mImageCache.erase(bufferId);
+            return;
+        }
     }
     ALOGV("Failed to find image for buffer: %" PRIu64, bufferId);
 }
diff --git a/libs/renderengine/gl/GLESRenderEngine.h b/libs/renderengine/gl/GLESRenderEngine.h
index a01162009b..dd60e50c67 100644
--- a/libs/renderengine/gl/GLESRenderEngine.h
+++ b/libs/renderengine/gl/GLESRenderEngine.h
@@ -17,9 +17,7 @@
 #ifndef SF_GLESRENDERENGINE_H_
 #define SF_GLESRENDERENGINE_H_
 
-#include <android-base/thread_annotations.h>
 #include <stdint.h>
-#include <sys/types.h>
 #include <condition_variable>
 #include <deque>
 #include <mutex>
@@ -30,8 +28,11 @@
 #include <EGL/egl.h>
 #include <EGL/eglext.h>
 #include <GLES2/gl2.h>
+#include <android-base/thread_annotations.h>
 #include <renderengine/RenderEngine.h>
 #include <renderengine/private/Description.h>
+#include <sys/types.h>
+#include "ImageManager.h"
 
 #define EGL_NO_CONFIG ((EGLConfig)0)
 
@@ -74,7 +75,7 @@ public:
     void bindExternalTextureImage(uint32_t texName, const Image& image) override;
     status_t bindExternalTextureBuffer(uint32_t texName, const sp<GraphicBuffer>& buffer,
                                        const sp<Fence>& fence) EXCLUDES(mRenderingMutex);
-    status_t cacheExternalTextureBuffer(const sp<GraphicBuffer>& buffer) EXCLUDES(mRenderingMutex);
+    void cacheExternalTextureBuffer(const sp<GraphicBuffer>& buffer) EXCLUDES(mRenderingMutex);
     void unbindExternalTextureBuffer(uint64_t bufferId) EXCLUDES(mRenderingMutex);
     status_t bindFrameBuffer(Framebuffer* framebuffer) override;
     void unbindFrameBuffer(Framebuffer* framebuffer) override;
@@ -101,6 +102,11 @@ public:
     // Returns true iff mFramebufferImageCache contains an image keyed by bufferId
     bool isFramebufferImageCachedForTesting(uint64_t bufferId)
             EXCLUDES(mFramebufferImageCacheMutex);
+    // These are wrappers around public methods above, but exposing Barrier
+    // objects so that tests can block.
+    std::shared_ptr<ImageManager::Barrier> cacheExternalTextureBufferForTesting(
+            const sp<GraphicBuffer>& buffer);
+    std::shared_ptr<ImageManager::Barrier> unbindExternalTextureBufferForTesting(uint64_t bufferId);
 
 protected:
     Framebuffer* getFramebufferForDrawing() override;
@@ -147,6 +153,9 @@ private:
     void setScissor(const Rect& region);
     void disableScissor();
     bool waitSync(EGLSyncKHR sync, EGLint flags);
+    status_t cacheExternalTextureBufferInternal(const sp<GraphicBuffer>& buffer)
+            EXCLUDES(mRenderingMutex);
+    void unbindExternalTextureBufferInternal(uint64_t bufferId) EXCLUDES(mRenderingMutex);
 
     // A data space is considered HDR data space if it has BT2020 color space
     // with PQ or HLG transfer function.
@@ -250,7 +259,9 @@ private:
         bool mRunning = true;
     };
     friend class FlushTracer;
+    friend class ImageManager;
     std::unique_ptr<FlushTracer> mFlushTracer;
+    std::unique_ptr<ImageManager> mImageManager = std::make_unique<ImageManager>(this);
 };
 
 } // namespace gl
diff --git a/libs/renderengine/gl/ImageManager.cpp b/libs/renderengine/gl/ImageManager.cpp
new file mode 100644
index 0000000000..5af0e4f857
--- /dev/null
+++ b/libs/renderengine/gl/ImageManager.cpp
@@ -0,0 +1,140 @@
+/*
+ * Copyright 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#define ATRACE_TAG ATRACE_TAG_GRAPHICS
+
+#include <pthread.h>
+
+#include <processgroup/sched_policy.h>
+#include <utils/Trace.h>
+#include "GLESRenderEngine.h"
+#include "ImageManager.h"
+
+namespace android {
+namespace renderengine {
+namespace gl {
+
+ImageManager::ImageManager(GLESRenderEngine* engine) : mEngine(engine) {
+    pthread_setname_np(mThread.native_handle(), "ImageManager");
+    // Use SCHED_FIFO to minimize jitter
+    struct sched_param param = {0};
+    param.sched_priority = 2;
+    if (pthread_setschedparam(mThread.native_handle(), SCHED_FIFO, &param) != 0) {
+        ALOGE("Couldn't set SCHED_FIFO for ImageManager");
+    }
+}
+
+ImageManager::~ImageManager() {
+    {
+        std::lock_guard<std::mutex> lock(mMutex);
+        mRunning = false;
+    }
+    mCondition.notify_all();
+    if (mThread.joinable()) {
+        mThread.join();
+    }
+}
+
+void ImageManager::cacheAsync(const sp<GraphicBuffer>& buffer,
+                              const std::shared_ptr<Barrier>& barrier) {
+    if (buffer == nullptr) {
+        {
+            std::lock_guard<std::mutex> lock(barrier->mutex);
+            barrier->isOpen = true;
+            barrier->result = BAD_VALUE;
+        }
+        barrier->condition.notify_one();
+        return;
+    }
+    ATRACE_CALL();
+    QueueEntry entry = {QueueEntry::Operation::Insert, buffer, buffer->getId(), barrier};
+    queueOperation(std::move(entry));
+}
+
+status_t ImageManager::cache(const sp<GraphicBuffer>& buffer) {
+    ATRACE_CALL();
+    auto barrier = std::make_shared<Barrier>();
+    cacheAsync(buffer, barrier);
+    std::lock_guard<std::mutex> lock(barrier->mutex);
+    barrier->condition.wait(barrier->mutex,
+                            [&]() REQUIRES(barrier->mutex) { return barrier->isOpen; });
+    return barrier->result;
+}
+
+void ImageManager::releaseAsync(uint64_t bufferId, const std::shared_ptr<Barrier>& barrier) {
+    ATRACE_CALL();
+    QueueEntry entry = {QueueEntry::Operation::Delete, nullptr, bufferId, barrier};
+    queueOperation(std::move(entry));
+}
+
+void ImageManager::queueOperation(const QueueEntry&& entry) {
+    {
+        std::lock_guard<std::mutex> lock(mMutex);
+        mQueue.emplace(entry);
+        ATRACE_INT("ImageManagerQueueDepth", mQueue.size());
+    }
+    mCondition.notify_one();
+}
+
+void ImageManager::threadMain() {
+    set_sched_policy(0, SP_FOREGROUND);
+    bool run;
+    {
+        std::lock_guard<std::mutex> lock(mMutex);
+        run = mRunning;
+    }
+    while (run) {
+        QueueEntry entry;
+        {
+            std::lock_guard<std::mutex> lock(mMutex);
+            mCondition.wait(mMutex,
+                            [&]() REQUIRES(mMutex) { return !mQueue.empty() || !mRunning; });
+            run = mRunning;
+
+            if (!mRunning) {
+                // if mRunning is false, then ImageManager is being destroyed, so
+                // bail out now.
+                break;
+            }
+
+            entry = mQueue.front();
+            mQueue.pop();
+            ATRACE_INT("ImageManagerQueueDepth", mQueue.size());
+        }
+
+        status_t result = NO_ERROR;
+        switch (entry.op) {
+            case QueueEntry::Operation::Delete:
+                mEngine->unbindExternalTextureBufferInternal(entry.bufferId);
+                break;
+            case QueueEntry::Operation::Insert:
+                result = mEngine->cacheExternalTextureBufferInternal(entry.buffer);
+                break;
+        }
+        if (entry.barrier != nullptr) {
+            {
+                std::lock_guard<std::mutex> entryLock(entry.barrier->mutex);
+                entry.barrier->result = result;
+                entry.barrier->isOpen = true;
+            }
+            entry.barrier->condition.notify_one();
+        }
+    }
+}
+
+} // namespace gl
+} // namespace renderengine
+} // namespace android
diff --git a/libs/renderengine/gl/ImageManager.h b/libs/renderengine/gl/ImageManager.h
new file mode 100644
index 0000000000..b5ba554c4f
--- /dev/null
+++ b/libs/renderengine/gl/ImageManager.h
@@ -0,0 +1,70 @@
+/*
+ * Copyright 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <condition_variable>
+#include <mutex>
+#include <queue>
+#include <thread>
+
+#include <ui/GraphicBuffer.h>
+
+namespace android {
+namespace renderengine {
+namespace gl {
+
+class GLESRenderEngine;
+
+class ImageManager {
+public:
+    struct Barrier {
+        std::mutex mutex;
+        std::condition_variable_any condition;
+        bool isOpen GUARDED_BY(mutex) = false;
+        status_t result GUARDED_BY(mutex) = NO_ERROR;
+    };
+    ImageManager(GLESRenderEngine* engine);
+    ~ImageManager();
+    void cacheAsync(const sp<GraphicBuffer>& buffer, const std::shared_ptr<Barrier>& barrier)
+            EXCLUDES(mMutex);
+    status_t cache(const sp<GraphicBuffer>& buffer);
+    void releaseAsync(uint64_t bufferId, const std::shared_ptr<Barrier>& barrier) EXCLUDES(mMutex);
+
+private:
+    struct QueueEntry {
+        enum class Operation { Delete, Insert };
+
+        Operation op = Operation::Delete;
+        sp<GraphicBuffer> buffer = nullptr;
+        uint64_t bufferId = 0;
+        std::shared_ptr<Barrier> barrier = nullptr;
+    };
+
+    void queueOperation(const QueueEntry&& entry);
+    void threadMain();
+    GLESRenderEngine* const mEngine;
+    std::thread mThread = std::thread([this]() { threadMain(); });
+    std::condition_variable_any mCondition;
+    std::mutex mMutex;
+    std::queue<QueueEntry> mQueue GUARDED_BY(mMutex);
+
+    bool mRunning GUARDED_BY(mMutex) = true;
+};
+
+} // namespace gl
+} // namespace renderengine
+} // namespace android
diff --git a/libs/renderengine/include/renderengine/RenderEngine.h b/libs/renderengine/include/renderengine/RenderEngine.h
index f92ccfbfc3..c6a7bd843a 100644
--- a/libs/renderengine/include/renderengine/RenderEngine.h
+++ b/libs/renderengine/include/renderengine/RenderEngine.h
@@ -116,10 +116,20 @@ public:
                                                const sp<Fence>& fence) = 0;
     // Caches Image resources for this buffer, but does not bind the buffer to
     // a particular texture.
-    virtual status_t cacheExternalTextureBuffer(const sp<GraphicBuffer>& buffer) = 0;
+    // Note that work is deferred to an additional thread, i.e. this call
+    // is made asynchronously, but the caller can expect that cache/unbind calls
+    // are performed in a manner that's conflict serializable, i.e. unbinding
+    // a buffer should never occur before binding the buffer if the caller
+    // called {bind, cache}ExternalTextureBuffer before calling unbind.
+    virtual void cacheExternalTextureBuffer(const sp<GraphicBuffer>& buffer) = 0;
     // Removes internal resources referenced by the bufferId. This method should be
     // invoked when the caller will no longer hold a reference to a GraphicBuffer
     // and needs to clean up its resources.
+    // Note that work is deferred to an additional thread, i.e. this call
+    // is made asynchronously, but the caller can expect that cache/unbind calls
+    // are performed in a manner that's conflict serializable, i.e. unbinding
+    // a buffer should never occur before binding the buffer if the caller
+    // called {bind, cache}ExternalTextureBuffer before calling unbind.
     virtual void unbindExternalTextureBuffer(uint64_t bufferId) = 0;
     // When binding a native buffer, it must be done before setViewportAndProjection
     // Returns NO_ERROR when binds successfully, NO_MEMORY when there's no memory for allocation.
diff --git a/libs/renderengine/include/renderengine/mock/RenderEngine.h b/libs/renderengine/include/renderengine/mock/RenderEngine.h
index e33bcfd994..b4d3ef24ba 100644
--- a/libs/renderengine/include/renderengine/mock/RenderEngine.h
+++ b/libs/renderengine/include/renderengine/mock/RenderEngine.h
@@ -51,7 +51,7 @@ public:
     MOCK_METHOD2(genTextures, void(size_t, uint32_t*));
     MOCK_METHOD2(deleteTextures, void(size_t, uint32_t const*));
     MOCK_METHOD2(bindExternalTextureImage, void(uint32_t, const renderengine::Image&));
-    MOCK_METHOD1(cacheExternalTextureBuffer, status_t(const sp<GraphicBuffer>&));
+    MOCK_METHOD1(cacheExternalTextureBuffer, void(const sp<GraphicBuffer>&));
     MOCK_METHOD3(bindExternalTextureBuffer,
                  status_t(uint32_t, const sp<GraphicBuffer>&, const sp<Fence>&));
     MOCK_METHOD1(unbindExternalTextureBuffer, void(uint64_t));
diff --git a/libs/renderengine/tests/Android.bp b/libs/renderengine/tests/Android.bp
index 9b483ef51d..e98babc30c 100644
--- a/libs/renderengine/tests/Android.bp
+++ b/libs/renderengine/tests/Android.bp
@@ -31,6 +31,7 @@ cc_test {
         "libgui",
         "liblog",
         "libnativewindow",
+        "libprocessgroup",
         "libsync",
         "libui",
         "libutils",
diff --git a/libs/renderengine/tests/RenderEngineTest.cpp b/libs/renderengine/tests/RenderEngineTest.cpp
index 7acaecf465..f47c7fd053 100644
--- a/libs/renderengine/tests/RenderEngineTest.cpp
+++ b/libs/renderengine/tests/RenderEngineTest.cpp
@@ -14,8 +14,10 @@
  * limitations under the License.
  */
 
-#include <gtest/gtest.h>
+#include <chrono>
+#include <condition_variable>
 
+#include <gtest/gtest.h>
 #include <renderengine/RenderEngine.h>
 #include <sync/sync.h>
 #include <ui/PixelFormat.h>
@@ -1001,8 +1003,15 @@ TEST_F(RenderEngineTest, drawLayers_fillsBufferAndCachesImages) {
     invokeDraw(settings, layers, mBuffer);
     uint64_t bufferId = layer.source.buffer.buffer->getId();
     EXPECT_TRUE(sRE->isImageCachedForTesting(bufferId));
-    sRE->unbindExternalTextureBuffer(bufferId);
+    std::shared_ptr<renderengine::gl::ImageManager::Barrier> barrier =
+            sRE->unbindExternalTextureBufferForTesting(bufferId);
+    std::lock_guard<std::mutex> lock(barrier->mutex);
+    ASSERT_TRUE(barrier->condition.wait_for(barrier->mutex, std::chrono::seconds(5),
+                                            [&]() REQUIRES(barrier->mutex) {
+                                                return barrier->isOpen;
+                                            }));
     EXPECT_FALSE(sRE->isImageCachedForTesting(bufferId));
+    EXPECT_EQ(NO_ERROR, barrier->result);
 }
 
 TEST_F(RenderEngineTest, drawLayers_bindExternalBufferWithNullBuffer) {
@@ -1019,21 +1028,52 @@ TEST_F(RenderEngineTest, drawLayers_bindExternalBufferCachesImages) {
     sRE->bindExternalTextureBuffer(texName, buf, nullptr);
     uint64_t bufferId = buf->getId();
     EXPECT_TRUE(sRE->isImageCachedForTesting(bufferId));
-    sRE->unbindExternalTextureBuffer(bufferId);
+    std::shared_ptr<renderengine::gl::ImageManager::Barrier> barrier =
+            sRE->unbindExternalTextureBufferForTesting(bufferId);
+    std::lock_guard<std::mutex> lock(barrier->mutex);
+    ASSERT_TRUE(barrier->condition.wait_for(barrier->mutex, std::chrono::seconds(5),
+                                            [&]() REQUIRES(barrier->mutex) {
+                                                return barrier->isOpen;
+                                            }));
+    EXPECT_EQ(NO_ERROR, barrier->result);
     EXPECT_FALSE(sRE->isImageCachedForTesting(bufferId));
 }
 
 TEST_F(RenderEngineTest, drawLayers_cacheExternalBufferWithNullBuffer) {
-    status_t result = sRE->cacheExternalTextureBuffer(nullptr);
-    ASSERT_EQ(BAD_VALUE, result);
+    std::shared_ptr<renderengine::gl::ImageManager::Barrier> barrier =
+            sRE->cacheExternalTextureBufferForTesting(nullptr);
+    std::lock_guard<std::mutex> lock(barrier->mutex);
+    ASSERT_TRUE(barrier->condition.wait_for(barrier->mutex, std::chrono::seconds(5),
+                                            [&]() REQUIRES(barrier->mutex) {
+                                                return barrier->isOpen;
+                                            }));
+    EXPECT_TRUE(barrier->isOpen);
+    EXPECT_EQ(BAD_VALUE, barrier->result);
 }
 
 TEST_F(RenderEngineTest, drawLayers_cacheExternalBufferCachesImages) {
     sp<GraphicBuffer> buf = allocateSourceBuffer(1, 1);
     uint64_t bufferId = buf->getId();
-    sRE->cacheExternalTextureBuffer(buf);
+    std::shared_ptr<renderengine::gl::ImageManager::Barrier> barrier =
+            sRE->cacheExternalTextureBufferForTesting(buf);
+    {
+        std::lock_guard<std::mutex> lock(barrier->mutex);
+        ASSERT_TRUE(barrier->condition.wait_for(barrier->mutex, std::chrono::seconds(5),
+                                                [&]() REQUIRES(barrier->mutex) {
+                                                    return barrier->isOpen;
+                                                }));
+        EXPECT_EQ(NO_ERROR, barrier->result);
+    }
     EXPECT_TRUE(sRE->isImageCachedForTesting(bufferId));
-    sRE->unbindExternalTextureBuffer(bufferId);
+    barrier = sRE->unbindExternalTextureBufferForTesting(bufferId);
+    {
+        std::lock_guard<std::mutex> lock(barrier->mutex);
+        ASSERT_TRUE(barrier->condition.wait_for(barrier->mutex, std::chrono::seconds(5),
+                                                [&]() REQUIRES(barrier->mutex) {
+                                                    return barrier->isOpen;
+                                                }));
+        EXPECT_EQ(NO_ERROR, barrier->result);
+    }
     EXPECT_FALSE(sRE->isImageCachedForTesting(bufferId));
 }
 
diff --git a/services/surfaceflinger/BufferLayerConsumer.cpp b/services/surfaceflinger/BufferLayerConsumer.cpp
index bd9bd81e2a..414814a6f4 100644
--- a/services/surfaceflinger/BufferLayerConsumer.cpp
+++ b/services/surfaceflinger/BufferLayerConsumer.cpp
@@ -494,7 +494,6 @@ void BufferLayerConsumer::onBufferAvailable(const BufferItem& item) {
         if (oldImage == nullptr || oldImage->graphicBuffer() == nullptr ||
             oldImage->graphicBuffer()->getId() != item.mGraphicBuffer->getId()) {
             mImages[item.mSlot] = std::make_shared<Image>(item.mGraphicBuffer, mRE);
-            mRE.cacheExternalTextureBuffer(item.mGraphicBuffer);
         }
     }
 }
@@ -531,6 +530,12 @@ void BufferLayerConsumer::dumpLocked(String8& result, const char* prefix) const
     ConsumerBase::dumpLocked(result, prefix);
 }
 
+BufferLayerConsumer::Image::Image(const sp<GraphicBuffer>& graphicBuffer,
+                                  renderengine::RenderEngine& engine)
+      : mGraphicBuffer(graphicBuffer), mRE(engine) {
+    mRE.cacheExternalTextureBuffer(mGraphicBuffer);
+}
+
 BufferLayerConsumer::Image::~Image() {
     if (mGraphicBuffer != nullptr) {
         ALOGV("Destroying buffer: %" PRId64, mGraphicBuffer->getId());
diff --git a/services/surfaceflinger/BufferLayerConsumer.h b/services/surfaceflinger/BufferLayerConsumer.h
index 901556a53f..617b1c2323 100644
--- a/services/surfaceflinger/BufferLayerConsumer.h
+++ b/services/surfaceflinger/BufferLayerConsumer.h
@@ -223,8 +223,7 @@ private:
     // Utility class for managing GraphicBuffer references into renderengine
     class Image {
     public:
-        Image(sp<GraphicBuffer> graphicBuffer, renderengine::RenderEngine& engine)
-              : mGraphicBuffer(graphicBuffer), mRE(engine) {}
+        Image(const sp<GraphicBuffer>& graphicBuffer, renderengine::RenderEngine& engine);
         virtual ~Image();
         const sp<GraphicBuffer>& graphicBuffer() { return mGraphicBuffer; }