[Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR). #161767

jonahwilliams · 2025-01-16T18:56:15Z

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.

… KHR).

flutter-dashboard · 2025-01-16T18:56:18Z

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 in Discord (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 the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

jonahwilliams · 2025-01-16T19:24:24Z

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

All nits.

chinmaygarde · 2025-01-16T19:34:07Z

engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/ahb/external_semaphore_vk.h

 ///             on the implementation.
 ///
-class ExternalFenceVK {
+class ExternalSemaphoreVK {


The docs above are outdated.

chinmaygarde · 2025-01-16T19:36:53Z

engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_impl_vk.h


+static constexpr const size_t kMaxPendingPresents = 2u;
+
+struct AHBFrameSynchronizerVK {


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.

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...

chinmaygarde · 2025-01-16T19:38:06Z

engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_impl_vk.cc

  is_valid_ = control && control->IsValid();
+
+  for (auto i = 0u; i < kMaxPendingPresents; i++) {
+    frame_data_.push_back(std::make_unique<AHBFrameSynchronizerVK>(


Let's add some error checking here? Perhaps a validity checks for all the handles.

…e do for KHR). (flutter/flutter#161767)

flutter/flutter@5517cc9...b9e86a5 2025-01-18 [email protected] Fix link to Linux custom embedder artifacts (flutter/flutter#161699) 2025-01-18 [email protected] Remove tests, GLFW examples, and non-artifact builds from `linux_host_engine`. (flutter/flutter#161786) 2025-01-18 [email protected] [Impeller] when mips are disabled, also disable from sampler options. (flutter/flutter#161765) 2025-01-18 [email protected] revert removing Twitter, retain BlueSky (flutter/flutter#161803) 2025-01-17 [email protected] fix reorderable_list_test.dart (flutter/flutter#161836) 2025-01-17 [email protected] [Release] Update the cherry-pick process. (flutter/flutter#161771) 2025-01-17 [email protected] Reland "#143249 Autocomplete options width" (flutter/flutter#161695) 2025-01-17 [email protected] Roll Dart to Version 3.8.0-1.0.dev (flutter/flutter#161781) 2025-01-17 [email protected] [Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR). (flutter/flutter#161767) 2025-01-17 [email protected] [Impeller] remove Adreno denylist entries. (flutter/flutter#161740) 2025-01-17 [email protected] Refactor event redispatching (flutter/flutter#161701) 2025-01-17 [email protected] [Impellerc] correctly pad arrays of vec3s in reflector. (flutter/flutter#161697) 2025-01-17 [email protected] Initialize dartLoader.rootDirectories so the Web stack trace mapper can convert package source paths (flutter/flutter#160383) 2025-01-16 [email protected] Make fl_keyboard_manager_handle_event async (flutter/flutter#161637) 2025-01-16 [email protected] Update social links in readme (flutter/flutter#161778) 2025-01-16 [email protected] Remove some stray printf debugging (flutter/flutter#161706) 2025-01-16 [email protected] Set meta tag in default index (flutter/flutter#161493) 2025-01-16 [email protected] remove usage of `Usage` from build system (flutter/flutter#160663) 2025-01-16 [email protected] route CLI command usage information through the logger instead of using `print` (flutter/flutter#161533) 2025-01-16 [email protected] Enable duplicate `linux_host_engine_test`. (flutter/flutter#161613) 2025-01-16 [email protected] Do not block vertical drag gestures in CupertinoSheetRoute body (flutter/flutter#161696) 2025-01-16 [email protected] [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 roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

… KHR). (flutter#161767) 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.

…e do for KHR). (flutter/flutter#161767)

flutter/flutter@5517cc9...b9e86a5 2025-01-18 [email protected] Fix link to Linux custom embedder artifacts (flutter/flutter#161699) 2025-01-18 [email protected] Remove tests, GLFW examples, and non-artifact builds from `linux_host_engine`. (flutter/flutter#161786) 2025-01-18 [email protected] [Impeller] when mips are disabled, also disable from sampler options. (flutter/flutter#161765) 2025-01-18 [email protected] revert removing Twitter, retain BlueSky (flutter/flutter#161803) 2025-01-17 [email protected] fix reorderable_list_test.dart (flutter/flutter#161836) 2025-01-17 [email protected] [Release] Update the cherry-pick process. (flutter/flutter#161771) 2025-01-17 [email protected] Reland "#143249 Autocomplete options width" (flutter/flutter#161695) 2025-01-17 [email protected] Roll Dart to Version 3.8.0-1.0.dev (flutter/flutter#161781) 2025-01-17 [email protected] [Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR). (flutter/flutter#161767) 2025-01-17 [email protected] [Impeller] remove Adreno denylist entries. (flutter/flutter#161740) 2025-01-17 [email protected] Refactor event redispatching (flutter/flutter#161701) 2025-01-17 [email protected] [Impellerc] correctly pad arrays of vec3s in reflector. (flutter/flutter#161697) 2025-01-17 [email protected] Initialize dartLoader.rootDirectories so the Web stack trace mapper can convert package source paths (flutter/flutter#160383) 2025-01-16 [email protected] Make fl_keyboard_manager_handle_event async (flutter/flutter#161637) 2025-01-16 [email protected] Update social links in readme (flutter/flutter#161778) 2025-01-16 [email protected] Remove some stray printf debugging (flutter/flutter#161706) 2025-01-16 [email protected] Set meta tag in default index (flutter/flutter#161493) 2025-01-16 [email protected] remove usage of `Usage` from build system (flutter/flutter#160663) 2025-01-16 [email protected] route CLI command usage information through the logger instead of using `print` (flutter/flutter#161533) 2025-01-16 [email protected] Enable duplicate `linux_host_engine_test`. (flutter/flutter#161613) 2025-01-16 [email protected] Do not block vertical drag gestures in CupertinoSheetRoute body (flutter/flutter#161696) 2025-01-16 [email protected] [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 roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

[Impeller] use 3 fences to synchronize AHB swapchains (like we do for…

3c780ad

… KHR).

github-actions bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Jan 16, 2025

Cleanups and renames.

efc6bd9

jonahwilliams requested a review from chinmaygarde January 16, 2025 19:07

chinmaygarde approved these changes Jan 16, 2025

View reviewed changes

jonahwilliams added 2 commits January 16, 2025 12:14

docs update and validity check.

d44c273

Update licenses.

28ced34

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 16, 2025

Merge branch 'master' into really-fix-ahb-swapchain

d064e8f

auto-submit bot added this pull request to the merge queue Jan 17, 2025

Merged via the queue into flutter:master with commit 32ef7f8 Jan 17, 2025
180 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 17, 2025

engine-flutter-autoroll mentioned this pull request Jan 17, 2025

Roll Flutter from 5517cc9b3b3b to 329745473284 (16 revisions) flutter/packages#8454

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 17, 2025

32ef7f8 [Impeller] use 3 fences to synchronize AHB swapchains (like w…

78421c4

…e do for KHR). (flutter/flutter#161767)

engine-flutter-autoroll mentioned this pull request Jan 17, 2025

Roll Flutter from 5517cc9b3b3b to bb2d34126cc8 (17 revisions) flutter/packages#8455

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 17, 2025

32ef7f8 [Impeller] use 3 fences to synchronize AHB swapchains (like w…

642bb17

…e do for KHR). (flutter/flutter#161767)

engine-flutter-autoroll mentioned this pull request Jan 18, 2025

Roll Flutter from 5517cc9b3b3b to b9e86a565f92 (22 revisions) flutter/packages#8457

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 18, 2025

32ef7f8 [Impeller] use 3 fences to synchronize AHB swapchains (like w…

f87f7bf

…e do for KHR). (flutter/flutter#161767)

engine-flutter-autoroll mentioned this pull request Jan 18, 2025

Roll Flutter from 5517cc9b3b3b to b9e86a565f92 (22 revisions) flutter/packages#8458

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 18, 2025

32ef7f8 [Impeller] use 3 fences to synchronize AHB swapchains (like w…

22e4cfe

…e do for KHR). (flutter/flutter#161767)

engine-flutter-autoroll mentioned this pull request Jan 19, 2025

Roll Flutter from 5517cc9b3b3b to b9e86a565f92 (22 revisions) flutter/packages#8459

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 19, 2025

32ef7f8 [Impeller] use 3 fences to synchronize AHB swapchains (like w…

ae7c928

…e do for KHR). (flutter/flutter#161767)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

32ef7f8 [Impeller] use 3 fences to synchronize AHB swapchains (like w…

d019c5d

…e do for KHR). (flutter/flutter#161767)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

32ef7f8 [Impeller] use 3 fences to synchronize AHB swapchains (like w…

1194e7e

…e do for KHR). (flutter/flutter#161767)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025

32ef7f8 [Impeller] use 3 fences to synchronize AHB swapchains (like w…

c82610a

…e do for KHR). (flutter/flutter#161767)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR). #161767

[Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR). #161767

Uh oh!

jonahwilliams commented Jan 16, 2025

Uh oh!

flutter-dashboard bot commented Jan 16, 2025

Uh oh!

jonahwilliams commented Jan 16, 2025

Uh oh!

chinmaygarde left a comment

Uh oh!

chinmaygarde Jan 16, 2025

Uh oh!

jonahwilliams Jan 16, 2025

Uh oh!

chinmaygarde Jan 16, 2025

Uh oh!

jonahwilliams Jan 16, 2025

Uh oh!

chinmaygarde Jan 16, 2025

Uh oh!

jonahwilliams Jan 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		static constexpr const size_t kMaxPendingPresents = 2u;

		struct AHBFrameSynchronizerVK {

[Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR). #161767

[Impeller] use 3 fences to synchronize AHB swapchains (like we do for KHR). #161767

Uh oh!

Conversation

jonahwilliams commented Jan 16, 2025

Uh oh!

flutter-dashboard bot commented Jan 16, 2025

Uh oh!

jonahwilliams commented Jan 16, 2025

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants