From 1863d94e9a32a210cfb50d7c22bbac30dc33e010 Mon Sep 17 00:00:00 2001 From: Derek Sollenberger Date: Wed, 5 Feb 2020 15:41:51 -0500 Subject: [PATCH] Ensure SkiaPipeline always has a valid colorspace. Previously we didn't assign a colorspace to the pipeline until it was provided a surface to render into. This resulted in undefined behavior if the application attempted to render an offscreen layer before the OS provided the main window with its surface. Now instead of deferring setting whether or not the application is wide gamut we do initialize it to a default setting when the pipeline is created. Bug: 148042673 Test: apct/device_boot_health_check_extra_postsubmit Change-Id: I84d743511e949ac977486470bb14eec936de7f88 --- core/jni/android_view_ThreadedRenderer.cpp | 8 +++++--- graphics/java/android/graphics/HardwareRenderer.java | 5 +++-- libs/hwui/pipeline/skia/SkiaOpenGLPipeline.cpp | 6 ++---- libs/hwui/pipeline/skia/SkiaOpenGLPipeline.h | 2 +- libs/hwui/pipeline/skia/SkiaPipeline.cpp | 2 ++ libs/hwui/pipeline/skia/SkiaPipeline.h | 4 +++- libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp | 5 ++--- libs/hwui/pipeline/skia/SkiaVulkanPipeline.h | 2 +- libs/hwui/renderthread/CanvasContext.cpp | 6 +++--- libs/hwui/renderthread/CanvasContext.h | 1 - libs/hwui/renderthread/IRenderPipeline.h | 4 +++- libs/hwui/tests/unit/SkiaPipelineTests.cpp | 2 +- 12 files changed, 26 insertions(+), 21 deletions(-) diff --git a/core/jni/android_view_ThreadedRenderer.cpp b/core/jni/android_view_ThreadedRenderer.cpp index 69ca17c082571..5a8225c1f21a5 100644 --- a/core/jni/android_view_ThreadedRenderer.cpp +++ b/core/jni/android_view_ThreadedRenderer.cpp @@ -147,10 +147,12 @@ static jlong android_view_ThreadedRenderer_createRootRenderNode(JNIEnv* env, job } static jlong android_view_ThreadedRenderer_createProxy(JNIEnv* env, jobject clazz, - jboolean translucent, jlong rootRenderNodePtr) { + jboolean translucent, jboolean isWideGamut, jlong rootRenderNodePtr) { RootRenderNode* rootRenderNode = reinterpret_cast(rootRenderNodePtr); ContextFactoryImpl factory(rootRenderNode); - return (jlong) new RenderProxy(translucent, rootRenderNode, &factory); + RenderProxy* proxy = new RenderProxy(translucent, rootRenderNode, &factory); + proxy->setWideGamut(isWideGamut); + return (jlong) proxy; } static void android_view_ThreadedRenderer_deleteProxy(JNIEnv* env, jobject clazz, @@ -627,7 +629,7 @@ static const JNINativeMethod gMethods[] = { { "nSetProcessStatsBuffer", "(I)V", (void*) android_view_ThreadedRenderer_setProcessStatsBuffer }, { "nGetRenderThreadTid", "(J)I", (void*) android_view_ThreadedRenderer_getRenderThreadTid }, { "nCreateRootRenderNode", "()J", (void*) android_view_ThreadedRenderer_createRootRenderNode }, - { "nCreateProxy", "(ZJ)J", (void*) android_view_ThreadedRenderer_createProxy }, + { "nCreateProxy", "(ZZJ)J", (void*) android_view_ThreadedRenderer_createProxy }, { "nDeleteProxy", "(J)V", (void*) android_view_ThreadedRenderer_deleteProxy }, { "nLoadSystemProperties", "(J)Z", (void*) android_view_ThreadedRenderer_loadSystemProperties }, { "nSetName", "(JLjava/lang/String;)V", (void*) android_view_ThreadedRenderer_setName }, diff --git a/graphics/java/android/graphics/HardwareRenderer.java b/graphics/java/android/graphics/HardwareRenderer.java index 3b864139cf713..d08bfcf45a5c0 100644 --- a/graphics/java/android/graphics/HardwareRenderer.java +++ b/graphics/java/android/graphics/HardwareRenderer.java @@ -157,7 +157,7 @@ public class HardwareRenderer { public HardwareRenderer() { mRootNode = RenderNode.adopt(nCreateRootRenderNode()); mRootNode.setClipToBounds(false); - mNativeProxy = nCreateProxy(!mOpaque, mRootNode.mNativeRenderNode); + mNativeProxy = nCreateProxy(!mOpaque, mIsWideGamut, mRootNode.mNativeRenderNode); if (mNativeProxy == 0) { throw new OutOfMemoryError("Unable to create hardware renderer"); } @@ -1085,7 +1085,8 @@ public class HardwareRenderer { private static native long nCreateRootRenderNode(); - private static native long nCreateProxy(boolean translucent, long rootRenderNode); + private static native long nCreateProxy(boolean translucent, boolean isWideGamut, + long rootRenderNode); private static native void nDeleteProxy(long nativeProxy); diff --git a/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.cpp b/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.cpp index e7efe2ff798b2..8d5acc6312742 100644 --- a/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.cpp +++ b/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.cpp @@ -171,17 +171,15 @@ static void setBufferCount(ANativeWindow* window, uint32_t extraBuffers) { } bool SkiaOpenGLPipeline::setSurface(ANativeWindow* surface, SwapBehavior swapBehavior, - ColorMode colorMode, uint32_t extraBuffers) { + uint32_t extraBuffers) { if (mEglSurface != EGL_NO_SURFACE) { mEglManager.destroySurface(mEglSurface); mEglSurface = EGL_NO_SURFACE; } - setSurfaceColorProperties(colorMode); - if (surface) { mRenderThread.requireGlContext(); - auto newSurface = mEglManager.createSurface(surface, colorMode, mSurfaceColorSpace); + auto newSurface = mEglManager.createSurface(surface, mColorMode, mSurfaceColorSpace); if (!newSurface) { return false; } diff --git a/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.h b/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.h index 3fe0f92b19247..e482cad6c953e 100644 --- a/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.h +++ b/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.h @@ -44,7 +44,7 @@ public: FrameInfo* currentFrameInfo, bool* requireSwap) override; DeferredLayerUpdater* createTextureLayer() override; bool setSurface(ANativeWindow* surface, renderthread::SwapBehavior swapBehavior, - renderthread::ColorMode colorMode, uint32_t extraBuffers) override; + uint32_t extraBuffers) override; void onStop() override; bool isSurfaceReady() override; bool isContextReady() override; diff --git a/libs/hwui/pipeline/skia/SkiaPipeline.cpp b/libs/hwui/pipeline/skia/SkiaPipeline.cpp index 6f4af3d26b3a2..29b4dd7f32e7b 100644 --- a/libs/hwui/pipeline/skia/SkiaPipeline.cpp +++ b/libs/hwui/pipeline/skia/SkiaPipeline.cpp @@ -44,6 +44,7 @@ namespace uirenderer { namespace skiapipeline { SkiaPipeline::SkiaPipeline(RenderThread& thread) : mRenderThread(thread) { + setSurfaceColorProperties(mColorMode); } SkiaPipeline::~SkiaPipeline() { @@ -584,6 +585,7 @@ void SkiaPipeline::dumpResourceCacheUsage() const { } void SkiaPipeline::setSurfaceColorProperties(ColorMode colorMode) { + mColorMode = colorMode; if (colorMode == ColorMode::SRGB) { mSurfaceColorType = SkColorType::kN32_SkColorType; mSurfaceColorSpace = SkColorSpace::MakeSRGB(); diff --git a/libs/hwui/pipeline/skia/SkiaPipeline.h b/libs/hwui/pipeline/skia/SkiaPipeline.h index af8414de4bc18..8341164edc199 100644 --- a/libs/hwui/pipeline/skia/SkiaPipeline.h +++ b/libs/hwui/pipeline/skia/SkiaPipeline.h @@ -50,6 +50,7 @@ public: bool createOrUpdateLayer(RenderNode* node, const DamageAccumulator& damageAccumulator, ErrorHandler* errorHandler) override; + void setSurfaceColorProperties(renderthread::ColorMode colorMode) override; SkColorType getSurfaceColorType() const override { return mSurfaceColorType; } sk_sp getSurfaceColorSpace() override { return mSurfaceColorSpace; } @@ -72,9 +73,10 @@ public: protected: void dumpResourceCacheUsage() const; - void setSurfaceColorProperties(renderthread::ColorMode colorMode); renderthread::RenderThread& mRenderThread; + + renderthread::ColorMode mColorMode = renderthread::ColorMode::SRGB; SkColorType mSurfaceColorType; sk_sp mSurfaceColorSpace; diff --git a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp index ad7c70614239f..535a19956e030 100644 --- a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp +++ b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp @@ -117,17 +117,16 @@ DeferredLayerUpdater* SkiaVulkanPipeline::createTextureLayer() { void SkiaVulkanPipeline::onStop() {} bool SkiaVulkanPipeline::setSurface(ANativeWindow* surface, SwapBehavior swapBehavior, - ColorMode colorMode, uint32_t extraBuffers) { + uint32_t extraBuffers) { if (mVkSurface) { mVkManager.destroySurface(mVkSurface); mVkSurface = nullptr; } - setSurfaceColorProperties(colorMode); if (surface) { mRenderThread.requireVkContext(); mVkSurface = - mVkManager.createSurface(surface, colorMode, mSurfaceColorSpace, mSurfaceColorType, + mVkManager.createSurface(surface, mColorMode, mSurfaceColorSpace, mSurfaceColorType, mRenderThread.getGrContext(), extraBuffers); } diff --git a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h index 31734783de7f9..c8bf233d8e1cc 100644 --- a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h +++ b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h @@ -43,7 +43,7 @@ public: FrameInfo* currentFrameInfo, bool* requireSwap) override; DeferredLayerUpdater* createTextureLayer() override; bool setSurface(ANativeWindow* surface, renderthread::SwapBehavior swapBehavior, - renderthread::ColorMode colorMode, uint32_t extraBuffers) override; + uint32_t extraBuffers) override; void onStop() override; bool isSurfaceReady() override; bool isContextReady() override; diff --git a/libs/hwui/renderthread/CanvasContext.cpp b/libs/hwui/renderthread/CanvasContext.cpp index c1435d1ea2d5d..0f3901cb225bc 100644 --- a/libs/hwui/renderthread/CanvasContext.cpp +++ b/libs/hwui/renderthread/CanvasContext.cpp @@ -161,9 +161,8 @@ void CanvasContext::setSurface(sp&& surface, bool enableTimeout) { mRenderAheadCapacity = mRenderAheadDepth; } - ColorMode colorMode = mWideColorGamut ? ColorMode::WideColorGamut : ColorMode::SRGB; bool hasSurface = mRenderPipeline->setSurface( - mNativeSurface ? mNativeSurface->getNativeWindow() : nullptr, mSwapBehavior, colorMode, + mNativeSurface ? mNativeSurface->getNativeWindow() : nullptr, mSwapBehavior, mRenderAheadCapacity); mFrameNumber = -1; @@ -225,7 +224,8 @@ void CanvasContext::setOpaque(bool opaque) { } void CanvasContext::setWideGamut(bool wideGamut) { - mWideColorGamut = wideGamut; + ColorMode colorMode = wideGamut ? ColorMode::WideColorGamut : ColorMode::SRGB; + mRenderPipeline->setSurfaceColorProperties(colorMode); } bool CanvasContext::makeCurrent() { diff --git a/libs/hwui/renderthread/CanvasContext.h b/libs/hwui/renderthread/CanvasContext.h index 0967b20e44ee8..629c741e87573 100644 --- a/libs/hwui/renderthread/CanvasContext.h +++ b/libs/hwui/renderthread/CanvasContext.h @@ -251,7 +251,6 @@ private: nsecs_t mLastDropVsync = 0; bool mOpaque; - bool mWideColorGamut = false; bool mUseForceDark = false; LightInfo mLightInfo; LightGeometry mLightGeometry = {{0, 0, 0}, 0}; diff --git a/libs/hwui/renderthread/IRenderPipeline.h b/libs/hwui/renderthread/IRenderPipeline.h index ef0aa98d4220d..ba0d64c5492d1 100644 --- a/libs/hwui/renderthread/IRenderPipeline.h +++ b/libs/hwui/renderthread/IRenderPipeline.h @@ -66,7 +66,7 @@ public: virtual bool swapBuffers(const Frame& frame, bool drew, const SkRect& screenDirty, FrameInfo* currentFrameInfo, bool* requireSwap) = 0; virtual DeferredLayerUpdater* createTextureLayer() = 0; - virtual bool setSurface(ANativeWindow* window, SwapBehavior swapBehavior, ColorMode colorMode, + virtual bool setSurface(ANativeWindow* window, SwapBehavior swapBehavior, uint32_t extraBuffers) = 0; virtual void onStop() = 0; virtual bool isSurfaceReady() = 0; @@ -80,6 +80,8 @@ public: virtual bool pinImages(std::vector& mutableImages) = 0; virtual bool pinImages(LsaVector>& images) = 0; virtual void unpinImages() = 0; + + virtual void setSurfaceColorProperties(ColorMode colorMode) = 0; virtual SkColorType getSurfaceColorType() const = 0; virtual sk_sp getSurfaceColorSpace() = 0; virtual GrSurfaceOrigin getSurfaceOrigin() = 0; diff --git a/libs/hwui/tests/unit/SkiaPipelineTests.cpp b/libs/hwui/tests/unit/SkiaPipelineTests.cpp index 307d13606cb8b..90bcd1c0e370c 100644 --- a/libs/hwui/tests/unit/SkiaPipelineTests.cpp +++ b/libs/hwui/tests/unit/SkiaPipelineTests.cpp @@ -398,7 +398,7 @@ RENDERTHREAD_SKIA_PIPELINE_TEST(SkiaPipeline, context_lost) { auto surface = context.surface(); auto pipeline = std::make_unique(renderThread); EXPECT_FALSE(pipeline->isSurfaceReady()); - EXPECT_TRUE(pipeline->setSurface(surface.get(), SwapBehavior::kSwap_default, ColorMode::SRGB, 0)); + EXPECT_TRUE(pipeline->setSurface(surface.get(), SwapBehavior::kSwap_default, 0)); EXPECT_TRUE(pipeline->isSurfaceReady()); renderThread.destroyRenderingContext(); EXPECT_FALSE(pipeline->isSurfaceReady());