From 6de390b300bcd0f8d6211aa3f103129dd1984cbe Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Wed, 28 Feb 2024 10:41:35 -0800 Subject: [PATCH] [Impeller] Fix GL depth state. (#51040) This patch fixes problems with GL depth (in service of https://github.com/flutter/engine/pull/50856): * The depth + stencil masks weren't being set prior to clearing. * When the driver identifies as desktop GL rather than ES, use `glClearDepth` and `glDepthRange` rather than `glClearDepthf` and `glDepthRangef`. --- impeller/renderer/backend/gles/BUILD.gn | 1 + .../renderer/backend/gles/proc_table_gles.cc | 24 ++++++++---- .../renderer/backend/gles/proc_table_gles.h | 20 +++++++++- .../renderer/backend/gles/render_pass_gles.cc | 25 +++++++------ .../renderer/backend/gles/test/mock_gles.cc | 22 +++++++---- .../renderer/backend/gles/test/mock_gles.h | 10 +++-- .../gles/test/proc_table_gles_unittests.cc | 37 +++++++++++++++++++ impeller/renderer/render_target.cc | 1 + 8 files changed, 109 insertions(+), 31 deletions(-) create mode 100644 impeller/renderer/backend/gles/test/proc_table_gles_unittests.cc diff --git a/impeller/renderer/backend/gles/BUILD.gn b/impeller/renderer/backend/gles/BUILD.gn index edace2b7aab40..6126927d44702 100644 --- a/impeller/renderer/backend/gles/BUILD.gn +++ b/impeller/renderer/backend/gles/BUILD.gn @@ -20,6 +20,7 @@ impeller_component("gles_unittests") { "test/mock_gles.cc", "test/mock_gles.h", "test/mock_gles_unittests.cc", + "test/proc_table_gles_unittests.cc", "test/specialization_constants_unittests.cc", ] deps = [ diff --git a/impeller/renderer/backend/gles/proc_table_gles.cc b/impeller/renderer/backend/gles/proc_table_gles.cc index 0ccb974ebd32b..8aeda6d733a1a 100644 --- a/impeller/renderer/backend/gles/proc_table_gles.cc +++ b/impeller/renderer/backend/gles/proc_table_gles.cc @@ -71,7 +71,11 @@ ProcTableGLES::Resolver WrappedResolver( }; } -ProcTableGLES::ProcTableGLES(Resolver resolver) { +ProcTableGLES::ProcTableGLES( // NOLINT(google-readability-function-size) + Resolver resolver) { + // The reason this constructor has anywhere near enough code to tip off + // `google-readability-function-size` is the proc macros, so ignore the lint. + if (!resolver) { return; } @@ -96,6 +100,18 @@ ProcTableGLES::ProcTableGLES(Resolver resolver) { FOR_EACH_IMPELLER_PROC(IMPELLER_PROC); + description_ = std::make_unique(*this); + + if (!description_->IsValid()) { + return; + } + + if (description_->IsES()) { + FOR_EACH_IMPELLER_ES_ONLY_PROC(IMPELLER_PROC); + } else { + FOR_EACH_IMPELLER_DESKTOP_ONLY_PROC(IMPELLER_PROC); + } + #undef IMPELLER_PROC #define IMPELLER_PROC(proc_ivar) \ @@ -109,12 +125,6 @@ ProcTableGLES::ProcTableGLES(Resolver resolver) { #undef IMPELLER_PROC - description_ = std::make_unique(*this); - - if (!description_->IsValid()) { - return; - } - if (!description_->HasDebugExtension()) { PushDebugGroupKHR.Reset(); PopDebugGroupKHR.Reset(); diff --git a/impeller/renderer/backend/gles/proc_table_gles.h b/impeller/renderer/backend/gles/proc_table_gles.h index b3630f5b9759b..88397c5d68278 100644 --- a/impeller/renderer/backend/gles/proc_table_gles.h +++ b/impeller/renderer/backend/gles/proc_table_gles.h @@ -113,7 +113,6 @@ struct GLProc { PROC(CheckFramebufferStatus); \ PROC(Clear); \ PROC(ClearColor); \ - PROC(ClearDepthf); \ PROC(ClearStencil); \ PROC(ColorMask); \ PROC(CompileShader); \ @@ -128,7 +127,6 @@ struct GLProc { PROC(DeleteTextures); \ PROC(DepthFunc); \ PROC(DepthMask); \ - PROC(DepthRangef); \ PROC(DetachShader); \ PROC(Disable); \ PROC(DisableVertexAttribArray); \ @@ -186,6 +184,22 @@ struct GLProc { PROC(GetShaderSource); \ PROC(ReadPixels); +// Calls specific to OpenGLES. +void(glClearDepthf)(GLfloat depth); +void(glDepthRangef)(GLfloat n, GLfloat f); + +#define FOR_EACH_IMPELLER_ES_ONLY_PROC(PROC) \ + PROC(ClearDepthf); \ + PROC(DepthRangef); + +// Calls specific to desktop GL. +void(glClearDepth)(GLdouble depth); +void(glDepthRange)(GLdouble n, GLdouble f); + +#define FOR_EACH_IMPELLER_DESKTOP_ONLY_PROC(PROC) \ + PROC(ClearDepth); \ + PROC(DepthRange); + #define FOR_EACH_IMPELLER_GLES3_PROC(PROC) PROC(BlitFramebuffer); #define FOR_EACH_IMPELLER_EXT_PROC(PROC) \ @@ -224,6 +238,8 @@ class ProcTableGLES { GLProc name = {"gl" #name, nullptr}; FOR_EACH_IMPELLER_PROC(IMPELLER_PROC); + FOR_EACH_IMPELLER_ES_ONLY_PROC(IMPELLER_PROC); + FOR_EACH_IMPELLER_DESKTOP_ONLY_PROC(IMPELLER_PROC); FOR_EACH_IMPELLER_GLES3_PROC(IMPELLER_PROC); FOR_EACH_IMPELLER_EXT_PROC(IMPELLER_PROC); diff --git a/impeller/renderer/backend/gles/render_pass_gles.cc b/impeller/renderer/backend/gles/render_pass_gles.cc index b3d61f9b255a8..12586b917fe47 100644 --- a/impeller/renderer/backend/gles/render_pass_gles.cc +++ b/impeller/renderer/backend/gles/render_pass_gles.cc @@ -214,12 +214,11 @@ struct RenderPassData { pass_data.clear_color.alpha // alpha ); if (pass_data.depth_attachment) { - // TODO(bdero): Desktop GL for Apple requires glClearDepth. glClearDepthf - // throws GL_INVALID_OPERATION. - // https://github.com/flutter/flutter/issues/136322 -#if !FML_OS_MACOSX - gl.ClearDepthf(pass_data.clear_depth); -#endif + if (gl.DepthRangef.IsAvailable()) { + gl.ClearDepthf(pass_data.clear_depth); + } else { + gl.ClearDepth(pass_data.clear_depth); + } } if (pass_data.stencil_attachment) { gl.ClearStencil(pass_data.clear_stencil); @@ -242,6 +241,9 @@ struct RenderPassData { gl.Disable(GL_CULL_FACE); gl.Disable(GL_BLEND); gl.ColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); + gl.DepthMask(GL_TRUE); + gl.StencilMaskSeparate(GL_FRONT, 0xFFFFFFFF); + gl.StencilMaskSeparate(GL_BACK, 0xFFFFFFFF); gl.Clear(clear_bits); @@ -315,12 +317,11 @@ struct RenderPassData { viewport.rect.GetHeight() // height ); if (pass_data.depth_attachment) { - // TODO(bdero): Desktop GL for Apple requires glDepthRange. glDepthRangef - // throws GL_INVALID_OPERATION. - // https://github.com/flutter/flutter/issues/136322 -#if !FML_OS_MACOSX - gl.DepthRangef(viewport.depth_range.z_near, viewport.depth_range.z_far); -#endif + if (gl.DepthRangef.IsAvailable()) { + gl.DepthRangef(viewport.depth_range.z_near, viewport.depth_range.z_far); + } else { + gl.DepthRange(viewport.depth_range.z_near, viewport.depth_range.z_far); + } } //-------------------------------------------------------------------------- diff --git a/impeller/renderer/backend/gles/test/mock_gles.cc b/impeller/renderer/backend/gles/test/mock_gles.cc index 30ea1ae3eefc7..d000dad400947 100644 --- a/impeller/renderer/backend/gles/test/mock_gles.cc +++ b/impeller/renderer/backend/gles/test/mock_gles.cc @@ -19,8 +19,12 @@ static std::mutex g_test_lock; static std::weak_ptr g_mock_gles; +static ProcTableGLES::Resolver g_resolver; + static std::vector g_extensions; +static const unsigned char* g_version; + // Has friend visibility into MockGLES to record calls. void RecordGLCall(const char* name) { if (auto mock_gles = g_mock_gles.lock()) { @@ -38,7 +42,7 @@ struct CheckSameSignature : std::true_type {}; void doNothing() {} auto const kMockVendor = (unsigned char*)"MockGLES"; -auto const kMockVersion = (unsigned char*)"3.0"; +const auto kMockShadingLanguageVersion = (unsigned char*)"GLSL ES 1.0"; auto const kExtensions = std::vector{ (unsigned char*)"GL_KHR_debug" // }; @@ -48,9 +52,9 @@ const unsigned char* mockGetString(GLenum name) { case GL_VENDOR: return kMockVendor; case GL_VERSION: - return kMockVersion; + return g_version; case GL_SHADING_LANGUAGE_VERSION: - return kMockVersion; + return kMockShadingLanguageVersion; default: return (unsigned char*)""; } @@ -160,17 +164,20 @@ static_assert(CheckSameSignature::value); std::shared_ptr MockGLES::Init( - const std::optional>& extensions) { + const std::optional>& extensions, + const char* version_string, + ProcTableGLES::Resolver resolver) { // If we cannot obtain a lock, MockGLES is already being used elsewhere. FML_CHECK(g_test_lock.try_lock()) << "MockGLES is already being used by another test."; + g_version = (unsigned char*)version_string; g_extensions = extensions.value_or(kExtensions); - auto mock_gles = std::shared_ptr(new MockGLES()); + auto mock_gles = std::shared_ptr(new MockGLES(std::move(resolver))); g_mock_gles = mock_gles; return mock_gles; } -const ProcTableGLES::Resolver kMockResolver = [](const char* name) { +const ProcTableGLES::Resolver kMockResolverGLES = [](const char* name) { if (strcmp(name, "glPopDebugGroupKHR") == 0) { return reinterpret_cast(&mockPopDebugGroupKHR); } else if (strcmp(name, "glPushDebugGroupKHR") == 0) { @@ -200,7 +207,8 @@ const ProcTableGLES::Resolver kMockResolver = [](const char* name) { } }; -MockGLES::MockGLES() : proc_table_(kMockResolver) {} +MockGLES::MockGLES(ProcTableGLES::Resolver resolver) + : proc_table_(std::move(resolver)) {} MockGLES::~MockGLES() { g_test_lock.unlock(); diff --git a/impeller/renderer/backend/gles/test/mock_gles.h b/impeller/renderer/backend/gles/test/mock_gles.h index d498e959145d2..4fcf387604e50 100644 --- a/impeller/renderer/backend/gles/test/mock_gles.h +++ b/impeller/renderer/backend/gles/test/mock_gles.h @@ -13,6 +13,8 @@ namespace impeller { namespace testing { +extern const ProcTableGLES::Resolver kMockResolverGLES; + /// @brief Provides a mocked version of the |ProcTableGLES| class. /// /// Typically, Open GLES at runtime will be provided the host's GLES bindings @@ -30,7 +32,9 @@ class MockGLES final { /// called once per test. static std::shared_ptr Init( const std::optional>& extensions = - std::nullopt); + std::nullopt, + const char* version_string = "OpenGL ES 3.0", + ProcTableGLES::Resolver resolver = kMockResolverGLES); /// @brief Returns a configured |ProcTableGLES| instance. const ProcTableGLES& GetProcTable() const { return proc_table_; } @@ -49,11 +53,11 @@ class MockGLES final { private: friend void RecordGLCall(const char* name); - MockGLES(); + explicit MockGLES(ProcTableGLES::Resolver resolver = kMockResolverGLES); void RecordCall(const char* name) { captured_calls_.emplace_back(name); } - const ProcTableGLES proc_table_; + ProcTableGLES proc_table_; std::vector captured_calls_; MockGLES(const MockGLES&) = delete; diff --git a/impeller/renderer/backend/gles/test/proc_table_gles_unittests.cc b/impeller/renderer/backend/gles/test/proc_table_gles_unittests.cc new file mode 100644 index 0000000000000..b7f188f950299 --- /dev/null +++ b/impeller/renderer/backend/gles/test/proc_table_gles_unittests.cc @@ -0,0 +1,37 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#include "flutter/testing/testing.h" // IWYU pragma: keep +#include "gtest/gtest.h" +#include "impeller/renderer/backend/gles/proc_table_gles.h" +#include "impeller/renderer/backend/gles/test/mock_gles.h" + +namespace impeller { +namespace testing { + +#define EXPECT_AVAILABLE(proc_ivar) \ + EXPECT_TRUE(mock_gles->GetProcTable().proc_ivar.IsAvailable()); +#define EXPECT_UNAVAILABLE(proc_ivar) \ + EXPECT_FALSE(mock_gles->GetProcTable().proc_ivar.IsAvailable()); + +TEST(ProcTableGLES, ResolvesCorrectClearDepthProcOnES) { + auto mock_gles = MockGLES::Init(std::nullopt, "OpenGL ES 3.0"); + EXPECT_TRUE(mock_gles->GetProcTable().GetDescription()->IsES()); + + FOR_EACH_IMPELLER_ES_ONLY_PROC(EXPECT_AVAILABLE); + FOR_EACH_IMPELLER_DESKTOP_ONLY_PROC(EXPECT_UNAVAILABLE); +} + +TEST(ProcTableGLES, ResolvesCorrectClearDepthProcOnDesktopGL) { + auto mock_gles = MockGLES::Init(std::nullopt, "OpenGL 4.0"); + EXPECT_FALSE(mock_gles->GetProcTable().GetDescription()->IsES()); + + FOR_EACH_IMPELLER_DESKTOP_ONLY_PROC(EXPECT_AVAILABLE); + FOR_EACH_IMPELLER_ES_ONLY_PROC(EXPECT_UNAVAILABLE); +} + +} // namespace testing +} // namespace impeller diff --git a/impeller/renderer/render_target.cc b/impeller/renderer/render_target.cc index 59ffc41b029bc..28940bad27efb 100644 --- a/impeller/renderer/render_target.cc +++ b/impeller/renderer/render_target.cc @@ -260,6 +260,7 @@ RenderTarget RenderTarget::CreateOffscreen( stencil_attachment_config.value()); } else { target.SetStencilAttachment(std::nullopt); + target.SetDepthAttachment(std::nullopt); } return target;