| Summary: | [Filters] Control applying the Filter by a new enum named FilterMode | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||
| Component: | Layout and Rendering | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | annulen, bfulgham, changseok, darin, dino, esprehn+autocc, ews-watchlist, fmalita, fred.wang, glenn, gyuyoung.kim, heycam, kondapallykalyan, pdr, ryuan.choi, schenney, sergio, simon.fraser, webkit-bug-importer, zalan | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 243816 | ||||||||
| Attachments: |
|
||||||||
Pull request: https://github.com/WebKit/WebKit/pull/6716 Created attachment 463643 [details]
Patch
Created attachment 463659 [details]
Patch
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. 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". A new PR was uploaded to https://github.com/WebKit/WebKit/pull/6716 Committed 256985@main (ccb8d41a1adb): <https://commits.webkit.org/256985@main> Reviewed commits have been landed. Closing PR #6716 and removing active labels. 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) |
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.