RE: move useProtectedContext calls to drawLayers
This change removes a race condition when calling RenderEngine from
multiple threads. Before this change, two competing threads could call
RenderEngine::useProtectedContext with different values, potentially
causing RenderEngine::drawLayers to use the wrong context.
We currently call RenderEngine from a single thread but that will change
with the upcoming refactor of SurfaceFlinger::renderScreenImpl.
Bug: 238643986
Test: presubmits
Change-Id: I78ca00797b1604cedae1f134e963b6b78f2b802f
diff --git a/libs/renderengine/RenderEngine.cpp b/libs/renderengine/RenderEngine.cpp
index 9d9cb6b..f1fc0a4 100644
--- a/libs/renderengine/RenderEngine.cpp
+++ b/libs/renderengine/RenderEngine.cpp
@@ -19,6 +19,7 @@
#include <cutils/properties.h>
#include <log/log.h>
#include "gl/GLESRenderEngine.h"
+#include "renderengine/ExternalTexture.h"
#include "threaded/RenderEngineThreaded.h"
#include "skia/SkiaGLRenderEngine.h"
@@ -70,10 +71,22 @@
base::unique_fd&& bufferFence) {
const auto resultPromise = std::make_shared<std::promise<FenceResult>>();
std::future<FenceResult> resultFuture = resultPromise->get_future();
+ updateProtectedContext(layers, buffer);
drawLayersInternal(std::move(resultPromise), display, layers, buffer, useFramebufferCache,
std::move(bufferFence));
return resultFuture;
}
+void RenderEngine::updateProtectedContext(const std::vector<LayerSettings>& layers,
+ const std::shared_ptr<ExternalTexture>& buffer) {
+ const bool needsProtectedContext =
+ (buffer && (buffer->getUsage() & GRALLOC_USAGE_PROTECTED)) ||
+ std::any_of(layers.begin(), layers.end(), [](const LayerSettings& layer) {
+ const std::shared_ptr<ExternalTexture>& buffer = layer.source.buffer.buffer;
+ return buffer && (buffer->getUsage() & GRALLOC_USAGE_PROTECTED);
+ });
+ useProtectedContext(needsProtectedContext);
+}
+
} // namespace renderengine
} // namespace android
diff --git a/libs/renderengine/gl/GLESRenderEngine.h b/libs/renderengine/gl/GLESRenderEngine.h
index 1ee5cba..1b34921 100644
--- a/libs/renderengine/gl/GLESRenderEngine.h
+++ b/libs/renderengine/gl/GLESRenderEngine.h
@@ -61,7 +61,7 @@
std::future<void> primeCache() override;
void genTextures(size_t count, uint32_t* names) override;
void deleteTextures(size_t count, uint32_t const* names) override;
- bool isProtected() const override { return mInProtectedContext; }
+ bool isProtected() const { return mInProtectedContext; }
bool supportsProtectedContent() const override;
void useProtectedContext(bool useProtectedContext) override;
void cleanupPostRender() override;
diff --git a/libs/renderengine/include/renderengine/RenderEngine.h b/libs/renderengine/include/renderengine/RenderEngine.h
index 199392c..9182feb 100644
--- a/libs/renderengine/include/renderengine/RenderEngine.h
+++ b/libs/renderengine/include/renderengine/RenderEngine.h
@@ -126,12 +126,8 @@
// ----- BEGIN NEW INTERFACE -----
// queries that are required to be thread safe
- virtual bool isProtected() const = 0;
virtual bool supportsProtectedContent() const = 0;
- // Attempt to switch RenderEngine into and out of protectedContext mode
- virtual void useProtectedContext(bool useProtectedContext) = 0;
-
// Notify RenderEngine of changes to the dimensions of the active display
// so that it can configure its internal caches accordingly.
virtual void onActiveDisplaySizeChanged(ui::Size size) = 0;
@@ -238,6 +234,13 @@
friend class RenderEngineTest_cleanupPostRender_cleansUpOnce_Test;
const RenderEngineType mRenderEngineType;
+ // Update protectedContext mode depending on whether or not any layer has a protected buffer.
+ void updateProtectedContext(const std::vector<LayerSettings>&,
+ const std::shared_ptr<ExternalTexture>&);
+
+ // Attempt to switch RenderEngine into and out of protectedContext mode
+ virtual void useProtectedContext(bool useProtectedContext) = 0;
+
virtual void drawLayersInternal(
const std::shared_ptr<std::promise<FenceResult>>&& resultPromise,
const DisplaySettings& display, const std::vector<LayerSettings>& layers,
diff --git a/libs/renderengine/skia/SkiaRenderEngine.h b/libs/renderengine/skia/SkiaRenderEngine.h
index e7c5b8f..1973c7d 100644
--- a/libs/renderengine/skia/SkiaRenderEngine.h
+++ b/libs/renderengine/skia/SkiaRenderEngine.h
@@ -68,7 +68,6 @@
std::future<void> primeCache() override final;
void cleanupPostRender() override final;
void cleanFramebufferCache() override final{ }
- bool isProtected() const override final{ return mInProtectedContext; }
bool supportsBackgroundBlur() override final {
return mBlurFilter != nullptr;
}
@@ -102,6 +101,8 @@
size_t getMaxViewportDims() const override final;
GrDirectContext* getActiveGrContext();
+ bool isProtected() const { return mInProtectedContext; }
+
// Implements PersistentCache as a way to monitor what SkSL shaders Skia has
// cached.
class SkSLCacheMonitor : public GrContextOptions::PersistentCache {
diff --git a/libs/renderengine/tests/RenderEngineThreadedTest.cpp b/libs/renderengine/tests/RenderEngineThreadedTest.cpp
index 1a96289..fe3a16d 100644
--- a/libs/renderengine/tests/RenderEngineThreadedTest.cpp
+++ b/libs/renderengine/tests/RenderEngineThreadedTest.cpp
@@ -17,8 +17,10 @@
#include <cutils/properties.h>
#include <gmock/gmock.h>
#include <gtest/gtest.h>
+#include <hardware/gralloc.h>
#include <renderengine/impl/ExternalTexture.h>
#include <renderengine/mock/RenderEngine.h>
+#include <ui/PixelFormat.h>
#include "../threaded/RenderEngineThreaded.h"
namespace android {
@@ -95,18 +97,6 @@
ASSERT_EQ(dims, result);
}
-TEST_F(RenderEngineThreadedTest, isProtected_returnsFalse) {
- EXPECT_CALL(*mRenderEngine, isProtected()).WillOnce(Return(false));
- status_t result = mThreadedRE->isProtected();
- ASSERT_EQ(false, result);
-}
-
-TEST_F(RenderEngineThreadedTest, isProtected_returnsTrue) {
- EXPECT_CALL(*mRenderEngine, isProtected()).WillOnce(Return(true));
- size_t result = mThreadedRE->isProtected();
- ASSERT_EQ(true, result);
-}
-
TEST_F(RenderEngineThreadedTest, supportsProtectedContent_returnsFalse) {
EXPECT_CALL(*mRenderEngine, supportsProtectedContent()).WillOnce(Return(false));
status_t result = mThreadedRE->supportsProtectedContent();
@@ -119,28 +109,6 @@
ASSERT_EQ(true, result);
}
-TEST_F(RenderEngineThreadedTest, useProtectedContext) {
- EXPECT_CALL(*mRenderEngine, useProtectedContext(true));
- auto& ipExpect = EXPECT_CALL(*mRenderEngine, isProtected()).WillOnce(Return(false));
- EXPECT_CALL(*mRenderEngine, supportsProtectedContent()).WillOnce(Return(true));
- EXPECT_CALL(*mRenderEngine, isProtected()).After(ipExpect).WillOnce(Return(true));
-
- mThreadedRE->useProtectedContext(true);
- ASSERT_EQ(true, mThreadedRE->isProtected());
-
- // call ANY synchronous function to ensure that useProtectedContext has completed.
- mThreadedRE->getContextPriority();
- ASSERT_EQ(true, mThreadedRE->isProtected());
-}
-
-TEST_F(RenderEngineThreadedTest, useProtectedContext_quickReject) {
- EXPECT_CALL(*mRenderEngine, useProtectedContext(false)).Times(0);
- EXPECT_CALL(*mRenderEngine, isProtected()).WillOnce(Return(false));
- mThreadedRE->useProtectedContext(false);
- // call ANY synchronous function to ensure that useProtectedContext has completed.
- mThreadedRE->getContextPriority();
-}
-
TEST_F(RenderEngineThreadedTest, PostRenderCleanup_skipped) {
EXPECT_CALL(*mRenderEngine, canSkipPostRenderCleanup()).WillOnce(Return(true));
EXPECT_CALL(*mRenderEngine, cleanupPostRender()).Times(0);
@@ -182,6 +150,68 @@
base::unique_fd bufferFence;
+ EXPECT_CALL(*mRenderEngine, useProtectedContext(false));
+ EXPECT_CALL(*mRenderEngine, drawLayersInternal)
+ .WillOnce([&](const std::shared_ptr<std::promise<FenceResult>>&& resultPromise,
+ const renderengine::DisplaySettings&,
+ const std::vector<renderengine::LayerSettings>&,
+ const std::shared_ptr<renderengine::ExternalTexture>&, const bool,
+ base::unique_fd&&) { resultPromise->set_value(Fence::NO_FENCE); });
+
+ ftl::Future<FenceResult> future =
+ mThreadedRE->drawLayers(settings, layers, buffer, false, std::move(bufferFence));
+ ASSERT_TRUE(future.valid());
+ auto result = future.get();
+ ASSERT_TRUE(result.ok());
+}
+
+TEST_F(RenderEngineThreadedTest, drawLayers_protectedLayer) {
+ renderengine::DisplaySettings settings;
+ auto layerBuffer = sp<GraphicBuffer>::make();
+ layerBuffer->usage |= GRALLOC_USAGE_PROTECTED;
+ renderengine::LayerSettings layer;
+ layer.source.buffer.buffer = std::make_shared<
+ renderengine::impl::ExternalTexture>(std::move(layerBuffer), *mRenderEngine,
+ renderengine::impl::ExternalTexture::Usage::
+ READABLE);
+ std::vector<renderengine::LayerSettings> layers = {std::move(layer)};
+ std::shared_ptr<renderengine::ExternalTexture> buffer = std::make_shared<
+ renderengine::impl::
+ ExternalTexture>(sp<GraphicBuffer>::make(), *mRenderEngine,
+ renderengine::impl::ExternalTexture::Usage::READABLE |
+ renderengine::impl::ExternalTexture::Usage::WRITEABLE);
+
+ base::unique_fd bufferFence;
+
+ EXPECT_CALL(*mRenderEngine, useProtectedContext(true));
+ EXPECT_CALL(*mRenderEngine, drawLayersInternal)
+ .WillOnce([&](const std::shared_ptr<std::promise<FenceResult>>&& resultPromise,
+ const renderengine::DisplaySettings&,
+ const std::vector<renderengine::LayerSettings>&,
+ const std::shared_ptr<renderengine::ExternalTexture>&, const bool,
+ base::unique_fd&&) { resultPromise->set_value(Fence::NO_FENCE); });
+
+ ftl::Future<FenceResult> future =
+ mThreadedRE->drawLayers(settings, layers, buffer, false, std::move(bufferFence));
+ ASSERT_TRUE(future.valid());
+ auto result = future.get();
+ ASSERT_TRUE(result.ok());
+}
+
+TEST_F(RenderEngineThreadedTest, drawLayers_protectedOutputBuffer) {
+ renderengine::DisplaySettings settings;
+ std::vector<renderengine::LayerSettings> layers;
+ auto graphicBuffer = sp<GraphicBuffer>::make();
+ graphicBuffer->usage |= GRALLOC_USAGE_PROTECTED;
+ std::shared_ptr<renderengine::ExternalTexture> buffer = std::make_shared<
+ renderengine::impl::
+ ExternalTexture>(std::move(graphicBuffer), *mRenderEngine,
+ renderengine::impl::ExternalTexture::Usage::READABLE |
+ renderengine::impl::ExternalTexture::Usage::WRITEABLE);
+
+ base::unique_fd bufferFence;
+
+ EXPECT_CALL(*mRenderEngine, useProtectedContext(true));
EXPECT_CALL(*mRenderEngine, drawLayersInternal)
.WillOnce([&](const std::shared_ptr<std::promise<FenceResult>>&& resultPromise,
const renderengine::DisplaySettings&,
diff --git a/libs/renderengine/threaded/RenderEngineThreaded.cpp b/libs/renderengine/threaded/RenderEngineThreaded.cpp
index b41e843..8aa41b3 100644
--- a/libs/renderengine/threaded/RenderEngineThreaded.cpp
+++ b/libs/renderengine/threaded/RenderEngineThreaded.cpp
@@ -90,7 +90,6 @@
}
mRenderEngine = factory();
- mIsProtected = mRenderEngine->isProtected();
pthread_setname_np(pthread_self(), mThreadName);
@@ -255,41 +254,11 @@
return mRenderEngine->getMaxViewportDims();
}
-bool RenderEngineThreaded::isProtected() const {
- waitUntilInitialized();
- std::lock_guard lock(mThreadMutex);
- return mIsProtected;
-}
-
bool RenderEngineThreaded::supportsProtectedContent() const {
waitUntilInitialized();
return mRenderEngine->supportsProtectedContent();
}
-void RenderEngineThreaded::useProtectedContext(bool useProtectedContext) {
- if (isProtected() == useProtectedContext ||
- (useProtectedContext && !supportsProtectedContent())) {
- return;
- }
-
- {
- std::lock_guard lock(mThreadMutex);
- mFunctionCalls.push([useProtectedContext, this](renderengine::RenderEngine& instance) {
- ATRACE_NAME("REThreaded::useProtectedContext");
- instance.useProtectedContext(useProtectedContext);
- if (instance.isProtected() != useProtectedContext) {
- ALOGE("Failed to switch RenderEngine context.");
- // reset the cached mIsProtected value to a good state, but this does not
- // prevent other callers of this method and isProtected from reading the
- // invalid cached value.
- mIsProtected = instance.isProtected();
- }
- });
- mIsProtected = useProtectedContext;
- }
- mCondition.notify_one();
-}
-
void RenderEngineThreaded::cleanupPostRender() {
if (canSkipPostRenderCleanup()) {
return;
@@ -334,6 +303,7 @@
mFunctionCalls.push([resultPromise, display, layers, buffer, useFramebufferCache,
fd](renderengine::RenderEngine& instance) {
ATRACE_NAME("REThreaded::drawLayers");
+ instance.updateProtectedContext(layers, buffer);
instance.drawLayersInternal(std::move(resultPromise), display, layers, buffer,
useFramebufferCache, base::unique_fd(fd));
});
diff --git a/libs/renderengine/threaded/RenderEngineThreaded.h b/libs/renderengine/threaded/RenderEngineThreaded.h
index bf2ebea..168e2d2 100644
--- a/libs/renderengine/threaded/RenderEngineThreaded.h
+++ b/libs/renderengine/threaded/RenderEngineThreaded.h
@@ -51,9 +51,7 @@
size_t getMaxTextureSize() const override;
size_t getMaxViewportDims() const override;
- bool isProtected() const override;
bool supportsProtectedContent() const override;
- void useProtectedContext(bool useProtectedContext) override;
void cleanupPostRender() override;
ftl::Future<FenceResult> drawLayers(const DisplaySettings& display,
@@ -84,6 +82,9 @@
void waitUntilInitialized() const;
static status_t setSchedFifo(bool enabled);
+ // No-op. This method is only called on leaf implementations of RenderEngine.
+ void useProtectedContext(bool) override {}
+
/* ------------------------------------------------------------------------
* Threading
*/
@@ -107,7 +108,6 @@
* Render Engine
*/
std::unique_ptr<renderengine::RenderEngine> mRenderEngine;
- std::atomic<bool> mIsProtected = false;
};
} // namespace threaded
} // namespace renderengine
diff --git a/services/surfaceflinger/CompositionEngine/src/Output.cpp b/services/surfaceflinger/CompositionEngine/src/Output.cpp
index 0622534..17aefc5 100644
--- a/services/surfaceflinger/CompositionEngine/src/Output.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/Output.cpp
@@ -1178,15 +1178,9 @@
bool needsProtected = std::any_of(layers.begin(), layers.end(), [](auto* layer) {
return layer->getLayerFE().getCompositionState()->hasProtectedContent;
});
- if (needsProtected != renderEngine.isProtected()) {
- renderEngine.useProtectedContext(needsProtected);
- }
- if (needsProtected != mRenderSurface->isProtected() &&
- needsProtected == renderEngine.isProtected()) {
+ if (needsProtected != mRenderSurface->isProtected()) {
mRenderSurface->setProtected(needsProtected);
}
- } else if (!outputState.isSecure && renderEngine.isProtected()) {
- renderEngine.useProtectedContext(false);
}
}
diff --git a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
index 514a8ff..a955178 100644
--- a/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/OutputTest.cpp
@@ -4010,39 +4010,11 @@
Layer mLayer2;
};
-TEST_F(OutputComposeSurfacesTest_HandlesProtectedContent, ifDisplayIsNotSecure) {
- mOutput.mState.isSecure = false;
- mLayer2.mLayerFEState.hasProtectedContent = true;
- EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(true));
- EXPECT_CALL(mRenderEngine, isProtected).WillOnce(Return(true));
- EXPECT_CALL(mRenderEngine, useProtectedContext(false));
-
- base::unique_fd fd;
- std::shared_ptr<renderengine::ExternalTexture> tex;
- mOutput.updateProtectedContentState();
- mOutput.dequeueRenderBuffer(&fd, &tex);
- mOutput.composeSurfaces(kDebugRegion, kDefaultRefreshArgs, tex, fd);
-}
-
-TEST_F(OutputComposeSurfacesTest_HandlesProtectedContent, ifRenderEngineDoesNotSupportIt) {
- mOutput.mState.isSecure = true;
- mLayer2.mLayerFEState.hasProtectedContent = true;
- EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(false));
-
- base::unique_fd fd;
- std::shared_ptr<renderengine::ExternalTexture> tex;
- mOutput.updateProtectedContentState();
- mOutput.dequeueRenderBuffer(&fd, &tex);
- mOutput.composeSurfaces(kDebugRegion, kDefaultRefreshArgs, tex, fd);
-}
-
TEST_F(OutputComposeSurfacesTest_HandlesProtectedContent, ifNoProtectedContentLayers) {
mOutput.mState.isSecure = true;
mLayer2.mLayerFEState.hasProtectedContent = false;
EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(true));
- EXPECT_CALL(mRenderEngine, isProtected).WillOnce(Return(true)).WillOnce(Return(false));
EXPECT_CALL(*mRenderSurface, isProtected).WillOnce(Return(true));
- EXPECT_CALL(mRenderEngine, useProtectedContext(false));
EXPECT_CALL(*mRenderSurface, setProtected(false));
base::unique_fd fd;
@@ -4060,10 +4032,7 @@
// For this test, we also check the call order of key functions.
InSequence seq;
- EXPECT_CALL(mRenderEngine, isProtected).WillOnce(Return(false));
- EXPECT_CALL(mRenderEngine, useProtectedContext(true));
EXPECT_CALL(*mRenderSurface, isProtected).WillOnce(Return(false));
- EXPECT_CALL(mRenderEngine, isProtected).WillOnce(Return(true));
EXPECT_CALL(*mRenderSurface, setProtected(true));
// Must happen after setting the protected content state.
EXPECT_CALL(*mRenderSurface, dequeueBuffer(_)).WillRepeatedly(Return(mOutputBuffer));
@@ -4081,7 +4050,6 @@
mOutput.mState.isSecure = true;
mLayer2.mLayerFEState.hasProtectedContent = true;
EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(true));
- EXPECT_CALL(mRenderEngine, isProtected).WillOnce(Return(true));
EXPECT_CALL(*mRenderSurface, isProtected).WillOnce(Return(true));
base::unique_fd fd;
@@ -4091,43 +4059,11 @@
mOutput.composeSurfaces(kDebugRegion, kDefaultRefreshArgs, tex, fd);
}
-TEST_F(OutputComposeSurfacesTest_HandlesProtectedContent, ifFailsToEnableInRenderEngine) {
- mOutput.mState.isSecure = true;
- mLayer2.mLayerFEState.hasProtectedContent = true;
- EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(true));
- EXPECT_CALL(mRenderEngine, isProtected).WillOnce(Return(false)).WillOnce(Return(false));
- EXPECT_CALL(*mRenderSurface, isProtected).WillOnce(Return(false));
- EXPECT_CALL(mRenderEngine, useProtectedContext(true));
-
- base::unique_fd fd;
- std::shared_ptr<renderengine::ExternalTexture> tex;
- mOutput.updateProtectedContentState();
- mOutput.dequeueRenderBuffer(&fd, &tex);
- mOutput.composeSurfaces(kDebugRegion, kDefaultRefreshArgs, tex, fd);
-}
-
-TEST_F(OutputComposeSurfacesTest_HandlesProtectedContent, ifAlreadyEnabledInRenderEngine) {
- mOutput.mState.isSecure = true;
- mLayer2.mLayerFEState.hasProtectedContent = true;
- EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(true));
- EXPECT_CALL(mRenderEngine, isProtected).WillOnce(Return(true)).WillOnce(Return(true));
- EXPECT_CALL(*mRenderSurface, isProtected).WillOnce(Return(false));
- EXPECT_CALL(*mRenderSurface, setProtected(true));
-
- base::unique_fd fd;
- std::shared_ptr<renderengine::ExternalTexture> tex;
- mOutput.updateProtectedContentState();
- mOutput.dequeueRenderBuffer(&fd, &tex);
- mOutput.composeSurfaces(kDebugRegion, kDefaultRefreshArgs, tex, fd);
-}
-
TEST_F(OutputComposeSurfacesTest_HandlesProtectedContent, ifAlreadyEnabledInRenderSurface) {
mOutput.mState.isSecure = true;
mLayer2.mLayerFEState.hasProtectedContent = true;
EXPECT_CALL(mRenderEngine, supportsProtectedContent()).WillRepeatedly(Return(true));
- EXPECT_CALL(mRenderEngine, isProtected).WillOnce(Return(false));
EXPECT_CALL(*mRenderSurface, isProtected).WillOnce(Return(true));
- EXPECT_CALL(mRenderEngine, useProtectedContext(true));
base::unique_fd fd;
std::shared_ptr<renderengine::ExternalTexture> tex;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 23843ab..68fab24 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -6522,7 +6522,6 @@
// Use an empty fence for the buffer fence, since we just created the buffer so
// there is no need for synchronization with the GPU.
base::unique_fd bufferFence;
- getRenderEngine().useProtectedContext(useProtected);
constexpr bool kUseFramebufferCache = false;
const auto future = getRenderEngine()
@@ -6534,9 +6533,6 @@
layer->onLayerDisplayed(future);
}
- // Always switch back to unprotected context.
- getRenderEngine().useProtectedContext(false);
-
return future;
}