Bug 232457 – [GPU Process] [Filters 3/23] Make SVGFilter and CSSFilter work in the same coordinate system

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

[Said Abou-Hallawa]

Currently SVGFilter sets the following members of Filter

1. AffineTransform m_absoluteTransform: this is the scaling part from the transformation from the target element to the outermost coordinate system
2. FloatSize m_filterResolution: this is the clamping scale if the size of the result ImageBuffers exceeds MaxClampedArea

And the CSSFilter sets the following member of Filter:

1. float m_filterScale: this is the document().deviceScaleFactor()

The discrepancy happens also when creating the result ImageBuffers. For SVGFilter, we create them with scaleFactor=1. This means the logicalSize == backendSize. But for CSSFilter we create them with scaleFactor = m_filterScale. This means the logicalSize != backendSize in this case.

We need to unify the coordinates system for both filters. We need also to replace the three members by a single FloatSize called "m_filterScale".

[Said Abou-Hallawa]

Created attachment 442749 [details]
Patch

[Said Abou-Hallawa]

Created attachment 442750 [details]
Patch for review

[Radar WebKit Bug Importer]

<rdar://problem/85035379>

[Said Abou-Hallawa]

Created attachment 443769 [details]
Patch

[Said Abou-Hallawa]

Created attachment 443772 [details]
Patch

[Said Abou-Hallawa]

Created attachment 443775 [details]
Patch

[Said Abou-Hallawa]

Created attachment 443781 [details]
Patch

[Said Abou-Hallawa]

Created attachment 443814 [details]
Patch

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Said Abou-Hallawa]

Created attachment 443837 [details]
Patch

[EWS]

Committed r285597 (244103@main): <https://commits.webkit.org/244103@main>

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

[Darin Adler]

Comment on attachment 443837 [details]
Patch

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

A few coding style tweak thoughts

> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:258
>      IntSize destSize = lastEffect.absolutePaintRect().size();
> -    destSize.scale(lastEffect.filter().filterScale());
>      FloatRect destRect = FloatRect(FloatPoint(), destSize);
>      return destRect;

Could refactor this to this one-liner, which I think would be easier to read:

    return { { }, lastEffect.absolutePaintRect().size() };

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:279
> +    paintingData.width = ceilf(effectDrawingRect.width());
> +    paintingData.height = ceilf(effectDrawingRect.height());
> +    paintingData.radiusX = ceilf(radiusX);
> +    paintingData.radiusY = ceilf(radiusY);

Wasteful to call ceilf on integers. Should remove these ceilf calls.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:287
>      IntSize scaledSize(rect.size());

Should remove "scaledSize" and just use rect.size().

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:301
>      IntSize scaledSize(rect.size());

Should remove "scaledSize" and just use rect.size().

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:315
>      IntRect scaledRect(rect);

Should remove "scaledRect" and just use rect.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:316
>      IntSize scaledPaintSize(m_absolutePaintRect.size());

Should probably either remove this, or rename it. It’s not scaled.

> Source/WebCore/platform/graphics/filters/SourceGraphic.cpp:38
>      Filter& filter = this->filter();
>      FloatRect paintRect = filter.sourceImageRect();
> -    paintRect.scale(filter.filterResolution().width(), filter.filterResolution().height());
>      setAbsolutePaintRect(enclosingIntRect(paintRect));

Should change this to a one-liner:

    setAbsolutePaintRect(enclosingIntRect(filter().sourceImageRect()));

> Source/WebCore/rendering/CSSFilter.h:65
> +    CSSFilter(float scaleFactor);

Maybe put "explicit" here.

> Source/WebCore/rendering/RenderLayerFilters.cpp:121
> +    // FIXME: this rebuilds the entire effects chain even if the filter style didn't change.

We capitalize sentences in FIXME comments.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:193
> +    AffineTransform effectiveTransform = AffineTransform(filterScale.width(), 0, 0, filterScale.height(), 0, 0);

I suggest using "auto" instead of writing AffineTransform twice.

[Said Abou-Hallawa]

Comment on attachment 443837 [details]
Patch

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

>> Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:258
>>      return destRect;
> 
> Could refactor this to this one-liner, which I think would be easier to read:
> 
>     return { { }, lastEffect.absolutePaintRect().size() };

The whole class was removed.

>> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:279
>> +    paintingData.radiusY = ceilf(radiusY);
> 
> Wasteful to call ceilf on integers. Should remove these ceilf calls.

This will be addressed in bug 234130.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:287
>>      IntSize scaledSize(rect.size());
> 
> Should remove "scaledSize" and just use rect.size().

The function FilterEffect::unmultipliedResult() was moved to FilterImage::getPixelBuffer(). This unnecessary local variable was removed.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:301
>>      IntSize scaledSize(rect.size());
> 
> Should remove "scaledSize" and just use rect.size().

The functions FilterEffect::premultipliedResult() and the FilterEffect::unmultipliedResult() were merged and moved to FilterImage::getPixelBuffer(). This unnecessary local variable was removed.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:315
>>      IntRect scaledRect(rect);
> 
> Should remove "scaledRect" and just use rect.

This function was moved to FilterImage.cpp and it now takes PixelBuffers for the source and the destination. This unnecessary local variable was removed.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:316
>>      IntSize scaledPaintSize(m_absolutePaintRect.size());
> 
> Should probably either remove this, or rename it. It’s not scaled.

This unnecessary local variable was removed.

>> Source/WebCore/platform/graphics/filters/SourceGraphic.cpp:38
>>      setAbsolutePaintRect(enclosingIntRect(paintRect));
> 
> Should change this to a one-liner:
> 
>     setAbsolutePaintRect(enclosingIntRect(filter().sourceImageRect()));

This function was completely removed and the base class method FilterEffect::calculateImageRect() is used instead. Also no scaling is required to calculate the imageRect. All imageRects are now in filter coordinates.

>> Source/WebCore/rendering/CSSFilter.h:65
>> +    CSSFilter(float scaleFactor);
> 
> Maybe put "explicit" here.

The constructor of CSSFilter was changed and it takes more than one argument. So no need to "explicit".

>> Source/WebCore/rendering/RenderLayerFilters.cpp:121
>> +    // FIXME: this rebuilds the entire effects chain even if the filter style didn't change.
> 
> We capitalize sentences in FIXME comments.

This will be addressed in bug 234130.

>> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:193
>> +    AffineTransform effectiveTransform = AffineTransform(filterScale.width(), 0, 0, filterScale.height(), 0, 0);
> 
> I suggest using "auto" instead of writing AffineTransform twice.

This will be addressed in bug 234130.