Add Image Compression Control Sample (no-op merge, please ignore)

JoseEmilio-ARM commented 8 months ago

:warning: Due to issues with GitHub, the PR was closed with the incorrect label 'merged' (the only commit is already present in main) and cannot be re-opened, so moved the review to a new PR: #979

Description

This sample showcases how to control image compression, in particular framebuffer attachments and the swapchain. The use case is a simple post-processing effect (chromatic aberration) applied to the output color of a forward-rendering pass. The developer can select "Default" compression, "Fixed rate" compression, or "None". The sample shows the impact each option has on bandwidth (write bytes counter) and memory footprint (image sizes once allocated). More info in the tutorial: samples/performance/image_compression_control/README.adoc

image_compression_control

Fixes #882

Also added a small change to the AFBC sample to use the new extension if available, to confirm that AFBC is being used.

Framework Changes

Image class:

Added helper functions to query for compression support, apply compression, and check that compression was applied. Also to return image size (to display footprint).

Swapchain class:

Similarly, added helper functions to query for compression support, apply compression, and check that compression was applied, for the swapchain images. This also requires exposing this control to the Render Context class.

PhysicalDevice/Device classes:

Moved is_extension_supported responsibilities to the PhysicalDevice class, so that we can check if an extension is supported (and therefore if we can request its features) before the Device is created.

String utils:

Added a few more enum-to-string functions for image flags.

General Checklist:

Please ensure the following points are checked:

[x] My code follows the coding style
[x] I have reviewed file licenses
[x] I have commented any added functions (in line with Doxygen)
[x] I have commented any code that could be hard to understand
[x] My changes do not add any new compiler warnings
[x] My changes do not add any new validation layer errors or warnings
[x] I have used existing framework/helper functions where possible
[x] My changes do not add any regressions
[x] I have tested every sample to ensure everything runs correctly
[x] This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:
- [x] I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
- [x] My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions. Tested on Windows and Android.

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

[x] I have tested the sample on at least one compliant Vulkan implementation
[x] If the sample is vendor-specific, I have tagged it appropriately
[x] I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
[x] Any dependent assets have been merged and published in downstream modules
[x] For new samples, I have added a paragraph with a summary to the appropriate chapter in the readme of the folder that the sample belongs to e.g. api samples readme
[x] For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
[x] For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

SaschaWillems commented 8 months ago

The crash on my setup is most probably caused by the lack of support for VK_EXT_image_compression_control_swapchain. But in that case, the sample shouldn't crash, but rather fail gracefully by e.g. reporting the extension as unsupported.

JoseEmilio-ARM commented 8 months ago

The crash on my setup is most probably caused by the lack of support for VK_EXT_image_compression_control_swapchain. But in that case, the sample shouldn't crash, but rather fail gracefully by e.g. reporting the extension as unsupported.

Thanks a lot for reviewing and testing 🙏 I tried it on Windows too (Quadro T2000) and I cannot reproduce the crash or the validation messages (SDK 1.3.250.1), do you get to see the logs before the crash? The extensions are marked as optional, so I get

[warning] Optional device extension VK_EXT_image_compression_control not available, some features may be disabled
[warning] Optional device extension VK_EXT_image_compression_control_swapchain not available, some features may be disabled

Then the sample should report that the extensions are not supported like this:

You think my drivers are more permissive, and perhaps there's a bug in request_extension_features? or maybe we are expected to guard the calls in request_gpu_features with checks like if (device->is_enabled(VK_EXT_IMAGE_COMPRESSION_CONTROL_EXTENSION_NAME))?

JoseEmilio-ARM commented 8 months ago

or maybe we are expected to guard the calls in request_gpu_features with checks

~~Added a small patch to test this theory, @SaschaWillems please let me know if you have a chance to test again, thanks!~~

My bad, that seems to break things for me. Looking at other samples, they all request features this way, not sure what the issue is with this particular extension and only on some platforms 🤔 any ideas?

SaschaWillems commented 8 months ago

I'm on a more recent build of the layers (built from source), and they report an error instead of a warning. This may cause the device to not get created.

Will check with layers from the latest SDK and report back.

SaschaWillems commented 8 months ago

