Add gamma correction passes

[danieldg]

While full support for arbitrary color spaces is more complex and should probably remain out of scope, tiny-skia currently does all its rendering operations assuming a linear color space. However, it's likely the most common input and output formats (PNG images, or RGBA bitmaps for a canvas) expect and provide colors using sRGB pixel values.

Skia itself claims to perform more complete color management now - see https://skia.org/docs/user/color/.

This adds a quick, only-mostly-accurate gamma correction pass to the highp pipeline. While sRGB uses a gamma of 2.2, the correction in this patch uses a gamma value of 2 so we can use multiplication and sqrt() instead of needing to make calls to f32::powf. The results are still pretty accruate, as demonstrated by the added example file: run it with and without the gamma correction in this patch, and look at the resulting PNG: without this patch, there is a distinct "ripple" effect that appears in lines other than the first (perfectly horizontal) one.

I expect that, rather than being applied unconditionally as is done in this patch, that a new setting would be added to the various Paint objects to enable sRGB color - but I didn't want to do that if you were uninterested in the idea.

[danieldg]

I improved the example a bit to also show that without gamma correction, the brightness of a line appears to change depending on where it is drawn. The 11th line should be identical between the two images, to help compare.

[RazrFalcon]

tiny-skia currently does all its rendering operations assuming a linear color space

Are you sure? It's sRGB to my knowledge.

Otherwise, this change is way beyond my expertise. The point of tiny-skia is that it works exactly like Skia. I do not plan diverging from its behaviour. Sure, we could make this change optional, but I'm not even sure what it does and why. Moreover, I'm not sure how it affect all pipeline stages.

And why gamma 2? Why not 2.2, 2.4? This change fills to restrictive and opinionated to me.

[CryZe]

As per IEC 61966-2-1 specification, what's stored in typical image files is "non-linear sR′G′B′ values". You technically should not do any math on them without converting them to "linear sRGB values". The relationship does involve pow(x, 2.4), but it's more complicated than that. Full formula here: https://en.wikipedia.org/wiki/SRGB#From_sRGB_to_CIE_XYZ

Although there's also "simple sRGB" that's literally just pow(x, 2.2).

For legacy reasons almost every application, including the entire web, does not do this. Games and some more modern GUI frameworks that use the GPU are the only applications that do this correctly. While I would like to somewhat see this, it also causes other issues with for example small fonts relying on "gamma being broken" (Photoshop using a gamma of 1.45 for text and Skia doing a bunch of background color guessing)

[danieldg]

Are you sure? It's sRGB to my knowledge.

Yes; the blend operations you are using are written to operate on a linear colorspace. sRGB is not a linear colorspace, so a conversion is needed to avoid incorrect blending.

The point of tiny-skia is that it works exactly like Skia.

That's the reason why I cited skia's color support page.

And why gamma 2? Why not 2.2, 2.4?

The choice of gamma 2 was to avoid the performance penalties associated with calling powf for both incoming and outgoing gamma. This PR's branch now has a patch that does "simple sRGB" which is a gamma of 2.2; while there are differences, it's much less apparent than the difference between 1 and 2.

Making the change optional would allow matching existing output which may be important (I see @CryZe has responded with more details there). I would propose adding a bool like anti_alias to Paint, but defaulting to false.

[RazrFalcon]

Ok, the main problem here is that I have zero knowledge about 2D graphics. This is just a port. It works exactly like Skia. That's the point. I have no idea if your change is "correct" or not. This is beyond my expertise.

I guess I don't mind adding something like Paint::gamma with an enum of None, Gamma2_0, Gamma2_2, Gamma2_4. Where None is default, if I understand correctly. Aka the old/default behaviour.

As for proper color spaces support, this is technically planned, but might take another 10 years. Who knows.

PS: the current patch disabled u16 pipeline completely, which is not what we want. Unless you know how to implement gamma for u16 pipeline as well.

[CryZe]

These seem to be the actual stages in Skia: https://github.com/google/skia/blob/da61fa87e85593ad01f39c0cb1379b0bfd6ad250/src/opts/SkRasterPipeline_opts.h#L2330-L2353

appended from here: https://github.com/google/skia/blob/da61fa87e85593ad01f39c0cb1379b0bfd6ad250/src/core/SkRasterPipeline.cpp#L501-L517

[RazrFalcon]

Yep, but we do not support skcms yet. And it would take a while to port.

[danieldg]

Yep, but we do not support skcms yet. And it would take a while to port.

Agreed, porting the entire color management framework would be the best way to fix this, but it would also be a lot more code.

PS: the current patch disabled u16 pipeline completely, which is not what we want.

Yes, I'm aware. Making the gamma passes optional will turn it back on again when gamma is disabled. I don't think it's possible to add gamma in the u16 case as it's written without suffering from a huge precision loss which is likely to be worse in the "normal" case where the blend op primarily uses only one of the two sources (interior of a filled path, etc). I think you would need to change the implementation of lowp to use the full u16 for the color value and expand calculations to to u32 during blend in order to do that - which is basically a rewrite of lowp, and halves its vector throughput. But someone more clever than I may have come up with a solution.

[RazrFalcon]

Having a match in the pipeline is a very bad idea. It would tank the performance. Prefer separate functions. Like we do with blend modes.

Overall it looks fine.

[RazrFalcon]

Please also add rendering tests for each gamma type.

[danieldg]

Please also add rendering tests for each gamma type.

The example code has been expanded to do this.

[RazrFalcon]

We need tests, not examples. Something that would run during cargo test.

[RazrFalcon]

Good. Can you add tests for gradients as well. Tests should covert all of the new code.

[danieldg]

I think my test is hitting #8 - https://fiddle.skia.org/c/f295473d61d37ff5fd1b57ca3699462a is how the 3-color part should look, but tiny-skia has some jerkiness in the anti-aliased edge. The test has linear as the top of the 4, and the skia fiddle is also linear. I did confirm that the output of the linear one is identical in 0.11.4.

[RazrFalcon]

Weird. Probably a separate bug. Will take a look later.

As for #8 - this should not be triggered by stroking. It's mainly for angled lines during filling, afaik.

[RazrFalcon]

CI errors can be fixed by adding this to required files:

#[cfg(all(not(feature = "std"), feature = "no-std-float"))]
use crate::NoStdFloat;

[RazrFalcon]

A typo in NEON code?

[danieldg]

What crazy person at ARM decided that orr was the right abbreviation for bitwise OR? That feels like a typo that slipped into the standard to me.

[RazrFalcon]

SIMD naming convention is pretty bizarre in general. Hopefully we would be able to use std::simd eventually.

[RazrFalcon]

Thanks. I will publish a new release soon.