[windows] Honor only valid resize requests #23990
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
|
||
| namespace flutter { | ||
|
|
||
| static bool IsValidResize(size_t cur_width, |
There was a problem hiding this comment.
Needs a comment.
Also, I'm not sure "Valid" is really the right descriptor here. The OS is communicating something valid with a 0x0 resize.
There was a problem hiding this comment.
Changed this to SurfaceWillUpdate, I am not a huge fan of it since the name expects the embedding to know when the engine will update the surface to synchronize resize events, open to alternative suggestions.
| std::unique_lock<std::mutex> lock(resize_mutex_); | ||
| if (!IsValidResize(resize_target_width_, resize_target_height_, width, | ||
| height)) { | ||
| return; |
There was a problem hiding this comment.
When the size doesn't change this is fine, but for 0x0 the surface actually is 0x0. Failing to send that information to the engine means will be out of sync with the surface it's drawing to. (I'm not clear on how problematic that could potentially be; e.g., is there potential for memory corruption as there would be in the case of a pixel buffer?)
Unless we're extremely confident that there will be no unexpected side-effects (given that this will need to be cherry-picked) I think for now, the safer approach would be to send the 0x0 metrics as we currently do and only bypass the resize state logic+blocking. We can leave a TODO with a pointer to flutter/flutter#65061 to resolve 0x0 handling in general, via something like a new embedder.h API that we call to tell the engine to suspend drawing, rather than doing nothing.
| resize_target_height_ = height; | ||
| SendWindowMetrics(width, height, binding_handler_->GetDpiScale()); | ||
|
|
||
| if (width > 0 && height > 0) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
iskakaushik
force-pushed
the
fix_windows_resize_deadlock
branch
from
f94e4ec to
ace3947
Compare
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM with comment nit.
|
|
||
| namespace flutter { | ||
|
|
||
| /// Returns true if the surface will be updated as part of the resize process. |
There was a problem hiding this comment.
Can you flesh this out to specifically say what other code this is required to stay in sync with?
Add more context around resizing.
iskakaushik
force-pushed
the
fix_windows_resize_deadlock
branch
from
eabd8ea to
5394774
Compare
iskakaushik
added
the
waiting for tree to go green
…24142) * add ffi_allocation_patch.dart to libraries.yaml 2 (#23954) * Use `runes` to get code units in CanvasKit. (#24024) * [windows] Honor only valid resize requests (#23990) * Load FlutterLoader when creating FlutterEngineGroup (#23980) * [canvaskit] remove the DOM node of unrendered platform view (#24001) * update dart to 2.12.0-259.8.beta * [web] Fix svg based stroke rendering. (#23969) * [canvaskit] fix text background, foreground, color; add text style tests (#23800) * [canvaskit] fix text background, foreground, familyFallback; add style tests * add leak test * update goldens_lock.yaml * remove solo * Warn when popping out of empty text style stack * [web] Fix alignment issue in rich paragraphs (#23965) * [web] Implement CanvasParagraph.getLineBoundary (#24037) * [web] Fix text alignment when transform + tight constraints + DOM rendering (#24036) * update licenses_third_party golden Co-authored-by: Daco Harkes <[email protected]> Co-authored-by: Harry Terkelsen <[email protected]> Co-authored-by: Kaushik Iska <[email protected]> Co-authored-by: xster <[email protected]> Co-authored-by: Yegor <[email protected]> Co-authored-by: Ferhat <[email protected]> Co-authored-by: Mouad Debbar <[email protected]>
3 participants
Fixes: flutter/flutter#74797