From 7c1f5566dcf58b10c681dcfa919f1355fd2f3036 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Thu, 14 Nov 2024 14:28:07 -0800 Subject: [PATCH 1/2] [Impeller] libImpeller: Reset the GL state when transitioning control back to the embedder. Impeller is resilient to OpenGL state being trampled upon when accessing the GL context. But the embedder may not necessarily be. Ideally, we'd be using saving the state and restoring it. But that might be too involved. For now, this sets the GL state is a sane "clean" state. We could, in theory, do this after each render pass but that unnecessarily increases API traffic. For now, I have added it at the transition of the embedder boundary. --- .../renderer/backend/gles/context_gles.cc | 12 ++++++++++ impeller/renderer/backend/gles/context_gles.h | 3 +++ .../renderer/backend/gles/render_pass_gles.cc | 24 +++++++++++-------- .../renderer/backend/gles/render_pass_gles.h | 2 ++ impeller/renderer/context.cc | 8 +++++++ impeller/renderer/context.h | 14 ++++++++--- impeller/toolkit/interop/surface.cc | 6 +++-- 7 files changed, 54 insertions(+), 15 deletions(-) diff --git a/impeller/renderer/backend/gles/context_gles.cc b/impeller/renderer/backend/gles/context_gles.cc index 7629eff28e7cd..c4070a5393cce 100644 --- a/impeller/renderer/backend/gles/context_gles.cc +++ b/impeller/renderer/backend/gles/context_gles.cc @@ -9,6 +9,7 @@ #include "impeller/base/validation.h" #include "impeller/renderer/backend/gles/command_buffer_gles.h" #include "impeller/renderer/backend/gles/gpu_tracer_gles.h" +#include "impeller/renderer/backend/gles/render_pass_gles.h" #include "impeller/renderer/command_queue.h" namespace impeller { @@ -145,4 +146,15 @@ std::shared_ptr ContextGLES::GetCommandQueue() const { return command_queue_; } +// |Context| +void ContextGLES::ResetThreadLocalState() const { + if (!IsValid()) { + return; + } + [[maybe_unused]] auto result = + reactor_->AddOperation([](const ReactorGLES& reactor) { + RenderPassGLES::ResetGLState(reactor.GetProcTable()); + }); +} + } // namespace impeller diff --git a/impeller/renderer/backend/gles/context_gles.h b/impeller/renderer/backend/gles/context_gles.h index 2a0f09aca39b1..d67747133769b 100644 --- a/impeller/renderer/backend/gles/context_gles.h +++ b/impeller/renderer/backend/gles/context_gles.h @@ -93,6 +93,9 @@ class ContextGLES final : public Context, // |Context| void Shutdown() override; + // |Context| + void ResetThreadLocalState() const override; + ContextGLES(const ContextGLES&) = delete; ContextGLES& operator=(const ContextGLES&) = delete; diff --git a/impeller/renderer/backend/gles/render_pass_gles.cc b/impeller/renderer/backend/gles/render_pass_gles.cc index f8c1a25fd997d..5ca2f5d47f8a1 100644 --- a/impeller/renderer/backend/gles/render_pass_gles.cc +++ b/impeller/renderer/backend/gles/render_pass_gles.cc @@ -172,6 +172,19 @@ static bool BindVertexBuffer(const ProcTableGLES& gl, return true; } +void RenderPassGLES::ResetGLState(const ProcTableGLES& gl) { + gl.Disable(GL_SCISSOR_TEST); + gl.Disable(GL_DEPTH_TEST); + gl.Disable(GL_STENCIL_TEST); + gl.Disable(GL_CULL_FACE); + gl.Disable(GL_BLEND); + gl.Disable(GL_DITHER); + gl.ColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); + gl.DepthMask(GL_TRUE); + gl.StencilMaskSeparate(GL_FRONT, 0xFFFFFFFF); + gl.StencilMaskSeparate(GL_BACK, 0xFFFFFFFF); +} + [[nodiscard]] bool EncodeCommandsInReactor( const RenderPassData& pass_data, const std::shared_ptr& transients_allocator, @@ -267,16 +280,7 @@ static bool BindVertexBuffer(const ProcTableGLES& gl, clear_bits |= GL_STENCIL_BUFFER_BIT; } - gl.Disable(GL_SCISSOR_TEST); - gl.Disable(GL_DEPTH_TEST); - gl.Disable(GL_STENCIL_TEST); - gl.Disable(GL_CULL_FACE); - gl.Disable(GL_BLEND); - gl.Disable(GL_DITHER); - gl.ColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); - gl.DepthMask(GL_TRUE); - gl.StencilMaskSeparate(GL_FRONT, 0xFFFFFFFF); - gl.StencilMaskSeparate(GL_BACK, 0xFFFFFFFF); + RenderPassGLES::ResetGLState(gl); gl.Clear(clear_bits); diff --git a/impeller/renderer/backend/gles/render_pass_gles.h b/impeller/renderer/backend/gles/render_pass_gles.h index e753a75eb6922..c0bd9e63e6cef 100644 --- a/impeller/renderer/backend/gles/render_pass_gles.h +++ b/impeller/renderer/backend/gles/render_pass_gles.h @@ -19,6 +19,8 @@ class RenderPassGLES final // |RenderPass| ~RenderPassGLES() override; + static void ResetGLState(const ProcTableGLES& gl); + private: friend class CommandBufferGLES; diff --git a/impeller/renderer/context.cc b/impeller/renderer/context.cc index e23874191a713..e563ac7a2e345 100644 --- a/impeller/renderer/context.cc +++ b/impeller/renderer/context.cc @@ -25,4 +25,12 @@ bool Context::FlushCommandBuffers() { return true; } +std::shared_ptr Context::GetIdleWaiter() const { + return nullptr; +} + +void Context::ResetThreadLocalState() const { + // Nothing to do. +} + } // namespace impeller diff --git a/impeller/renderer/context.h b/impeller/renderer/context.h index 657083c04bf87..2735ac2a67a59 100644 --- a/impeller/renderer/context.h +++ b/impeller/renderer/context.h @@ -222,9 +222,17 @@ class Context { /// rendering a 2D workload. [[nodiscard]] virtual bool FlushCommandBuffers(); - virtual std::shared_ptr GetIdleWaiter() const { - return nullptr; - } + virtual std::shared_ptr GetIdleWaiter() const; + + //---------------------------------------------------------------------------- + /// Resets any thread local state that may interfere with embedders. + /// + /// Today, only the OpenGL backend can trample on thread local state that the + /// embedder can access. This call puts the GL state in a sane "clean" state. + /// + /// Impeller itself is resilient to a dirty thread local state table. + /// + virtual void ResetThreadLocalState() const; protected: Context(); diff --git a/impeller/toolkit/interop/surface.cc b/impeller/toolkit/interop/surface.cc index 11e6def21ef5d..b5e7f7282439f 100644 --- a/impeller/toolkit/interop/surface.cc +++ b/impeller/toolkit/interop/surface.cc @@ -62,8 +62,10 @@ bool Surface::DrawDisplayList(const DisplayList& dl) const { auto skia_cull_rect = SkIRect::MakeWH(cull_rect.GetWidth(), cull_rect.GetHeight()); - return RenderToOnscreen(content_context, render_target, display_list, - skia_cull_rect, /*reset_host_buffer=*/true); + auto result = RenderToOnscreen(content_context, render_target, display_list, + skia_cull_rect, /*reset_host_buffer=*/true); + context_->GetContext()->ResetThreadLocalState(); + return result; } } // namespace impeller::interop From 07def42b6956cef6970af668bde3d0e9f96a5b61 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Thu, 14 Nov 2024 15:27:51 -0800 Subject: [PATCH 2/2] Test. --- .../toolkit/interop/impeller_unittests.cc | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/impeller/toolkit/interop/impeller_unittests.cc b/impeller/toolkit/interop/impeller_unittests.cc index e45389eb502e6..e2c90dd35f3f4 100644 --- a/impeller/toolkit/interop/impeller_unittests.cc +++ b/impeller/toolkit/interop/impeller_unittests.cc @@ -192,6 +192,44 @@ TEST_P(InteropPlaygroundTest, CanCreateOpenGLImage) { })); } +TEST_P(InteropPlaygroundTest, ClearsOpenGLStancilStateAfterTransition) { + auto context = GetInteropContext(); + auto impeller_context = context->GetContext(); + if (impeller_context->GetBackendType() != + impeller::Context::BackendType::kOpenGLES) { + GTEST_SKIP() << "This test works with OpenGL handles is only suitable for " + "that backend."; + return; + } + const auto& gl_context = ContextGLES::Cast(*impeller_context); + const auto& gl = gl_context.GetReactor()->GetProcTable(); + auto builder = + Adopt(ImpellerDisplayListBuilderNew(nullptr)); + auto paint = Adopt(ImpellerPaintNew()); + ImpellerColor color = {0.0, 0.0, 1.0, 1.0}; + ImpellerPaintSetColor(paint.GetC(), &color); + ImpellerRect rect = {10, 20, 100, 200}; + ImpellerDisplayListBuilderDrawRect(builder.GetC(), &rect, paint.GetC()); + color = {1.0, 0.0, 0.0, 1.0}; + ImpellerPaintSetColor(paint.GetC(), &color); + ImpellerDisplayListBuilderTranslate(builder.GetC(), 110, 210); + ImpellerDisplayListBuilderClipRect(builder.GetC(), &rect, + kImpellerClipOperationDifference); + ImpellerDisplayListBuilderDrawRect(builder.GetC(), &rect, paint.GetC()); + auto dl = Adopt( + ImpellerDisplayListBuilderCreateDisplayListNew(builder.GetC())); + ASSERT_TRUE(dl); + ASSERT_TRUE( + OpenPlaygroundHere([&](const auto& context, const auto& surface) -> bool { + ImpellerSurfaceDrawDisplayList(surface.GetC(), dl.GetC()); + // OpenGL state is reset even though the operations above enable a + // stencil check. + GLboolean stencil_enabled = true; + gl.GetBooleanv(GL_STENCIL_TEST, &stencil_enabled); + return stencil_enabled == GL_FALSE; + })); +} + TEST_P(InteropPlaygroundTest, CanCreateParagraphs) { // Create a typography context. auto type_context = Adopt(ImpellerTypographyContextNew());