[Impeller] use lossy texture compression on iOS for decode images and MSAA resolve textures. #40520
Conversation
|
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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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. |
| texture_descriptor.format = pixel_format.value(); | ||
| texture_descriptor.size = {image_info.width(), image_info.height()}; | ||
| texture_descriptor.mip_count = texture_descriptor.size.MipCount(); | ||
| texture_descriptor.compression_type = impeller::CompressionType::kLossy; |
There was a problem hiding this comment.
This will be really unhappy for photos, especially if they're wide gamut.
Some people might not care but we probably need a flag of some sort to control whether you get this or not, at the very least for images.
There was a problem hiding this comment.
The images I've checked look identical with this flag on/off. Do you have any particular ones in mind?
There was a problem hiding this comment.
@gaaclarke probably has some from the wide gamut work
There was a problem hiding this comment.
Flutter gallery has some photographs that are in the adobergb colorspace you can test with those. Also it's worth verifying that it doesn't mess up the gamut, there is an integration test in //flutter/dev/integration_tests/wide_gamut_test/.
Usually where these compression problems come into play in games isn't complex images but really simple images like pixel art. Unity defaults to compressed textures but you have to turn them off frequently if you are using simple images.
There was a problem hiding this comment.
As a stopgap, maybe this should only apply to images that come from a lossless image decoder (e.g. PNG, BMP, lossless WEBP, raw rgba).
There was a problem hiding this comment.
We're not aware of any circumstances where this causes any loss in fidelity
chinmaygarde
left a comment
There was a problem hiding this comment.
IMO, we should have this on by default and assess opportunities where we need configurability.
We already massage the image usage to make sure we opt into lossless compression. While lossless compression is great for bandwidth savings, there are usually (at least with Metal) no memory savings because Metal cannot guarantee any savings without compression and so must allocate an arena big enough for the entire uncompressed texture. In most cases, this is this bigger arena is wasted. We can observe this by comparing the results of lossy vs lossless compression to see cases where they are identical.
Compression doesn't necessarily mean a reduction in gamut so I wouldn't expect wide-gamut images to be uniquely affected by this but we should definitely eyeball the existing test.
I like this change also because this is yet another guard against accidentally messing up the flags that kick us out of lossless compression and cause us to take a bandwidth hit. I care way more about memory bandwidth for our use case that memory (power usage, heat, etc..).
We can file a followup issue to catalog cases where this compression is causing significant deviation from developer intent. My sense is that this is the right thing to do for intermediate and final render targets. But I have no evidence that points to cases where we shouldn't enable this globally.
|
Are we going to land this before the release train? |
|
I'm OK with landing this now |
On iOS devices that support this (apple8+), this dramatically reduces memory usage.
Wonderous after swiping through all wonders at ToT:
Wonderous after swiping through all wonders with patch: