Bug 248193 – [Filters] Control applying the Filter by a new enum named FilterMode

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

RenderingMode has been used to control whether the Filter should use accelerated or unaccelerated ImageBuffers. To support extended CoreGraphics filters, we need even a new way to apply the filter which does not require creating temporary ImageBuffers.
    
FilterMode will be used for this purpose. FilterEffect and Filter will now provide OptionSet<FilterMode> telling which modes they support. The Settings will provide a preferred FilterMode. Filter will fallback to FilterMode::Software if it does not support the preferred FilterMode.

[Radar WebKit Bug Importer]

<rdar://problem/102590236>

[Said Abou-Hallawa]

Pull request: https://github.com/WebKit/WebKit/pull/6716

[Said Abou-Hallawa]

Created attachment 463643 [details]
Patch

[Said Abou-Hallawa]

Created attachment 463659 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 463659 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FilterMode.h:44
> +        WebCore::FilterMode::Accelerated,

We use “accelerated” and “software” pretty broadly and I’m not sure they have much meaning any more. I think “accelerated”here means “Use Core Image” but I’m not sure.

> Source/WebCore/rendering/CSSFilter.cpp:45
> +RefPtr<CSSFilter> CSSFilter::create(RenderElement& renderer, const FilterOperations& operations, FilterMode filterMode, const FloatSize& filterScale, ClipOperation clipOperation, const FloatRect& targetBoundingBox, const GraphicsContext& destinationContext)

It’s not super clear what the FilterMode argument here represents. A CSSFIlter internally can compute it’s supportedFilterModes, so I think this input is “allowed filter modes” or something?

> Source/WebCore/rendering/CSSFilter.cpp:320
> +    OptionSet<FilterMode> filterModes;

You could initialize this with “Software” since I think we guarantee that we can always support software mode.

[Said Abou-Hallawa]

Comment on attachment 463659 [details]
Patch

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

>> Source/WebCore/platform/graphics/filters/FilterMode.h:44
>> +        WebCore::FilterMode::Accelerated,
> 
> We use “accelerated” and “software” pretty broadly and I’m not sure they have much meaning any more. I think “accelerated”here means “Use Core Image” but I’m not sure.

Yes for Cocoa platforms it means using CoreImage. But I think it is still accelerated mode since CoreImage requires creating IOSurface for its filters. I chose Accelerated because I did not want to limit it for using CoreImage.

But if you prefer a different name, let's do this in a separate patch since there are many parts need to change. Even the internal feature key has the name "AcceleratedFiltersEnabled".

>> Source/WebCore/rendering/CSSFilter.cpp:45
>> +RefPtr<CSSFilter> CSSFilter::create(RenderElement& renderer, const FilterOperations& operations, FilterMode filterMode, const FloatSize& filterScale, ClipOperation clipOperation, const FloatRect& targetBoundingBox, const GraphicsContext& destinationContext)
> 
> It’s not super clear what the FilterMode argument here represents. A CSSFIlter internally can compute it’s supportedFilterModes, so I think this input is “allowed filter modes” or something?

Yes you are right. I renamed it to "preferredFilterMode". The "preferredFilterMode" comes from the Settings. CSSFilter/SVGFilter will use "preferredFilterMode" if the whole chain supports it.

>> Source/WebCore/rendering/CSSFilter.cpp:320
>> +    OptionSet<FilterMode> filterModes;
> 
> You could initialize this with “Software” since I think we guarantee that we can always support software mode.

But I am using the condition "if (!filterModes)" below to initialize it with the first "functionModes".

[Said Abou-Hallawa]

A new PR was uploaded to https://github.com/WebKit/WebKit/pull/6716

[EWS]

Committed 256985@main (ccb8d41a1adb): <https://commits.webkit.org/256985@main>

Reviewed commits have been landed. Closing PR #6716 and removing active labels.

[Darin Adler]

Comment on attachment 463659 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:118
> +        modes = modes | FilterMode::Accelerated;

modes.add(FilterMode::Accelerated)

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:62
> +    OptionSet<FilterMode> modes = FilterMode::Software;

I think you can just write OptionSet here because of deduction guides.

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:65
> +        modes = modes | FilterMode::Accelerated;

modes.add(FilterMode::Accelerated)