Improved HDR support#1124
Conversation
…_ASTC_HDR_4x4_RGBA
MarkCallow
left a comment
MarkCallow
left a comment
There was a problem hiding this comment.
Thank you for the quick work on these issues. I have just the one comment.
I will review the test updates next.
| options.swizzle->c_str()); | ||
| } | ||
|
|
||
| // Add KTXmapRange metadata |
There was a problem hiding this comment.
There needs to be a CLI option to set the range both here and in ktx encode. In here it must be possible to set it for any supported formats, not just UASTC_HDR formats. I think it is not necessary to set it unless the option has been specified.
The spec says
"This metadata entry may be used with:
any floating point format. In the case of a combined depth-stencil format, e.g. D32_SFLOAT_S8_UINT it must be applied only to the depth component.
any linear *_UNORM or *_SNORM format.
It is incompatible with and must not be used with:
non-linear formats such as those with sRGB encoding,
true integer or boolean formats.
If KTXmapRange appears in a KTX file containing these formats, the file is invalid."
There was a problem hiding this comment.
It occurs to me that an explicit option will be hard to use. You have to find the minimum and maximum values in the input file to get the correct values. We should automate this. I have to think about an easy way to achieve this. If the minimum and maximum values are separated by 16 float bits or less then it can be fully automatic. If the input range is greater than representable by 16 bits then some input from the user is needed.
Suggestions welcome.
|
Files changed says 104 cts files were updated but there is no PR in the CTS repo. Where are the changes? |
|
Following our earlier discussion, to speed up the KTX test validation process, we’ve been granted Git permissions to commit the HDR CTS directly to the main branch. The latest commit on CTS repo is b59806b9c524c2b89b35f2d4ec8b8396efd6d53b |
Thank you, @agkar. |
In future please create a branch and a PR from that branch. Only merge to main when the corresponding KTX-Software PR is ready for merge. Putting modified tests in main before the corresponding KTX-Software code is ready impedes other separately developed changes from being merged. |
|
Thank you for fixing the crash. I am going to merge this, despite the unresolved issue of a map-range option, in order to unblock other work. Please make another PR for any map-range related changes. |
Fixes KhronosGroup#1122, KhronosGroup#1119, KhronosGroup#1120 and KhronosGroup#1125. --------- Co-authored-by: agkar <a.gkaravelis@gmail.com> Co-authored-by: Anastasios Gkaravelis <47293854+agkar@users.noreply.github.com>
3 participants
Addressing issues #1122, #1119 and #1120