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

WebGLRenderer: Add support for AgX Tone Mapping #27366

Merged
merged 20 commits into from
Dec 19, 2023

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Dec 12, 2023

Fix #27362
Related to #26479

Description

Adds support for AGX Tone Mapping based on the implementation from this article.

Do we know of any defacto test images for AGX to make sure it looks right?

Here are some comparison screenshots when env map intensity is set to 3.0:

Tone Mapping ScreenShot
None image
ACES image
AGX image

Copy link

github-actions bot commented Dec 12, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
669 kB (166.1 kB) 670.7 kB (166.8 kB) +1.73 kB

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
449.8 kB (109 kB) 451.5 kB (109.7 kB) +1.7 kB

@gkjohnson gkjohnson requested a review from donmccurdy December 12, 2023 11:51
@gkjohnson gkjohnson added this to the r160 milestone Dec 12, 2023
@gkjohnson gkjohnson changed the title WebGLRendere: Add support for AGX Tone Mapping WebGLRenderer: Add support for AGX Tone Mapping Dec 12, 2023
@gkjohnson gkjohnson changed the title WebGLRenderer: Add support for AGX Tone Mapping WebGLRenderer: Add support for AgX Tone Mapping Dec 12, 2023
src/constants.js Outdated Show resolved Hide resolved
test/unit/src/constants.tests.js Outdated Show resolved Hide resolved
@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 12, 2023

Adds support for AGX Tone Mapping based on the implementation from this article.

We can look at Filament as well, see google/filament#7236. It does reference the same article, perhaps they are effectively the same.

Do we want to support the two looks, "Punchy" and "Golden", as well? These are represented in Blender by selecting the AgX view transform and then one of the looks. But if we want this in three.js, we could represent them simply as three enums. Perhaps:

  • AgXToneMapping or AgXDefaultToneMapping
  • AgXPunchyToneMapping
  • AgXGoldenToneMapping

I don't feel strongly whether we need the looks in this PR. But it might be nice to name the default something we'll still be happy with if we add them later.


Do we know of any defacto test images for AGX to make sure it looks right?

In the past I've compared tone mapping implementations by creating a Linear-sRGB OpenEXR 'source' image, then running it through Blender's tone mappers using the OCIO CLI, and tone mapping it in three.js using EXRLoader and MeshBasicMaterial. The results should match exactly, other than very slight differences in our approximation and the LUTs. I'll share a sample 'source' image and reference renders from OCIO, a bit later.

EDIT: There are these common reference images too: https://projects.blender.org/blender/blender/pulls/106355 ... I've seen the source images somewhere, but will have to dig around for those.

@donmccurdy
Copy link
Collaborator

I don't feel strongly whether we need the looks in this PR...

On second thought, let's not add looks in this PR — Blender 4.0 stable does not represent them as it did in the alpha/beta releases, Golden is removed, and we may need to look closer at what has changed.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Dec 12, 2023

We can look at Filament as well, see google/filament#7236. It does reference the same article, perhaps they are effectively the same.

Thanks! I think I'd prefer to avoid digging through their rendition of it especially If they're referencing the same implementation. But I'd definitely like to get some test images to make sure it matches the other implementations.

Do we want to support the two looks, "Punchy" and "Golden", as well? These are represented in Blender by selecting the AgX view transform and then one of the looks.

I'll skip adding them in this PR but even if we do long term I'm okay with AgXToneMapping, AgXPunchyToneMapping, & AgXGoldenToneMapping.

EDIT: There are these common reference images too: https://projects.blender.org/blender/blender/pulls/106355 ... I've seen the source images somewhere, but will have to dig around for those.

I saw at least one of these images in filament's PR, as well. If these are canonical test images it would be nice to know where they're from.

Unfortunately I'm not all that fluent in Blender so making my own versions through Blender may be difficult.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 12, 2023

If these are canonical test images it would be nice to know where they're from.

Here's the source, by the author of AgX:

https://github.com/sobotka/Testing_Imagery

