Bug 233613 - [GPU Process] [Filters 15/23] Calculate the result image rectangle of the FilterEffect in filter coordinates
Summary: [GPU Process] [Filters 15/23] Calculate the result image rectangle of the Fil...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks: 231253
  Show dependency treegraph
 
Reported: 2021-11-29 18:56 PST by Said Abou-Hallawa
Modified: 2021-11-30 13:58 PST (History)
19 users (show)

See Also:


Attachments
Patch (98.12 KB, patch)
2021-11-29 20:29 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (98.91 KB, patch)
2021-11-29 20:47 PST, Said Abou-Hallawa
heycam: review+
Details | Formatted Diff | Diff
Patch (100.27 KB, patch)
2021-11-30 00:20 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2021-11-29 18:56:44 PST
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.
Comment 1 Said Abou-Hallawa 2021-11-29 20:29:01 PST
Created attachment 445375 [details]
Patch
Comment 2 Said Abou-Hallawa 2021-11-29 20:47:37 PST
Created attachment 445376 [details]
Patch
Comment 3 Cameron McCormack (:heycam) 2021-11-29 22:26:47 PST
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.
Comment 4 Cameron McCormack (:heycam) 2021-11-29 22:28:23 PST
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"
Comment 5 Said Abou-Hallawa 2021-11-30 00:20:35 PST
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.
Comment 6 Said Abou-Hallawa 2021-11-30 00:20:53 PST
Created attachment 445390 [details]
Patch
Comment 7 EWS 2021-11-30 01:19:23 PST
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].
Comment 8 Radar WebKit Bug Importer 2021-11-30 01:20:21 PST
<rdar://problem/85855673>
Comment 9 Cameron McCormack (:heycam) 2021-11-30 13:58:00 PST
(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).