With SDK 1.3.275 I'm not getting any validation errors, I still get the assertion I posted above. If I ignore it, the sample will run though. So maybe something isn't properly initialized at first start? May be caused by what I described in #920.

JoseEmilio-ARM commented 8 months ago

With SDK 1.3.275 I'm not getting any validation errors, I still get the assertion I posted above. If I ignore it, the sample will run though. So maybe something isn't properly initialized at first start? May be caused by what I described in #920.

Thank you very much for checking, I'll look into it, although for some reason I cannot reproduce the assertion. Out of curiosity, do you get the same issue with other samples that require an extension that your platform does not support?

SaschaWillems commented 8 months ago

Hard to test, as my GPU supports all extension we use in our samples. But "faking" this by requesting an unsupported extension in a sample looks to work properly. If I request an unsupported extension in an api based sample it displays an error and closes gracefully. Maybe it's something related to the ARM framework class after all?

This is where the error happens:

It triggers the assert, but only on the first frame of the sample. So I guess the first frame of the sample tries to access something not (yet) created. Hence why it works if I skip this. For the next frame, everything is correct.

JoseEmilio-ARM commented 8 months ago

It triggers the assert, but only on the first frame of the sample. So I guess the first frame of the sample tries to access something not (yet) created. Hence why it works if I skip this. For the next frame, everything is correct.

It could well be a framework issue, but finding it hard to debug without being able to reproduce. I did a clean-build on a different PC with a different GPU (RTX 3070) and the sample still works fine 🤔

On the other hand I found that the new Swapchain::query_supported_fixed_rate_compression was using extension features, so added a new guard against it (see latest commit). Any chance you could try again? and if so, could you share the sample's LOGs up to the point of the assertion?

SaschaWillems commented 8 months ago

The assert on the first frame is sadly still present.

What's odd: The last three lines are

[info] Depth format selected: VK_FORMAT_D32_SFLOAT
[info] Depth format selected: VK_FORMAT_D32_SFLOAT
[info] Depth format selected: VK_FORMAT_D32_SFLOAT

Not sure why the sample (or framework) does that three times in a row.

I do see an error on the log though:

[error] [framework\core\swapchain.cpp:645] (Swapchain) To query fixed-rate compression support, device extension VK_EXT_image_compression_control_swapchain must be enabled

Here is the log up to the first assert: log_tex_compression.txt

JoseEmilio-ARM commented 8 months ago

@SaschaWillems thank you very much for your help, the logs allowed me to find a few more issues. Also realized I was testing release builds and that's why I could not reproduce the assertion, apologies!

I think both the assertion and the validation layer error should now be fixed.

The repeated logs about the selected depth format are expected, I think they are related to the fact that we create a render target per RenderFrame in flight, which at the moment is one per swapchain image. Agree this should be improved as we probably do not need more than 2.

gary-sweet commented 8 months ago

This works well for me. We only support one level of compression on our hardware, so the low/high selector in the UI just appears to do nothing. Would it be sensible to hide that UI selector if only one level is available?

JoseEmilio-ARM commented 8 months ago

This works well for me. We only support one level of compression on our hardware, so the low/high selector in the UI just appears to do nothing. Would it be sensible to hide that UI selector if only one level is available?

Thank you very much for testing! Good point, certainly. Please let me know if my last commit works as expected.

JoseEmilio-ARM commented 8 months ago

@jherico I updated the Image and Swapchain classes in this PR (so that their constructors can take the new structures, introduced by the compression control extensions, in their pNext chains)

I had to rebase the Image changes now that #906 was merged. What do you think of how I updated the Image using the new Builder? It seems that #973 will conflict too...

JoseEmilio-ARM commented 8 months ago

LGTM, but bear in mind I'm not a Khronos member, so I don't think my review technically carries any weight.

Thank you very much! Your suggestions made it a much better PR 🙇

JoseEmilio-ARM commented 8 months ago

Apologies, for some reason, after the rebase, the PR kept picking up #910 as a change, even though it is the head of main right now. I tried another force-push with just that commit, and the PR was automatically closed. Does anyone know if it possible to re-open it? Otherwise, I'll create a new PR, sorry for the inconvenience!

KhronosGroup / Vulkan-Samples