Those are source files, mostly in Linear-sRGB. I'd stay out of this repository's subfolders. The images are not tone-mapped, but loading them with EXRLoader and setting .colorSpace = LinearSRGBColorSpace on MeshBasicMaterial (with tone mapping enabled) should work. You can see the expected results in the Blender PRs, or I can create references tone mapped with Blender 4.0's AgX implementation later.

@WestLangley
Copy link
Collaborator

Here are some comparison screenshots when env map intensity is set to 3.0:

Can you please explain why you increased the env map intensity? (I assume you felt it was necessary.) Tone mapping will not "fix" an over-exposed scene.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 12, 2023

I think it's useful here for illustration purposes — without tone mapping we see the "Notorious Six" begin to appear. Whether the image is over-exposed is a more subjective judgment, and I don't feel strongly without more context than an isolated model. If we limit our examples to only use exposure ranges that look nice without tone mapping, that would be a mistake.

@WestLangley
Copy link
Collaborator

without tone mapping we see the "Notorious Six" begin to appear
If we limit our examples to only use exposure ranges that look nice without tone mapping, that would be a mistake.

I did not mean to suggest we do either of those.

Tone mapping requires the scene to be (at least reasonably) pre-exposed. It is not useful to compare tone mapping algorithms on a test case that is over-exposed.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 12, 2023

Agreed! But, I don't consider .envMapIntensity=3.0 to indicate the image is over-exposed. Comparing tone mapping does require that we consider a wider range of input than we otherwise might. Increasing or decreasing .envMapIntensity is a quick way to do that. An image is over-exposed when it looks over-exposed. That we can increase light intensity somewhat, without immediately and obviously over-exposing the resulting image, is a goal.

But I assume you are maybe trying to get at questions about the relative brightness of images produced with AgX and ACES Filmic, and whether we should make adjustments there as we did with ACES Filmic for the typical display environments of three.js users? I agree if so, and I think the OpenEXR test imagery will be helpful in that discussion.

@WestLangley
Copy link
Collaborator

@donmccurdy I think we are concerned about the same things. :-) But I was particularly interested if @gkjohnson had a reason for setting the env intensity so high. Did he compensate for that by reducing exposure in his test case?

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Dec 13, 2023

It's as Don suggested - I felt it was more interesting to show the tone mapping performed with brighter values. No tonemapping exposure adjustment was done and it was increased by using envMapIntensity.

Tone mapping will not "fix" an over-exposed scene.

I never suggested this was the case. I'm not sure what you're meaning by this.

Here's the source, by the author of AgX:

https://github.com/sobotka/Testing_Imagery

Thanks! I'll take a look at these later and use the Blender versions to compare.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 13, 2023

I reached out to Troy about AgX's intended viewing conditions. There's more to the thread that is useful, but a short summary of the immediately-relevant parts might be:

  1. AgX is designed with a room of "average" brightness in mind
  2. Increased contrast may be desirable to accommodate a darker viewing environment, or for stylistic reasons

https://fosstodon.org/@donmccurdy/111570779516336527

In most AgX implementations, "looks" are exposed as convenient ways to customize contrast and other characteristics of the tone mapper.

@gkjohnson
Copy link
Collaborator Author

I'm seeing some differences between the comparison images here. I'll look through the code to make sure I didn't flub any of the port and other settings are flipped somewhere. Also @donmccurdy - that folder only seemed to contain one of the images from the Blender PR. Do you know where the others came from?

It might also be helpful to have a few sample images from blender that we generate so we know all the settings used. Would it be possible to generate a few samples from the EXR images using Blender for this?

Linear ACES AgX Comparison Image Comparison source
image image image image Filament PR
image image image Blender PR
image image image image Blender PR

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 13, 2023

I think I've seen two of these (the bar scene and the underwater scene) before in AgX tests. Perhaps the rest are Blender's own tests, I'm not sure. In any case, we can run Blender's own color management transforms on the files from this repository.

Steps on macOS:

