Bug 220716 – Rework color clamping logic to be more consistent and clear

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Rework color clamping logic to be consistent and clear

[Sam Weinig]

Created attachment 417850 [details]
Patch

[Sam Weinig]

Created attachment 417851 [details]
Patch

[Sam Weinig]

Created attachment 417855 [details]
Patch

[Sam Weinig]

Created attachment 417856 [details]
Patch

[Sam Weinig]

Created attachment 417915 [details]
Patch

[Sam Weinig]

Created attachment 417932 [details]
Patch

[Darin Adler]

Comment on attachment 417932 [details]
Patch

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

Seems like infinity can be clamped. It’s only NaN that needs a check. Do we really want all those std::isfinite checks?

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:907
> +        if (!std::isfinite(*alpha))
> +            return;

Is the treatment of infinities here something that comes from the DOM specification? Silently ignoring NaN makes some sense to me, although I could also imagine specifying some other behavior. Silently ignoring infinity is more of a surprise.

> Source/WebCore/platform/graphics/Color.h:399
> +            // FIXME: Define meaningful black points for all color types.

I wonder about how strict these functions should be. How far off from the most opaque and black can the color be and still qualify? These are floating point "==" operations in disguise!

> Source/WebCore/platform/graphics/ColorConversion.cpp:82
> +    ASSERT(c >= 0);
> +    ASSERT(c <= 1);

I don’t get the naming: If this function is "clamping" then why does it only accept values in the range [0,1]?

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:227
> +// FIXME: This should likely just use hueRotateColorMatrix(amount).

Got to admit I don’t understand what’s going on here.

[Simon Fraser (smfr)]

Comment on attachment 417932 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:907
> +        if (!std::isfinite(*alpha))
> +            return;

Does this behavior change need to be tested? What's the behavior in other browsers?

