[Windows] Make the view own its EGL surface (#50421)

This makes the view own its EGL surface. This will allow us to support multiple EGL surfaces once the engine supports having multiple views.

Some notable changes:

1. EGL surface resizing logic is now entirely in `FlutterWindowsView`. Previously some resizing logic was in the `egl::Manager`, however, the view has to handle resizing failures so this unifies the logic in one place.
2. The `OnEmptyFrameGenerated` and `OnFrameGenerated` now return `false` (aka "don't present") if the surface is invalid. This simplifies the compositor as it no longer needs to check for invalid surfaces
3. This introduces a `ViewModifier` testing helper to allow overriding a view's surface. This isn't strictly necessary, tests can setup a surface by mocking several EGL methods and calling `FlutterWindowsView::CreateRenderSurface()`. However, this is verbose & heavily tied to implementation details. The `ViewModifier` avoids this boilerplate.
4. `CompositorOpenGL`'s initialization now makes the render context current without any render surfaces. Previously it also made the view's surface current, which was unnecessary.

Part of flutter/flutter#137267

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Author: loic-sharma

shell/platform/windows/BUILD.gn

@@ -230,6 +230,7 @@ executable("flutter_windows_unittests") {
     "testing/test_keyboard.cc",
     "testing/test_keyboard.h",
     "testing/test_keyboard_unittests.cc",
+    "testing/view_modifier.h",
     "testing/windows_test.cc",
     "testing/windows_test.h",
     "testing/windows_test_config_builder.cc",

shell/platform/windows/compositor_opengl.cc

@@ -93,12 +93,8 @@ bool CompositorOpenGL::CollectBackingStore(const FlutterBackingStore* store) {
 
 bool CompositorOpenGL::Present(const FlutterLayer** layers,
                                size_t layers_count) {
-  if (!engine_->view()) {
-    return false;
-  }
-
-  if (!engine_->egl_manager()->surface() ||
-      !engine_->egl_manager()->surface()->IsValid()) {
+  FlutterWindowsView* view = engine_->view();
+  if (!view) {
     return false;
   }
 
@@ -111,7 +107,7 @@ bool CompositorOpenGL::Present(const FlutterLayer** layers,
       return false;
     }
 
-    return ClearSurface();
+    return Clear(view);
   }
 
   // TODO: Support compositing layers and platform views.
@@ -129,11 +125,16 @@ bool CompositorOpenGL::Present(const FlutterLayer** layers,
 
   // Check if this frame can be presented. This resizes the surface if a resize
   // is pending and |width| and |height| match the target size.
-  if (!engine_->view()->OnFrameGenerated(width, height)) {
+  if (!view->OnFrameGenerated(width, height)) {
     return false;
   }
 
-  if (!engine_->egl_manager()->surface()->MakeCurrent()) {
+  // |OnFrameGenerated| should return false if the surface isn't valid.
+  FML_DCHECK(view->surface() != nullptr);
+  FML_DCHECK(view->surface()->IsValid());
+
+  egl::WindowSurface* surface = view->surface();
+  if (!surface->MakeCurrent()) {
     return false;
   }
 
@@ -159,11 +160,11 @@ bool CompositorOpenGL::Present(const FlutterLayer** layers,
                        GL_NEAREST            // filter
   );
 
-  if (!engine_->egl_manager()->surface()->SwapBuffers()) {
+  if (!surface->SwapBuffers()) {
     return false;
   }
 
-  engine_->view()->OnFramePresented();
+  view->OnFramePresented();
   return true;
 }
 
@@ -175,12 +176,7 @@ bool CompositorOpenGL::Initialize() {
     return false;
   }
 
-  egl::Surface* surface = manager->surface();
-  if (!surface || !surface->IsValid()) {
-    return false;
-  }
-
-  if (!surface->MakeCurrent()) {
+  if (!manager->render_context()->MakeCurrent()) {
     return false;
   }
 
@@ -195,24 +191,31 @@ bool CompositorOpenGL::Initialize() {
   return true;
 }
 
-bool CompositorOpenGL::ClearSurface() {
+bool CompositorOpenGL::Clear(FlutterWindowsView* view) {
   FML_DCHECK(is_initialized_);
 
-  // Resize the surface if needed.
-  engine_->view()->OnEmptyFrameGenerated();
+  // Check if this frame can be presented. This resizes the surface if needed.
+  if (!view->OnEmptyFrameGenerated()) {
+    return false;
+  }
+
+  // |OnEmptyFrameGenerated| should return false if the surface isn't valid.
+  FML_DCHECK(view->surface() != nullptr);
+  FML_DCHECK(view->surface()->IsValid());
 
-  if (!engine_->egl_manager()->surface()->MakeCurrent()) {
+  egl::WindowSurface* surface = view->surface();
+  if (!surface->MakeCurrent()) {
     return false;
   }
 
   gl_->ClearColor(0.0f, 0.0f, 0.0f, 0.0f);
   gl_->Clear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT);
 
-  if (!engine_->egl_manager()->surface()->SwapBuffers()) {
+  if (!surface->SwapBuffers()) {
     return false;
   }
 
-  engine_->view()->OnFramePresented();
+  view->OnFramePresented();
   return true;
 }
 

shell/platform/windows/compositor_opengl.h

@@ -53,7 +53,7 @@ class CompositorOpenGL : public Compositor {
   bool Initialize();
 
   // Clear the view's surface and removes any previously presented layers.
-  bool ClearSurface();
+  bool Clear(FlutterWindowsView* view);
 };
 
 }  // namespace flutter

shell/platform/windows/compositor_opengl_unittests.cc

@@ -9,11 +9,13 @@
 #include "flutter/shell/platform/windows/compositor_opengl.h"
 #include "flutter/shell/platform/windows/egl/manager.h"
 #include "flutter/shell/platform/windows/flutter_windows_view.h"
+#include "flutter/shell/platform/windows/testing/egl/mock_context.h"
 #include "flutter/shell/platform/windows/testing/egl/mock_manager.h"
 #include "flutter/shell/platform/windows/testing/egl/mock_window_surface.h"
 #include "flutter/shell/platform/windows/testing/engine_modifier.h"
 #include "flutter/shell/platform/windows/testing/flutter_windows_engine_builder.h"
 #include "flutter/shell/platform/windows/testing/mock_window_binding_handler.h"
+#include "flutter/shell/platform/windows/testing/view_modifier.h"
 #include "flutter/shell/platform/windows/testing/windows_test.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -22,6 +24,7 @@ namespace flutter {
 namespace testing {
 
 namespace {
+using ::testing::AnyNumber;
 using ::testing::Return;
 
 const unsigned char* MockGetString(GLenum name) {
@@ -66,19 +69,17 @@ class CompositorOpenGLTest : public WindowsTest {
  protected:
   FlutterWindowsEngine* engine() { return engine_.get(); }
   egl::MockManager* egl_manager() { return egl_manager_; }
-  egl::MockWindowSurface* surface() { return surface_.get(); }
+  egl::MockContext* render_context() { return render_context_.get(); }
+  egl::MockWindowSurface* surface() { return surface_; }
 
-  void UseHeadlessEngine(bool add_surface = true) {
+  void UseHeadlessEngine() {
     auto egl_manager = std::make_unique<egl::MockManager>();
+    render_context_ = std::make_unique<egl::MockContext>();
     egl_manager_ = egl_manager.get();
 
-    if (add_surface) {
-      surface_ = std::make_unique<egl::MockWindowSurface>();
-      EXPECT_CALL(*egl_manager_, surface)
-          .WillRepeatedly(Return(surface_.get()));
-    } else {
-      EXPECT_CALL(*egl_manager_, surface).WillRepeatedly(Return(nullptr));
-    }
+    EXPECT_CALL(*egl_manager_, render_context)
+        .Times(AnyNumber())
+        .WillRepeatedly(Return(render_context_.get()));
 
     FlutterWindowsEngineBuilder builder{GetContext()};
 
@@ -88,21 +89,32 @@ class CompositorOpenGLTest : public WindowsTest {
   }
 
   void UseEngineWithView(bool add_surface = true) {
-    UseHeadlessEngine(add_surface);
+    UseHeadlessEngine();
 
     auto window = std::make_unique<MockWindowBindingHandler>();
     EXPECT_CALL(*window.get(), SetView).Times(1);
     EXPECT_CALL(*window.get(), GetWindowHandle).WillRepeatedly(Return(nullptr));
 
     view_ = std::make_unique<FlutterWindowsView>(std::move(window));
 
+    if (add_surface) {
+      auto surface = std::make_unique<egl::MockWindowSurface>();
+      surface_ = surface.get();
+
+      EXPECT_CALL(*surface_, Destroy).Times(AnyNumber());
+
+      ViewModifier modifier{view_.get()};
+      modifier.SetSurface(std::move(surface));
+    }
+
     engine_->SetView(view_.get());
   }
 
  private:
   std::unique_ptr<FlutterWindowsEngine> engine_;
   std::unique_ptr<FlutterWindowsView> view_;
-  std::unique_ptr<egl::MockWindowSurface> surface_;
+  std::unique_ptr<egl::MockContext> render_context_;
+  egl::MockWindowSurface* surface_;
   egl::MockManager* egl_manager_;
 
   FML_DISALLOW_COPY_AND_ASSIGN(CompositorOpenGLTest);
@@ -118,8 +130,7 @@ TEST_F(CompositorOpenGLTest, CreateBackingStore) {
   FlutterBackingStoreConfig config = {};
   FlutterBackingStore backing_store = {};
 
-  EXPECT_CALL(*surface(), IsValid).WillOnce(Return(true));
-  EXPECT_CALL(*surface(), MakeCurrent).WillOnce(Return(true));
+  EXPECT_CALL(*render_context(), MakeCurrent).WillOnce(Return(true));
   ASSERT_TRUE(compositor.CreateBackingStore(config, &backing_store));
   ASSERT_TRUE(compositor.CollectBackingStore(&backing_store));
 }
@@ -132,8 +143,7 @@ TEST_F(CompositorOpenGLTest, InitializationFailure) {
   FlutterBackingStoreConfig config = {};
   FlutterBackingStore backing_store = {};
 
-  EXPECT_CALL(*surface(), IsValid).WillOnce(Return(true));
-  EXPECT_CALL(*surface(), MakeCurrent).WillOnce(Return(false));
+  EXPECT_CALL(*render_context(), MakeCurrent).WillOnce(Return(false));
   EXPECT_FALSE(compositor.CreateBackingStore(config, &backing_store));
 }
 
@@ -145,15 +155,15 @@ TEST_F(CompositorOpenGLTest, Present) {
   FlutterBackingStoreConfig config = {};
   FlutterBackingStore backing_store = {};
 
-  EXPECT_CALL(*surface(), IsValid).WillRepeatedly(Return(true));
-  EXPECT_CALL(*surface(), MakeCurrent).WillOnce(Return(true));
+  EXPECT_CALL(*render_context(), MakeCurrent).WillOnce(Return(true));
   ASSERT_TRUE(compositor.CreateBackingStore(config, &backing_store));
 
   FlutterLayer layer = {};
   layer.type = kFlutterLayerContentTypeBackingStore;
   layer.backing_store = &backing_store;
   const FlutterLayer* layer_ptr = &layer;
 
+  EXPECT_CALL(*surface(), IsValid).WillRepeatedly(Return(true));
   EXPECT_CALL(*surface(), MakeCurrent).WillOnce(Return(true));
   EXPECT_CALL(*surface(), SwapBuffers).WillOnce(Return(true));
   EXPECT_TRUE(compositor.Present(&layer_ptr, 1));
@@ -168,8 +178,9 @@ TEST_F(CompositorOpenGLTest, PresentEmpty) {
 
   // The context will be bound twice: first to initialize the compositor, second
   // to clear the surface.
+  EXPECT_CALL(*render_context(), MakeCurrent).WillOnce(Return(true));
   EXPECT_CALL(*surface(), IsValid).WillRepeatedly(Return(true));
-  EXPECT_CALL(*surface(), MakeCurrent).Times(2).WillRepeatedly(Return(true));
+  EXPECT_CALL(*surface(), MakeCurrent).WillOnce(Return(true));
   EXPECT_CALL(*surface(), SwapBuffers).WillOnce(Return(true));
   EXPECT_TRUE(compositor.Present(nullptr, 0));
 }
@@ -182,8 +193,7 @@ TEST_F(CompositorOpenGLTest, HeadlessPresentIgnored) {
   FlutterBackingStoreConfig config = {};
   FlutterBackingStore backing_store = {};
 
-  EXPECT_CALL(*surface(), IsValid).WillOnce(Return(true));
-  EXPECT_CALL(*surface(), MakeCurrent).WillOnce(Return(true));
+  EXPECT_CALL(*render_context(), MakeCurrent).WillOnce(Return(true));
   ASSERT_TRUE(compositor.CreateBackingStore(config, &backing_store));
 
   FlutterLayer layer = {};
@@ -204,11 +214,12 @@ TEST_F(CompositorOpenGLTest, NoSurfaceIgnored) {
   FlutterBackingStoreConfig config = {};
   FlutterBackingStore backing_store = {};
 
-  ASSERT_FALSE(compositor.CreateBackingStore(config, &backing_store));
+  EXPECT_CALL(*render_context(), MakeCurrent).WillOnce(Return(true));
+  ASSERT_TRUE(compositor.CreateBackingStore(config, &backing_store));
 
   FlutterLayer layer = {};
   layer.type = kFlutterLayerContentTypeBackingStore;
-  layer.backing_store = nullptr;
+  layer.backing_store = &backing_store;
   const FlutterLayer* layer_ptr = &layer;
 
   EXPECT_FALSE(compositor.Present(&layer_ptr, 1));

shell/platform/windows/egl/manager.cc

@@ -263,11 +263,11 @@ bool Manager::IsValid() const {
   return is_valid_;
 }
 
-bool Manager::CreateWindowSurface(HWND hwnd, size_t width, size_t height) {
-  FML_DCHECK(surface_ == nullptr || !surface_->IsValid());
-
+std::unique_ptr<WindowSurface> Manager::CreateWindowSurface(HWND hwnd,
+                                                            size_t width,
+                                                            size_t height) {
   if (!hwnd || !is_valid_) {
-    return false;
+    return nullptr;
   }
 
   // Disable ANGLE's automatic surface resizing and provide an explicit size.
@@ -286,43 +286,11 @@ bool Manager::CreateWindowSurface(HWND hwnd, size_t width, size_t height) {
       surface_attributes);
   if (surface == EGL_NO_SURFACE) {
     LogEGLError("Surface creation failed.");
-    return false;
+    return nullptr;
   }
 
-  surface_ = std::make_unique<WindowSurface>(
-      display_, render_context_->GetHandle(), surface, width, height);
-  return true;
-}
-
-void Manager::ResizeWindowSurface(HWND hwnd, size_t width, size_t height) {
-  FML_CHECK(surface_ != nullptr);
-
-  auto const existing_width = surface_->width();
-  auto const existing_height = surface_->height();
-  auto const existing_vsync = surface_->vsync_enabled();
-
-  if (width != existing_width || height != existing_height) {
-    // TODO: Destroying the surface and re-creating it is expensive.
-    // Ideally this would use ANGLE's automatic surface sizing instead.
-    // See: https://github.com/flutter/flutter/issues/79427
-    if (!surface_->Destroy()) {
-      FML_LOG(ERROR) << "Manager::ResizeSurface failed to destroy surface";
-      return;
-    }
-
-    if (!CreateWindowSurface(hwnd, width, height)) {
-      FML_LOG(ERROR) << "Manager::ResizeSurface failed to create surface";
-      return;
-    }
-
-    if (!surface_->MakeCurrent() ||
-        !surface_->SetVSyncEnabled(existing_vsync)) {
-      // Surfaces block until the v-blank by default.
-      // Failing to update the vsync might result in unnecessary blocking.
-      // This regresses performance but not correctness.
-      FML_LOG(ERROR) << "Manager::ResizeSurface failed to set vsync";
-    }
-  }
+  return std::make_unique<WindowSurface>(display_, render_context_->GetHandle(),
+                                         surface, width, height);
 }
 
 bool Manager::HasContextCurrent() {
@@ -355,9 +323,5 @@ Context* Manager::resource_context() const {
   return resource_context_.get();
 }
 
-WindowSurface* Manager::surface() const {
-  return surface_.get();
-}
-
 }  // namespace egl
 }  // namespace flutter