outdated shell commands, see later posts
# Install OpenColorIO (OCIO) and OpenImageIO (OIIO).
brew install opencolorio openimageio

# Save path to Blender's OCIO config to an alias.
OCIO=/Applications/Blender.app/Contents/Resources/4.0/datafiles/colormanagement/config.ocio

# Standard (no tone mapping).
oiiotool --colorconfig "$OCIO" -i "blue_bar_709.exr" --colorconvert "Linear Rec.709" "sRGB" -o ./out/blue_bar_standard.png

# AgX, Medium Contrast.
oiiotool --colorconfig "$OCIO" -i "blue_bar_709.exr" --colorconvert "Linear Rec.709" "AgX Base sRGB" --ociolook "Medium Contrast" -o ./out/blue_bar_agx.png

I don't know the color spaces of these images unless identified by the filename, so I'm limiting my tests to those with "Rec 709" or "BT 709" in the name. These are Linear-sRGB, for our purposes.

[ invalid screenshots removed, see updated versions below ]

@EaryChow
Copy link

EaryChow commented Dec 13, 2023

I think I've seen two of these (the bar scene and the underwater scene) before in AgX tests. Perhaps the rest are Blender's own tests

The EXRs were attached to this post:
https://projects.blender.org/blender/blender/pulls/106355#issuecomment-984579

https://projects.blender.org/attachments/8ab6aa53-418a-4d72-89be-cce7e36d3dd0
https://projects.blender.org/attachments/a0a63b21-efaa-446e-99a6-414cd4195e77
https://projects.blender.org/attachments/695e0f5d-c735-4bd3-96de-916953c1643e
https://projects.blender.org/attachments/e5e8689b-f51e-46b8-b5f7-7d0d246e3c20
https://projects.blender.org/attachments/f9f3a886-095b-45d9-88ce-6a83677574ad
https://projects.blender.org/attachments/fc3b88fe-4ac0-414f-9201-2036448aeb15

Also if you are looking at the EXRs in Blender, do it in the compositor, don't do it in EEVEE with emission plane. EEVEE clips the negatives.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Dec 16, 2023

I've ported Filaments implementation in this PR, now, which converts our rec 709 Linear sRGB input colors to rec2020 Linear colors using the transform matrix from this paper, performs the AgX conversion, then converts back to rec 709.

Generally I think the image looks better - as in the relative colors more closely reflect Blenders image and the color transitions on the cheek are smoother. The color is still a bit more washed out, though, which makes me think the Blender "guard rails" and luminance compensation are doing a good amount of work. @EaryChow Would that explain the differences in the Blender and Filament port images?

Blender Iolite Engine Port Filament Port

There is also a "Lower Guard Rail" step added to deal with negative values. The approach we used was simply offsetting the values upwards until there is no negative, at this point the overall luminance is also brightened, so we scale the luminance back down.

Is there any write up or documentation what the "guard rail" and luminance compensation are doing and why? From looking around at this post a bit I'm assuming no, but maybe that's changed. Unfortunately I'm not so familiar with python and associated libraries so a more abstract write up would be easier understand first if it's available.

That said, though, it doesn't look like Filament implements the guard rail approach at all.

@gkjohnson
Copy link
Collaborator Author

Also, regarding the failed tests - does CI run on WebGL 1? The tests that are failing should work in WebGL 2. It says that a transpose and inverse function are not supported.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 16, 2023

The PR in its current state indeed breaks any tone mapping usage in WebGL 1. I've tested that with WebGL1Renderer. Any tone mapping technique will trigger the compilation error.

At least up to r163 we have to make sure to support WebGL 1.

Maybe you can use something like the following to support GLSL < 3.0.

#ifndef inverse
    #define inverse( mat3 ) inverseFallback( mat3 )
#endif

#ifndef transpose
    #define transpose( mat3 ) transposeFallback( mat3 )
#endif

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 16, 2023

