Bug 233613 – [GPU Process] [Filters 15/23] Calculate the result image rectangle of the FilterEffect in filter coordinates

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

[Said Abou-Hallawa]

The FilterEffect lengths (like the stdDeviation of FEGaussianBlur) are in filter coordinates. Using the filter coordinates will simplify the image rectangle calculations. Only when creating the FilterImage we can convert the image rectangle from filter coordinates to user space coordinates.

[Said Abou-Hallawa]

Created attachment 445375 [details]
Patch

[Said Abou-Hallawa]

Created attachment 445376 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 445376 [details]
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.

[Cameron McCormack (:heycam)]

Comment on attachment 445376 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/SpotLightSource.cpp:44
>  static const float antiAliasTreshold = 0.016f;

Pre-existing: "antialiasThreshold"

[Said Abou-Hallawa]

Comment on attachment 445376 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:646
>> +    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?

Using save()/restore() or using GraphicsContextStateSaver is expensive. In the old code we were creating an AffineTransform and we were applying its inverse before drawing and applying it after drawing. So I think scaling with the reciprocal of filter.filterScale() is more accurate than applying the inverse of AffineTransform. But I will think about it.

>> Source/WebCore/platform/graphics/filters/FEComposite.cpp:95
>>          // 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".

Fixed.

>> 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)

Fixed.

>> Source/WebCore/platform/graphics/filters/FEDropShadow.cpp:60
>>  
> 
> Pre-existing, but why don't we inflate imageRectWithOffset instead of the final imageRect value?

Because the final imageRect will be different if { m_dx, m_dy } are negative. Suppose 

inputs[0]->imageRect() = { 30, 30, 100, 100 }
{ m_dx, m_dy } = { -10, -10 }
{ m_stdX, m_stdY } = { 10, 10 } => kernelSize = { 10, 10 }

Current calculations are:

imageRect =  { 30, 30, 100, 100 }
imageRectWithOffset = { 20, 20, 100, 100 } // after imageRectWithOffset.move({ -10, -10 })
imageRect = { 20, 20, 110, 110 }. // after imageRect.unite({ 20, 20, 100, 100 })
imageRect = { 5, 5, 140, 140 }. // after imageRect.inflate({15, 15})

Calculations with your suggestion are:

imageRect =  { 30, 30, 100, 100 }
imageRectWithOffset = { 20, 20, 100, 100 } // after imageRectWithOffset.move({ -10, -10 })
imageRectWithOffset = { 5, 5, 130, 130 }. // after imageRect.inflate({15, 15})
imageRect = { 5, 5, 130, 130 }. // after imageRect.unite({ 5, 5, 130, 130 })

>> Source/WebCore/platform/graphics/filters/SpotLightSource.cpp:44
>>  static const float antiAliasTreshold = 0.016f;
> 
> Pre-existing: "antialiasThreshold"

Fixed.

>> 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.

Fixed.

[Said Abou-Hallawa]

Created attachment 445390 [details]
Patch

[EWS]

Committed r286287 (244646@main): <https://commits.webkit.org/244646@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/85855673>

[Cameron McCormack (:heycam)]

(In reply to Said Abou-Hallawa from comment #5)
> >> Source/WebCore/platform/graphics/filters/FEDropShadow.cpp:60
> >>  
> > 
> > Pre-existing, but why don't we inflate imageRectWithOffset instead of the final imageRect value?
> 
> Because the final imageRect will be different if { m_dx, m_dy } are
> negative. Suppose 
> 
> inputs[0]->imageRect() = { 30, 30, 100, 100 }
> { m_dx, m_dy } = { -10, -10 }
> { m_stdX, m_stdY } = { 10, 10 } => kernelSize = { 10, 10 }
> 
> Current calculations are:
> 
> imageRect =  { 30, 30, 100, 100 }
> imageRectWithOffset = { 20, 20, 100, 100 } // after
> imageRectWithOffset.move({ -10, -10 })
> imageRect = { 20, 20, 110, 110 }. // after imageRect.unite({ 20, 20, 100,
> 100 })
> imageRect = { 5, 5, 140, 140 }. // after imageRect.inflate({15, 15})
> 
> Calculations with your suggestion are:
> 
> imageRect =  { 30, 30, 100, 100 }
> imageRectWithOffset = { 20, 20, 100, 100 } // after
> imageRectWithOffset.move({ -10, -10 })
> imageRectWithOffset = { 5, 5, 130, 130 }. // after imageRect.inflate({15,
> 15})
> imageRect = { 5, 5, 130, 130 }. // after imageRect.unite({ 5, 5, 130, 130 })

But isn't that fine?  The rendered result of using the filter is a blurred shadow covering { 5, 5, 130, 130 } with the unblurred input covering { 30, 30, 100, 100 } composited on top.  The image rect doesn't need to cover the extra space down to (140, 140).

[Arcady Goldmints-Orlov]

*** Bug 233611 has been marked as a duplicate of this bug. ***