Bug 233618 – [GPU Process] [Filters 16/23] Move the FilterEffect boundaries to a new class FilterEffectGeometry

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

The FilterEffect boundaries will be moved from FilterEffect to Filter which will keep it in a HashMap. The FilterEffect boundaries are used in calculating the primitive subregion.

[Radar WebKit Bug Importer]

<rdar://problem/85881969>

[Said Abou-Hallawa]

Created attachment 445454 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 445454 [details]
Patch

Cancelling review while Said looks into making the FilterEffectGeometry members not optional.

[Said Abou-Hallawa]

Created attachment 445511 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 445511 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        with the effectBoundaries. According to the sepcs[1], primitive sub region

"According to the spec"
"subregion"

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:60
> +    // If there is no input effects, take the effect boundaries as unite rect.
> +    if (!m_inputEffects.isEmpty()) {
> +        for (auto& effect : m_inputEffects) {
> +            auto inputPrimitiveSubregion = effect->determineFilterPrimitiveSubregion(filter);
> +            primitiveSubregion.unite(inputPrimitiveSubregion);
>          }
>      } else
> -        subregion = filter.filterRegion();
> +        primitiveSubregion = effectBoundaries;
>  
> -    // After calling determineFilterPrimitiveSubregion on the target effect, reset the subregion again for <feTile>.
> +    // Don't use the input's subregion for FETile.
>      if (filterType() == FilterEffect::Type::FETile)
> -        subregion = filter.filterRegion();
> +        primitiveSubregion = effectBoundaries;

Any reason not to adjust the condition above (`!m_inputEffects.isEmpty()`) so that we can re-use the other `primitiveSubregion = effectBoundaries` assignment above?

> Source/WebCore/svg/graphics/filters/SVGFilter.h:59
> +    FloatRect effectBoundaries(FilterEffect&) const override;

`const FilterEffect&`?

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:59
> +    auto boundaries = SVGLengthContext::resolveRectangle(&element, primitiveUnits, targetBoundingBox);
> +    auto effectBoundaries = filterRegion;
> +
> +    if (element.hasAttribute(SVGNames::xAttr))
> +        effectBoundaries.setX(boundaries.x());
> +
> +    if (element.hasAttribute(SVGNames::yAttr))
> +        effectBoundaries.setY(boundaries.y());
> +
> +    if (element.hasAttribute(SVGNames::widthAttr))
> +        effectBoundaries.setWidth(boundaries.width());
> +
> +    if (element.hasAttribute(SVGNames::heightAttr))
> +        effectBoundaries.setHeight(boundaries.height());

The SVGFilterPrimitiveStandardAttributes m_x/m_y/m_width/m_height member variables, which is what SVGLengthContext::resolveContext look up above, already have default values of 0%/0%/100%/100%. I don't think there is a need to check for the attributes' presence on the element and do this, since the x/y/width/height values on boundaries should already have the correct values.

[Cameron McCormack (:heycam)]

Comment on attachment 445511 [details]
Patch

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

>> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:59
>> +        effectBoundaries.setHeight(boundaries.height());
> 
> The SVGFilterPrimitiveStandardAttributes m_x/m_y/m_width/m_height member variables, which is what SVGLengthContext::resolveContext look up above, already have default values of 0%/0%/100%/100%. I don't think there is a need to check for the attributes' presence on the element and do this, since the x/y/width/height values on boundaries should already have the correct values.

I think I was wrong here. I was mislead by https://www.w3.org/TR/filter-effects-1/#CommonAttributes stating that x/y/width/height have initial values of 0%/0%/100%/100%. But actually they have implicit values determined by the union of the input primitives' regions. So I think the code you have here is fine.

[Said Abou-Hallawa]

Comment on attachment 445511 [details]
Patch

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

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:60
>> +        primitiveSubregion = effectBoundaries;
> 
> Any reason not to adjust the condition above (`!m_inputEffects.isEmpty()`) so that we can re-use the other `primitiveSubregion = effectBoundaries` assignment above?

We need to call determineFilterPrimitiveSubregion() for the input of <feTile> if it is not going to be used. By the way in the pervious patch https://bugs.webkit.org/attachment.cgi?id=445454&action=review I was doing what you are suggesting and the <feTile> tests failed https://ews-build.s3-us-west-2.amazonaws.com/macOS-Catalina-Release-WK2-Tests-EWS/r445454-23896/results.html.

[Said Abou-Hallawa]

I think I was missing something important about the filterRegion(). 

1. filterRegion can be known and calculated at the time we build the SVGFilter through RenderSVGResourceFilter::applyResource(). We use the targetBoundingBox to calculate it. The defaults for its x/y/width/height are -10%, -10%, 120%, 120% from targetBoundingBox.
2. But it has to be calculated for the referenced SVGFilter after we build the CSSFilter though RenderLayerFilters::beginFilterEffect(). 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 based on the targetBoundingBox.

So we can't assume the filterRegion() is known at the time we build the SVGFilter. But we can use them when we apply the effects because at that time it has to be set for all kinds of filters: CSSFilter, SVGFilter and referenced SVGFilter.

So I am changing the patch back to the original approach.

[Said Abou-Hallawa]

Created attachment 445772 [details]
Patch

[EWS]

Committed r286466 (244806@main): <https://commits.webkit.org/244806@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445772 [details].