Bug 215956 – CoreImage Implementation of CSS Filters invert(), opacity(), brightness(), contrast()

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

Implementing invert(), opacity(), brightness(), contrast(), which are based on FEComponentTransfer using CoreImage

[Radar WebKit Bug Importer]

<rdar://problem/67968206>

Created attachment 407622 [details]
Patch

This patch depends on path 213673, so it will temporarily contain changes already introduced in 213673. Once 213673 is landed, the only changes in this patch should only be the two additional functions, 
  - feComponentTransferLinearFunctionImage
  - componentTransferLinearFunctionOnly

Created attachment 407625 [details]
Patch

Created attachment 407920 [details]
Patch

[Darin Adler]

Comment on attachment 407920 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:31
> +#import "FEComponentTransfer.h"

Could forward declare this instead of including it in the header. Nothing in the header requires the full definition.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:62
> +    static bool isNullOrLinearComponentTransferFunction(const FEComponentTransfer&);

Could just make this a free function in the .cpp file instead of a static member function.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:72
> +    if (effect.redFunction().type != FECOMPONENTTRANSFER_TYPE_UNKNOWN
> +        && effect.redFunction().type != FECOMPONENTTRANSFER_TYPE_LINEAR)
> +        return false;
> +    if (effect.greenFunction().type != FECOMPONENTTRANSFER_TYPE_UNKNOWN
> +        && effect.greenFunction().type != FECOMPONENTTRANSFER_TYPE_LINEAR)
> +        return false;
> +    if (effect.blueFunction().type != FECOMPONENTTRANSFER_TYPE_UNKNOWN
> +        && effect.blueFunction().type != FECOMPONENTTRANSFER_TYPE_LINEAR)
> +        return false;
> +    if (effect.alphaFunction().type != FECOMPONENTTRANSFER_TYPE_UNKNOWN
> +        && effect.alphaFunction().type != FECOMPONENTTRANSFER_TYPE_LINEAR)
> +        return false;
> +    return true;

This very wordy thing could be cut down to size with a lambda:

    auto isNullOrLinear = [] (const ComponentTransferFunction& function) {
        return function.type == FECOMPONENTTRANSFER_TYPE_UNKNOWN || function.type == FECOMPONENTTRANSFER_TYPE_LINEAR;
    };
    return isNullOrLinear(effect.redFunction()) && isNullOrLinear(effect.greenFunction())
        && isNullOrLinear(effect.blueFunction()) && isNullOrLinear(effect.alphaFunction());

I think the && is also easier to read than the backwards logic and early returns.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:243
> +    if (!isNullOrLinearComponentTransferFunction(effect))
> +        return nullptr;

Seems like this should be an assertion instead of a "return null" thing.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:245
> +    
> +

Extra blank line here.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:247
> +    CIFilter* componentTransferFilter = [CIFilter filterWithName:@"CIColorPolynomial"];

auto

Also how about using a shorter local variable name, like "filter"? The function is a narrow enough scope that the meaning of the name is clear enough, I think.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:252
> +    ComponentTransferFunction redFunction = effect.redFunction();
> +    ComponentTransferFunction greenFunction = effect.greenFunction();
> +    ComponentTransferFunction blueFunction = effect.blueFunction();
> +    ComponentTransferFunction alphaFunction = effect.alphaFunction();

auto&

There is no need to copy these functions out of the effect object.

Could also consider shorter names. I think we could call the locals just red, green, blue, alpha given the scope of a short function like this. No real ambiguity about what they mean.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:263
> +    if (redFunction.type == FECOMPONENTTRANSFER_TYPE_LINEAR)
> +        [componentTransferFilter setValue:[CIVector vectorWithX:redFunction.intercept   Y:redFunction.slope    Z:0  W:0] forKey:@"inputRedCoefficients"];
> +    if (greenFunction.type == FECOMPONENTTRANSFER_TYPE_LINEAR)
> +        [componentTransferFilter setValue:[CIVector vectorWithX:greenFunction.intercept Y:greenFunction.slope  Z:0  W:0] forKey:@"inputGreenCoefficients"];
> +    if (blueFunction.type == FECOMPONENTTRANSFER_TYPE_LINEAR)
> +        [componentTransferFilter setValue:[CIVector vectorWithX:blueFunction.intercept  Y:blueFunction.slope   Z:0  W:0] forKey:@"inputBlueCoefficients"];
> +    if (alphaFunction.type == FECOMPONENTTRANSFER_TYPE_LINEAR)
> +        [componentTransferFilter setValue:[CIVector vectorWithX:alphaFunction.intercept Y:alphaFunction.slope  Z:0  W:0] forKey:@"inputAlphaCoefficients"];

Code can be simplified with a lambda:

    auto setCoefficients = [&] (NSString *key, const ComponentFunction& function) {
        if (function.type == FECOMPONENTTRANSFER_TYPE_LINEAR)
            [componentTransferFilter setValue:[CIVector vectorWithX:function.intercept Y:function.slope Z:0 W:0] forKey:key];
    };
    setCoefficients(@"inputRedCoefficients", effect.redFunction());
    setCoefficients(@"inputGreenCoefficients", effect.greenFunction());
    setCoefficients(@"inputBlueCoefficients", effect.blueFunction());
    setCoefficients(@"inputAlphaCoefficients", effect.alphaFunction());

This replaces 12 lines of more hard-to-read code.

Created attachment 407926 [details]
Patch

[Darin Adler]

Comment on attachment 407926 [details]
Patch

Please check out my comments on the earlier version of the patch.

(In reply to Darin Adler from comment #8)
> Comment on attachment 407926 [details]
> Patch
> 
> Please check out my comments on the earlier version of the patch.

Oh whoops just saw the comments, I immediately obsoleted the one you commented on because I forgot to include the test files. Working on it right now.

(In reply to Darin Adler from comment #6)
> Comment on attachment 407920 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407920&action=review
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:31
> > +#import "FEComponentTransfer.h"
> 
> Could forward declare this instead of including it in the header. Nothing in
> the header requires the full definition.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:62
> > +    static bool isNullOrLinearComponentTransferFunction(const FEComponentTransfer&);
> 
> Could just make this a free function in the .cpp file instead of a static
> member function.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:72
> > +    if (effect.redFunction().type != FECOMPONENTTRANSFER_TYPE_UNKNOWN
> > +        && effect.redFunction().type != FECOMPONENTTRANSFER_TYPE_LINEAR)
> > +        return false;
> > +    if (effect.greenFunction().type != FECOMPONENTTRANSFER_TYPE_UNKNOWN
> > +        && effect.greenFunction().type != FECOMPONENTTRANSFER_TYPE_LINEAR)
> > +        return false;
> > +    if (effect.blueFunction().type != FECOMPONENTTRANSFER_TYPE_UNKNOWN
> > +        && effect.blueFunction().type != FECOMPONENTTRANSFER_TYPE_LINEAR)
> > +        return false;
> > +    if (effect.alphaFunction().type != FECOMPONENTTRANSFER_TYPE_UNKNOWN
> > +        && effect.alphaFunction().type != FECOMPONENTTRANSFER_TYPE_LINEAR)
> > +        return false;
> > +    return true;
> 
> This very wordy thing could be cut down to size with a lambda:
> 
>     auto isNullOrLinear = [] (const ComponentTransferFunction& function) {
>         return function.type == FECOMPONENTTRANSFER_TYPE_UNKNOWN ||
> function.type == FECOMPONENTTRANSFER_TYPE_LINEAR;
>     };
>     return isNullOrLinear(effect.redFunction()) &&
> isNullOrLinear(effect.greenFunction())
>         && isNullOrLinear(effect.blueFunction()) &&
> isNullOrLinear(effect.alphaFunction());
> 
> I think the && is also easier to read than the backwards logic and early
> returns.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:243
> > +    if (!isNullOrLinearComponentTransferFunction(effect))
> > +        return nullptr;
> 
> Seems like this should be an assertion instead of a "return null" thing.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:245
> > +    
> > +
> 
> Extra blank line here.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:247
> > +    CIFilter* componentTransferFilter = [CIFilter filterWithName:@"CIColorPolynomial"];
> 
> auto
> 
> Also how about using a shorter local variable name, like "filter"? The
> function is a narrow enough scope that the meaning of the name is clear
> enough, I think.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:252
> > +    ComponentTransferFunction redFunction = effect.redFunction();
> > +    ComponentTransferFunction greenFunction = effect.greenFunction();
> > +    ComponentTransferFunction blueFunction = effect.blueFunction();
> > +    ComponentTransferFunction alphaFunction = effect.alphaFunction();
> 
> auto&
since redFunction(), and those function are all marked const, we need "const auto&"
> 
> There is no need to copy these functions out of the effect object.
> 
> Could also consider shorter names. I think we could call the locals just
> red, green, blue, alpha given the scope of a short function like this. No
> real ambiguity about what they mean.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:263
> > +    if (redFunction.type == FECOMPONENTTRANSFER_TYPE_LINEAR)
> > +        [componentTransferFilter setValue:[CIVector vectorWithX:redFunction.intercept   Y:redFunction.slope    Z:0  W:0] forKey:@"inputRedCoefficients"];
> > +    if (greenFunction.type == FECOMPONENTTRANSFER_TYPE_LINEAR)
> > +        [componentTransferFilter setValue:[CIVector vectorWithX:greenFunction.intercept Y:greenFunction.slope  Z:0  W:0] forKey:@"inputGreenCoefficients"];
> > +    if (blueFunction.type == FECOMPONENTTRANSFER_TYPE_LINEAR)
> > +        [componentTransferFilter setValue:[CIVector vectorWithX:blueFunction.intercept  Y:blueFunction.slope   Z:0  W:0] forKey:@"inputBlueCoefficients"];
> > +    if (alphaFunction.type == FECOMPONENTTRANSFER_TYPE_LINEAR)
> > +        [componentTransferFilter setValue:[CIVector vectorWithX:alphaFunction.intercept Y:alphaFunction.slope  Z:0  W:0] forKey:@"inputAlphaCoefficients"];
> 
> Code can be simplified with a lambda:
> 
>     auto setCoefficients = [&] (NSString *key, const ComponentFunction&
> function) {
>         if (function.type == FECOMPONENTTRANSFER_TYPE_LINEAR)
>             [componentTransferFilter setValue:[CIVector
> vectorWithX:function.intercept Y:function.slope Z:0 W:0] forKey:key];
>     };
>     setCoefficients(@"inputRedCoefficients", effect.redFunction());
>     setCoefficients(@"inputGreenCoefficients", effect.greenFunction());
>     setCoefficients(@"inputBlueCoefficients", effect.blueFunction());
>     setCoefficients(@"inputAlphaCoefficients", effect.alphaFunction());
> 
> This replaces 12 lines of more hard-to-read code.

Created attachment 408006 [details]
Patch

[Darin Adler]

Comment on attachment 408006 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:68
> +        return isNullOrLinear(effect.redFunction()) && isNullOrLinear(effect.greenFunction())
> +            && isNullOrLinear(effect.blueFunction()) && isNullOrLinear(effect.alphaFunction());

This is indented too much.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:241
> +    CIFilter* filter = [CIFilter filterWithName:@"CIColorPolynomial"];

I suggest auto here.

Created attachment 408019 [details]
Ready for commit

Created attachment 408024 [details]
Ready for commit

Created attachment 408026 [details]
Patch (missing reviewer name)

Created attachment 408247 [details]
Ready for Commit

[EWS]

Committed r266740: <https://trac.webkit.org/changeset/266740>

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