Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@chinmaygarde
Copy link
Contributor

@chinmaygarde chinmaygarde commented Jun 8, 2024

@chinmaygarde chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Jun 8, 2024
@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.

@chinmaygarde
Copy link
Contributor Author

Followup of #53079 with fixups (see the second patch) after guidance from Greg & Jesse.

I don't have a device on hand that has these extensions to check if this actually works and I don't think the swapchain stuff is expected to work on the Pixels (I'll still wire it up). So still WIP. Will finish verification next week when I get back to my desk.

@jonahwilliams
Copy link
Contributor

I can confirm that my pixel 8 is selecting vk::ImageCompressionFixedRateFlagBitsEXT::e4Bpc! That said, I don't know if its doing anything :)

@chinmaygarde
Copy link
Contributor Author

I mean, if you can't see it (and its actually applied), then good :) Testing.

@chinmaygarde
Copy link
Contributor Author

Got a hold of a Pixel 8a. The diffs aren't noticeable (but are present).
AFRC

@jonahwilliams
Copy link
Contributor

interesting, those are all raster images. Is there a diff on a page with a lot of text?

@chinmaygarde
Copy link
Contributor Author

Curiously, on a wall of text, I could find no differences. Thinking I messed up, I tried 4 different builds with logging. Again, no difference. Thinking that that AFRC conditions were not met, I tried logging like so but didn't see any logs either. So whatever its doing, there seems to be no difference in rendered output. I almost can't believe it.

if (!image_info_chain.isLinked<vk::ImageCompressionControlEXT>() &&
    desc.compression_type == CompressionType::kLossy) {
  FML_LOG(ERROR)
      << "Requested lossy compression but could not satisfy conditions.";
}

Text

@chinmaygarde
Copy link
Contributor Author

I'm going to back out the swapchain stuff because that is not meant to work. And what we have here we are doing on iOS too. So we have some experience with this. I'm going to get memory usage numbers first though.

@jonahwilliams
Copy link
Contributor

is the AFRC only applying to offscreen rendering and not the onscreen ?

@chinmaygarde
Copy link
Contributor Author

Yes, but the images themselves should have AFRC enabled.

@chinmaygarde
Copy link
Contributor Author

Rapidly swiping the Wonders in the Wondrous app shows the memory usage drops from ~730 MB to ~520 MB reliable. I tried different sections for similar results. I think this matches up with what we were observing on iOS too right?

Screenshot 2024-06-17 at 3 02 23 PM Screenshot 2024-06-17 at 2 56 57 PM

@jonahwilliams
Copy link
Contributor

Yes that seems about right for ballpark. We saw about 50% for iOS, this is less than that but I don't know what else is in that heap.

@chinmaygarde chinmaygarde removed the Work in progress (WIP) Not ready (yet) for review! label Jun 17, 2024
@chinmaygarde
Copy link
Contributor Author

Ok, I've cleanup up the swapchain stuff and verified similar behavior to iOS with lossy compression. Though this took longer than expected, I'm satisfied we are doing the best we can and there is parity between the platforms. We can try to wire up the onscreen surfaces compression or mess with other rates at a later time. But I'd like to close this loop on this one. PTAL.

Copy link
Contributor

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

I'm not sure why this is crashing on the tests...

Comment on lines +651 to +655
if (compression_type != CompressionType::kLossy) {
return std::nullopt;
}
if (!supports_texture_fixed_rate_compression_) {
return std::nullopt;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: combine these into one?

if (compression_type != CompressionType::kLossy || !supports_texture_fixed_rate_compression_) {
  return std::nullopt;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@chinmaygarde
Copy link
Contributor Author

The failures look real. I probably forgot to unlink an item in the structure chain. Taking a look.

@chinmaygarde
Copy link
Contributor Author

I believe I have fixed everything and addressed all comments. PTAL.

@chinmaygarde
Copy link
Contributor Author

Ping @jonahwilliams. I've addressed all comments verified memory savings. A review please?

Copy link
Contributor

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

LGTM!

@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 11, 2024
@auto-submit auto-submit bot merged commit df8ded5 into flutter:main Jul 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 12, 2024
flutter/engine@36dccf7...6b36113

2024-07-12 skia-flutter-autoroll@skia.org Roll Skia from 9ea5603242c1 to 923034db7728 (1 revision) (flutter/engine#53841)
2024-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from 797d3df745d1 to e986ed9d0bc1 (1 revision) (flutter/engine#53840)
2024-07-12 skia-flutter-autoroll@skia.org Roll Skia from 7a91f0a4b7a0 to 9ea5603242c1 (1 revision) (flutter/engine#53839)
2024-07-12 skia-flutter-autoroll@skia.org Roll Skia from 9529b8ad9e45 to 7a91f0a4b7a0 (2 revisions) (flutter/engine#53838)
2024-07-12 skia-flutter-autoroll@skia.org Roll Skia from 38f355af4f36 to 9529b8ad9e45 (1 revision) (flutter/engine#53837)
2024-07-12 skia-flutter-autoroll@skia.org Roll Skia from 14c8d318615d to 38f355af4f36 (1 revision) (flutter/engine#53836)
2024-07-12 skia-flutter-autoroll@skia.org Roll Skia from ddf045505cb9 to 14c8d318615d (1 revision) (flutter/engine#53835)
2024-07-12 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 0e47sje8wkJ08sGJ6... to VlZIUknh6dnA23owe... (flutter/engine#53834)
2024-07-12 jason-simmons@users.noreply.github.com Manual roll Dart SDK from fb546f313557 to 797d3df745d1 (8 revisions) (flutter/engine#53832)
2024-07-11 jonahwilliams@google.com [Impeller] Ensure full transform is applied to text contents (flutter/engine#53819)
2024-07-11 skia-flutter-autoroll@skia.org Roll ICU from 43953f57b037 to 9408c6fd4a39 (6 revisions) (flutter/engine#53827)
2024-07-11 matanlurey@users.noreply.github.com Update Life-of-a-Flutter-Frame.md (flutter/engine#53829)
2024-07-11 matanlurey@users.noreply.github.com Update Setting-up-the-Engine-development-environment.md (flutter/engine#53828)
2024-07-11 25964451+p-mazhnik@users.noreply.github.com [web] retrieve hostElement for an implicit view (flutter/engine#53296)
2024-07-11 jonahwilliams@google.com [dart:ui] remove expensive index assertion in Vertices. (flutter/engine#53558)
2024-07-11 chinmaygarde@google.com [Impeller] Enable fixed-rate compression support in Vulkan. (flutter/engine#53292)
2024-07-11 skia-flutter-autoroll@skia.org Roll Skia from 037d5f8a727f to ddf045505cb9 (1 revision) (flutter/engine#53824)
2024-07-11 chinmaygarde@google.com Add instructions for source debugging with Xcode when using RBE. (flutter/engine#53822)
2024-07-11 skia-flutter-autoroll@skia.org Roll Skia from 004c81523e44 to 037d5f8a727f (1 revision) (flutter/engine#53818)
2024-07-11 jonahwilliams@google.com [Impeller] move more aiks tests to DL.  (flutter/engine#53792)
2024-07-11 skia-flutter-autoroll@skia.org Roll Skia from ec4a1e03f7b0 to 004c81523e44 (23 revisions) (flutter/engine#53813)
2024-07-11 skia-flutter-autoroll@skia.org Roll Skia from 2783ba54bf8e to ec4a1e03f7b0 (9 revisions) (flutter/engine#53797)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 0e47sje8wkJ0 to VlZIUknh6dnA

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 bdero@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 frcvulkan2 branch August 22, 2024 18:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Impeller] Support lossy texture compression in the Vulkan backend.

2 participants