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 AgX tonemapper #7236

Merged
merged 6 commits into from
Oct 13, 2023
Merged

Add AgX tonemapper #7236

merged 6 commits into from
Oct 13, 2023

Conversation

bejado
Copy link
Member

@bejado bejado commented Oct 5, 2023

AgX:
Screenshot 2023-10-05 at 11 39 32 AM

ACES (legacy):
Screenshot 2023-10-05 at 11 39 24 AM

@bejado bejado requested a review from romainguy October 5, 2023 18:44
Copy link
Collaborator

@romainguy romainguy left a comment

Choose a reason for hiding this comment

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

How well does this interact with luminance scaling? Can you try? If it doesn't behave well (since AgX kind of does it already) we may want to no-op luminanceScaling with AgX.

}

float3 AgxToneMapper::operator()(float3 v) const noexcept {
// TODO: It's unclear if we need to temporarily transform to 709 primaries again.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you don't change ColorGrading.cpp the values you'll get here are in Rec2020. See selectColorGradingTransformIn()/selectColorGradingTransformOut(). Seems like if we don't do anything we'll match Blender? Might be worth checking Blender's implementation to see what they do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, which I why I originally converted back to sRGB with the commented-out: v = Rec2020_to_sRGB * v;. The way I have it matches Blender (and looks good to me), but I'm not sure if it's right, as Sobotka's original AgX seems to keep everything in 709.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can put you in touch with Troy on Discord if you want. Does Blender maybe use a different inset/outset matrix? The idea is to just slightly expand/contract the gamut.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically yes, Blender's uses different primaries_rotate and primaries_scale parameters, resulting in a different inset/outset matrices. I'm not sure if they adjusted the parameters for aesthetic reasons or to somehow compensate for Rec2020.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect it was made to map better to Rec2020. That said we don't render in 2020, we only use 2020 during color grading.

filament/include/filament/ToneMapper.h Show resolved Hide resolved
filament/src/ToneMapper.cpp Show resolved Hide resolved
@bejado
Copy link
Member Author

bejado commented Oct 6, 2023

How well does this interact with luminance scaling? Can you try? If it doesn't behave well (since AgX kind of does it already) we may want to no-op luminanceScaling with AgX.

output.mp4

I wouldn't say it breaks it, but I do I think I prefer AgX without luminance scaling.

@romainguy
Copy link
Collaborator

Agreed, it's better without, but prob not worth no-oping luminance scaling when AgX is selected.

Copy link
Collaborator

@romainguy romainguy left a comment

Choose a reason for hiding this comment

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

LGTM so far, let me know what agxLook() does. Might be something optional driven by a parameter?

NEW_RELEASE_NOTES.md Outdated Show resolved Hide resolved
@bejado
Copy link
Member Author

bejado commented Oct 11, 2023

LGTM so far, let me know what agxLook() does. Might be something optional driven by a parameter?

FYI

looks.mp4

}

// ASC CDL
val = pow(val * slope + offset, power);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: our existing color grading APIs let you apply a CDL.

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.

3 participants