Bug 215957 - CoreImage Implementation of CSS Filter blur()
Summary: CoreImage Implementation of CSS Filter blur()
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: frankhome61
URL:
Keywords: InRadar
Depends on: 213673
Blocks:
  Show dependency treegraph
 
Reported: 2020-08-28 16:29 PDT by frankhome61
Modified: 2020-09-11 17:14 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.60 KB, patch)
2020-08-31 14:25 PDT, frankhome61
no flags Details | Formatted Diff | Diff
Patch (8.70 KB, patch)
2020-09-09 10:22 PDT, frankhome61
no flags Details | Formatted Diff | Diff
Patch (8.62 KB, patch)
2020-09-09 10:57 PDT, frankhome61
no flags Details | Formatted Diff | Diff
Patch (8.58 KB, patch)
2020-09-09 11:11 PDT, frankhome61
no flags Details | Formatted Diff | Diff
Work in progress (don't review yet) (8.84 KB, patch)
2020-09-09 16:32 PDT, frankhome61
no flags Details | Formatted Diff | Diff
Patch (Work in Progress) (8.84 KB, patch)
2020-09-10 10:06 PDT, frankhome61
frankhome61: review?
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Valuable CSS/SVG tests (1.19 MB, application/zip)
2020-09-11 10:25 PDT, frankhome61
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description frankhome61 2020-08-28 16:29:31 PDT
Implementing CSS filter blur, using CoreImage
Comment 1 Radar WebKit Bug Importer 2020-08-28 16:30:06 PDT
<rdar://problem/67968258>
Comment 2 frankhome61 2020-08-31 14:25:58 PDT
Created attachment 407626 [details]
Patch
Comment 3 Simon Fraser (smfr) 2020-08-31 14:36:01 PDT
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 4 Sam Weinig 2020-08-31 15:03:17 PDT
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&.
Comment 5 frankhome61 2020-08-31 17:55:44 PDT
(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().
Comment 6 frankhome61 2020-09-09 10:22:21 PDT
Created attachment 408337 [details]
Patch
Comment 7 Simon Fraser (smfr) 2020-09-09 10:37:36 PDT
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.
Comment 8 Simon Fraser (smfr) 2020-09-09 10:41:20 PDT
(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.
Comment 9 frankhome61 2020-09-09 10:57:06 PDT
Created attachment 408346 [details]
Patch
Comment 10 frankhome61 2020-09-09 11:11:27 PDT
Created attachment 408348 [details]
Patch
Comment 11 Simon Fraser (smfr) 2020-09-09 11:18:31 PDT
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.
Comment 12 frankhome61 2020-09-09 16:32:59 PDT
Created attachment 408383 [details]
Work in progress (don't review yet)
Comment 13 frankhome61 2020-09-10 10:06:58 PDT
Created attachment 408455 [details]
Patch (Work in Progress)
Comment 14 frankhome61 2020-09-10 10:11:56 PDT
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.
Comment 15 frankhome61 2020-09-11 10:25:15 PDT
Created attachment 408542 [details]
Valuable CSS/SVG tests

I have attached some valuable test html on CSS and SVG filters.
Comment 16 Peng Liu 2020-09-11 16:20:44 PDT
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 17 Peng Liu 2020-09-11 16:23:01 PDT
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.