@gkjohnson I think we've diverged on the references somehow. Your Blender reference image for "red_xmas_rec709" above and in #27366 (comment) doesn't match my own image from #27366 (comment) (see script at end of that post), perhaps others as well.

In case my previous shell scripts — accidentally using Looks from older Blender Filmic — were leading things astray, I've pushed my script to a gist I can update more easily:

https://gist.github.com/donmccurdy/ca1d7cbb107435b40fa510f783f91ee8

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Dec 17, 2023

@donmccurdy

I think we've diverged on the references somehow. Your Blender reference image for "red_xmas_rec709" above and in #27366 (comment) doesn't match my own image from #27366 (comment) (see script at end of that post), perhaps others as well.

Thanks for the catch. I've been copy and pasting the image from your comment which seems to be changing the resulting color of the image for some reason - at least the reds are becoming brighter. I've updated both the previous comments to use the direct URLs from the previous comments to make sure the images are the same, now. The colors are still a bit washed out compared to the latest commit but less so.

And an example of the color changing:

Original URL Copy / Pasted

@Mugen87

The PR in its current state indeed breaks any tone mapping usage in WebGL 1. I've tested that with WebGL1Renderer. Any tone mapping technique will trigger the compilation error.

I can fix the PR when the results are good enough - I was just surprised to see that the CI was using WebGL 1. It seems like it could cause problems when WebGL1 support is dropped or cause rendering discrepancies.

@EaryChow
Copy link

EaryChow commented Dec 17, 2023

Would that explain the differences in the Blender and Filament port images?

I have already posted the sorce code of Blender implementation:
https://github.com/EaryChow/AgX_LUT_Gen

which converts our rec 709 Linear sRGB input colors to rec2020 Linear colors using the transform matrix from this paper, performs the AgX conversion, then converts back to rec 709.

I believe you haven't implemented any rotation of primaries as part of the inset yet, judging from your previous blue bar example.

The result is that, previously with Linear Rec.709 working primaries, the Rec.709 R, G, B, three strips from the sweep will be attenuated along chromaticity-linear paths, resulting in things like Abney Effect. You can take a look at your blue bar example, though there is also an issue of wider gamut, Abney Effect is one of the main reasons the blue looks so "purple".

And now as we changed the working primaries to Rec.2020, the Notorious Six skewing is now coupled with the descrepency between primaries, the effect will be now amplified. Now only Rec.2020's R, G, B will be attenuated chromaticity-linearly. And Rec.2020 primaries have different chromaticity angles compare with Rec.709:
image

Therefore, following per-channel mechanism's Notorious Six behavior, Rec.709 blue will be skewed towards magenta, Green will be twowards yellow, etc.. Some of this slewings might be desireable, some might be not, so we need a handle of control.

The solution we applied is that, along with the inset, we also do a rotation of primaries, for example, rotate the blue towards cyan a little bit, so that it doesn't go to magenta. How much R, G, B each should rotate is a matter of taste and testings on various EXRs. Therefore you would see different implentations of AgX tend to have their own fine tuned inset-and-rotation matrix.

Again, the matrix used by Blender is included in the scripts I shared. Specifically from AgXBaseRec2020.py:

# inset matrix from Troy's SB2383 script, setting is rotate = [3.0, -1, -2.0], inset = [0.4, 0.22, 0.13]
# link to the script: https://github.com/sobotka/SB2383-Configuration-Generation/blob/main/generate_config.py
# the relevant part is at line 88 and 89
inset_matrix = numpy.array([[0.856627153315983, 0.0951212405381588, 0.0482516061458583],
                            [0.137318972929847, 0.761241990602591, 0.101439036467562],
                            [0.11189821299995, 0.0767994186031903, 0.811302368396859]])

This is the matrix implemented in Blender, to be used in Linear Rec.2020. Again, different implementations of AgX tend to use their own tested inset-and-rotation matrix, so this is just "my tested matrix", not "The Matrix". Feel free to generate your own matrix from the SB2383 script and fine tune it against as many challenging EXRs as possible.

