[webkit-reviews] review granted: [Bug 233613] [GPU Process] [Filters 15/23] Calculate the result image rectangle of the FilterEffect in filter coordinates : [Attachment 445376] Patch

[bugzilla-daemon at webkit.org]

Cameron McCormack (:heycam) <heycam at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 233613: [GPU Process] [Filters 15/23] Calculate the result image rectangle
of the FilterEffect in filter coordinates
https://bugs.webkit.org/show_bug.cgi?id=233613

Attachment 445376: Patch

https://bugs.webkit.org/attachment.cgi?id=445376&action=review




--- Comment #3 from Cameron McCormack (:heycam) <heycam at apple.com> ---
Comment on attachment 445376
  --> https://bugs.webkit.org/attachment.cgi?id=445376
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:646
> +    scale({ 1 / filter.filterScale().width(), 1 /
filter.filterScale().height() });
>      drawImageBuffer(*imageBuffer, result->absoluteImageRect());
> +    scale(filter.filterScale());

I meant to mention this in a previous patch too, but I think I forgot. There's
always risk with applying a transform and then its inverse later that there is
sufficient loss of precision that you don't end up with the original CTM. Not
sure if it's a practical issue but something to consider?

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:95
>	   // For In and Atop the first effect just influences the result of
>	   // the second effect. So just use the absolute paint rect of the
second effect here.

Update this comment so it no longer talks about the "absolute paint rect".

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:99
>	   // Arithmetic may influnce the compele filter primitive region. So
we can't

Pre-existing: "complete" (or "entire" would sound better)

> Source/WebCore/platform/graphics/filters/FEDropShadow.cpp:60
>      // We take the half kernel size and multiply it with three, because we
run box blur three times.
> -    absolutePaintRect.inflateX(3 * kernelSize.width() * 0.5f);
> -    absolutePaintRect.inflateY(3 * kernelSize.height() * 0.5f);
> +    imageRect.inflateX(3 * kernelSize.width() * 0.5f);
> +    imageRect.inflateY(3 * kernelSize.height() * 0.5f);
>  

Pre-existing, but why don't we inflate imageRectWithOffset instead of the final
imageRect value?

> Source/WebCore/platform/graphics/filters/Filter.cpp:77
> +FloatRect Filter::clipToMaxEffectRect(const FloatRect& imageRect, const
FloatRect& primitiveSubregion) const
> +{
> +    auto maxEffectRect = this->maxEffectRect(primitiveSubregion);
> +    return m_clipOperation == ClipOperation::Intersect ?
intersection(imageRect, maxEffectRect) : unionRect(imageRect, maxEffectRect);
> +}

Seems weird to me that the function is called "clipToMaxEffectRect" but
m_clipOperation causes it to either clip or to unite. But I don't have a better
name suggestion right now.

> Source/WebCore/rendering/RenderLayerFilters.cpp:135
> +    auto logicalSize = filter.sourceImageRect().size() *
filter.filterScale();

Use scaledByFilterScale maybe? Size is the easy one to write out instead of
calling the function, but may as well consistently use it.