> Source/WebCore/platform/graphics/Color.cpp:147
> +        else {

else after return

> Source/WebCore/platform/graphics/ColorTypes.h:89
> +        { 0, 360 }

I'm not sure that clamping between 0 and 360 is right for angles (e.g. if they are the result of calc()). We should just mod instead.

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 417932 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417932&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:907
> > +        if (!std::isfinite(*alpha))
> > +            return;
> 
> Does this behavior change need to be tested? What's the behavior in other
> browsers?

Hm, what would this do? It would end up doing this:


    return std::clamp(std::lround(f * 255.0f), 0l, 255l);

with f as NaN, which from my reading is unspecified behavior. 

I don't think any other browsers implement these non-standard functions, I am not too worried about being more strict with NaN here.
> 
> > Source/WebCore/platform/graphics/Color.cpp:147
> > +        else {
> 
> else after return

Unfortunately, in this constexpr condition, it is needed.

> 
> > Source/WebCore/platform/graphics/ColorTypes.h:89
> > +        { 0, 360 }
> 
> I'm not sure that clamping between 0 and 360 is right for angles (e.g. if
> they are the result of calc()). We should just mod instead.

These ranges are really meant to keep the values in the valid range. I don't believe we currently ever clamp an HSLA value, it would be a bit weird to do so.

[Sam Weinig]

https://bugs.webkit.org/show_bug.cgi?id=220760 needs to be landed first, which resolve the remaining 3 assertions in mac-debug-1.

[Sam Weinig]

(In reply to Darin Adler from comment #7)
> Comment on attachment 417932 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417932&action=review
> 
> Seems like infinity can be clamped. It’s only NaN that needs a check. Do we
> really want all those std::isfinite checks?
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:907
> > +        if (!std::isfinite(*alpha))
> > +            return;
> 
> Is the treatment of infinities here something that comes from the DOM
> specification? Silently ignoring NaN makes some sense to me, although I
> could also imagine specifying some other behavior. Silently ignoring
> infinity is more of a surprise.

These functions are non-standard so the spec has nothing to say and the other browsers don't implemented them. I picked std::isfinite and silent early return, as that seemed to be the pattern in the file for this type of thing, though that is mostly for lengths, so checking for NaN could work here too.

> 
> > Source/WebCore/platform/graphics/Color.h:399
> > +            // FIXME: Define meaningful black points for all color types.
> 
> I wonder about how strict these functions should be. How far off from the
> most opaque and black can the color be and still qualify? These are floating
> point "==" operations in disguise!
> 
> > Source/WebCore/platform/graphics/ColorConversion.cpp:82
> > +    ASSERT(c >= 0);
> > +    ASSERT(c <= 1);
> 
> I don’t get the naming: If this function is "clamping" then why does it only
> accept values in the range [0,1]?

Oh, blerg, those shouldn't have been left in actually.

> 
> > Source/WebCore/platform/graphics/filters/FilterOperation.cpp:227
> > +// FIXME: This should likely just use hueRotateColorMatrix(amount).
> 
> Got to admit I don’t understand what’s going on here.

The existing code for InvertLightnessFilterOperation::transformColor/InvertLightnessFilterOperation::inverseTransformColor used the following pattern:

    auto hsla = toHSLA(color);
    
    // Rotate the hue 180deg.
    hsla.hue = std::fmod(hsla.hue + 0.5f, 1.0f);
    
    // Convert back to RGB.
    auto hueRotatedSRGBA = toSRGBA(hsla);

to rotate the hue. But in InvertLightnessFilterOperation::inverseTransformColor, this was done on after calling:

    auto convertedToLightMode = asSRGBA(toLightModeMatrix.transformedColorComponents(asColorComponents(color)));

and that can create an SRGB color with values greater than 1.0, which we no longer allow and was causing an assertion. If we clamp to SRGB there, we get different results in the hue rotation (manifesting in failing tests). So, what this does is implement the hue rotation directly on the unclamped color components so we clamp only after all the transformations are done.

The comment is indicating that using the conversion to and from HSL is an inefficient way to do a hue rotation and there is a relatively straightforward linear matrix transform that achieves the same effect (which we already implement called hueRotateColorMatrix(amount).

I will beef up the comment to explain this.

[Sam Weinig]

(In reply to Darin Adler from comment #7)
> Comment on attachment 417932 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417932&action=review
> 
> Seems like infinity can be clamped. It’s only NaN that needs a check. Do we
> really want all those std::isfinite checks?
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:907
> > +        if (!std::isfinite(*alpha))
> > +            return;
> 
> Is the treatment of infinities here something that comes from the DOM
> specification? Silently ignoring NaN makes some sense to me, although I
> could also imagine specifying some other behavior. Silently ignoring
> infinity is more of a surprise.
> 
> > Source/WebCore/platform/graphics/Color.h:399
> > +            // FIXME: Define meaningful black points for all color types.
> 
> I wonder about how strict these functions should be. How far off from the
> most opaque and black can the color be and still qualify? These are floating
> point "==" operations in disguise!

Actually, I think I have a way to do this. These functions are weird and we should probably remove them, but not today :(.

[Sam Weinig]

Created attachment 417972 [details]
Patch

[Sam Weinig]

Created attachment 417978 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 417978 [details]
Patch

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

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:490
> +        return makeClampingComponents<SRGBA<uint8_t>>(red, green, blue, alpha); // FIXME: Already clamped, doesn't need to re-clamp. Update parseColorIntOrPercentage/parseAlphaValue to return uint8_t and replace call to makeClampingComponents<SRGBA<uint8_t>> with direct construction of SRGBA<uint8_t>.

A bit hard to parse: "make components that clamp", or "make, clamping components in the process".

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #15)
> Comment on attachment 417978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417978&action=review
> 
> > Source/WebCore/css/parser/CSSParserFastPaths.cpp:490
> > +        return makeClampingComponents<SRGBA<uint8_t>>(red, green, blue, alpha); // FIXME: Already clamped, doesn't need to re-clamp. Update parseColorIntOrPercentage/parseAlphaValue to return uint8_t and replace call to makeClampingComponents<SRGBA<uint8_t>> with direct construction of SRGBA<uint8_t>.
> 
> A bit hard to parse: "make components that clamp", or "make, clamping
> components in the process".

Hm, yeah, I am not a huge fan of the name either. Here are some other ideas:

- makeColorByClampingComponents<SRGBA<uint8_t>>(...)
- colorByClampingComponents<SRGBA<uint8_t>>(...)
- clampColorComponentTo<SRGBA<uint8_t>>(...)
- clampColorTo<SRGBA<uint8_t>>(...)
- clampTo<SRGBA<uint8_t>>(...) would require a bit of finagling of the existing clampTo<> but I think I could make it work.

Any other ones?

[Darin Adler]

If there’s a make function already, we could use "clamping" as a suffix to that name. If it’s makeFromComponents, then makeFromComponentsClamping would be fine I think.

To get a good name we should be thinking about what we are making (a color object?) from what (from color components in a particular color space?) and what special handling we are including (clamping values? converting to another color space?).

Those other names also seem OK.

[Darin Adler]

Comment on attachment 417978 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:907
> +        if (std::isnan(*alpha))
> +            return;

While this is an acceptable rule for the canvas functions, I also think that the clamping functions should either have well-defined behavior if passed NaNs, or assert the passed values are not NaNs.

Kind of annoyed because often we deal with NaNs at the bindings level and maybe we could do that here.

Also if this is not standard and therefore others won’t be adding test coverage to the shared cross-browser test suite, then we should add test coverage of bizarre values.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1311
> +    if (alpha && std::isnan(*alpha))
> +        return;

Maybe here we could just translate the NaN to null? Not sure that’s better, but it all seems so arbitrary.

[Sam Weinig]

(In reply to Darin Adler from comment #18)
> Comment on attachment 417978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417978&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:907
> > +        if (std::isnan(*alpha))
> > +            return;
> 
> While this is an acceptable rule for the canvas functions, I also think that
> the clamping functions should either have well-defined behavior if passed
> NaNs, or assert the passed values are not NaNs.
> 
> Kind of annoyed because often we deal with NaNs at the bindings level and
> maybe we could do that here.
> 
> Also if this is not standard and therefore others won’t be adding test
> coverage to the shared cross-browser test suite, then we should add test
> coverage of bizarre values.

Yes, will add some tests.

> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1311
> > +    if (alpha && std::isnan(*alpha))
> > +        return;
> 
> Maybe here we could just translate the NaN to null? Not sure that’s better,
> but it all seems so arbitrary.

We could also throw. It's definitely a bad value.

[Sam Weinig]

Created attachment 418003 [details]
Patch

[Sam Weinig]

Created attachment 418039 [details]
Patch

[EWS]

Invalid ChangeLog at /Volumes/Data/worker/Commit-Queue/build/Tools/ChangeLog

[EWS]

Invalid ChangeLog at /Volumes/Data/worker/Commit-Queue/build/Tools/ChangeLog

[Sam Weinig]

Created attachment 418045 [details]
Patch

[EWS]

Committed r271695: <https://trac.webkit.org/changeset/271695>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418045 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73456131>