Bug 181196 – SVG lighting colors need to be converted into linearSRGB

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Simon Fraser (smfr)]

Created attachment 330263 [details]
Testcase

Attached test case shows that light colors are not correctly converted between sRGB and linearRGB. It should match a rect with a green fill.

[Simon Fraser (smfr)]

Created attachment 330264 [details]
Patch

[Daniel Bates]

Comment on attachment 330264 [details]
Patch

All of the math and constants feels like magic. Is there any source we can reference for these algorithms? Can we use more universal initializer syntax (i.e. {} initialization)?

[Simon Fraser (smfr)]

https://trac.webkit.org/r226315

[Radar WebKit Bug Importer]

<rdar://problem/36261359>

[Darin Adler]

Comment on attachment 330264 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330264&action=review

> Source/WebCore/platform/graphics/ColorUtilities.cpp:47
> +    if (c < 0.0031308)
> +        return 12.92 * c;
> +
> +    return clampTo<float>(1.055 * powf(c, 1.0 / 2.4) - 0.055, 0, 1);

I know you moved this code, but I never carefully reviewed it when it was in the transformColorSpace function.

This code is doing a *lot* of conversion to double because all the constants here are doubles; if we want float constants we should use 1.0f rather than 1.0 for all six of these. But maybe we are converting to double intentionally because I do see the call to clampTo<float>? What I don’t understand, though, then is why we are using doubles in the first if statement as well, and also why we are using powf rather than the double version of pow if we are intended to do the arithmetic as doubles.
 
Even if we do want the float version, std::pow is what we would normally use in C++ code; that’s overloaded based on the type of the arguments.

I’d like to see some tests for these functions in API tests to make sure they are doing exactly what we want.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:55
> +    if (c <= 0.04045)
> +        return c / 12.92;
> +
> +    return clampTo<float>(powf((c + 0.055) / 1.055, 2.4), 0, 1);

Ditto.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:58
> +Color linearToSRGBColor(const Color& color)

This is a very peculiar function. In modern Color the Color contains information about what its current color space is. It’s not OK to have a non-Extended Color that is not actually sRGB. We should use something other than Color for that. We should discuss how to fix this from a design point of view. I suppose it can wait until I do my Color class improvements, which might take a while.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #5)
> Comment on attachment 330264 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330264&action=review
> 
> > Source/WebCore/platform/graphics/ColorUtilities.cpp:47
> > +    if (c < 0.0031308)
> > +        return 12.92 * c;
> > +
> > +    return clampTo<float>(1.055 * powf(c, 1.0 / 2.4) - 0.055, 0, 1);
> 
> I know you moved this code, but I never carefully reviewed it when it was in
> the transformColorSpace function.
> 
> This code is doing a *lot* of conversion to double because all the constants
> here are doubles; if we want float constants we should use 1.0f rather than
> 1.0 for all six of these. But maybe we are converting to double
> intentionally because I do see the call to clampTo<float>? What I don’t
> understand, though, then is why we are using doubles in the first if
> statement as well, and also why we are using powf rather than the double
> version of pow if we are intended to do the arithmetic as doubles.
>  
> Even if we do want the float version, std::pow is what we would normally use
> in C++ code; that’s overloaded based on the type of the arguments.
> 
> I’d like to see some tests for these functions in API tests to make sure
> they are doing exactly what we want.
> 
> > Source/WebCore/platform/graphics/ColorUtilities.cpp:55
> > +    if (c <= 0.04045)
> > +        return c / 12.92;
> > +
> > +    return clampTo<float>(powf((c + 0.055) / 1.055, 2.4), 0, 1);
> 
> Ditto.

These should probably just do float math.


> > Source/WebCore/platform/graphics/ColorUtilities.cpp:58
> > +Color linearToSRGBColor(const Color& color)
> 
> This is a very peculiar function. In modern Color the Color contains
> information about what its current color space is. It’s not OK to have a
> non-Extended Color that is not actually sRGB. We should use something other
> than Color for that. We should discuss how to fix this from a design point
> of view. I suppose it can wait until I do my Color class improvements, which
> might take a while.

