Fix error-prone usage of `ColorBgra` with straight vs premultiplied alpha

[cameronwhite]

Related to #1543, it's very easy to introduce transparency-related bugs with ColorBgra from mixing up straight alpha vs premultiplied alpha.

Since this is our pixel type for directly manipulating a Cairo.ImageSurface's data (which is always premultiplied alpha), I think ColorBgra also should only ever store premultiplied alpha

• [x] Check usages of ColorBgra.FromUInt32(), e.g. RandomColorBgra()
• [x] Remove usage of ColorBgra.Blend(), which expects straight alpha (see #1546 )
• [x] Replace ColorBgra.NewAlpha() with a version that expects the color to be in premultiplied alpha (done in #1645)
• [ ] ToPremultipliedAlpha() and ToStraightAlpha() should be made private if possible. The conversion functions to and from Cairo.Color (which is in straight alpha) should do the alpha conversion as well since it's very easy to accidentally miss this.
• [x] Verify the correctness of ColorDifference() which seems to multiply by alpha (done in #1639)
• [x] The HexString-related methods could likely be merged into Cairo.Color (done in #1626)
• [ ] Audit the UnaryPixelOps and UserBlendOps for correctness

[Lehonti]

Should there even be ToStraightAlpha and ToPremultipliedAlpha methods, even if they are private? Methods could just convert to Cairo.Color directly and at no point would there be a ColorBgra storing a color with straight alpha (even privately), only the variables inside the method would. If we need to bundle intermediate values we could just use the BgraAggregate or BgraBundle structure we once talked about. Unless there is some other reasoning involved for making the methods private instead of removing them.

[cameronwhite]

Should there even be ToStraightAlpha and ToPremultipliedAlpha methods, even if they are private?

I don't think they necessarily need to remain around as private methods on ColorBgra, but I'd like to have some function available for doing the conversion so that we're not duplicating the math in places that need it - perhaps utility methods which convert a single byte value to / from premultiplied alpha would be the way to go?

Having to convert to Cairo.Color isn't a good solution IMO since that also involves going from 0-255 byte values to 0-1 float values

[Lehonti]

I agree, a method for converting a single byte could be a good solution. I didn't mean that we should use Cairo.Color as an intermediate value, I just thought that would be the main reason one would want to convert to straight alpha, since ColorBgra isn't supposed to hold it :)

[cameronwhite]

Yeah in general that's definitely the case :) There are some specific operations like changing the pixel's alpha (e.g. in RandomColorBgra() or ColorBgra.NewAlpha()) where having those utility methods is useful

[Lehonti]

Audit the UnaryPixelOps and UserBlendOps for correctness

@cameronwhite would it be a good idea to refactor the blend ops, perhaps after creating tests that cover different scenarios? I remember you said that they are rarely ever touched and didn't think that refactoring them was a good idea, at least back then. But could you expand a little bit on that? I am asking because currently their implementations consist of unintelligible and presumably auto-generated code, and if we are to modify them after a problem arises (see, for example, a potential issue with the Xor blend more in #1695... coincidentally, Xor is the only blend op whose code I ever touched, and I wonder if I accidentally introduced a bug or if it behaves as described in the issue for some different reason) we would need to understand what is happening.

[cameronwhite]

Yeah at the time I didn't think it was worth spending time on, since it was code that worked and wasn't being actively modified.

Before doing much refactoring I would suggest first figuring out if they are working correctly with transparent colors (premultiplied alpha), to see whether there is a larger set of fixes needed

[Lehonti]

Audit the UnaryPixelOps and UserBlendOps for correctness

Also related: #1696

[cameronwhite]

Based on #1696, it looks like the blend ops are likely all assuming unpremultiplied alpha. So, I think we should:

• Remove the CreateWithOpacity() methods. These are unused in Pinta so there's no point spending effort to fix them
• Add tests for the blend modes
• Fix / rewrite the blend ops

[Lehonti]

If we are going to rewrite the blend ops, we might find this W3C recommendation draft useful for reference: https://www.w3.org/TR/compositing-1/#blending