Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] Refactor KHR swapchains to make it easy to reuse backend agnostic components. #52002

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

chinmaygarde
Copy link
Member

Just a refactor to make room for the AHB swapchains implementation while also ensuring that the MSAA and depth-stencil transients memoization as well as the existing surface implementation can be reused by that swapchain backend.

This does a few major things:

  • Make an abstract implementation of swapchains, SwapchainVK. This currently has KHRSwapchainVK as its sole subclass but will soon have AHBSwapchainVK.
  • There is no more per swapchain backend memoization of the MSAA and depth-stencil textures. This is now moved to SwapchainTransientsVK and can be shared by both backend. This leads into the next change. This also avoids the round trip of the textures first being set on each swapchain image and then accessed to create the onscreen renderpass. Now the transients can access the textures directly.
  • KHRSurfaceVK no longer wraps a KHRSwapchainImageVK. Instead, it deals with TextureSourceVKs (which used to be the base class of KHRSwapchainImageVK). This surface can now magically work with AHBTextureSourceVK since they have a common base class. Since the surface is now backend agnostic, it has been renamed to SurfaceVK.

There is one minor functional change over the previous implementation thought. Earlier, the transients would be created and cached when the swapchain was resized. Now, the same will happen when the first surface frame is attempted to be acquired at the new size. This effectively means that swapchain resized should be faster and do less work if no frames are rendered at the new size (continuous window resized maybe).

…gnostic components.

Just a refactor to make room for the AHB swapchains implementation while also ensuring that the MSAA and depth-stencil transients memoization as well as the existing surface implementation can be reused by that swapchain backend.

This does a few major things:

* Make an abstract implementation of swapchains, SwapchainVK. This currently has KHRSwapchainVK as its sole subclass but will soon have AHBSwapchainVK.
* There is no more per swapchain backend memoization of the MSAA and depth-stencil textures. This is now moved to SwapchainTransientsVK and can be shared by both backend. This leads into the next change. This also avoids the round trip of the textures first being set on each swapchain image and then accessed to create the onscreen renderpass. Now the transients can access the textures directly. 
* KHRSurfaceVK no longer wraps a KHRSwapchainImageVK. Instead, it deals with TextureSourceVKs (which used to be the base class of KHRSwapchainImageVK). This surface can now magically work with AHBTextureSourceVK since they have a common base class. Since the surface is now backend agnostic, it has been renamed to SurfaceVK.

There is one minor functional change over the previous implementation thought. Earlier, the transients would be created and cached when the swapchain was resized. Now, the same will happen when the first surface frame is attempted to be acquired at the new size. This effectively means that swapchain resized should be faster and do less work if no frames are rendered at the new size (continuous window resized maybe).
@flutter-dashboard
Copy link

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Overall LGTM with one nit.

@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2024
Copy link
Contributor

auto-submit bot commented Apr 9, 2024

auto label is removed for flutter/engine/52002, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2024
@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2024
@auto-submit auto-submit bot merged commit 8dd3ba8 into flutter:main Apr 9, 2024
30 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 10, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 10, 2024
…146538)

flutter/engine@e4e2748...eaf73cd

2024-04-09 jason-simmons@users.noreply.github.com Update embedder example apps to run with the current engine tree (flutter/engine#51995)
2024-04-09 chinmaygarde@google.com [Impeller] Refactor KHR swapchains to make it easy to reuse backend agnostic components. (flutter/engine#52002)
2024-04-09 skia-flutter-autoroll@skia.org Roll Skia from 74b0e26886f0 to 45eeeddb0074 (1 revision) (flutter/engine#52007)
2024-04-09 zanderso@users.noreply.github.com Try postsubmit_overrides for one mac build (flutter/engine#51676)
2024-04-09 bdero@google.com [Impeller] Remove stencil clipping logic. (flutter/engine#51999)
2024-04-09 skia-flutter-autoroll@skia.org Roll Skia from a86861d21ae1 to 74b0e26886f0 (2 revisions) (flutter/engine#52000)

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 chinmaygarde@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://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
@chinmaygarde chinmaygarde deleted the ahb_swapchain_cleanup branch April 10, 2024 04:52
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
…lutter#146538)

flutter/engine@e4e2748...eaf73cd

2024-04-09 jason-simmons@users.noreply.github.com Update embedder example apps to run with the current engine tree (flutter/engine#51995)
2024-04-09 chinmaygarde@google.com [Impeller] Refactor KHR swapchains to make it easy to reuse backend agnostic components. (flutter/engine#52002)
2024-04-09 skia-flutter-autoroll@skia.org Roll Skia from 74b0e26886f0 to 45eeeddb0074 (1 revision) (flutter/engine#52007)
2024-04-09 zanderso@users.noreply.github.com Try postsubmit_overrides for one mac build (flutter/engine#51676)
2024-04-09 bdero@google.com [Impeller] Remove stencil clipping logic. (flutter/engine#51999)
2024-04-09 skia-flutter-autoroll@skia.org Roll Skia from a86861d21ae1 to 74b0e26886f0 (2 revisions) (flutter/engine#52000)

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 chinmaygarde@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://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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
None yet
2 participants