Bug 233849

Summary: [GPU Process] (REGRESSION r285597): Set the filterRegion of the CSSFilter after it is created
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: Layout and RenderingAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, dino, esprehn+autocc, ews-watchlist, fmalita, fred.wang, glenn, gyuyoung.kim, heycam, kondapallykalyan, macpherson, menard, pdr, schenney, sergio, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=233843
https://bugs.webkit.org/show_bug.cgi?id=231253
https://bugs.webkit.org/show_bug.cgi?id=232457
Bug Depends on:    
Bug Blocks: 231253    
Attachments:
Description Flags
Combined Patch (this bug + bug 233843)
none
Patch for review
none
Patch
heycam: review+, ews-feeder: commit-queue-
Patch none

Description Said Abou-Hallawa 2021-12-03 19:09:44 PST
The filterRegion() of the CSSFilter is the union of the targetBoundingBox and its outsets. The outsets of the CSSFilter is the union of the outsets of all its functions including the referenced SVGFilter. Notice that if the outsets of a FilterFunction are relative they are calculated relative on the targetBoundingBox. So we have to create the CSSFilter first giving the create() function the targetBoundingBox. Then we calculate the outsets of the CSSFilter and finally we calculate and set its filterRegion().
Comment 1 Said Abou-Hallawa 2021-12-03 19:31:29 PST
Created attachment 445938 [details]
Combined Patch (this bug + bug 233843)
Comment 2 Said Abou-Hallawa 2021-12-03 19:57:19 PST
Created attachment 445944 [details]
Patch for review
Comment 3 Said Abou-Hallawa 2021-12-03 22:51:37 PST
Created attachment 445961 [details]
Patch
Comment 4 Cameron McCormack (:heycam) 2021-12-04 16:54:13 PST
Comment on attachment 445961 [details]
Patch

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

> Source/WebCore/ChangeLog:27
> +           b) Recreate the sourceImage if needed.
> +           c) setup the context for drawing the target renderer.

"e)" and "f)"

> Source/WebCore/rendering/RenderLayerFilters.cpp:174
> +        filterRegion += filter.outsets();
> +        filterRegion = intersection(filterBoxRect, filterRegion);

What about:

filterRegion += filter.outsets();
filterRegion.intersect(filterBoxRect);

> Source/WebCore/rendering/RenderLayerFilters.cpp:185
> +    if (m_sourceImageRect != filterRegion) {
> +        m_sourceImageRect = filterRegion;
> +        hasUpdatedBackingStore = true;
>      }

After this point, m_sourceImageRect and filterRegion are the same, so I think we should consistently use one or the other in the remainder of the function.

> Source/WebCore/rendering/RenderLayerFilters.cpp:202
> +    filter.setFilterRegion(m_sourceImageRect);

So here I think we should use filterRegion.

> Source/WebCore/rendering/RenderLayerFilters.cpp:-189
> -    if (!sourceGraphicsContext || filter.filterRegion().isEmpty() || ImageBuffer::sizeNeedsClamping(filter.filterRegion().size()))

In this old code, how would it be that ImageBuffer::sizeNeedsClamping would return true, since earlier we call clampFilterRegionIfNeeded?

> Source/WebCore/svg/graphics/filters/SVGFilter.h:50
> +    SVGFilter(RenderingMode, const FloatSize& filterScale, ClipOperation, const FloatRect& filterRegion, const FloatRect& targetBoundingBox,  SVGUnitTypes::SVGUnitType primitiveUnits);

Nit: two spaces after "targetBoundingBox,".
Comment 5 Said Abou-Hallawa 2021-12-06 02:17:29 PST
Created attachment 446015 [details]
Patch
Comment 6 EWS 2021-12-06 10:20:52 PST
Committed r286546 (244878@main): <https://commits.webkit.org/244878@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446015 [details].
Comment 7 Radar WebKit Bug Importer 2021-12-06 10:21:34 PST
<rdar://problem/86110979>