Fix damage calculation when not providing populate_existing_damage for gl embedder #45611
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ajihyf
force-pushed
the
fix-embedder-damage
branch
2 times, most recently
from
92bb9ae
to
4d805dc
Compare
|
Thanks for raising this, I facing same problem. |
ajihyf
force-pushed
the
fix-embedder-damage
branch
2 times, most recently
from
82e8b3d
to
a518c0b
Compare
|
This looks like a sound change. Perhaps @knopp can also review? |
chinmaygarde
approved these changes
Sep 21, 2023
knopp
approved these changes
Sep 21, 2023
shell/platform/embedder/embedder.cc
Outdated
|
|
||
| // Verify that at least one damage rectangle was provided. | ||
| if (existing_damage.num_rects <= 0 || existing_damage.damage == nullptr) { | ||
| FML_LOG(INFO) << "No damage was provided. Forcing full repaint."; | ||
| existing_damage_rect = SkIRect::MakeEmpty(); | ||
| partial_repaint_enabled = false; | ||
| } else if (existing_damage.num_rects > 1) { | ||
| // Log message notifying users that multi-damage is not yet available in |
There was a problem hiding this comment.
Maybe we should join the rectangles here?
There was a problem hiding this comment.
Done. Also added one more test.
ajihyf
force-pushed
the
fix-embedder-damage
branch
2 times, most recently
from
0277297
to
a3ddee6
Compare
|
|
|
Thanks for doing this @ajihyf . How about rebasing the change? It should trigger the CI tests. |
ajihyf
force-pushed
the
fix-embedder-damage
branch
from
a3ddee6
to
49337d5
Compare
chinmaygarde
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Sep 28, 2023
…_damage for gl embedder (flutter/engine#45611)
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Sep 28, 2023
…135717) flutter/engine@cc7c3c1...485543c 2023-09-28 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from cu6apvEZ2P6zhishc... to l2RxJKPfYn7QzGOoL... (flutter/engine#46382) 2023-09-28 30870216+gaaclarke@users.noreply.github.com Remove opacity layer dcheck. (flutter/engine#46160) 2023-09-28 matanlurey@users.noreply.github.com Add initial support for 4x MSAA in OpenGLES backend. (flutter/engine#46381) 2023-09-28 matej.knopp@gmail.com Reland: [macOS] performKeyEquivalent cleanup (flutter/engine#46377) 2023-09-28 matej.knopp@gmail.com [macOS] TextInputPlugin should mark navigation events in IME popover as handled (flutter/engine#46141) 2023-09-28 30870216+gaaclarke@users.noreply.github.com Removed unnecessary dynamic dispatch (flutter/engine#46369) 2023-09-28 joel.winarske@gmail.com [Impeller] Fix OpenGLES EGL_BAD_ACCESS due to context being current on multiple threads. (flutter/engine#46287) 2023-09-28 ajihyf@gmail.com Fix damage calculation when not providing populate_existing_damage for gl embedder (flutter/engine#45611) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from cu6apvEZ2P6z to l2RxJKPfYn7Q If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Mairramer
pushed a commit
to Mairramer/flutter
that referenced
this pull request
Oct 10, 2023
…lutter#135717) flutter/engine@cc7c3c1...485543c 2023-09-28 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from cu6apvEZ2P6zhishc... to l2RxJKPfYn7QzGOoL... (flutter/engine#46382) 2023-09-28 30870216+gaaclarke@users.noreply.github.com Remove opacity layer dcheck. (flutter/engine#46160) 2023-09-28 matanlurey@users.noreply.github.com Add initial support for 4x MSAA in OpenGLES backend. (flutter/engine#46381) 2023-09-28 matej.knopp@gmail.com Reland: [macOS] performKeyEquivalent cleanup (flutter/engine#46377) 2023-09-28 matej.knopp@gmail.com [macOS] TextInputPlugin should mark navigation events in IME popover as handled (flutter/engine#46141) 2023-09-28 30870216+gaaclarke@users.noreply.github.com Removed unnecessary dynamic dispatch (flutter/engine#46369) 2023-09-28 joel.winarske@gmail.com [Impeller] Fix OpenGLES EGL_BAD_ACCESS due to context being current on multiple threads. (flutter/engine#46287) 2023-09-28 ajihyf@gmail.com Fix damage calculation when not providing populate_existing_damage for gl embedder (flutter/engine#45611) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from cu6apvEZ2P6z to l2RxJKPfYn7Q If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
harryterkelsen
pushed a commit
that referenced
this pull request
Oct 23, 2023
…r gl embedder (#45611) # Description This PR fixes the `gl_populate_existing_damage` in embedder.cc, which currently returns an empty rectangle when a full repaint is needed. This leads to the `frame_damage` being considered empty in rasterizer.cc, causing incorrect partial repaint within Flutter when `gl_populate_existing_damage` is not provided by the embedder. # Related Issue flutter/flutter#119601 # Tests Add a new test. Also fixes damage calculation related tests in EmbedderTest by * Use a new Dart embedder fixture entry point `render_gradient_retained` which retains the old layer to make sure layer tree diff happens. * Add `latch.Wait()` after `SetGLPresentCallback` to make sure assertions have been executed. * Make `existing_damage_rects` static since it should be valid after `populate_existing_damage` returns.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes the
gl_populate_existing_damagein embedder.cc, which currently returns an empty rectangle when a full repaint is needed. This leads to theframe_damagebeing considered empty in rasterizer.cc, causing incorrect partial repaint within Flutter whengl_populate_existing_damageis not provided by the embedder.Related Issue
flutter/flutter#119601
Tests
Add a new test. Also fixes damage calculation related tests in EmbedderTest by
render_gradient_retainedwhich retains the old layer to make sure layer tree diff happens.latch.Wait()afterSetGLPresentCallbackto make sure assertions have been executed.existing_damage_rectsstatic since it should be valid afterpopulate_existing_damagereturns.