Bug 213673 – CoreImage Implementation of SourceGraphic and saturate(), hue-rotate(), grayscale() and sepia()

• Component: CSS
• Status: RESOLVED FIXED

SourceGraphic contains the graphics elements that were the original input into filters. The first step to render CSS filters is to implement SourceGraphic in CoreImage, since for each possible CSS filter graph, the root is a SourceGraphic. This is a good starting point to transitioning to CoreImage rendering.

[Radar WebKit Bug Importer]

<rdar://problem/64832571>

Created attachment 405293 [details]
Patch

[Sam Weinig]

Comment on attachment 405293 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:196
> +    CIFilter* colorMatrixFilter = [CIFilter filterWithName:@"CIColorMatrix"];
> +    [colorMatrixFilter setValue:inputImage forKey:kCIInputImageKey];
> +    
> +    switch (colorMatrixEffect.type()) {
> +    case FECOLORMATRIX_TYPE_SATURATE:
> +    case FECOLORMATRIX_TYPE_HUEROTATE: {
> +        [colorMatrixFilter setValue:[CIVector vectorWithX:components[0] Y:components[1] Z:components[2] W:0] forKey:@"inputRVector"];
> +        [colorMatrixFilter setValue:[CIVector vectorWithX:components[3] Y:components[4] Z:components[5] W:0] forKey:@"inputGVector"];
> +        [colorMatrixFilter setValue:[CIVector vectorWithX:components[6] Y:components[7] Z:components[8] W:0] forKey:@"inputBVector"];
> +        [colorMatrixFilter setValue:[CIVector vectorWithX:0             Y:0             Z:0             W:1] forKey:@"inputAVector"];
> +        [colorMatrixFilter setValue:[CIVector vectorWithX:0             Y:0             Z:0             W:0] forKey:@"inputBiasVector"];
> +        break;
> +    }

Seems like this is starting to duplicate code that's already in PlatformCAFilters::setFiltersOnLayer(). Can we avoid this?

> PerformanceTests/MotionMark/tests/master/resources/focus.js:84
> -        Utilities.setElementPrefixedProperty(this.particle, "filter", "blur(" + blur + "px) opacity(" + opacity + "%)");
> -        this.particle.style.transform = "translate3d(" + left + "%, " + top + "%, 0)";
> +        Utilities.setElementPrefixedProperty(this.particle, "filter", "saturate(" + blur + "%)");
> +        this.particle.style.transform = "translate2d(" + left + "%, " + top + "%, 0)";

This seems unrelated and probably not something you want to check in.

[Sam Weinig]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 405293 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405293&action=review
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:196
> > +    CIFilter* colorMatrixFilter = [CIFilter filterWithName:@"CIColorMatrix"];
> > +    [colorMatrixFilter setValue:inputImage forKey:kCIInputImageKey];
> > +    
> > +    switch (colorMatrixEffect.type()) {
> > +    case FECOLORMATRIX_TYPE_SATURATE:
> > +    case FECOLORMATRIX_TYPE_HUEROTATE: {
> > +        [colorMatrixFilter setValue:[CIVector vectorWithX:components[0] Y:components[1] Z:components[2] W:0] forKey:@"inputRVector"];
> > +        [colorMatrixFilter setValue:[CIVector vectorWithX:components[3] Y:components[4] Z:components[5] W:0] forKey:@"inputGVector"];
> > +        [colorMatrixFilter setValue:[CIVector vectorWithX:components[6] Y:components[7] Z:components[8] W:0] forKey:@"inputBVector"];
> > +        [colorMatrixFilter setValue:[CIVector vectorWithX:0             Y:0             Z:0             W:1] forKey:@"inputAVector"];
> > +        [colorMatrixFilter setValue:[CIVector vectorWithX:0             Y:0             Z:0             W:0] forKey:@"inputBiasVector"];
> > +        break;
> > +    }
> 
> Seems like this is starting to duplicate code that's already in
> PlatformCAFilters::setFiltersOnLayer(). Can we avoid this?

Nevermind, looks like I am wrong here. HUEROTATE is implemented via [CAFilter filterWithType:kCAFilterColorHueRotate] in PlatformCAFilters (other filters do use CIColorMatrix though). 

Does make me wonder if we should use hueAdjustFilter and other builtin filters for this.

[Darin Adler]

Comment on attachment 405293 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:61
> +    CIImage* feSourceGraphicImage(FilterEffect&);
> +    CIImage* feColorMatrixImage(FilterEffect&, Vector<CIImage*>&);

WebKit coding style says we write CIImage * with a space before the * because this is an Objective-C class.

The input should be const Vector&, not Vector&.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:62
> +        FEColorMatrix& colorMatrixEffect = static_cast<FEColorMatrix&>(effect);

auto&

But also, maybe this can go inside the switch; don’t really need a separate local variable.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:151
> +    SourceGraphic& sourceGraphicEffect = static_cast<SourceGraphic&>(effect);

auto&

Also consider merging with the next line.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:152
> +    ImageBuffer* sourceImage = sourceGraphicEffect.filter().sourceImage();

auto.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:155
> +    NativeImagePtr sourceCGImageRef = sourceImage->copyNativeImage();

auto

Also consider merging with the next line.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:162
> +    CIImage* inputImage = inputImages.at(0);

Normal we’d prefer to use inputImages[0] rather than calling at(). But also, why are we passing in a vector just to extract the first image from it?

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:163
> +    FEColorMatrix& colorMatrixEffect = static_cast<FEColorMatrix&>(effect);

auto&

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:164
> +    Vector<float> values = FEColorMatrix::normalizedFloats(colorMatrixEffect.values());

auto

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:168
> +    case FECOLORMATRIX_TYPE_SATURATE: {

No need for braces here in WebKit coding style. Please don’t use them.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:172
> +    case FECOLORMATRIX_TYPE_HUEROTATE: {

Ditto.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:184
> +    CIFilter* colorMatrixFilter = [CIFilter filterWithName:@"CIColorMatrix"];

auto

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:189
> +    case FECOLORMATRIX_TYPE_HUEROTATE: {

No need for braces here in WebKit coding style. Please don’t use them.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:197
> +    case FECOLORMATRIX_TYPE_MATRIX: {

Ditto.

> Source/WebCore/platform/graphics/filters/FEColorMatrix.h:39
>  class FEColorMatrix : public FilterEffect {

Should be marked final, and platformApplySoftware also should.

> Source/WebCore/platform/graphics/filters/FEColorMatrix.h:41
> +    friend class FilterEffectRendererCoreImage;

We almost never use friend in WebKit. Is this the best solution? What do we need access to? We already have both get and set for all the data members.

(In reply to Sam Weinig from comment #3)
> Comment on attachment 405293 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405293&action=review
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:196
> > +    CIFilter* colorMatrixFilter = [CIFilter filterWithName:@"CIColorMatrix"];
> > +    [colorMatrixFilter setValue:inputImage forKey:kCIInputImageKey];
> > +    
> > +    switch (colorMatrixEffect.type()) {
> > +    case FECOLORMATRIX_TYPE_SATURATE:
> > +    case FECOLORMATRIX_TYPE_HUEROTATE: {
> > +        [colorMatrixFilter setValue:[CIVector vectorWithX:components[0] Y:components[1] Z:components[2] W:0] forKey:@"inputRVector"];
> > +        [colorMatrixFilter setValue:[CIVector vectorWithX:components[3] Y:components[4] Z:components[5] W:0] forKey:@"inputGVector"];
> > +        [colorMatrixFilter setValue:[CIVector vectorWithX:components[6] Y:components[7] Z:components[8] W:0] forKey:@"inputBVector"];
> > +        [colorMatrixFilter setValue:[CIVector vectorWithX:0             Y:0             Z:0             W:1] forKey:@"inputAVector"];
> > +        [colorMatrixFilter setValue:[CIVector vectorWithX:0             Y:0             Z:0             W:0] forKey:@"inputBiasVector"];
> > +        break;
> > +    }
> 
> Seems like this is starting to duplicate code that's already in
> PlatformCAFilters::setFiltersOnLayer(). Can we avoid this?
> 
> > PerformanceTests/MotionMark/tests/master/resources/focus.js:84
> > -        Utilities.setElementPrefixedProperty(this.particle, "filter", "blur(" + blur + "px) opacity(" + opacity + "%)");
> > -        this.particle.style.transform = "translate3d(" + left + "%, " + top + "%, 0)";
> > +        Utilities.setElementPrefixedProperty(this.particle, "filter", "saturate(" + blur + "%)");
> > +        this.particle.style.transform = "translate2d(" + left + "%, " + top + "%, 0)";
> 
> This seems unrelated and probably not something you want to check in.


no, this won't get checked in, this temporary change is for Simon to help me with some performance issues

(In reply to Darin Adler from comment #5)
> Comment on attachment 405293 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405293&action=review
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:61
> > +    CIImage* feSourceGraphicImage(FilterEffect&);
> > +    CIImage* feColorMatrixImage(FilterEffect&, Vector<CIImage*>&);
> 
> WebKit coding style says we write CIImage * with a space before the *
> because this is an Objective-C class.
> 
> The input should be const Vector&, not Vector&.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:62
> > +        FEColorMatrix& colorMatrixEffect = static_cast<FEColorMatrix&>(effect);
> 
> auto&
> 
> But also, maybe this can go inside the switch; don’t really need a separate
> local variable.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:151
> > +    SourceGraphic& sourceGraphicEffect = static_cast<SourceGraphic&>(effect);
> 
> auto&
> 
> Also consider merging with the next line.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:152
> > +    ImageBuffer* sourceImage = sourceGraphicEffect.filter().sourceImage();
> 
> auto.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:155
> > +    NativeImagePtr sourceCGImageRef = sourceImage->copyNativeImage();
> 
> auto
> 
> Also consider merging with the next line.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:162
> > +    CIImage* inputImage = inputImages.at(0);
> 
> Normal we’d prefer to use inputImages[0] rather than calling at(). But also,
> why are we passing in a vector just to extract the first image from it?
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:163
> > +    FEColorMatrix& colorMatrixEffect = static_cast<FEColorMatrix&>(effect);
> 
> auto&
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:164
> > +    Vector<float> values = FEColorMatrix::normalizedFloats(colorMatrixEffect.values());
> 
> auto
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:168
> > +    case FECOLORMATRIX_TYPE_SATURATE: {
> 
> No need for braces here in WebKit coding style. Please don’t use them.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:172
> > +    case FECOLORMATRIX_TYPE_HUEROTATE: {
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:184
> > +    CIFilter* colorMatrixFilter = [CIFilter filterWithName:@"CIColorMatrix"];
> 
> auto
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:189
> > +    case FECOLORMATRIX_TYPE_HUEROTATE: {
> 
> No need for braces here in WebKit coding style. Please don’t use them.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:197
> > +    case FECOLORMATRIX_TYPE_MATRIX: {
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/filters/FEColorMatrix.h:39
> >  class FEColorMatrix : public FilterEffect {
> 
> Should be marked final, and platformApplySoftware also should.
> 
> > Source/WebCore/platform/graphics/filters/FEColorMatrix.h:41
> > +    friend class FilterEffectRendererCoreImage;
> 
> We almost never use friend in WebKit. Is this the best solution? What do we
> need access to? We already have both get and set for all the data members.

There are some functions in FEColorMatrix, for example, calculateSaturateComponents that is a protected member function, not sure if we want to make it public though. Any thoughts? This applies to other FilterEffects as well

[Darin Adler]

Comment on attachment 405293 [details]
Patch

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

>>> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:61
>>> +    CIImage* feColorMatrixImage(FilterEffect&, Vector<CIImage*>&);
>> 
>> WebKit coding style says we write CIImage * with a space before the * because this is an Objective-C class.
>> 
>> The input should be const Vector&, not Vector&.
> 
> There are some functions in FEColorMatrix, for example, calculateSaturateComponents that is a protected member function, not sure if we want to make it public though. Any thoughts? This applies to other FilterEffects as well

My first thought would be to make things public before starting to use friend.

Created attachment 407508 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 407508 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Tests: css3/filters/effect-grayscale-square.html
> +               css3/filters/effect-hue-rotate-square.html
> +               css3/filters/effect-saturate-square.html
> +               css3/filters/effect-sepia-square.html

Since you're adding caching of CIFilters here, you need
1. a test that renders twice, changing the value of an input in between
2. a test that renders twice, changing the filter chain
3. Maybe more.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:61
> +    CIImage *feSourceGraphicImage(SourceGraphic&);

imageForSourceGraphic

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:62
> +    CIImage *feColorMatrixImage(FEColorMatrix&, const Vector<CIImage *>&, int);

imageForFEMatrixFilter. Name the non-obvious parameters.

I think it would nicer to explicitly ref-count these and return RetainPtr<CIImage>

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:65
> +    HashMap<int, Vector<RetainPtr<CIFilter>>> m_ciFilterStorageMap;

What is the key? Why did you have to change this?

I think we can also improve the name of m_ciFilterStorageMap. It's clear from the type that it's a map, and of course it's storage. The name should describe how it maps. fooToFooMap.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:57
> +CIContext *FilterEffectRendererCoreImage::getCIContext()
> +{
> +    static NeverDestroyed<RetainPtr<CIContext>> ciContext;
> +    if (!ciContext.get())
> +        ciContext.get() = adoptNS([CIContext contextWithOptions: @{ kCIContextWorkingColorSpace: (__bridge id)CGColorSpaceCreateWithName(kCGColorSpaceSRGB)}]);
> +    return [ciContext.get() retain];
> +}

I don't think we can use a single shared CIContext. We risk state leaking between documents, which might lead to things like timing attacks. Maybe cache one per Document (e.g. via RenderLayerCompositor).

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:67
> +        switch (static_cast<FEColorMatrix&>(effect).type()) {

You should add SPECIALIZE_TYPE_TRAITS for FilterEffect subclasses to avoid this static_cast. This should be a downcast<>.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:109
> +CIImage *FilterEffectRendererCoreImage::connectCIFilters(FilterEffect& effect, int traversalOrder)

Order or index?

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:172
> +CIImage *FilterEffectRendererCoreImage::feColorMatrixImage(FEColorMatrix& effect, const Vector<CIImage *>& inputImages, int traversalOrder)

This is both creating the filter, and setting the inputImage. Might be clearer to split those two responsibilities.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:204
> +        Vector<RetainPtr<CIFilter>> filtersToBeCached;
> +        filtersToBeCached.insert(0, colorMatrixFilter);
> +        m_ciFilterStorageMap.add(traversalOrder, filtersToBeCached);

There's a way to do this with map.ensure().

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:271
> +    CIRenderDestination* destination = [[[CIRenderDestination alloc] initWithIOSurface:(::IOSurface*)surface.surface()] autorelease];

Use RetainPtr<> and avoid autorelease.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:297
> +    , m_ciFilterStorageMap()

Not necessary.

[Darin Adler]

Comment on attachment 407508 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:55
> +    static NeverDestroyed<RetainPtr<CIContext>> ciContext;
> +    if (!ciContext.get())
> +        ciContext.get() = adoptNS([CIContext contextWithOptions: @{ kCIContextWorkingColorSpace: (__bridge id)CGColorSpaceCreateWithName(kCGColorSpaceSRGB)}]);

This should just be a single line. There’s no need for this three line form.

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 407508 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407508&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Tests: css3/filters/effect-grayscale-square.html
> > +               css3/filters/effect-hue-rotate-square.html
> > +               css3/filters/effect-saturate-square.html
> > +               css3/filters/effect-sepia-square.html
> 
> Since you're adding caching of CIFilters here, you need
> 1. a test that renders twice, changing the value of an input in between
> 2. a test that renders twice, changing the filter chain
> 3. Maybe more.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:61
> > +    CIImage *feSourceGraphicImage(SourceGraphic&);
> 
> imageForSourceGraphic
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:62
> > +    CIImage *feColorMatrixImage(FEColorMatrix&, const Vector<CIImage *>&, int);
> 
> imageForFEMatrixFilter. Name the non-obvious parameters.
> 
> I think it would nicer to explicitly ref-count these and return
> RetainPtr<CIImage>
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:65
> > +    HashMap<int, Vector<RetainPtr<CIFilter>>> m_ciFilterStorageMap;
> 
> What is the key? Why did you have to change this?

So originally I chose to use FilterEffect nodes as the key, but turns out FilterEffects gets destroyed and rebuilt on each render update, so that cannot be a key anymore, so instead I chose one thing that's consistent across each draw is the graph itself, as in the structure of the filter graph, and thus the traversal order of this graph is consistent between updates. And therefore it could be used as a hash map key

> 
> I think we can also improve the name of m_ciFilterStorageMap. It's clear
> from the type that it's a map, and of course it's storage. The name should
> describe how it maps. fooToFooMap.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:57
> > +CIContext *FilterEffectRendererCoreImage::getCIContext()
> > +{
> > +    static NeverDestroyed<RetainPtr<CIContext>> ciContext;
> > +    if (!ciContext.get())
> > +        ciContext.get() = adoptNS([CIContext contextWithOptions: @{ kCIContextWorkingColorSpace: (__bridge id)CGColorSpaceCreateWithName(kCGColorSpaceSRGB)}]);
> > +    return [ciContext.get() retain];
> > +}
> 
> I don't think we can use a single shared CIContext. We risk state leaking
> between documents, which might lead to things like timing attacks. Maybe
> cache one per Document (e.g. via RenderLayerCompositor).
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:67
> > +        switch (static_cast<FEColorMatrix&>(effect).type()) {
> 
> You should add SPECIALIZE_TYPE_TRAITS for FilterEffect subclasses to avoid
> this static_cast. This should be a downcast<>.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:109
> > +CIImage *FilterEffectRendererCoreImage::connectCIFilters(FilterEffect& effect, int traversalOrder)
> 
> Order or index?
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:172
> > +CIImage *FilterEffectRendererCoreImage::feColorMatrixImage(FEColorMatrix& effect, const Vector<CIImage *>& inputImages, int traversalOrder)
> 
> This is both creating the filter, and setting the inputImage. Might be
> clearer to split those two responsibilities.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:204
> > +        Vector<RetainPtr<CIFilter>> filtersToBeCached;
> > +        filtersToBeCached.insert(0, colorMatrixFilter);
> > +        m_ciFilterStorageMap.add(traversalOrder, filtersToBeCached);
> 
> There's a way to do this with map.ensure().
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:271
> > +    CIRenderDestination* destination = [[[CIRenderDestination alloc] initWithIOSurface:(::IOSurface*)surface.surface()] autorelease];
> 
> Use RetainPtr<> and avoid autorelease.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:297
> > +    , m_ciFilterStorageMap()
> 
> Not necessary.

[Myles C. Maxfield]

> I don't think we can use a single shared CIContext. We risk state leaking between documents, which might lead to things like timing attacks. Maybe cache one per Document (e.g. via RenderLayerCompositor).

There is no (architectural) state on the CIContext that is modified by our filter code, so the only state we would be leaking are things like "has this web process rendered a blur filter before" or "has this web process rendered a color matrix filter before." It seems like this wouldn't be sensitive information; so what if the web process has rendered that stuff before?

Indeed, it may even be better to have a singleton because that timing attack is possible: the second webpage the user navigates to would perform faster because it can re-use shader compilation artifacts etc. from the first webpage the user navigates to. It seems like a waste of perf trying to protect benign information.

(This is also how fonts work; we don't clear the font cache on every page load...)

Created attachment 407650 [details]
Patch

Created attachment 407691 [details]
Patch

[Darin Adler]

Comment on attachment 407691 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:53
> +    static NeverDestroyed<RetainPtr<CIContext>> ciContext = adoptNS([CIContext contextWithOptions: @{ kCIContextWorkingColorSpace: (__bridge id)CGColorSpaceCreateWithName(kCGColorSpaceSRGB)}]);

This over-releases. It’s incorrect to call adoptNS on the result of +[CIContext contextWithOptions:] because that function returns something that’s autoreleased. Simplest way to fix this is to remove the adoptNS.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:54
> +    return [ciContext.get() retain];

This over-retains. Putting a CIContext into a RetainPtr<CIContext> will already retain it. This double retains it every time this function is called.

[Darin Adler]

Comment on attachment 407691 [details]
Patch

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

>> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:54
>> +    return [ciContext.get() retain];
> 
> This over-retains. Putting a CIContext into a RetainPtr<CIContext> will already retain it. This double retains it every time this function is called.

Simplest way to fix this is to just write:

    return ciContext;

[Darin Adler]

Comment on attachment 407691 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:163
> +        auto& surface = downcast<AcceleratedImageBuffer>(*sourceImage).surface();
> +        IOSurfaceRef surfaceRef = surface.surface();
> +        return RetainPtr<CIImage>([CIImage imageWithIOSurface:surfaceRef]);

This should not require an explicit cast to RetainPtr. Also, the local variables aren’t helping. I suggest doing it in a single line:

    return [CIImage imageWithIOSurface:downcast<AcceleratedImageBuffer>(*sourceImage).surface().surface()];

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:166
> +    NativeImagePtr sourceCGImageRef = sourceImage->copyNativeImage();
> +    return RetainPtr<CIImage>([CIImage imageWithCGImage:sourceCGImageRef.get()]);

This should not require an explicit cast to RetainPtr. Also, the local variable isn’t helping. I suggest doing it in a single line:

    return [CIImage imageWithCGImage:sourceImage->copyNativeImage().get()];

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:218
> +    return RetainPtr<CIImage>(colorMatrixFilter.outputImage);

This should not require an explicit cast to RetainPtr.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:258
> +    RetainPtr<CIRenderDestination> destination = adoptNS([[CIRenderDestination alloc] initWithIOSurface:(::IOSurface*)surface.surface()]);

Pleas use auto here.

I am surprised that a cast to IOSurface* is required. If it’s not, please omit it. Even if it is required, I suggest the (IOSurface*) syntax rather than (::IOSurface*) syntax.

(In reply to Darin Adler from comment #18)
> Comment on attachment 407691 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407691&action=review
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:163
> > +        auto& surface = downcast<AcceleratedImageBuffer>(*sourceImage).surface();
> > +        IOSurfaceRef surfaceRef = surface.surface();
> > +        return RetainPtr<CIImage>([CIImage imageWithIOSurface:surfaceRef]);
> 
> This should not require an explicit cast to RetainPtr. Also, the local
> variables aren’t helping. I suggest doing it in a single line:
> 
>     return [CIImage
> imageWithIOSurface:downcast<AcceleratedImageBuffer>(*sourceImage).surface().
> surface()];
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:166
> > +    NativeImagePtr sourceCGImageRef = sourceImage->copyNativeImage();
> > +    return RetainPtr<CIImage>([CIImage imageWithCGImage:sourceCGImageRef.get()]);
> 
> This should not require an explicit cast to RetainPtr. Also, the local
> variable isn’t helping. I suggest doing it in a single line:
> 
>     return [CIImage imageWithCGImage:sourceImage->copyNativeImage().get()];
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:218
> > +    return RetainPtr<CIImage>(colorMatrixFilter.outputImage);
> 
> This should not require an explicit cast to RetainPtr.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:258
> > +    RetainPtr<CIRenderDestination> destination = adoptNS([[CIRenderDestination alloc] initWithIOSurface:(::IOSurface*)surface.surface()]);
> 
> Pleas use auto here.
> 
> I am surprised that a cast to IOSurface* is required. If it’s not, please
> omit it. Even if it is required, I suggest the (IOSurface*) syntax rather
> than (::IOSurface*) syntax.

The problem here is that there is a name collision, there is a WebCore::IOSurface and an Objective-C IOSurface, and without ::IOSurface* the compiler always resolve (IOSurface*) to WebCore::IOSurface*

[Darin Adler]

(In reply to guowei_yang from comment #19)
> (In reply to Darin Adler from comment #18)
> > I am surprised that a cast to IOSurface* is required. If it’s not, please
> > omit it. Even if it is required, I suggest the (IOSurface*) syntax rather
> > than (::IOSurface*) syntax.
> 
> The problem here is that there is a name collision, there is a
> WebCore::IOSurface and an Objective-C IOSurface, and without ::IOSurface*
> the compiler always resolve (IOSurface*) to WebCore::IOSurface*

OK. But can you just remove the cast?

(In reply to Darin Adler from comment #20)
> (In reply to guowei_yang from comment #19)
> > (In reply to Darin Adler from comment #18)
> > > I am surprised that a cast to IOSurface* is required. If it’s not, please
> > > omit it. Even if it is required, I suggest the (IOSurface*) syntax rather
> > > than (::IOSurface*) syntax.
> > 
> > The problem here is that there is a name collision, there is a
> > WebCore::IOSurface and an Objective-C IOSurface, and without ::IOSurface*
> > the compiler always resolve (IOSurface*) to WebCore::IOSurface*
> 
> OK. But can you just remove the cast?

Hmmm I don't think so, .surface() returns an IOSurfaceRef, which is "an rvalue of type 'IOSurfaceRef' (aka '__IOSurface *')", but initWithIOSurface expects "IOSurface * _Nonnull"

[Darin Adler]

(In reply to guowei_yang from comment #21)
> Hmmm I don't think so, .surface() returns an IOSurfaceRef, which is "an
> rvalue of type 'IOSurfaceRef' (aka '__IOSurface *')", but initWithIOSurface
> expects "IOSurface * _Nonnull"

I see, it’s a toll-free bridged type, but it’s the CoreFoundation vs. Objective-C version of that type. So this is a bridge cast.

Comment on attachment 407691 [details]
Patch

I am going to temporarily change the review status to r? so that I can investigate failed builds

Created attachment 407790 [details]
Patch

[Darin Adler]

Comment on attachment 407790 [details]
Patch

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

Looks good. A few optional ideas for further refinement.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:53
> +    static NeverDestroyed<RetainPtr<CIContext>> ciContext = [CIContext contextWithOptions: @{ kCIContextWorkingColorSpace: (__bridge id)CGColorSpaceCreateWithName(kCGColorSpaceSRGB)}];

Another way to write this is:

    static auto context = makeNeverDestroyed([CIContext ...]);

I think it’s nicer.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:156
> +    ImageBuffer* sourceImage = effect.filter().sourceImage();

auto

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:170
> +    Vector<float> values = FEColorMatrix::normalizedFloats(effect.values());

auto

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:190
> +    CIFilter *colorMatrixFilter = [CIFilter filterWithName:@"CIColorMatrix"];

auto

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:252
> +    auto ciContext = getCIContext();

Not sure we need a local variable.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:179
> +        Vector<float> normalizedValues(values.size());
> +        for (size_t i = 0; i < values.size(); ++i)
> +            normalizedValues[i] = normalizedFloat(values[i]);
> +        return normalizedValues;

This could be nicer if it used Vector::map instead of writing a loop.

> Source/WebCore/rendering/CSSFilter.cpp:343
> -        setSourceImage(ImageBuffer::create(logicalSize, renderingMode(), filterScale()));
> +        if (m_filterRenderer)
> +            setSourceImage(ImageBuffer::create(logicalSize, RenderingMode::Accelerated, filterScale()));
> +        else
> +            setSourceImage(ImageBuffer::create(logicalSize, renderingMode(), filterScale()));

Could do it with a local instead of an if statement:

    auto mode = m_filterRenderer ? RenderingMode::Accelerated : renderingMode();
    setSourceImage(ImageBuffer::create(logicalSize, mode, filterScale()));

[Simon Fraser (smfr)]

Comment on attachment 407790 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:47
> +    RetainPtr<CIContext> getCIContext();

This should be a static function. I think you should name it sharedCIContext() to make it clear it returns a global.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:61
> +    RetainPtr<CIImage> imageForSourceGraphic(SourceGraphic&);

Can the argument be const SourceGraphic&?

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:62
> +    RetainPtr<CIImage> imageForFEColorMatrix(FEColorMatrix&, const Vector<RetainPtr<CIImage>>&);

const FEColorMatrix&

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:188
> +    case FECOLORMATRIX_TYPE_SATURATE: {
> +        FEColorMatrix::calculateSaturateComponents(components, values[0]);
> +        break;
> +    }
> +    case FECOLORMATRIX_TYPE_HUEROTATE: {
> +        FEColorMatrix::calculateHueRotateComponents(components, values[0]);
> +        break;
> +    }
> +    case FECOLORMATRIX_TYPE_MATRIX:
> +        break;
> +        
> +    case FECOLORMATRIX_TYPE_UNKNOWN:
> +    case FECOLORMATRIX_TYPE_LUMINANCETOALPHA: // FIXME: Add Luminance to Alpha Implementation
> +        return nullptr;
> +    }

You don't need any of the braces.

(In reply to Simon Fraser (smfr) from comment #26)
> Comment on attachment 407790 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407790&action=review
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:47
> > +    RetainPtr<CIContext> getCIContext();
> 
> This should be a static function. I think you should name it
> sharedCIContext() to make it clear it returns a global.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:61
> > +    RetainPtr<CIImage> imageForSourceGraphic(SourceGraphic&);
> 
> Can the argument be const SourceGraphic&?
Unfortunately, I need to access some of the non-const functions in FilterEffect and/or it's derived class I don't think this argument could be const
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:62
> > +    RetainPtr<CIImage> imageForFEColorMatrix(FEColorMatrix&, const Vector<RetainPtr<CIImage>>&);
> 
> const FEColorMatrix&
ditto
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:188
> > +    case FECOLORMATRIX_TYPE_SATURATE: {
> > +        FEColorMatrix::calculateSaturateComponents(components, values[0]);
> > +        break;
> > +    }
> > +    case FECOLORMATRIX_TYPE_HUEROTATE: {
> > +        FEColorMatrix::calculateHueRotateComponents(components, values[0]);
> > +        break;
> > +    }
> > +    case FECOLORMATRIX_TYPE_MATRIX:
> > +        break;
> > +        
> > +    case FECOLORMATRIX_TYPE_UNKNOWN:
> > +    case FECOLORMATRIX_TYPE_LUMINANCETOALPHA: // FIXME: Add Luminance to Alpha Implementation
> > +        return nullptr;
> > +    }
> 
> You don't need any of the braces.

(In reply to guowei_yang from comment #27)
> (In reply to Simon Fraser (smfr) from comment #26)
> > Comment on attachment 407790 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=407790&action=review
> > 
> > > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:47
> > > +    RetainPtr<CIContext> getCIContext();
> > 
> > This should be a static function. I think you should name it
> > sharedCIContext() to make it clear it returns a global.
> > 
> > > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:61
> > > +    RetainPtr<CIImage> imageForSourceGraphic(SourceGraphic&);
> > 
> > Can the argument be const SourceGraphic&?
> Unfortunately, I need to access some of the non-const functions in
> FilterEffect and/or it's derived class I don't think this argument could be
> const
> > 
> > > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:62
> > > +    RetainPtr<CIImage> imageForFEColorMatrix(FEColorMatrix&, const Vector<RetainPtr<CIImage>>&);
> > 
> > const FEColorMatrix&
> ditto
Actually, this can be const, but not SourceGraphic, since FilterEffect::filter() is not const
> > 
> > > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:188
> > > +    case FECOLORMATRIX_TYPE_SATURATE: {
> > > +        FEColorMatrix::calculateSaturateComponents(components, values[0]);
> > > +        break;
> > > +    }
> > > +    case FECOLORMATRIX_TYPE_HUEROTATE: {
> > > +        FEColorMatrix::calculateHueRotateComponents(components, values[0]);
> > > +        break;
> > > +    }
> > > +    case FECOLORMATRIX_TYPE_MATRIX:
> > > +        break;
> > > +        
> > > +    case FECOLORMATRIX_TYPE_UNKNOWN:
> > > +    case FECOLORMATRIX_TYPE_LUMINANCETOALPHA: // FIXME: Add Luminance to Alpha Implementation
> > > +        return nullptr;
> > > +    }
> > 
> > You don't need any of the braces.

(In reply to Darin Adler from comment #25)
> Comment on attachment 407790 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407790&action=review
> 
> Looks good. A few optional ideas for further refinement.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:53
> > +    static NeverDestroyed<RetainPtr<CIContext>> ciContext = [CIContext contextWithOptions: @{ kCIContextWorkingColorSpace: (__bridge id)CGColorSpaceCreateWithName(kCGColorSpaceSRGB)}];
> 
> Another way to write this is:
> 
>     static auto context = makeNeverDestroyed([CIContext ...]);
Seems that using auto will resolve the return type to be WTF::NeverDestroyed<CIContext *, WTF::AnyThreadsAccessTraits>, which didn't get converted to RetainPtr, so I am keeping it as is
> 
> I think it’s nicer.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:156
> > +    ImageBuffer* sourceImage = effect.filter().sourceImage();
> 
> auto
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:170
> > +    Vector<float> values = FEColorMatrix::normalizedFloats(effect.values());
> 
> auto
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:190
> > +    CIFilter *colorMatrixFilter = [CIFilter filterWithName:@"CIColorMatrix"];
> 
> auto
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:252
> > +    auto ciContext = getCIContext();
> 
> Not sure we need a local variable.
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.h:179
> > +        Vector<float> normalizedValues(values.size());
> > +        for (size_t i = 0; i < values.size(); ++i)
> > +            normalizedValues[i] = normalizedFloat(values[i]);
> > +        return normalizedValues;
> 
> This could be nicer if it used Vector::map instead of writing a loop.
> 
> > Source/WebCore/rendering/CSSFilter.cpp:343
> > -        setSourceImage(ImageBuffer::create(logicalSize, renderingMode(), filterScale()));
> > +        if (m_filterRenderer)
> > +            setSourceImage(ImageBuffer::create(logicalSize, RenderingMode::Accelerated, filterScale()));
> > +        else
> > +            setSourceImage(ImageBuffer::create(logicalSize, renderingMode(), filterScale()));
> 
> Could do it with a local instead of an if statement:
> 
>     auto mode = m_filterRenderer ? RenderingMode::Accelerated :
> renderingMode();
>     setSourceImage(ImageBuffer::create(logicalSize, mode, filterScale()));

Created attachment 407804 [details]
Ready for commit

[Darin Adler]

Comment on attachment 407790 [details]
Patch

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

>>> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:53
>>> +    static NeverDestroyed<RetainPtr<CIContext>> ciContext = [CIContext contextWithOptions: @{ kCIContextWorkingColorSpace: (__bridge id)CGColorSpaceCreateWithName(kCGColorSpaceSRGB)}];
>> 
>> Another way to write this is:
>> 
>>     static auto context = makeNeverDestroyed([CIContext ...]);
>> 
>> I think it’s nicer.
> 
> Seems that using auto will resolve the return type to be WTF::NeverDestroyed<CIContext *, WTF::AnyThreadsAccessTraits>, which didn't get converted to RetainPtr, so I am keeping it as is

Yes, overlooked that. It would need to be:

    static auto context = makeNeverDestroyed(retainPtr([CIContext ...]));

Created attachment 407837 [details]
Ready for Commit

[EWS]

[e-mail] does not have committer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 407837 [details] from commit queue.

[EWS]

Committed r266542: <https://trac.webkit.org/changeset/266542>

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