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

[windows] Honor only valid resize requests #23990

Merged
merged 2 commits into from
Jan 28, 2021
Merged

[windows] Honor only valid resize requests #23990

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ace3947
[windows] Honor only valid resize requests
iskakaushik Jan 27, 2021
5394774
Update flutter_windows_view.cc
iskakaushik Jan 27, 2021
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
32 changes: 28 additions & 4 deletions shell/platform/windows/flutter_windows_view.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,24 @@

namespace flutter {

/// Returns true if the surface will be updated as part of the resize process.
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jan 27, 2021

Choose a reason for hiding this comment

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

Can you flesh this out to specifically say what other code this is required to stay in sync with?

///
/// This is called on window resize to determine if the platform thread needs
/// to be blocked until the frame with the right size has been rendered. It
/// should be kept in-sync with how the engine deals with a new surface request
/// as seen in `CreateOrUpdateSurface` in `GPUSurfaceGL`.
static bool SurfaceWillUpdate(size_t cur_width,
size_t cur_height,
size_t target_width,
size_t target_height) {
// TODO (https://github.com/flutter/flutter/issues/65061) : Avoid special
// handling for zero dimensions.
bool non_zero_dims = target_height > 0 && target_width > 0;
bool not_same_size =
(cur_height != target_height) || (cur_width != target_width);
return non_zero_dims && not_same_size;
}

FlutterWindowsView::FlutterWindowsView(
std::unique_ptr<WindowBindingHandler> window_binding) {
surface_manager_ = std::make_unique<AngleSurfaceManager>();
Expand Down Expand Up @@ -80,12 +98,18 @@ uint32_t FlutterWindowsView::GetFrameBufferId(size_t width, size_t height) {
void FlutterWindowsView::OnWindowSizeChanged(size_t width, size_t height) {
// Called on the platform thread.
std::unique_lock<std::mutex> lock(resize_mutex_);
resize_status_ = ResizeState::kResizeStarted;
resize_target_width_ = width;
resize_target_height_ = height;

bool surface_will_update = SurfaceWillUpdate(
resize_target_width_, resize_target_height_, width, height);
if (surface_will_update) {
resize_status_ = ResizeState::kResizeStarted;
resize_target_width_ = width;
resize_target_height_ = height;
}

SendWindowMetrics(width, height, binding_handler_->GetDpiScale());

if (width > 0 && height > 0) {
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jan 27, 2021

Choose a reason for hiding this comment

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

Hm, I assumed from the bug description that we were blocking on an update that never came, but it looks like that already shouldn't have been the case. What exactly is causing the hang then?

Copy link
Contributor Author

@iskakaushik iskakaushik Jan 27, 2021

Choose a reason for hiding this comment

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

The hang is caused because we don't ask for an FBO when the onscreen surface that is held transitively by the rasterizer is the same size as the requested size.

The resizing logic here assumes that every request to OnWindowSizeChanged will result in a new FBO being requested which is violated when we minimize-restore as we don't destroy the on-screen surface when that happens.

I will follow your recommendation and link to #23990 (comment) with a pointer that we likely want to destroy the on-screen surface when the application is minimized which would resolve this. For now, I will special case this in the resizing logic.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, we're already keeping the old surface around? I thought it was controlled directly by the size we got from the OS (and thus would be zero-sized).

If we have a surface at the old size still, then I think I'm fine with your approach here, as long as you comment as to why.

if (surface_will_update) {
// Block the platform thread until:
// 1. GetFrameBufferId is called with the right frame size.
// 2. Any pending SwapBuffers calls have been invoked.
Expand Down