Bug 181196

Created attachment 330264 [details] Patch

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)?

<rdar://problem/36261359>

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.

(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.

Created attachment 330730 [details] Patch

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.

(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.

(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.