[Impeller] Fix GL depth state. (#51040)

This patch fixes problems with GL depth (in service of #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`.
flutter · Feb 28, 2024 · 6de390b · 6de390b
1 parent 2c949da
commit 6de390b
Show file tree

Hide file tree

Showing 8 changed files with 109 additions and 31 deletions.
diff --git 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
@@ -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<DescriptionGLES>(*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<DescriptionGLES>(*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
@@ -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<decltype(gl##name)> 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
@@ -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
@@ -19,8 +19,12 @@ static std::mutex g_test_lock;
 
 static std::weak_ptr<MockGLES> g_mock_gles;
 
+static ProcTableGLES::Resolver g_resolver;
+
 static std::vector<const unsigned char*> 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<Ret(Args...), Ret(Args...)> : 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<const unsigned char*>{
     (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<decltype(mockDeleteQueriesEXT),  //
                                  decltype(glDeleteQueriesEXT)>::value);
 
 std::shared_ptr<MockGLES> MockGLES::Init(
-    const std::optional<std::vector<const unsigned char*>>& extensions) {
+    const std::optional<std::vector<const unsigned char*>>& 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<MockGLES>(new MockGLES());
+  auto mock_gles = std::shared_ptr<MockGLES>(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<void*>(&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
@@ -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<MockGLES> Init(
       const std::optional<std::vector<const unsigned char*>>& 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<std::string> 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
@@ -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 <optional>
+
+#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
@@ -260,6 +260,7 @@ RenderTarget RenderTarget::CreateOffscreen(
                                         stencil_attachment_config.value());
   } else {
     target.SetStencilAttachment(std::nullopt);
+    target.SetDepthAttachment(std::nullopt);
   }
 
   return target;