Bug 215957

<rdar://problem/67968258>

Created attachment 407626 [details]
Patch

Comment on attachment 407626 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:60
>      static bool supportsCoreImageRendering(FilterEffect&);
>      static bool canRenderUsingCIFilters(FilterEffect&);
>      
> +    CIImage* feGaussianBlurImage(FilterEffect&, Vector<CIImage*>&, int);

The should take a const FilterEffect&

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

I don't like the int as the key.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:136
> +    Vector<RetainPtr<CIFilter>> cachedCIFilters = m_ciFilterStorageMap.get(traversalOrder);

This line is repeated at 143.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:138
> +    FEGaussianBlur& blurEffect = static_cast<FEGaussianBlur&>(effect);

downcast<>, you need to add the macros.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:146
> +        gaussianBlurFilter = [CIFilter filterWithName:@"CIGaussianBlurXY"];

RetainPtr<>. Can we make CIFilters without autorelease?

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:149
> +        filtersToBeCached.insert(0, gaussianBlurFilter);
> +        m_ciFilterStorageMap.add(traversalOrder, filtersToBeCached);

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

Comment on attachment 407626 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:55
> +    CIImage* connectCIFilters(FilterEffect&, int);

The int parameter here should have a parameter name, as it is not clear from context what it means.

>> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:138
>> +    FEGaussianBlur& blurEffect = static_cast<FEGaussianBlur&>(effect);
> 
> downcast<>, you need to add the macros.

Also probably better to do this in the caller and change the parameter to be a const FEGaussianBlur&.

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 407626 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407626&action=review
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:60
> >      static bool supportsCoreImageRendering(FilterEffect&);
> >      static bool canRenderUsingCIFilters(FilterEffect&);
> >      
> > +    CIImage* feGaussianBlurImage(FilterEffect&, Vector<CIImage*>&, int);
> 
> The should take a const FilterEffect&
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.h:63
> > +    HashMap<int, Vector<RetainPtr<CIFilter>>> m_ciFilterStorageMap;
> 
> I don't like the int as the key.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:136
> > +    Vector<RetainPtr<CIFilter>> cachedCIFilters = m_ciFilterStorageMap.get(traversalOrder);
> 
> This line is repeated at 143.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:138
> > +    FEGaussianBlur& blurEffect = static_cast<FEGaussianBlur&>(effect);
> 
> downcast<>, you need to add the macros.
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:146
> > +        gaussianBlurFilter = [CIFilter filterWithName:@"CIGaussianBlurXY"];
> 
> RetainPtr<>. Can we make CIFilters without autorelease?
Unfortunately, CIFilter can only be obtained by calling [CIFilter filterWithName:] which returns an autoreleased pointer. Same applies to obtaining the output CIImage from a given filter, fooFilter.outputImage
> 
> > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:149
> > +        filtersToBeCached.insert(0, gaussianBlurFilter);
> > +        m_ciFilterStorageMap.add(traversalOrder, filtersToBeCached);
> 
> There's a way to do this with ensure().

Created attachment 408337 [details]
Patch

Comment on attachment 408337 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:264
> +    effect.setIsAlphaImage(effect.inputEffect(0)->isAlphaImage());

Is this necessary? I would expect we'd need to change any state on the FEGaussianBlur to make the CIFilter for it.

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:272
> +    FloatRect outputExtent = FloatRect(gaussianBlurFilter.outputImage.extent);

auto outputExtent = FloatRect { gaussianBlurFilter.outputImage.extent) };

> LayoutTests/css3/filters/effect-blur-square-expected.html:4
> +    <title>CSS Filter Test: Test for Saturate short hand</title>

That title seems wrong.

> LayoutTests/css3/filters/effect-blur-square.html:4
> +    <title>CSS Filter Test: Test for Saturate short hand</title>

Nope.

> LayoutTests/css3/filters/effect-blur-square.html:22
> +.container{
> +    position:relative;
> +    width:600px;
> +    height:600px;
> +    overflow:hidden;
> +}
> +
> +#background {
> +    background: red;
> +    left:-15px;
> +    right:-15px;
> +    top:-15px;
> +    bottom:-15px;
> +    filter: blur(10px);
> +    position: absolute;
> +}

We normally indent the CSS. No blank lines at the ends.

> LayoutTests/css3/filters/effect-blur-square.html:27
> +    <p>You should see a red rectangle.</p>

Successful tests should never show red. You should use green.

(In reply to Simon Fraser (smfr) from comment #7)
> Is this necessary? I would expect we'd need to change any state on the
> FEGaussianBlur to make the CIFilter for it.

I would *not* expect.

Created attachment 408346 [details]
Patch

Created attachment 408348 [details]
Patch

Comment on attachment 408348 [details]
Patch

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

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:260
> +RetainPtr<CIImage> FilterEffectRendererCoreImage::imageForFEGaussianBlur(FEGaussianBlur& effect, const Vector<RetainPtr<CIImage>>& inputImages)

Argument can now be const FEGaussianBlur&.

> LayoutTests/css3/filters/effect-blur-square.html:21
> +    .container{
> +        position:relative;
> +        width:400px;
> +        height:400px;
> +        overflow:hidden;
> +    }
> +
> +    #background {
> +        background: green;
> +        left:-25px;
> +        right:-25px;
> +        top:-25px;
> +        bottom:-25px;
> +        filter: blur(5px);
> +        position: absolute;
> +    }

Be consistent with your formatting. Space after :

I don't see how this test tests blurring. It would also pass if there were no filter applied.

Created attachment 408383 [details]
Work in progress (don't review yet)

Created attachment 408455 [details]
Patch (Work in Progress)

2 tests failing on iOS and 4 tests failing on Mac-wk2. 

css3/filters/filters-on-svg-root.html: very minor pixel difference around the circle
fast/filter-image/filter-image-blur.html: the blur amount looks different, however the render result looks different between Minibrowser vs. Safari. 

css3/filters/svg-blur-filter-clipped.html and css3/filters/effect-blur-square.html: cannot reproduce the result on Jazz/GoldenGate. Might be a bug in Mojave that was later fixed in Jazz. 

As my internship is ending in 2 days, I am leaving all relevant code & comments here in case I am not able to fix this tomorrow.

Created attachment 408542 [details]
Valuable CSS/SVG tests

I have attached some valuable test html on CSS and SVG filters.

Comment on attachment 408455 [details]
Patch (Work in Progress)

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

> LayoutTests/css3/filters/effect-blur-square-expected.html:1
> +<!DOCTYPE html>

The file name must be effect-blur-square-expected.txt?

Comment on attachment 408455 [details]
Patch (Work in Progress)

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

>> LayoutTests/css3/filters/effect-blur-square-expected.html:1
>> +<!DOCTYPE html>
> 
> The file name must be effect-blur-square-expected.txt?

Oh, sorry, please ignore this comment.