Bug 209635 – Hovering over countries at https://covidinc.io/ shows bizarre rendering artifacts

• Component: SVG
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

rdar://problem/60935010

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

The bug is that RenderSVGResourceClipper::applyClippingToContext() caches clipperMaskImage but doesn't account for the fact that the repaint rect can change on each invocation.

[Simon Fraser (smfr)]

Created attachment 394703 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 394732 [details]
Patch

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/259137/webkit