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 HDR to Metal + Perceptual HDR #12603

Merged
merged 3 commits into from Mar 21, 2024

Conversation

Sam-Belliveau
Copy link
Contributor

@Sam-Belliveau Sam-Belliveau commented Mar 1, 2024

This PR:

  1. Add HDR to Metal
  2. Add PerceptualHDR

PerceptualHDR Menu:
image

@MayImilae
Copy link
Contributor

Likely not ready to be merged unless it shows some huge advantage as it just removes a feature atm.

You should probably mark this PR as draft then.

@Sam-Belliveau Sam-Belliveau marked this pull request as draft March 1, 2024 21:47
@Sam-Belliveau Sam-Belliveau force-pushed the oklab_resampling branch 2 times, most recently from 6526e62 to c0985f2 Compare March 2, 2024 02:51
Copy link
Contributor

@Filoppi Filoppi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I find this PR to have more negatives than positives at the moment.
I don't like most of the code restructuring, I find it unnecessary (because it's working, and syntax doesn't matter as much) and detrimental (because it adds more problems and removes comments and code that made the shader easier to read).
To my eyes, it looks like "random" changes to refactor it in your style, instead of my style. Not saying any of the two styles is better, but that's exactly the point, these are just stylistic changes, I don't see this PR as making the code more readable to everybody, I feel like it might just be more readable to you :D.

I'm not against spacing changes, so if you want to make a PR for that, feel free to do so.

Scaling brightness by oklab in HDR isn't really wanted here.
The original idea of blending texture samples with oklab could have been elaborated more into something better though, as long as we can get the most physically accurate (which is a bit different than perceptually accurate) color mixing.

That said, I don't want to be rude, I appreciate the attempt. Let's build something from this.

Data/Sys/Shaders/default_pre_post_process.glsl Outdated Show resolved Hide resolved
Data/Sys/Shaders/default_pre_post_process.glsl Outdated Show resolved Hide resolved
Data/Sys/Shaders/default_pre_post_process.glsl Outdated Show resolved Hide resolved
Data/Sys/Shaders/default_pre_post_process.glsl Outdated Show resolved Hide resolved
Data/Sys/Shaders/default_pre_post_process.glsl Outdated Show resolved Hide resolved
Data/Sys/Shaders/default_pre_post_process.glsl Outdated Show resolved Hide resolved
@Sam-Belliveau Sam-Belliveau force-pushed the oklab_resampling branch 2 times, most recently from ee193c0 to 1758cc2 Compare March 4, 2024 03:39
Source/Core/VideoBackends/Metal/MTLMain.mm Outdated Show resolved Hide resolved
Data/Sys/Shaders/default_pre_post_process.glsl Outdated Show resolved Hide resolved
Data/Sys/Shaders/default_pre_post_process.glsl Outdated Show resolved Hide resolved
@Sam-Belliveau Sam-Belliveau marked this pull request as ready for review March 5, 2024 23:15
@Filoppi
Copy link
Contributor

Filoppi commented Mar 6, 2024

Please just implement this as a post process, just like AutoHDR.glsl that's where it belongs with the way things are designed.
You are applying a subjective SDR to HDR upgrade, and that should not be part of the default feature set of "color correction", as it's something "random", based on taste.

@Sam-Belliveau
Copy link
Contributor Author

I really disagree with the idea that this is something random based on taste. I showed before my exact derivation, reasoning, and verification that it is not distorting the underlying picture.

I explained exactly the problem it's solving. (dark colors becoming over exposed) I explain the requirements for an algorithm to resolve it, and I picked the single function that exists that solves this problem. There are no tunable parameters for the perceptual mode, because only one solution exists.

It fits the requirements for HDR Paper Nits. White is the exact same as in the Linear mode. Great lengths were made on my end to make sure this was more than just a random feature.

It was part of a larger effort undertaken to clean up this default post process shader. This is more than just a post process function, this is an entirely valid, mathematically backed way to scale SDR to HDR given certain constraints.

This is the form that I believe this feature should be in, because it's simple and offers substantive improvements. It would take extra work for me to go through my refactor and undo all and make it only support a single form of HDR Scaling.

I'd be willing to consider a post process effect in the future but this is the state of this project right now.

@Sam-Belliveau Sam-Belliveau changed the title Simplified Color Correction Menu & Improved HDR with OkLab Improved HDR with OkLab Mar 6, 2024
@iwubcode
Copy link
Contributor

iwubcode commented Mar 6, 2024

I am not as passionate as either of you about color output, HDR, or gamma. Regardless, I will give some general thoughts /opinions.

Maybe @Filoppi can remind me, why is HDR Paper White Nits even an option for the color spaces? To me it seems like an odd option to have in a color correction window about the original GC/Wii. From what I recall, you said that the HDR range was needed to show those color spaces. Can we not determine the appropriate white balance for those original spaces?

Even calling this window "color correction" gives off the wrong expectations. This window isn't about color correction or HDR, that is a post process function. Therefore I would lean toward not including anything that isn't related to our original goal (console accuracy) in this window. (Additionally, I'd like to avoid making this window too large)

Also, while it is great we cleaned the shader up, I don't think we should get too attached to our current shader setup for post processing. I can't make any promises but I'd like to rework shader management in the future (may or may not be a part of post processing enhancement).

Having done a number of ports of Reshade shaders and also worked on a number of custom shaders with my new material system, it's apparent that our current approach doesn't scale very well. Our global functions are difficult to find without reading through a lot of source and users get everything we put in global state when they implement their own shader. We really need some common code approach that I'll call a "library".

Sorry this was a long amount of text. I'll give it some more thought but my initial impression is that OkLab would be good to include in a Dolphin provided "library" but the perceptual color conversion makes more sense in a post processing shader that the user can opt-in to use.

@Sam-Belliveau Sam-Belliveau force-pushed the oklab_resampling branch 2 times, most recently from b613972 to 3a37bcf Compare March 9, 2024 04:34
@Sam-Belliveau Sam-Belliveau changed the title Improved HDR with OkLab Add HDR to Metal + Perceptual HDR Mar 9, 2024
Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shader part LGTM.

Copy link
Contributor

@Filoppi Filoppi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the few comments left, code looks good now. Thanks for making this a post process shader.

Copy link
Contributor

@TellowKrinkle TellowKrinkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metal part LGTM
(Though I'd prefer if the Metal changes were in a separate commit from the PerceptualHDR shader)

@JMC47 JMC47 merged commit 03f2b99 into dolphin-emu:master Mar 21, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants