Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Windows] Refactor logic when window resize completes #49872

Merged
merged 2 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shell/platform/windows/angle_surface_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ bool AngleSurfaceManager::MakeResourceCurrent() {
egl_resource_context_) == EGL_TRUE);
}

EGLBoolean AngleSurfaceManager::SwapBuffers() {
bool AngleSurfaceManager::SwapBuffers() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anytime we can avoid leaking implementation details (including types) from behind interfaces, it's a nice plus. Users of our surface managers shouldn't need to know about the underlying usage of GL types.

return (eglSwapBuffers(egl_display_, render_surface_));
}

Expand Down
2 changes: 1 addition & 1 deletion shell/platform/windows/angle_surface_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class AngleSurfaceManager {

// Swaps the front and back buffers of the DX11 swapchain backing surface if
// not null.
EGLBoolean SwapBuffers();
virtual bool SwapBuffers();

// Creates a |EGLSurface| from the provided handle.
EGLSurface CreateSurfaceFromHandle(EGLenum handle_type,
Expand Down
14 changes: 12 additions & 2 deletions shell/platform/windows/compositor_opengl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,12 @@ bool CompositorOpenGL::Present(const FlutterLayer** layers,
GL_NEAREST // filter
);

return engine_->view()->SwapBuffers();
if (!engine_->surface_manager()->SwapBuffers()) {
return false;
}

engine_->view()->OnFramePresented();
return true;
Comment on lines +157 to +162
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be refactored into a separate function? It looks like this and the below addition to this file are identical.

Copy link
Member Author

@loic-sharma loic-sharma Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that they're identical, but I think the logic is short enough that it's okay to repeat. I don't feel super strongly about this though, let me know if you do and I'd be happy to update this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought. I don't think it's too important either way.

}

bool CompositorOpenGL::Initialize() {
Expand Down Expand Up @@ -188,7 +193,12 @@ bool CompositorOpenGL::ClearSurface() {
gl_->ClearColor(0.0f, 0.0f, 0.0f, 0.0f);
gl_->Clear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT);

return engine_->view()->SwapBuffers();
if (!engine_->surface_manager()->SwapBuffers()) {
return false;
}

engine_->view()->OnFramePresented();
return true;
}

} // namespace flutter
32 changes: 5 additions & 27 deletions shell/platform/windows/compositor_opengl_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "flutter/shell/platform/windows/flutter_windows_view.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_angle_surface_manager.h"
#include "flutter/shell/platform/windows/testing/mock_window_binding_handler.h"
#include "flutter/shell/platform/windows/testing/windows_test.h"
#include "gmock/gmock.h"
Expand Down Expand Up @@ -56,36 +57,13 @@ const impeller::ProcTableGLES::Resolver kMockResolver = [](const char* name) {
}
};

class MockAngleSurfaceManager : public AngleSurfaceManager {
public:
MockAngleSurfaceManager() : AngleSurfaceManager(false) {}

MOCK_METHOD(bool, MakeCurrent, (), (override));

private:
FML_DISALLOW_COPY_AND_ASSIGN(MockAngleSurfaceManager);
};

class MockFlutterWindowsView : public FlutterWindowsView {
public:
MockFlutterWindowsView(std::unique_ptr<WindowBindingHandler> window)
: FlutterWindowsView(std::move(window)) {}
virtual ~MockFlutterWindowsView() = default;

MOCK_METHOD(bool, SwapBuffers, (), (override));

private:
FML_DISALLOW_COPY_AND_ASSIGN(MockFlutterWindowsView);
};

