From ee3754ab24a551f846e524e08a3bab425a70ab40 Mon Sep 17 00:00:00 2001 From: Stan Iliev Date: Fri, 15 Mar 2019 12:07:35 -0400 Subject: [PATCH] Fix TextureView glitch and memory leak Keep EglImage/VkImage alive until the last SkImage object using it is destroyed. Fix memory leak with GL pipeline only, caused by destroying EGLImage on the wrong thread. Make sure cached SkImage dtor is invoked on RenderThread. Bug: 128523183 Bug: 128688107 Test: Ran settings app, systrace and CTS Change-Id: I32c14c7174a2a97e54fbfaa49dffb4e9a160f90d --- libs/hwui/renderthread/RenderThread.h | 2 + libs/hwui/surfacetexture/ImageConsumer.cpp | 154 ++++++++++++++++----- libs/hwui/surfacetexture/ImageConsumer.h | 18 ++- 3 files changed, 132 insertions(+), 42 deletions(-) diff --git a/libs/hwui/renderthread/RenderThread.h b/libs/hwui/renderthread/RenderThread.h index 9298be65c5e8f..5f43b488bcf26 100644 --- a/libs/hwui/renderthread/RenderThread.h +++ b/libs/hwui/renderthread/RenderThread.h @@ -41,6 +41,7 @@ namespace android { class Bitmap; +class AutoBackendTextureRelease; namespace uirenderer { @@ -135,6 +136,7 @@ private: friend class DispatchFrameCallbacks; friend class RenderProxy; friend class DummyVsyncSource; + friend class android::AutoBackendTextureRelease; friend class android::uirenderer::TestUtils; friend class android::uirenderer::WebViewFunctor; friend class android::uirenderer::skiapipeline::VkFunctorDrawHandler; diff --git a/libs/hwui/surfacetexture/ImageConsumer.cpp b/libs/hwui/surfacetexture/ImageConsumer.cpp index 077a8f73b0da5..65d95ad36a6fd 100644 --- a/libs/hwui/surfacetexture/ImageConsumer.cpp +++ b/libs/hwui/surfacetexture/ImageConsumer.cpp @@ -24,13 +24,17 @@ #include "renderthread/VulkanManager.h" #include "utils/Color.h" #include +#include // Macro for including the SurfaceTexture name in log messages #define IMG_LOGE(x, ...) ALOGE("[%s] " x, st.mName.string(), ##__VA_ARGS__) +using namespace android::uirenderer::renderthread; + namespace android { void ImageConsumer::onFreeBufferLocked(int slotIndex) { + // This callback may be invoked on any thread. mImageSlots[slotIndex].clear(); } @@ -46,55 +50,141 @@ void ImageConsumer::onReleaseBufferLocked(int buf) { mImageSlots[buf].eglFence() = EGL_NO_SYNC_KHR; } +/** + * AutoBackendTextureRelease manages EglImage/VkImage lifetime. It is a ref-counted object + * that keeps GPU resources alive until the last SKImage object using them is destroyed. + */ +class AutoBackendTextureRelease { +public: + static void releaseProc(SkImage::ReleaseContext releaseContext); + + AutoBackendTextureRelease(GrContext* context, GraphicBuffer* buffer); + + const GrBackendTexture& getTexture() const { return mBackendTexture; } + + void ref() { mUsageCount++; } + + void unref(bool releaseImage); + + inline sk_sp getImage() { return mImage; } + + void makeImage(sp& graphicBuffer, android_dataspace dataspace, + GrContext* context); + +private: + // The only way to invoke dtor is with unref, when mUsageCount is 0. + ~AutoBackendTextureRelease() {} + + GrBackendTexture mBackendTexture; + GrAHardwareBufferUtils::DeleteImageProc mDeleteProc; + GrAHardwareBufferUtils::DeleteImageCtx mDeleteCtx; + + // Starting with refcount 1, because the first ref is held by SurfaceTexture. Additional refs + // are held by SkImages. + int mUsageCount = 1; + + // mImage is the SkImage created from mBackendTexture. + sk_sp mImage; +}; + +AutoBackendTextureRelease::AutoBackendTextureRelease(GrContext* context, GraphicBuffer* buffer) { + bool createProtectedImage = + 0 != (buffer->getUsage() & GraphicBuffer::USAGE_PROTECTED); + GrBackendFormat backendFormat = GrAHardwareBufferUtils::GetBackendFormat( + context, + reinterpret_cast(buffer), + buffer->getPixelFormat(), + false); + mBackendTexture = GrAHardwareBufferUtils::MakeBackendTexture( + context, + reinterpret_cast(buffer), + buffer->getWidth(), + buffer->getHeight(), + &mDeleteProc, + &mDeleteCtx, + createProtectedImage, + backendFormat, + false); +} + +void AutoBackendTextureRelease::unref(bool releaseImage) { + if (!RenderThread::isCurrent()) { + // EGLImage needs to be destroyed on RenderThread to prevent memory leak. + // ~SkImage dtor for both pipelines needs to be invoked on RenderThread, because it is not + // thread safe. + RenderThread::getInstance().queue().post([this, releaseImage]() { unref(releaseImage); }); + return; + } + + if (releaseImage) { + mImage.reset(); + } + + mUsageCount--; + if (mUsageCount <= 0) { + if (mBackendTexture.isValid()) { + mDeleteProc(mDeleteCtx); + mBackendTexture = {}; + } + delete this; + } +} + +void AutoBackendTextureRelease::releaseProc(SkImage::ReleaseContext releaseContext) { + AutoBackendTextureRelease* textureRelease = + reinterpret_cast(releaseContext); + textureRelease->unref(false); +} + +void AutoBackendTextureRelease::makeImage(sp& graphicBuffer, + android_dataspace dataspace, GrContext* context) { + SkColorType colorType = GrAHardwareBufferUtils::GetSkColorTypeFromBufferFormat( + graphicBuffer->getPixelFormat()); + mImage = SkImage::MakeFromTexture(context, + mBackendTexture, + kTopLeft_GrSurfaceOrigin, + colorType, + kPremul_SkAlphaType, + uirenderer::DataSpaceToColorSpace(dataspace), + releaseProc, + this); + if (mImage.get()) { + // The following ref will be counteracted by releaseProc, when SkImage is discarded. + ref(); + } +} + void ImageConsumer::ImageSlot::createIfNeeded(sp graphicBuffer, android_dataspace dataspace, bool forceCreate, GrContext* context) { - if (!mImage.get() || dataspace != mDataspace || forceCreate) { + if (!mTextureRelease || !mTextureRelease->getImage().get() || dataspace != mDataspace + || forceCreate) { if (!graphicBuffer.get()) { clear(); return; } - if (!mBackendTexture.isValid()) { - clear(); - bool createProtectedImage = - 0 != (graphicBuffer->getUsage() & GraphicBuffer::USAGE_PROTECTED); - GrBackendFormat backendFormat = GrAHardwareBufferUtils::GetBackendFormat( - context, - reinterpret_cast(graphicBuffer.get()), - graphicBuffer->getPixelFormat(), - false); - mBackendTexture = GrAHardwareBufferUtils::MakeBackendTexture( - context, - reinterpret_cast(graphicBuffer.get()), - graphicBuffer->getWidth(), - graphicBuffer->getHeight(), - &mDeleteProc, - &mDeleteCtx, - createProtectedImage, - backendFormat, - false); + if (!mTextureRelease) { + mTextureRelease = new AutoBackendTextureRelease(context, graphicBuffer.get()); } + mDataspace = dataspace; - SkColorType colorType = GrAHardwareBufferUtils::GetSkColorTypeFromBufferFormat( - graphicBuffer->getPixelFormat()); - mImage = SkImage::MakeFromTexture(context, - mBackendTexture, - kTopLeft_GrSurfaceOrigin, - colorType, - kPremul_SkAlphaType, - uirenderer::DataSpaceToColorSpace(dataspace)); + mTextureRelease->makeImage(graphicBuffer, dataspace, context); } } void ImageConsumer::ImageSlot::clear() { - mImage.reset(); - if (mBackendTexture.isValid()) { - mDeleteProc(mDeleteCtx); - mBackendTexture = {}; + if (mTextureRelease) { + // The following unref counteracts the initial mUsageCount of 1, set by default initializer. + mTextureRelease->unref(true); + mTextureRelease = nullptr; } } +sk_sp ImageConsumer::ImageSlot::getImage() { + return mTextureRelease ? mTextureRelease->getImage() : nullptr; +} + sk_sp ImageConsumer::dequeueImage(bool* queueEmpty, SurfaceTexture& st, uirenderer::RenderState& renderState) { BufferItem item; diff --git a/libs/hwui/surfacetexture/ImageConsumer.h b/libs/hwui/surfacetexture/ImageConsumer.h index eee0a0ac3512a..2fdece9898761 100644 --- a/libs/hwui/surfacetexture/ImageConsumer.h +++ b/libs/hwui/surfacetexture/ImageConsumer.h @@ -25,7 +25,6 @@ #include #include #include -#include namespace GrAHardwareBufferUtils { typedef void* DeleteImageCtx; @@ -38,6 +37,7 @@ namespace uirenderer { class RenderState; } +class AutoBackendTextureRelease; class SurfaceTexture; /* @@ -81,16 +81,14 @@ private: void createIfNeeded(sp graphicBuffer, android_dataspace dataspace, bool forceCreate, GrContext* context); + void clear(); inline EGLSyncKHR& eglFence() { return mEglFence; } - inline sk_sp getImage() { return mImage; } + sk_sp getImage(); private: - // mImage is the SkImage created from mGraphicBuffer. - sk_sp mImage; - // the dataspace associated with the current image android_dataspace mDataspace; @@ -100,11 +98,11 @@ private: */ EGLSyncKHR mEglFence; - GrBackendTexture mBackendTexture; - - GrAHardwareBufferUtils::DeleteImageProc mDeleteProc; - - GrAHardwareBufferUtils::DeleteImageCtx mDeleteCtx; + /** + * mTextureRelease may outlive ImageConsumer, if the last ref is held by an SkImage. + * ImageConsumer holds one ref to mTextureRelease, which is decremented by "clear". + */ + AutoBackendTextureRelease* mTextureRelease = nullptr; }; /**