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

Add tonemapper that hashes to a random color #234

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

a-e-k
Copy link

@a-e-k a-e-k commented Jul 12, 2024

Hi Thomas,

Thanks for this fantastic image viewer. I have gotten a lot of use out of it over the last few years.

This PR adds a new tonemapping option that simply hashes input values to random(ish) colors.

Rationale:

My primary motivation was in viewing uint id channels (e.g., obj id, prim id, or mat id), where it's useful to show a solid block of color for each id but to decorrelate the colors of adjacent id values. However, in testing it on non-id images, I found that it was also useful for revealing regions of constant color -- in particular clipped blacks and whites on LDR images.

Hash:

The hash function itself is based on a popular hash function for shaders floating around on the web. I have modified it to map a vec3 to a vec3. I have also replaced the constants with random floating point values whose mantissas are prime numbers with not-too-clumpy bit patterns (no three 0's or 1's in a row), just to add what I hope is a bit better mixing.

I would have preferred to have used one of the better hash functions from this JCGT paper, but I also did not want to add a dependency on a shader version that supports something like floatBitsToUint(). For a simple visual decorrelation, I think this is sufficient.

Testing:

I have tested this both on an NVIDIA card under Fedora (KDE Plasma on Wayland) with the OpenGL and GLES backends, and on AMD integrated graphics under Windows (Ubuntu on WSL2) with just the OpenGL backend.

I have not tested it on native Windows with MSVC, nor on a Mac with Metal. However, I have tried to match the style of the rest of the Metal shader, so hopefully it just works.

@a-e-k a-e-k force-pushed the aek/hash branch 2 times, most recently from 5f7a741 to 8a0a3ca Compare July 12, 2024 07:59
@Tom94
Copy link
Owner

Tom94 commented Jul 12, 2024

Hi Andrew, thanks a bunch for the contribution! This sounds neat to have (though I didn't test it yet).

Could you also implement a CPU-side version of the tonemap in ImageCanvas::applyTonemap? This is used to save images (to disk or to clipboard) and should ideally match the GPU side. I am slightly concerned about shader-side optimizations (like fp reordering or lower precision) causing significant deviations in hash function behavior -- fingers crossed.

@a-e-k a-e-k force-pushed the aek/hash branch 2 times, most recently from 0e192b3 to 4258fec Compare July 23, 2024 09:17
@a-e-k
Copy link
Author

a-e-k commented Jul 23, 2024

Hello again Thomas,

Sorry for the delay. I have been tinkering with hash functions. You were correct to be concerned about deviations in the hash function behavior. I have updated the branch with two possible solutions, one in each commit.

The first commit changes the original hash function up a bit and replaces the sin() and fract() functions with high frequency triangle waves. The triangle wave seems a bit more portable with respect to precision than a sine. And replacing the plain fract with something continuous makes it more tolerant of small deviations (whereas before the values could noticeably roll over). I have verified that CPU-side and GPU-side implementations visually match under typical settings (at least with a high-end desktop GPU.) The down side is that the hash function breaks down below about -5 exposure and above +11 exposure (on LDR images), and the CPU and GPU results diverge near this point.

The second commit replaces the original function with an implementation of PCG3D from the Jarzynski and Olano. This gives me rock-solid matching results between the CPU-side and the GPU-side when using the GLSL backend, even with crazy exposures like +40 or -40. Nor does the hash break down at those exposures. However, I do see differing hashed colors with the GLES backend. The downside of this option was that I had to bump the ubershader to GLSL 3.3/3.0 ES to use floatBitsToUint() and the bit arithmetic.

If you prefer the second option, I'm happy to rework this branch to split the bumping of the ubershader language version from the addition of the hash tonemapper. (I'd also understand if you wish to decline this PR at this time.)

@Tom94
Copy link
Owner

Tom94 commented Jul 23, 2024

Hi Andrew, thank you very much for the follow-up. That's so much extra effort you've expended!

In a vacuum I'd prefer the PCG solution, but you bring up the downsides yourself... I'm really hesitant to bump GL and GLES versions since some people use tev on ancient machines (or, say, VMs with virtual drivers that only support ancient GL / GLES versions). Plus, the discrepancy you report between GLES and GL bothers me more than the exposure limits on the "tamer" hash function you came up with.

Long story short: I'd love to go with your first proposed option. I don't think it's a big deal that the hash function breaks down at very low or very high exposures -- at least for the purposes that you initially implemented the hash tonemap for -- and there seems to be no other downside from your description.

Once again, thanks for all the time you put into this. Lmk once the PR is updated to reflect what you'd like me to merge!

@a-e-k a-e-k force-pushed the aek/hash branch 2 times, most recently from 6274f3c to 1964b05 Compare August 9, 2024 09:39
@a-e-k
Copy link
Author

a-e-k commented Aug 11, 2024

Hi Tom,

I think it's now in a good state for reviewing, testing, and merging. Compared to my last go-round:

  • It now scales inputs greater than or equal 1024 by 1/1024, up to three times. On my low-LDR testing, this now allows it to handle exposure values up to 30+, which should be enough to accommodate any integers channels. I had experimented with log-mapping the inputs instead, but couldn't get it to always match reliably. So this is sort of a "cheap" log that seems more consistent. Testing on WideFloatRange.exr, it now appears to behave reasonably well on raw input values from about 3.6e-4 to 1.3e12.
  • I've modified some of the constants to try to get what seems to like a better mix. Note that the hash function does give Moire patterns on some inputs, such as WideColorGamut.exr. That's mostly just a limitation of trying to create a GLES SL 1.0 compatible function that can match the CPU output. Testing on more typical photographic and digitally painted images, it seems to give the expected random looking dot patterns. (Except where JPGs get blocky. :-)
  • The preamble for GLES now sets highp precision on the sampler2Ds. I found that this was necessary on my machine to get the GLES output to match GL and the CPU output in all cases. Otherwise, the values being read from the image were just inexact enough to cause differences when hashed.

@Tom94
Copy link
Owner

Tom94 commented Aug 20, 2024

Hi Andrew, thanks once again for the thorough revision!

I was about to merge, but while testing I noticed that mip mapping ruins the effect of the hash function if the zoom level is even slightly below 1.

The obvious fix would be to disable all texture interpolation while hash tonemapping is enabled, but this turns out to involve changes to nanogui internals which weren't programmed with dynamically changing texture parameters in mind. I'm happy to make those changes myself, but don't yet know when I'll get to it.

(FWIW: I also considered explicit shader-side LoD selection & clamping to texel centers, but GLES 2.0 sadly doesn't support this.)

@a-e-k
Copy link
Author

a-e-k commented Sep 3, 2024

Thanks, Thomas. I'll confess that when testing, I'd mostly been using either 1:1 pixels, or zoomed in on my test images, rather than zoomed out.

In any event, there's no rush. I'm happy to keep this PR open as long as needed. (I usually build this tool from source rather than use the pre-built releases,, so building it from my own branch is no problem until then.)

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.

2 participants