class CompositorOpenGLTest : public WindowsTest {
public:
CompositorOpenGLTest() = default;
virtual ~CompositorOpenGLTest() = default;

protected:
FlutterWindowsEngine* engine() { return engine_.get(); }
MockFlutterWindowsView* view() { return view_.get(); }
MockAngleSurfaceManager* surface_manager() { return surface_manager_; }

void UseHeadlessEngine() {
Expand All @@ -106,14 +84,14 @@ class CompositorOpenGLTest : public WindowsTest {
EXPECT_CALL(*window.get(), SetView).Times(1);
EXPECT_CALL(*window.get(), GetWindowHandle).WillRepeatedly(Return(nullptr));

view_ = std::make_unique<MockFlutterWindowsView>(std::move(window));
view_ = std::make_unique<FlutterWindowsView>(std::move(window));

engine_->SetView(view_.get());
}

private:
std::unique_ptr<FlutterWindowsEngine> engine_;
std::unique_ptr<MockFlutterWindowsView> view_;
std::unique_ptr<FlutterWindowsView> view_;
MockAngleSurfaceManager* surface_manager_;

FML_DISALLOW_COPY_AND_ASSIGN(CompositorOpenGLTest);
Expand Down Expand Up @@ -163,7 +141,7 @@ TEST_F(CompositorOpenGLTest, Present) {
const FlutterLayer* layer_ptr = &layer;

EXPECT_CALL(*surface_manager(), MakeCurrent).WillOnce(Return(true));
EXPECT_CALL(*view(), SwapBuffers).WillOnce(Return(true));
EXPECT_CALL(*surface_manager(), SwapBuffers).WillOnce(Return(true));
EXPECT_TRUE(compositor.Present(&layer_ptr, 1));

ASSERT_TRUE(compositor.CollectBackingStore(&backing_store));
Expand All @@ -179,7 +157,7 @@ TEST_F(CompositorOpenGLTest, PresentEmpty) {
EXPECT_CALL(*surface_manager(), MakeCurrent)
.Times(2)
.WillRepeatedly(Return(true));
EXPECT_CALL(*view(), SwapBuffers).WillOnce(Return(true));
EXPECT_CALL(*surface_manager(), SwapBuffers).WillOnce(Return(true));
EXPECT_TRUE(compositor.Present(nullptr, 0));
}

Expand Down
4 changes: 0 additions & 4 deletions shell/platform/windows/flutter_window.cc
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,6 @@ float FlutterWindow::GetDpiScale() {
return static_cast<float>(GetCurrentDPI()) / static_cast<float>(base_dpi);
}

bool FlutterWindow::IsVisible() {
return IsWindowVisible(GetWindowHandle());
}

PhysicalWindowBounds FlutterWindow::GetPhysicalWindowBounds() {
return {GetCurrentWidth(), GetCurrentHeight()};
}
Expand Down
3 changes: 0 additions & 3 deletions shell/platform/windows/flutter_window.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,6 @@ class FlutterWindow : public KeyboardManager::WindowDelegate,
// |FlutterWindowBindingHandler|
virtual float GetDpiScale() override;

// |FlutterWindowBindingHandler|
virtual bool IsVisible() override;

// |FlutterWindowBindingHandler|
virtual PhysicalWindowBounds GetPhysicalWindowBounds() override;

Expand Down
1 change: 0 additions & 1 deletion shell/platform/windows/flutter_window_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ class MockFlutterWindow : public FlutterWindow {
MOCK_METHOD(void, OnSetCursor, (), (override));
MOCK_METHOD(float, GetScrollOffsetMultiplier, (), (override));
MOCK_METHOD(float, GetDpiScale, (), (override));
MOCK_METHOD(bool, IsVisible, (), (override));
MOCK_METHOD(void, UpdateCursorRect, (const Rect&), (override));
MOCK_METHOD(void, OnResetImeComposing, (), (override));
MOCK_METHOD(UINT, Win32DispatchMessage, (UINT, WPARAM, LPARAM), (override));
Expand Down
32 changes: 10 additions & 22 deletions shell/platform/windows/flutter_windows_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -590,42 +590,30 @@ void FlutterWindowsView::SendPointerEventWithData(
}
}

bool FlutterWindowsView::SwapBuffers() {
// Called on an engine-controlled (non-platform) thread.
void FlutterWindowsView::OnFramePresented() {
// Called on the engine's raster thread.
std::unique_lock<std::mutex> lock(resize_mutex_);

switch (resize_status_) {
// SwapBuffer requests during resize are ignored until the frame with the
// right dimensions has been generated. This is marked with
// kFrameGenerated resize status.
case ResizeState::kResizeStarted:
return false;
// The caller must first call |OnFrameGenerated| or
// |OnEmptyFrameGenerated| before calling this method. This status
// indicates the caller did not call these methods or ignored their
// result.
FML_UNREACHABLE();
case ResizeState::kFrameGenerated: {
bool visible = binding_handler_->IsVisible();
bool swap_buffers_result;
// For visible windows swap the buffers while resize handler is waiting.
// For invisible windows unblock the handler first and then swap buffers.
// SwapBuffers waits for vsync and there's no point doing that for
// invisible windows.
if (visible) {
swap_buffers_result = engine_->surface_manager()->SwapBuffers();
}
// A frame was generated for a pending resize.
// Unblock the platform thread.
resize_status_ = ResizeState::kDone;
lock.unlock();
resize_cv_.notify_all();

// Blocking the raster thread until DWM flushes alleviates glitches where
// previous size surface is stretched over current size view.
windows_proc_table_->DwmFlush();

if (!visible) {
swap_buffers_result = engine_->surface_manager()->SwapBuffers();
}
return swap_buffers_result;
}
case ResizeState::kDone:
default:
return engine_->surface_manager()->SwapBuffers();
return;
}
}

Expand Down
16 changes: 5 additions & 11 deletions shell/platform/windows/flutter_windows_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,6 @@ class FlutterWindowsView : public WindowBindingHandlerDelegate {
// Tells the engine to generate a new frame
void ForceRedraw();

// Swap the view's surface buffers. Must be called on the engine's raster
// thread. Returns true if the buffers were swapped.
//
// |OnFrameGenerated| or |OnEmptyFrameGenerated| must be called before this
// method.
//
// If the view is resizing, this returns false if the frame is not the target
// size. Otherwise, it unblocks the platform thread and blocks the raster
// thread until the v-blank.
virtual bool SwapBuffers();

// Callback to clear a previously presented software bitmap.
virtual bool ClearSoftwareBitmap();

Expand Down Expand Up @@ -103,6 +92,11 @@ class FlutterWindowsView : public WindowBindingHandlerDelegate {
// |height| match the target size.
bool OnFrameGenerated(size_t width, size_t height);

// Called on the raster thread after |CompositorOpenGL| presents a frame.
//
// This completes a view resize if one is pending.
virtual void OnFramePresented();

// Sets the cursor that should be used when the mouse is over the Flutter
// content. See mouse_cursor.dart for the values and meanings of cursor_name.
void UpdateFlutterCursor(const std::string& cursor_name);
Expand Down
2 changes: 2 additions & 0 deletions shell/platform/windows/testing/mock_angle_surface_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class MockAngleSurfaceManager : public AngleSurfaceManager {
MOCK_METHOD(bool, ClearCurrent, (), (override));
MOCK_METHOD(void, SetVSyncEnabled, (bool), (override));

MOCK_METHOD(bool, SwapBuffers, (), (override));

private:
FML_DISALLOW_COPY_AND_ASSIGN(MockAngleSurfaceManager);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ class MockWindowBindingHandler : public WindowBindingHandler {
MOCK_METHOD(void, SetView, (WindowBindingHandlerDelegate * view), (override));
MOCK_METHOD(HWND, GetWindowHandle, (), (override));
MOCK_METHOD(float, GetDpiScale, (), (override));
MOCK_METHOD(bool, IsVisible, (), (override));
MOCK_METHOD(PhysicalWindowBounds, GetPhysicalWindowBounds, (), (override));
MOCK_METHOD(void,
UpdateFlutterCursor,
Expand Down
3 changes: 0 additions & 3 deletions shell/platform/windows/window_binding_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,6 @@ class WindowBindingHandler {
// Returns the scale factor for the backing window.
virtual float GetDpiScale() = 0;

// Returns whether the HWND is currently visible.
virtual bool IsVisible() = 0;

// Returns the bounds of the backing window in physical pixels.
virtual PhysicalWindowBounds GetPhysicalWindowBounds() = 0;

Expand Down