Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ktx create not doing verbatim copy of EXR half_float. Also "Not enough precision" error message is unclear. #970

Open
MarkCallow opened this issue Jan 1, 2025 · 3 comments
Assignees

Comments

@MarkCallow
Copy link
Collaborator

When loading an EXR file with half float channels and --format R32B32B32A32_SFLOAT ktx create raises an error

Not enough precision to convert 16 bit input to 32 bit output for R32B32B32A32_SFLOAT.

However ExrInput::ReadImage happily accepts a targetBitDepth of 32 which it passes on to LoadEXRImageFromMemory which returns float values. If I comment out this code

if (inputBitDepth < bitDepth)
where the error is raised then ktx create successfully creates the texture. So two questions:

  1. Why is it attempting to convert to float when it already has float? It requested float and received float.
  2. "Not enough precision"? TinyEXR is managing to do the conversion. Is this poor error message wording and ktx create simply does not implement such conversions?

The half-float file I've been using for testing is https://raw.githubusercontent.com/AcademySoftwareFoundation/openexr-images/main/ScanLines/Blobbies.jpg.

@aqnuep
Copy link
Collaborator

aqnuep commented Jan 3, 2025

It was an explicit requirement for the tool to do verbatim copies on create/extract, that is for a 16-bit float input file one must use a 16-bit float format. Pretty much it was a general goal to have 1:1 mapping whenever possible (there are exceptions like depth/stencil, packed float, certain integer, etc. formats, of course). Sure, it would be possible to convert, both up (FP32) or down (e.g. UNORM), so if it is deemed preferable to do conversion then it should be straightforward to add support.

@aqnuep aqnuep assigned MarkCallow and unassigned aqnuep Jan 3, 2025
@MarkCallow
Copy link
Collaborator Author

MarkCallow commented Jan 5, 2025

Verbatim copies is fine. With that in mind there are two issues with the current code:

  1. A verbatim copy is not being done. The half float input is being converted to float by TinyEXR then it is converted back to half-float by CommandCreate::convert. Float is being requested due to
    case ImageInputFormatType::exr_float:
    image = std::make_unique<rgba32fimage>(width, height);
    loadFormat = createFormatDescriptor(VK_FORMAT_R32G32B32A32_SFLOAT, *this);
    break;
    inputImageFormat is exr_float because of
    case TINYEXR_PIXELTYPE_HALF:
    bitDepth = std::max(bitDepth, 16u);
    formatType = ImageInputFormatType::exr_float;
    qualifiers = KHR_DF_SAMPLE_DATATYPE_SIGNED | KHR_DF_SAMPLE_DATATYPE_FLOAT;
    break;
    No exr_half type is declared. Probably one needs to be added.
  2. The error message is misleading. My first reaction was "Why doesn't 16-bit input have enough precision to convert to 32-bit output? Something is not right in the code." Due to time pressure I immediately stopped thinking about it and used a half-float output format. Later I returned to investigate. The message must more clearly indicate that a different output format should be used.

I'll update the title of this issue.

@MarkCallow MarkCallow changed the title ktx create "Not enough precision" check flawed ktx create not doing verbatim copy of EXR half_float. Also "Not enough precision" error message is unclear. Jan 5, 2025
@MarkCallow MarkCallow assigned aqnuep and unassigned MarkCallow Jan 5, 2025
@aqnuep
Copy link
Collaborator

aqnuep commented Jan 6, 2025

The reason why it is loaded as a FP32 is that some other parameters may require transforming the data in various fashions (e.g. generate mipmaps) and there's no native FP16 support to be able to implement it on the CPU, particularly in the image manipulation library KTX inherited from basisu. Adding that would require pulling in an IEEE compliant half library that is actually able to do math on halfs and even that would be less efficient than the current solution.

Functionally, this works today, i.e. you're able to create R16G16B16A16_SFLOAT from an FP16 EXR.

If you'd like to add "native" (of course, emulated) FP16 support to the imaging stack, you can use a library like the following:
https://github.com/ROCm/HIP-CPU/blob/master/external/half/half.hpp

@aqnuep aqnuep assigned MarkCallow and unassigned aqnuep Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants