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

Optional gamma correction parameter #350

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

biserhong
Copy link

This PR adds gamma correction functionality to the /singleband and /rgb endpoints. It is requested by adding an optional gamma_factor parameter.

An important observation about enabling this functionality is that the exponential function preserves the relative percentile ranges in the [0, 1] interval. So the computed percentiles in the metadata can be carried through after gamma correcting a tile.

@dionhaefner
Copy link
Collaborator

Thanks for the implementation. Can you say a bit more about the use case for this? Any reason why this can't be done offline (during preprocessing) or using /compute?

@biserhong
Copy link
Author

Thanks for the implementation. Can you say a bit more about the use case for this? Any reason why this can't be done offline (during preprocessing) or using /compute?

We use terracotta to display images customers purchase at SkyFi. For true colour 3-band composites, rendering the data linearly does not provide a representation that is similar to what the human eye would perceive. Applying a gamma correction between 1.6 and 2.2, on an otherwise linear colour space makes for something that is a lot closer to a “natural colour composite”. We also want the preview on the map to be as close as possible to the png previews we send customers along with the raster, to which gamma correction is applied.

Regarding the option of doing it offline, we would like to offer users the ability to adjust min/max and gamma factor on the fly. The /compute endpoint also only outputs a single band image and cannot use the precomputed metadata for color stretching.

I also think gamma correction would be a good addition to the capabilities of terracotta since it’s a standard and common processing technique, and its implementation doesn’t really affect any other functionality.

@vincentsarago
Copy link

FYI in rio-tiler/titiler we use https://github.com/vincentsarago/color-operations (fork of mapbox/rio-color), Just sharing if you want to support more color operations in the future ;-)

https://github.com/cogeotiff/rio-tiler/blob/dd5ce03f8f463ec93f3a8787d7155d318397cd7c/rio_tiler/models.py#L524-L536

@dionhaefner
Copy link
Collaborator

Thanks @vincentsarago! I like this a lot, and a feature like this has been on the roadmap for a long time: #23.

I'd strongly prefer an implementation that doesn't assume a specific color transformation like gamma, and instead implements support for a range of transforms (like the ones provided via color-operations / rio-color). @biserhong do you think that would be feasible?

@biserhong
Copy link
Author

biserhong commented Nov 13, 2024

@vincentsarago @dionhaefner To be honest, I also used rio-color's implementation of the same functionality but had to change it a bit to handle the float case (so color-operations actually has almost identical functions).

However, I added color-operations as a dependency to use your implementation.

Some notes:

  • You still need a wrapper gamma_correction function to handle normalization and rescaling before stretching.
  • As far as I can tell we essentially cannot use the operations parsing because terracotta is predicated on directly working only on single bands (at least not in a straightforward way).
  • To be able to string together several color operations and still reuse the precomputed percentiles would probably have to be thought through as well to see if it would work in all cases.

@dionhaefner
Copy link
Collaborator

Thanks @biserhong !

I'm hesitant to introduce features that complicate the API for a singular use case like gamma correction. I can see how this is probably fine for the few most important use cases, but inevitably leads to a kitchen sink of parameters over time that becomes a nightmare to clean up / refactor later on.

A for me much more sustainable design would be to introduce a parameter like /rgb/x/y/z.png?color_transform="gamma R 0.5" that exposes a collection of methods behind a single API. Would that fit your needs?

@biserhong
Copy link
Author

Thanks @biserhong !

I'm hesitant to introduce features that complicate the API for a singular use case like gamma correction. I can see how this is probably fine for the few most important use cases, but inevitably leads to a kitchen sink of parameters over time that becomes a nightmare to clean up / refactor later on.

A for me much more sustainable design would be to introduce a parameter like /rgb/x/y/z.png?color_transform="gamma R 0.5" that exposes a collection of methods behind a single API. Would that fit your needs?

It would work for me.

I'll have to check what each of the color transforms does exactly and it might turn out that for some transformation we might not be able to use the precomputed percentiles (it might also be ok, I just don't know). Are you ok with that? @vincentsarago do you have any input on this?

@dionhaefner
Copy link
Collaborator

Percentiles are stable under any monotonous map, so we should be fine for all operations that are applied band-wise. In other cases, I think it's okay to be slightly off – the practical impact of that should be small (of course it should be documented explicitly that the given percentiles relate to untransformed data).

@biserhong
Copy link
Author

Sorry this took so long, I had to focus on some other work for a bit.

@dionhaefner Can you take a look at the latest commit and let me know if you agree with the approach?

I'll clean up the rest of the PR and tidy up the code and variables if we agree to continue this way.

terracotta/image.py Outdated Show resolved Hide resolved
Comment on lines +199 to +200
for func in parse_operations(color_transform):
arr = func(arr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this assume a particular scaling already?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. It may be easier to apply stretching before color transform then. I.e., normalize to [0,1] using stretch_range and clamp spillover to 0 / 1, then apply color transform, then convert to uint8. That way we don't rely on a transformation of the stretch range?

Copy link
Author

Choose a reason for hiding this comment

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

I think it may not be a good idea since it would change the result.

The color stretch fills the [0, 1] range whereas the normalization to_math_type does is relative to the dtype range. So when you apply the color transform I think that would shift where values fall along the curve.

This is what chatgpt said in the case of gamma correction:

Gamma correction is typically applied before color stretching, particularly when working with image data in scientific imaging, photography, or graphic design.

Here's why:

    Gamma Correction First: Gamma correction adjusts the image data to a linear color space, which compensates for the nonlinear response of human vision and many display systems. This linearization step ensures that subsequent adjustments, like color stretching, are applied to data that more accurately represents real-world light intensities.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, but who says that the dtype range is appropriate? The user-provided stretch range is telling us how to map the values in the raster to a linear scale.

Summoning @vincentsarago in case you want to weigh in on the appropriate order of linear scaling (and clamping out of range values) vs. color correction :)

@dionhaefner
Copy link
Collaborator

Awesome @biserhong! I like this a lot, especially how we specify stretch ranges in untransformed space which should work even with multi-band transforms.

I reckon there will be quite a bit of testing required to make sure scaling works as expected but from a UX perspective I really like it, so should be good to port to other endpoints as well.

@biserhong
Copy link
Author

Thanks for the quick feedback @dionhaefner! I'll try to get something more complete this week.

@biserhong
Copy link
Author

@dionhaefner I removed all references to the gamma factor and replaced it with the color transform.

I excluded the saturation option for now because it's only valid for rgb and it's not monotonic.

What do you think? I can proceed to tests if you agree with this approach.

@@ -88,7 +90,8 @@ def get_band_future(band_key: str) -> Future:
keys = (*some_keys, band_key)
metadata = driver.get_metadata(keys)

band_stretch_range = list(metadata["range"])
band_range = list(metadata["range"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why introduce this variable? Looks unused.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh now I see, you're using this further below. But beware, since you're setting this in a loop only the value for the last band will be used in the color transform!

missing=None,
example="gamma 1 1.5, sigmoidal 1 15 0.5",
description="Color transform DSL string from color-operations."
"All color operations for singleband should specify band 1.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what this means. Could you elaborate?

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