Skip to content

tiffload: add 16-bit float support #3626

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

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

DarthSim
Copy link
Contributor

Hey there 👋

This PR adds support for 16-bit float TIFFs. I didn't use _Float16 because it's not generally available.

Here are some test images: tiff3_f16_f32.zip

@kmilos
Copy link

kmilos commented Aug 28, 2023

I didn't use _Float16 because it's not generally available.

You could additionally introduce a dependency on Imath and let it handle the platform specific/intrinsic stuff if performance is the ultimate goal? And leave this non-Imath fallback if dependency is not wanted/available...

@DarthSim
Copy link
Contributor Author

@kmilos I believe it's a little bit too much for a single rare case 🙂

@jcupitt
Copy link
Member

jcupitt commented Aug 28, 2023

Ah good point. We already have imath as an optional dependency via openexr, so I think just adding #ifdef HAVE_OPENEXR would be enough to let you use imath_float_to_half() and imath_half_to_float().

Does anyone know if imath has runtime dispatch? Ie. the SSE2 / AVX512 / whatever path is picked at runtime?

@kmilos
Copy link

kmilos commented Aug 28, 2023

Does anyone know if imath has runtime dispatch? Ie. the SSE2 / AVX512 / whatever path is picked at runtime?

I don't think so.

@jcupitt
Copy link
Member

jcupitt commented Aug 28, 2023

I had a quick grep and I think it's probably using a look up table by default:

https://github.com/AcademySoftwareFoundation/Imath/blob/main/src/Imath/half.h#L116

You need to build with extra compiler flags to enable the use of intrinsics. Anyway, it's likely to be quicker than bit twiddling, and we probably already have this LUT inside the openexr loader, so it probably makes sense to use it.

@kmilos
Copy link

kmilos commented Aug 28, 2023

You need to build with extra compiler flags to enable the use of intrinsics.

I guess -march and -mcpu determine if F16C extension is available or not. Been around for a while though...

Anyway, it's likely to be quicker than bit twiddling, and we probably already have this LUT inside the openexr loader, so it probably makes sense to use it.

Some idea of difference is already given by the same header file:

https://github.com/AcademySoftwareFoundation/Imath/blob/4d4e01c8c9fb9b9dbbefa2e135b21778c9cd331e/src/Imath/half.h#L162-L177

@kleisauke
Copy link
Member

Once PR #3618 lands, another option would be to use Highway's PromoteTo() function, which will map to _mm{,256,512}_cvtph_ps() on x86.
https://github.com/google/highway/blob/b39942c49b6e6a24f9dec69b1a02143c8ad21eef/hwy/ops/x86_128-inl.h#L7942-L7945
https://github.com/google/highway/blob/b39942c49b6e6a24f9dec69b1a02143c8ad21eef/hwy/ops/x86_256-inl.h#L6025-L6034
https://github.com/google/highway/blob/b39942c49b6e6a24f9dec69b1a02143c8ad21eef/hwy/ops/x86_512-inl.h#L4624-L4633

This should be safe to use if its availability is verified at both compile- and run time with HWY_HAVE_FLOAT16 / hwy::HaveFloat16().

I might take a look at this, although 16-bit float TIFFs aren't that common.

@DarthSim DarthSim force-pushed the feature/tiff-16-bit-float branch from e695017 to 9203c29 Compare August 28, 2023 16:09
@DarthSim
Copy link
Contributor Author

it's likely to be quicker than bit twiddling

It's definitely quicker. But since 16-bit float TIFF seems to be an extremely rare case, I decided to not go hardcore here.

we probably already have this LUT inside the openexr loader

It uses ImfHalfToFloatArray. We could optionally use it if we have HAVE_OPENEXR defined

@jcupitt
Copy link
Member

jcupitt commented Aug 29, 2023

Ah I keep forgetting about highway, you're right.

But I agree with @DarthSim --- this is not likely to be a hotspot, so just do something simple for now. We can put in a highway path if this function ever shows up on profiles.

@DarthSim DarthSim force-pushed the feature/tiff-16-bit-float branch from 9203c29 to 20c2bcf Compare August 30, 2023 10:54
@DarthSim
Copy link
Contributor Author

Applied some code style fixes

@kleisauke kleisauke merged commit 8dfee77 into libvips:master Sep 21, 2023
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

Successfully merging this pull request may close these issues.

4 participants