engine icon indicating copy to clipboard operation
engine copied to clipboard

[Impeller] Enable fixed-rate compression support in Vulkan.

Open chinmaygarde opened this issue 1 year ago • 15 comments

Fixes https://github.com/flutter/flutter/issues/129501

chinmaygarde avatar Jun 08 '24 19:06 chinmaygarde

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.

flutter-dashboard[bot] avatar Jun 08 '24 19:06 flutter-dashboard[bot]

Followup of https://github.com/flutter/engine/pull/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.

chinmaygarde avatar Jun 08 '24 19:06 chinmaygarde

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

jonahwilliams avatar Jun 09 '24 17:06 jonahwilliams

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

chinmaygarde avatar Jun 10 '24 17:06 chinmaygarde

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

chinmaygarde avatar Jun 17 '24 20:06 chinmaygarde

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

jonahwilliams avatar Jun 17 '24 20:06 jonahwilliams

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 avatar Jun 17 '24 20:06 chinmaygarde

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.

chinmaygarde avatar Jun 17 '24 20:06 chinmaygarde

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

jonahwilliams avatar Jun 17 '24 22:06 jonahwilliams

Yes, but the images themselves should have AFRC enabled.

chinmaygarde avatar Jun 17 '24 22:06 chinmaygarde

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

chinmaygarde avatar Jun 17 '24 22:06 chinmaygarde

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.

jonahwilliams avatar Jun 17 '24 22:06 jonahwilliams

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.

chinmaygarde avatar Jun 17 '24 22:06 chinmaygarde

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

chinmaygarde avatar Jun 27 '24 20:06 chinmaygarde

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

chinmaygarde avatar Jul 03 '24 18:07 chinmaygarde

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

chinmaygarde avatar Jul 11 '24 20:07 chinmaygarde