Apart from that, now we have moved the working primaries to Rec.2020, the image that AgX forms out of the box is now in Rec.2020 display hardware medium. It will contain values outside of Rec.709 display medium. Direct color matrix transform from Rec.2020 to Rec.709 will be equivalent to a hard clamping, which may not be desireable at cases. In this case, yes we used the lower guard rail for that.

Is there any write up or documentation what the "guard rail" and luminance compensation are doing and why? From looking around at this post a bit I'm assuming no, but maybe that's changed

The simple explaination is basically:

  1. Use a simple unclipped transform to the target primaries (ignore this step if pre-formation)
  2. Calculate luminance of the source tristimulus, store it for later.
  3. Calculate the lowest value of the tristimulus, if that value is positive, output offset as 0, if value is negative, output the abs() of the value as offset.
  4. Source tristimulus + offset, now there is no negative. But everything has been brightened.
  5. Calculate the post-offset luminance, scale the offsetted stimulus down until it's no longer brighter than pre-offset luminance (that we stored ealier).

That's basically what Sakari did in the script that he shared. I ended up adding a few lines to deal with real-camera footages that have negative luminance error. But overall it still pretty much works the same way.

For reference, we used the Lower Guard Rail pre-formation in Linear Rec.2020, and then post-Rec.2020-formation as we try to produce the images for other display hardware mediums.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Dec 19, 2023

@EaryChow Thanks for your explanations! The latest implement is based on Filament's version which is in turn based on Blenders so the inset and outset matrices are the same as the ones you've referenced.


I've discussed with @donmccurdy offline and I think the current state of the PR is good to merge. The implementation is currently based on Filament's real time implementation and the look mostly matches the Blender results aside from brightness. Unfortunately because Blender's "guard rail" approach to accommodate negative values requires analyzing the full image it's not something we can easily fit into the current three.js fragment-based tone mapping system - Filament doesn't support this either. I think we can leave it for future work potentially.

To round things out here's the final set of image comparisons:

three.js AgX Blender AgX
red_xmas red_xmas_rec709_agx
blue_bar blue_bar_709_agx
Matas_Alexa Matas_Alexa_Mini_sample_BT709_agx
mery_lightsaber mery_lightSaber_lin_srgb_agx
sweep Sweep_sRGB_Linear_Half_Zip_agx

And screenshots from the tonemapping demo.

Env Intensity None ACES AgX
1.0 image image image
3.0 image image image

@ektogamat
Copy link

Fantastic work guys! Congratulations and thank you for all your hard work here 👏🏻

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 19, 2023

I think the current state of the PR is good to merge.

Do we still need a fallback for WebGL 1?

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 19, 2023

Ah wait, now I see bbbd075 👍 .

@Mugen87 Mugen87 merged commit e041e08 into mrdoob:dev Dec 19, 2023
12 checks passed
@gkjohnson gkjohnson deleted the agx-tonemapping branch December 19, 2023 15:30
AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
* Add support for AGX tone mapping

* Update tonemapping example

* Update test

* AGX -> AgX

* glsl lint, suffixes fix

* Adjust tone mapping constant values

* Update src/renderers/shaders/ShaderChunk/tonemapping_pars_fragment.glsl.js

Co-authored-by: Don McCurdy <[email protected]>

* Update src/renderers/shaders/ShaderChunk/tonemapping_pars_fragment.glsl.js

Co-authored-by: Don McCurdy <[email protected]>

* Update OutputPass.js

Add `AgX` support.

* Update OutputShader.js

Add `AgX` support.

* Clamp the values to be > 0.0

* Add rec2020 matrices

* Fix rec2020 conversion matrices

* Update implementation to be based on filament using rec 2020 color space

* Spaces -> tabs

* linting

* Code cleanup

* Support WebGL1, rearrange

* Comments update

* Remove redundant clamp

---------

Co-authored-by: Don McCurdy <[email protected]>
Co-authored-by: Michael Herzog <[email protected]>
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.

Support AgX tone mapping
6 participants