Repository navigation
Fix error-prone usage of ColorBgra with straight vs premultiplied alpha #1553
Description
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
- Check usages of
ColorBgra.FromUInt32(), e.g.RandomColorBgra() - Remove usage of
ColorBgra.Blend(), which expects straight alpha (see Add a replacement for ColorBgra.Blend() which didn't use premultiplied alpha #1546 ) - Replace
ColorBgra.NewAlpha()with a version that expects the color to be in premultiplied alpha (done in Port ColorBgra.NewAlpha() to work with premultiplied alpha #1645) -
ToPremultipliedAlpha()andToStraightAlpha()should be made private if possible. The conversion functions to and fromCairo.Color(which is in straight alpha) should do the alpha conversion as well since it's very easy to accidentally miss this. - Verify the correctness of
ColorDifference()which seems to multiply by alpha (done in Fix the color difference comparison's handling of alpha #1639) - The
HexString-related methods could likely be merged intoCairo.Color(done in Consolidate methods for converting colors to/from hex strings #1626) - Audit the UnaryPixelOps and UserBlendOps for correctness
Activity
Consolidate methods for converting colors to/from hex strings
Consolidate methods for converting colors to/from hex strings
Fix the color difference comparison's handling of alpha
Fix the color difference comparison's handling of alpha
Minor refactoring to use safer ColorBgra methods
Port ColorBgra.NewAlpha() to work with premultiplied alpha
Lehonti commentedon Aug 1, 2025
Should there even be
ToStraightAlphaandToPremultipliedAlphamethods, even if they are private? Methods could just convert toCairo.Colordirectly and at no point would there be aColorBgrastoring 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 theBgraAggregateorBgraBundlestructure we once talked about. Unless there is some other reasoning involved for making the methods private instead of removing them.Port ColorBgra.NewAlpha() to work with premultiplied alpha
cameronwhite commentedon Aug 1, 2025
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
bytevalue to / from premultiplied alpha would be the way to go?Having to convert to
Cairo.Colorisn't a good solution IMO since that also involves going from 0-255 byte values to 0-1 float valuesLehonti commentedon Aug 1, 2025
I agree, a method for converting a single byte could be a good solution. I didn't mean that we should use
Cairo.Coloras an intermediate value, I just thought that would be the main reason one would want to convert to straight alpha, sinceColorBgraisn't supposed to hold it :)cameronwhite commentedon Aug 1, 2025
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
Remove unused ColorBgra.Blend method for straight alpha
Fix incorrect blending in the Pixelate effect
Convert between straight / premultiplied alpha when converting ColorB…
Convert between straight / premultiplied alpha when converting ColorB…
Lehonti commentedon Aug 26, 2025
@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 commentedon Aug 27, 2025
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 commentedon Aug 27, 2025
Also related: #1696
cameronwhite commentedon Aug 29, 2025
Based on #1696, it looks like the blend ops are likely all assuming unpremultiplied alpha.
So, I think we should:
CreateWithOpacity()methods. These are unused in Pinta so there's no point spending effort to fix themLehonti commentedon Aug 29, 2025
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