Bug 233613

Created attachment 445375 [details] Patch

Created attachment 445376 [details] Patch

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

Created attachment 445390 [details] Patch

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

<rdar://problem/85855673>

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

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