I had the same thought when writing the, but ignored the voices in the back of my head! This could return FloatComponents or something.

[Simon Fraser (smfr)]

Created attachment 330730 [details]
Patch

[Darin Adler]

Comment on attachment 330730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330730&action=review

> Source/WebCore/platform/graphics/ColorUtilities.cpp:53
> +    return clampTo<float>(1.055f * powf(c, 1.0f / 2.4f) - 0.055f, 0, 1);

Should use std::pow instead of powf.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:61
> +    return clampTo<float>(powf((c + 0.055f) / 1.055f, 2.4f), 0, 1);

Should use std::pow instead of powf.

> Source/WebCore/platform/graphics/filters/FELighting.cpp:299
> +        static_cast<uint8_t>(lightStrength * lightingData.colorVector.x() * 255.0f),
> +        static_cast<uint8_t>(lightStrength * lightingData.colorVector.y() * 255.0f),
> +        static_cast<uint8_t>(lightStrength * lightingData.colorVector.z() * 255.0f)

Shouldn’t this round instead of clamping? I’d also like to see us use a shared function for this. The function colorFloatToRGBAByte inside Color.cpp is the one I mean, although that may not be the right location nor name for it.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #8)
> Comment on attachment 330730 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330730&action=review
> 
> > Source/WebCore/platform/graphics/ColorUtilities.cpp:53
> > +    return clampTo<float>(1.055f * powf(c, 1.0f / 2.4f) - 0.055f, 0, 1);
> 
> Should use std::pow instead of powf.
> 
> > Source/WebCore/platform/graphics/ColorUtilities.cpp:61
> > +    return clampTo<float>(powf((c + 0.055f) / 1.055f, 2.4f), 0, 1);
> 
> Should use std::pow instead of powf.
> 
> > Source/WebCore/platform/graphics/filters/FELighting.cpp:299
> > +        static_cast<uint8_t>(lightStrength * lightingData.colorVector.x() * 255.0f),
> > +        static_cast<uint8_t>(lightStrength * lightingData.colorVector.y() * 255.0f),
> > +        static_cast<uint8_t>(lightStrength * lightingData.colorVector.z() * 255.0f)
> 
> Shouldn’t this round instead of clamping?

For lighting, the spec doesn't say. For feTurbulence, it does say that you just multiply by 255 then clamp.

I'll try to figure out what other browsers do.

> I’d also like to see us use a
> shared function for this. The function colorFloatToRGBAByte inside Color.cpp
> is the one I mean, although that may not be the right location nor name for
> it.

Will look.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #8)
> Comment on attachment 330730 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330730&action=review
> 
> > Source/WebCore/platform/graphics/ColorUtilities.cpp:53
> > +    return clampTo<float>(1.055f * powf(c, 1.0f / 2.4f) - 0.055f, 0, 1);
> 
> Should use std::pow instead of powf.
> 
> > Source/WebCore/platform/graphics/ColorUtilities.cpp:61
> > +    return clampTo<float>(powf((c + 0.055f) / 1.055f, 2.4f), 0, 1);
> 
> Should use std::pow instead of powf.

Both fixed.

> > Source/WebCore/platform/graphics/filters/FELighting.cpp:299
> > +        static_cast<uint8_t>(lightStrength * lightingData.colorVector.x() * 255.0f),
> > +        static_cast<uint8_t>(lightStrength * lightingData.colorVector.y() * 255.0f),
> > +        static_cast<uint8_t>(lightStrength * lightingData.colorVector.z() * 255.0f)
> 
> Shouldn’t this round instead of clamping? I’d also like to see us use a
> shared function for this. The function colorFloatToRGBAByte inside Color.cpp
> is the one I mean, although that may not be the right location nor name for
> it.

I didn't change this; Filters code multiples by 255 in various places, and they all need review.