Hovering over countries at https://covidinc.io/ shows bizarre rendering artifacts Steps To Reproduce: 1. https://covidinc.io/ 2. Hover over a country. The "topo map" image is corrupted. Putting the window into the background (therefore changing the tiling) also affects it.
rdar://problem/60935010
We’re clipping via: * frame #0: 0x000000014bef76ba WebCore`WebCore::RenderSVGResourceClipper::pathOnlyClipping(this=0x000000016aed3530, context=0x00007ffee33d59a8, animatedLocalTransform=0x00007ffee33d3750, objectBoundingBox={ x = 0.0, y = 0.0, width = 1431.0, height = 0.0 }) at RenderSVGResourceClipper.cpp:108:20 frame #1: 0x000000014bef7267 WebCore`WebCore::RenderSVGResourceClipper::applyClippingToContext(this=0x000000016aed3530, renderer=0x000000016aed7960, objectBoundingBox={ x = 0.0, y = 0.0, width = 1431.0, height = 0.0 }, repaintRect={ x = 1024.0, y = 1024.0, width = 407.0, height = 10.0 }, context=0x00007ffee33d59a8) at RenderSVGResourceClipper.cpp:136:41 frame #2: 0x000000014bc9c185 WebCore`WebCore::RenderLayer::setupClipPath(this=0x000000016b3d9000, context=0x00007ffee33d59a8, paintingInfo=0x00007ffee33d4620, offsetFromRoot={ width = 0px (0), height = 0px (0) }, rootRelativeBounds={ x = 183.609px (11751), y = 236.156px (15114), width = 1207.98px (77311), height = 872.859px (55863) }, rootRelativeBoundsComputed=0x00007ffee33d3e57) at RenderLayer.cpp:4504:70 frame #3: 0x000000014bc9a86d WebCore`WebCore::RenderLayer::paintLayerContents(this=0x000000016b3d9000, context=0x00007ffee33d59a8, paintingInfo=0x00007ffee33d4620, paintFlags={ size = 2 }) at RenderLayer.cpp:4623:23 frame #4: 0x000000014bc9a2b4 WebCore`WebCore::RenderLayer::paintLayerContentsAndReflection(this=0x000000016b3d9000, context=0x00007ffee33d59a8, paintingInfo=0x00007ffee33d4620, paintFlags={ size = 2 }) at RenderLayer.cpp:4416:5 frame #5: 0x000000014bc9926b WebCore`WebCore::RenderLayer::paintLayerWithEffects(this=0x000000016b3d9000, context=0x00007ffee33d59a8, paintingInfo=0x00007ffee33d4620, paintFlags={ size = 2 }) at RenderLayer.cpp:4398:5 but bail here: // Fall back to masking if there is more than one clipping path. if (!clipPath.isEmpty()) return false; Not sure why, we can build a path with multiple subpaths.
The bug is that RenderSVGResourceClipper::applyClippingToContext() caches clipperMaskImage but doesn't account for the fact that the repaint rect can change on each invocation.
Created attachment 394703 [details] Patch
Created attachment 394732 [details] Patch
Comment on attachment 394732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394732&action=review > Source/WebCore/ChangeLog:10 > + when using a image buffer mask. However, it created and rendered into this image buffer However, it is created...? > Source/WebCore/rendering/RenderLayer.cpp:4497 > + auto referenceBox = snapRectToDevicePixels(rootRelativeBounds.value(), renderer().document().deviceScaleFactor()); The description mentioned the ImageBuffer wrong caching so I am confused about this change. Is this cleanup or it is part of this fix? > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:131 > bool RenderSVGResourceClipper::applyClippingToContext(RenderElement& renderer, const FloatRect& objectBoundingBox, const FloatRect& repaintRect, GraphicsContext& context) I think we can get rid of repaintRect. It is only used in the LOG_WITH_STREAM statement and in the if-statement which recreates the ImageBuffer. I think the caller should avoid calling this function if the repaintRect.isEmpty(). > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:146 > + auto canUseExistingImageBuffer = [&]() { > + return clipperData.imageBuffer && clipperData.objectBoundingBox == objectBoundingBox && clipperData.absoluteTransform == absoluteTransform; > + }; Should not be this a function of ClipperData? > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:156 > + clipperData.imageBuffer = WTFMove(maskImage); > + clipperData.objectBoundingBox = objectBoundingBox; > + clipperData.absoluteTransform = absoluteTransform; Can't this code be a constructor of ClipperData? clipperData = { WTFMove(maskImage), objectBoundingBox, absoluteTransform }; > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:177 > + clipperData.imageBuffer = nullptr; Should not we clear everything, i.e. clipperData = { };? > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:187 > +bool RenderSVGResourceClipper::drawContentIntoMaskImage(ImageBuffer& maskImageBuffer, const FloatRect& objectBoundingBox) I think maskImageBuffer should be const ImageBuffer&. > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:239 > - SVGRenderingContext::renderSubtreeToImageBuffer(clipperMaskImage.get(), isUseElement ? *child.renderer() : *renderer, maskContentTransformation); > + SVGRenderingContext::renderSubtreeToImageBuffer(&maskImageBuffer, isUseElement ? *child.renderer() : *renderer, maskContentTransformation); Can't we just pass the maskImageBuffer->context() instead of maskImageBuffer since renderSubtreeToImageBuffer() is using the ImageBuffer to get its context()? > Source/WebCore/rendering/svg/RenderSVGResourceClipper.h:33 > -typedef std::unique_ptr<ImageBuffer> ClipperMaskImage; > +using ClipperMaskImage = std::unique_ptr<ImageBuffer>; Do we still need this? It seems you replaced all the reference to ClipperMaskImage by either ImageBuffer or ClipperData.
(In reply to Said Abou-Hallawa from comment #6) > Comment on attachment 394732 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394732&action=review > > > Source/WebCore/ChangeLog:10 > > + when using a image buffer mask. However, it created and rendered into this image buffer > > However, it is created...? it => the function. > > Source/WebCore/rendering/RenderLayer.cpp:4497 > > + auto referenceBox = snapRectToDevicePixels(rootRelativeBounds.value(), renderer().document().deviceScaleFactor()); > > The description mentioned the ImageBuffer wrong caching so I am confused > about this change. Is this cleanup or it is part of this fix? The last paragraph of the changelog explains this. > > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:131 > > bool RenderSVGResourceClipper::applyClippingToContext(RenderElement& renderer, const FloatRect& objectBoundingBox, const FloatRect& repaintRect, GraphicsContext& context) > > I think we can get rid of repaintRect. It is only used in the > LOG_WITH_STREAM statement and in the if-statement which recreates the > ImageBuffer. I think the caller should avoid calling this function if the > repaintRect.isEmpty(). > > > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:146 > > + auto canUseExistingImageBuffer = [&]() { > > + return clipperData.imageBuffer && clipperData.objectBoundingBox == objectBoundingBox && clipperData.absoluteTransform == absoluteTransform; > > + }; > > Should not be this a function of ClipperData? > > > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:156 > > + clipperData.imageBuffer = WTFMove(maskImage); > > + clipperData.objectBoundingBox = objectBoundingBox; > > + clipperData.absoluteTransform = absoluteTransform; > > Can't this code be a constructor of ClipperData? > clipperData = { WTFMove(maskImage), objectBoundingBox, absoluteTransform > }; > > > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:177 > > + clipperData.imageBuffer = nullptr; > > Should not we clear everything, i.e. clipperData = { };? > > > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:187 > > +bool RenderSVGResourceClipper::drawContentIntoMaskImage(ImageBuffer& maskImageBuffer, const FloatRect& objectBoundingBox) > > I think maskImageBuffer should be const ImageBuffer&. > > > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:239 > > - SVGRenderingContext::renderSubtreeToImageBuffer(clipperMaskImage.get(), isUseElement ? *child.renderer() : *renderer, maskContentTransformation); > > + SVGRenderingContext::renderSubtreeToImageBuffer(&maskImageBuffer, isUseElement ? *child.renderer() : *renderer, maskContentTransformation); > > Can't we just pass the maskImageBuffer->context() instead of maskImageBuffer > since renderSubtreeToImageBuffer() is using the ImageBuffer to get its > context()? > > > Source/WebCore/rendering/svg/RenderSVGResourceClipper.h:33 > > -typedef std::unique_ptr<ImageBuffer> ClipperMaskImage; > > +using ClipperMaskImage = std::unique_ptr<ImageBuffer>; > > Do we still need this? It seems you replaced all the reference to > ClipperMaskImage by either ImageBuffer or ClipperData. I will fix all of these.
(In reply to Said Abou-Hallawa from comment #6) > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:239 > > - SVGRenderingContext::renderSubtreeToImageBuffer(clipperMaskImage.get(), isUseElement ? *child.renderer() : *renderer, maskContentTransformation); > > + SVGRenderingContext::renderSubtreeToImageBuffer(&maskImageBuffer, isUseElement ? *child.renderer() : *renderer, maskContentTransformation); Will clean up via bug 209679.
https://trac.webkit.org/changeset/259137/webkit