- Notifications
You must be signed in to change notification settings - Fork30k
[Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR).#161767
Merged
auto-submit[bot] merged 5 commits intoflutter:masterfromJan 17, 2025
Merged
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel inDiscord (don't just cc them here, they won't see it!). 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 theTree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, soall reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
ContributorAuthor
jonahwilliams commentedJan 16, 2025
The reason for having both the signal semaphore in fence is that there is a difference between completion of command buffer execution and the memory visibility of the input attachment. Not necessariyl on all devices of course, but it matches KHR so seems reasonable. |
chinmaygarde approved these changesJan 16, 2025
Contributor
chinmaygarde left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
All nits.
| /// on the implementation. | ||
| /// | ||
| classExternalFenceVK { | ||
| classExternalSemaphoreVK { |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The docs above are outdated.
ContributorAuthor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| static constexpr const size_t kMaxPendingPresents = 2u; | ||
| struct AHBFrameSynchronizerVK { |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we just use the same synchronizer util (with some mods if necessary) for both KHR and ABH? The KHR synchronizer was just an implementation detail and I don't think I did a great job documenting it.
ContributorAuthor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is different behavior in how/when we initialize the semaphores, specifically the external ones. I'd like to keep the swapchains as separate as possible until I know this one actually works in prod...
| is_valid_ = control && control->IsValid(); | ||
| for (auto i = 0u; i < kMaxPendingPresents; i++) { | ||
| frame_data_.push_back(std::make_unique<AHBFrameSynchronizerVK>( |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's add some error checking here? Perhaps a validity checks for all the handles.
ContributorAuthor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
Merged via the queue intoflutter:master with commitJan 17, 2025
32ef7f8 180 checks passed
Uh oh!
There was an error while loading.Please reload this page.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull requestJan 17, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull requestJan 17, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull requestJan 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull requestJan 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull requestJan 19, 2025
auto-submitbot pushed a commit to flutter/packages that referenced this pull requestJan 20, 2025
flutter/flutter@5517cc9...b9e86a52025-01-18 737941+loic-sharma@users.noreply.github.com Fix link to Linux custom embedder artifacts (flutter/flutter#161699)2025-01-18 matanlurey@users.noreply.github.com Remove tests, GLFW examples, and non-artifact builds from `linux_host_engine`. (flutter/flutter#161786)2025-01-18 jonahwilliams@google.com [Impeller] when mips are disabled, also disable from sampler options. (flutter/flutter#161765)2025-01-18 lukepighetti@gmail.com revert removing Twitter, retain BlueSky (flutter/flutter#161803)2025-01-17 yjbanov@google.com fix reorderable_list_test.dart (flutter/flutter#161836)2025-01-17 kevinjchisholm@google.com [Release] Update the cherry-pick process. (flutter/flutter#161771)2025-01-17 victorsanniay@gmail.com Reland "#143249 Autocomplete options width" (flutter/flutter#161695)2025-01-17 a-siva@users.noreply.github.com Roll Dart to Version 3.8.0-1.0.dev (flutter/flutter#161781)2025-01-17 jonahwilliams@google.com [Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR). (flutter/flutter#161767)2025-01-17 jonahwilliams@google.com [Impeller] remove Adreno denylist entries. (flutter/flutter#161740)2025-01-17 robert.ancell@canonical.com Refactor event redispatching (flutter/flutter#161701)2025-01-17 jonahwilliams@google.com [Impellerc] correctly pad arrays of vec3s in reflector. (flutter/flutter#161697)2025-01-17 jason-simmons@users.noreply.github.com Initialize dartLoader.rootDirectories so the Web stack trace mapper can convert package source paths (flutter/flutter#160383)2025-01-16 robert.ancell@canonical.com Make fl_keyboard_manager_handle_event async (flutter/flutter#161637)2025-01-16 kevmoo@users.noreply.github.com Update social links in readme (flutter/flutter#161778)2025-01-16 robert.ancell@canonical.com Remove some stray printf debugging (flutter/flutter#161706)2025-01-16 58529443+srujzs@users.noreply.github.com Set meta tag in default index (flutter/flutter#161493)2025-01-16 andrewrkolos@gmail.com remove usage of `Usage` from build system (flutter/flutter#160663)2025-01-16 andrewrkolos@gmail.com route CLI command usage information through the logger instead of using `print` (flutter/flutter#161533)2025-01-16 matanlurey@users.noreply.github.com Enable duplicate `linux_host_engine_test`. (flutter/flutter#161613)2025-01-16 58190796+MitchellGoodwin@users.noreply.github.com Do not block vertical drag gestures in CupertinoSheetRoute body (flutter/flutter#161696)2025-01-16 jonahwilliams@google.com [Impeller] Update partial repaint to use a fullsize onscreen. (flutter/flutter#161626)If this roll has caused a breakage, revert this CL and stop the rollerusing the controls here:https://autoroll.skia.org/r/flutter-packagesPlease CC stuartmorgan@google.com on the revert to ensure that a humanis aware of the problem.To file a bug in Packages:https://github.com/flutter/flutter/issues/new/chooseTo report a problem with the AutoRoller itself, please file a bug:https://issues.skia.org/issues/new?component=1389291&template=1850622Documentation for the AutoRoller is here:https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
jonahwilliams pushed a commit to jonahwilliams/flutter that referenced this pull requestJan 24, 2025
… KHR). (flutter#161767)Testing on a newer Adreno, I can still see rendering artifacts. Thischange matches the syncrhonization to what we do for KHR, and on theAdreno I see no more rendering artifacts.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull requestMay 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull requestMay 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull requestMay 21, 2025
androidseb pushed a commit to androidseb/packages that referenced this pull requestJun 8, 2025
flutter/flutter@5517cc9...b9e86a52025-01-18 737941+loic-sharma@users.noreply.github.com Fix link to Linux custom embedder artifacts (flutter/flutter#161699)2025-01-18 matanlurey@users.noreply.github.com Remove tests, GLFW examples, and non-artifact builds from `linux_host_engine`. (flutter/flutter#161786)2025-01-18 jonahwilliams@google.com [Impeller] when mips are disabled, also disable from sampler options. (flutter/flutter#161765)2025-01-18 lukepighetti@gmail.com revert removing Twitter, retain BlueSky (flutter/flutter#161803)2025-01-17 yjbanov@google.com fix reorderable_list_test.dart (flutter/flutter#161836)2025-01-17 kevinjchisholm@google.com [Release] Update the cherry-pick process. (flutter/flutter#161771)2025-01-17 victorsanniay@gmail.com Reland "#143249 Autocomplete options width" (flutter/flutter#161695)2025-01-17 a-siva@users.noreply.github.com Roll Dart to Version 3.8.0-1.0.dev (flutter/flutter#161781)2025-01-17 jonahwilliams@google.com [Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR). (flutter/flutter#161767)2025-01-17 jonahwilliams@google.com [Impeller] remove Adreno denylist entries. (flutter/flutter#161740)2025-01-17 robert.ancell@canonical.com Refactor event redispatching (flutter/flutter#161701)2025-01-17 jonahwilliams@google.com [Impellerc] correctly pad arrays of vec3s in reflector. (flutter/flutter#161697)2025-01-17 jason-simmons@users.noreply.github.com Initialize dartLoader.rootDirectories so the Web stack trace mapper can convert package source paths (flutter/flutter#160383)2025-01-16 robert.ancell@canonical.com Make fl_keyboard_manager_handle_event async (flutter/flutter#161637)2025-01-16 kevmoo@users.noreply.github.com Update social links in readme (flutter/flutter#161778)2025-01-16 robert.ancell@canonical.com Remove some stray printf debugging (flutter/flutter#161706)2025-01-16 58529443+srujzs@users.noreply.github.com Set meta tag in default index (flutter/flutter#161493)2025-01-16 andrewrkolos@gmail.com remove usage of `Usage` from build system (flutter/flutter#160663)2025-01-16 andrewrkolos@gmail.com route CLI command usage information through the logger instead of using `print` (flutter/flutter#161533)2025-01-16 matanlurey@users.noreply.github.com Enable duplicate `linux_host_engine_test`. (flutter/flutter#161613)2025-01-16 58190796+MitchellGoodwin@users.noreply.github.com Do not block vertical drag gestures in CupertinoSheetRoute body (flutter/flutter#161696)2025-01-16 jonahwilliams@google.com [Impeller] Update partial repaint to use a fullsize onscreen. (flutter/flutter#161626)If this roll has caused a breakage, revert this CL and stop the rollerusing the controls here:https://autoroll.skia.org/r/flutter-packagesPlease CC stuartmorgan@google.com on the revert to ensure that a humanis aware of the problem.To file a bug in Packages:https://github.com/flutter/flutter/issues/new/chooseTo report a problem with the AutoRoller itself, please file a bug:https://issues.skia.org/issues/new?component=1389291&template=1850622Documentation for the AutoRoller is here:https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
FMorschel pushed a commit to FMorschel/packages that referenced this pull requestJun 9, 2025
flutter/flutter@5517cc9...b9e86a52025-01-18 737941+loic-sharma@users.noreply.github.com Fix link to Linux custom embedder artifacts (flutter/flutter#161699)2025-01-18 matanlurey@users.noreply.github.com Remove tests, GLFW examples, and non-artifact builds from `linux_host_engine`. (flutter/flutter#161786)2025-01-18 jonahwilliams@google.com [Impeller] when mips are disabled, also disable from sampler options. (flutter/flutter#161765)2025-01-18 lukepighetti@gmail.com revert removing Twitter, retain BlueSky (flutter/flutter#161803)2025-01-17 yjbanov@google.com fix reorderable_list_test.dart (flutter/flutter#161836)2025-01-17 kevinjchisholm@google.com [Release] Update the cherry-pick process. (flutter/flutter#161771)2025-01-17 victorsanniay@gmail.com Reland "#143249 Autocomplete options width" (flutter/flutter#161695)2025-01-17 a-siva@users.noreply.github.com Roll Dart to Version 3.8.0-1.0.dev (flutter/flutter#161781)2025-01-17 jonahwilliams@google.com [Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR). (flutter/flutter#161767)2025-01-17 jonahwilliams@google.com [Impeller] remove Adreno denylist entries. (flutter/flutter#161740)2025-01-17 robert.ancell@canonical.com Refactor event redispatching (flutter/flutter#161701)2025-01-17 jonahwilliams@google.com [Impellerc] correctly pad arrays of vec3s in reflector. (flutter/flutter#161697)2025-01-17 jason-simmons@users.noreply.github.com Initialize dartLoader.rootDirectories so the Web stack trace mapper can convert package source paths (flutter/flutter#160383)2025-01-16 robert.ancell@canonical.com Make fl_keyboard_manager_handle_event async (flutter/flutter#161637)2025-01-16 kevmoo@users.noreply.github.com Update social links in readme (flutter/flutter#161778)2025-01-16 robert.ancell@canonical.com Remove some stray printf debugging (flutter/flutter#161706)2025-01-16 58529443+srujzs@users.noreply.github.com Set meta tag in default index (flutter/flutter#161493)2025-01-16 andrewrkolos@gmail.com remove usage of `Usage` from build system (flutter/flutter#160663)2025-01-16 andrewrkolos@gmail.com route CLI command usage information through the logger instead of using `print` (flutter/flutter#161533)2025-01-16 matanlurey@users.noreply.github.com Enable duplicate `linux_host_engine_test`. (flutter/flutter#161613)2025-01-16 58190796+MitchellGoodwin@users.noreply.github.com Do not block vertical drag gestures in CupertinoSheetRoute body (flutter/flutter#161696)2025-01-16 jonahwilliams@google.com [Impeller] Update partial repaint to use a fullsize onscreen. (flutter/flutter#161626)If this roll has caused a breakage, revert this CL and stop the rollerusing the controls here:https://autoroll.skia.org/r/flutter-packagesPlease CC stuartmorgan@google.com on the revert to ensure that a humanis aware of the problem.To file a bug in Packages:https://github.com/flutter/flutter/issues/new/chooseTo report a problem with the AutoRoller itself, please file a bug:https://issues.skia.org/issues/new?component=1389291&template=1850622Documentation for the AutoRoller is here:https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
e: impellerImpeller rendering backend issues and features requests engineflutter/engine related. See also e: labels.
2 participants
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.
Testing on a newer Adreno, I can still see rendering artifacts. This change matches the syncrhonization to what we do for KHR, and on the Adreno I see no more rendering artifacts.