⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 259137 in webkit


Ignore:
Timestamp:
Mar 27, 2020, 1:57:41 PM (7 years ago)
Author:
Simon Fraser
Message:

Hovering over countries at ​https://covidinc.io/ shows bizarre rendering artifacts
​https://bugs.webkit.org/show_bug.cgi?id=209635
<rdar://problem/60935010>

Reviewed by Said Abou-Hallawa.
Source/WebCore:

RenderSVGResourceClipper::applyClippingToContext() cached an ImageBuffer per RenderObject
when using a image buffer mask. However, the function created and rendered into this image buffer
using repaintRect, which can change between invocations. Painting with different repaintRects
is very common when rendering into page tiles.

The buffer can only be re-used if the inputs used to create the buffer (objectBoundingBox, absoluteTransform)
are the same, so store those and compare them when determining when to use the cached buffer, and
don't use repaintRect when setting up the buffer.

This revealed another problem where renderers with visual overflow could be truncated by
the clipping, tested by imported/mozilla/svg/svg-integration/clipPath-html-03.xhtml, which occurred
because RenderLayer::setupClipPath() used the 'svgReferenceBox' for the clipping bounds, which
is the content box of the renderer excluding overflow. Fix this by using the bounds of the layer,
which includes the bounds of descendants.

Tests: svg/clip-path/clip-path-on-overflowing.html

svg/clip-path/resource-clipper-multiple-repaints.html

  • rendering/RenderLayer.cpp:

(WebCore::RenderLayer::setupClipPath):

  • rendering/svg/RenderSVGResourceClipper.cpp:

(WebCore::RenderSVGResourceClipper::removeAllClientsFromCache):
(WebCore::RenderSVGResourceClipper::applyClippingToContext):
(WebCore::RenderSVGResourceClipper::drawContentIntoMaskImage):
(WebCore::RenderSVGResourceClipper::addRendererToClipper):
(WebCore::RenderSVGResourceClipper::resourceBoundingBox):

  • rendering/svg/RenderSVGResourceClipper.h:

LayoutTests:

Ref test that exercises the code path by painting into a tiled compositing
layer.

  • svg/clip-path/clip-path-on-overflowing-expected.html: Added.
  • svg/clip-path/clip-path-on-overflowing.html: Added.
  • svg/clip-path/mask-nested-clip-path-010-expected.svg:
  • svg/clip-path/mask-nested-clip-path-010.svg: Copied from imported/mozilla/svg/svg-integration/clipPath-html-03.xhtml,

and modified to have a non-zero offset for better testing of the clipping bounds computation.

  • svg/clip-path/resource-clipper-multiple-repaints-expected.html: Added.
  • svg/clip-path/resource-clipper-multiple-repaints.html: Added.
Location:
trunk
Files:
4 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r259132 r259137  
     12020-03-27  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Hovering over countries at https://covidinc.io/ shows bizarre rendering artifacts
     4        https://bugs.webkit.org/show_bug.cgi?id=209635
     5        <rdar://problem/60935010>
     6
     7        Reviewed by Said Abou-Hallawa.
     8       
     9        Ref test that exercises the code path by painting into a tiled compositing
     10        layer.
     11
     12        * svg/clip-path/clip-path-on-overflowing-expected.html: Added.
     13        * svg/clip-path/clip-path-on-overflowing.html: Added.
     14        * svg/clip-path/mask-nested-clip-path-010-expected.svg:
     15        * svg/clip-path/mask-nested-clip-path-010.svg: Copied from imported/mozilla/svg/svg-integration/clipPath-html-03.xhtml,
     16        and modified to have a non-zero offset for better testing of the clipping bounds computation.
     17        * svg/clip-path/resource-clipper-multiple-repaints-expected.html: Added.
     18        * svg/clip-path/resource-clipper-multiple-repaints.html: Added.
     19
    1202020-03-27  Jason Lawrence  <lawrence.j@apple.com>
    221

  • trunk/LayoutTests/svg/clip-path/mask-nested-clip-path-010-expected.svg

    r164592 r259137  
    11<svg xmlns="http://www.w3.org/2000/svg" width="10000" height="400">
    22    <rect x="99" width="101" height="100" fill="green"/>
    3     <rect x="299" width="101" height="100" fill="green"/>
     3    <rect x="295" width="105" height="100" fill="green"/>
    44</svg>

  • trunk/LayoutTests/svg/clip-path/mask-nested-clip-path-010.svg

    r164592 r259137  
    2929  <g mask="url(#crop)" transform="translate(100, 0)">
    3030    <rect width="10000" height="400" fill="red" clip-path="url(#clip2)"/>
    31     <rect x="199" width="101" height="100" fill="green"/>
     31    <rect x="195" width="105" height="100" fill="green"/>
    3232  </g>
    3333</svg>

  • trunk/Source/WebCore/ChangeLog

    r259135 r259137  
     12020-03-27  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Hovering over countries at https://covidinc.io/ shows bizarre rendering artifacts
     4        https://bugs.webkit.org/show_bug.cgi?id=209635
     5        <rdar://problem/60935010>
     6
     7        Reviewed by Said Abou-Hallawa.
     8
     9        RenderSVGResourceClipper::applyClippingToContext() cached an ImageBuffer per RenderObject
     10        when using a image buffer mask. However, the function created and rendered into this image buffer
     11        using repaintRect, which can change between invocations. Painting with different repaintRects
     12        is very common when rendering into page tiles.
     13
     14        The buffer can only be re-used if the inputs used to create the buffer (objectBoundingBox, absoluteTransform)
     15        are the same, so store those and compare them when determining when to use the cached buffer, and
     16        don't use repaintRect when setting up the buffer.
     17
     18        This revealed another problem where renderers with visual overflow could be truncated by
     19        the clipping, tested by imported/mozilla/svg/svg-integration/clipPath-html-03.xhtml, which occurred
     20        because RenderLayer::setupClipPath() used the 'svgReferenceBox' for the clipping bounds, which
     21        is the content box of the renderer excluding overflow. Fix this by using the bounds of the layer,
     22        which includes the bounds of descendants.
     23
     24        Tests: svg/clip-path/clip-path-on-overflowing.html
     25               svg/clip-path/resource-clipper-multiple-repaints.html
     26
     27        * rendering/RenderLayer.cpp:
     28        (WebCore::RenderLayer::setupClipPath):
     29        * rendering/svg/RenderSVGResourceClipper.cpp:
     30        (WebCore::RenderSVGResourceClipper::removeAllClientsFromCache):
     31        (WebCore::RenderSVGResourceClipper::applyClippingToContext):
     32        (WebCore::RenderSVGResourceClipper::drawContentIntoMaskImage):
     33        (WebCore::RenderSVGResourceClipper::addRendererToClipper):
     34        (WebCore::RenderSVGResourceClipper::resourceBoundingBox):
     35        * rendering/svg/RenderSVGResourceClipper.h:
     36
    1372020-03-27  Chris Dumez  <cdumez@apple.com>
    238

  • trunk/Source/WebCore/rendering/RenderLayer.cpp

    r259128 r259137  
    44694469bool RenderLayer::setupClipPath(GraphicsContext& context, const LayerPaintingInfo& paintingInfo, const LayoutSize& offsetFromRoot, Optional<LayoutRect>& rootRelativeBounds)
    44704470{
    4471     if (!renderer().hasClipPath() || context.paintingDisabled())
     4471    if (!renderer().hasClipPath() || context.paintingDisabled() || paintingInfo.paintDirtyRect.isEmpty())
    44724472        return false;
    44734473
    … …  
    44954495        if (element && element->renderer() && is<RenderSVGResourceClipper>(element->renderer())) {
    44964496            context.save();
    4497             float deviceSaleFactor = renderer().document().deviceScaleFactor();
    4498             FloatRect referenceBox = snapRectToDevicePixels(computeReferenceBox(renderer(), CSSBoxType::ContentBox, paintingOffsetFromRoot, rootRelativeBounds.value()), deviceSaleFactor);
    4499             FloatPoint offset {referenceBox.location()};
     4497            auto referenceBox = snapRectToDevicePixels(rootRelativeBounds.value(), renderer().document().deviceScaleFactor());
     4498            auto offset = referenceBox.location();
    45004499            context.translate(offset);
    4501             FloatRect svgReferenceBox {FloatPoint(), referenceBox.size()};
    4502             downcast<RenderSVGResourceClipper>(*element->renderer()).applyClippingToContext(renderer(), svgReferenceBox, paintingInfo.paintDirtyRect, context);
    4503             context.translate(FloatPoint(-offset.x(), -offset.y()));
     4500            FloatRect svgReferenceBox { {}, referenceBox.size() };
     4501            downcast<RenderSVGResourceClipper>(*element->renderer()).applyClippingToContext(renderer(), svgReferenceBox, context);
     4502            context.translate(-offset);
    45044503            return true;
    45054504        }

  • trunk/Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp

    r257616 r259137  
    3030#include "HitTestResult.h"
    3131#include "IntRect.h"
     32#include "Logging.h"
    3233#include "RenderSVGText.h"
    3334#include "RenderStyle.h"
    … …  
    3940#include "SVGUseElement.h"
    4041#include <wtf/IsoMallocInlines.h>
     42#include <wtf/text/TextStream.h>
    4143
    4244namespace WebCore {
    … …  
    5355void RenderSVGResourceClipper::removeAllClientsFromCache(bool markForInvalidation)
    5456{
    55     m_clipBoundaries = FloatRect();
     57    m_clipBoundaries = { };
    5658    m_clipper.clear();
    5759
    … …  
    7173    ASSERT_UNUSED(resourceMode, !resourceMode);
    7274
    73     return applyClippingToContext(renderer, renderer.objectBoundingBox(), renderer.repaintRectInLocalCoordinates(), *context);
     75    auto repaintRect = renderer.repaintRectInLocalCoordinates();
     76    if (repaintRect.isEmpty())
     77        return true;
     78
     79    return applyClippingToContext(renderer, renderer.objectBoundingBox(), *context);
    7480}
    7581
    … …  
    127133}
    128134
    129 bool RenderSVGResourceClipper::applyClippingToContext(RenderElement& renderer, const FloatRect& objectBoundingBox, const FloatRect& repaintRect, GraphicsContext& context)
    130 {
    131     ClipperMaskImage& clipperMaskImage = addRendererToClipper(renderer);
    132     bool shouldCreateClipperMaskImage = !clipperMaskImage;
     135bool RenderSVGResourceClipper::applyClippingToContext(RenderElement& renderer, const FloatRect& objectBoundingBox, GraphicsContext& context)
     136{
     137    ClipperData& clipperData = addRendererToClipper(renderer);
     138   
     139    LOG_WITH_STREAM(SVG, stream << "RenderSVGResourceClipper " << this << " applyClippingToContext: renderer " << &renderer << " objectBoundingBox " << objectBoundingBox << " (existing image buffer " << clipperData.imageBuffer.get() << ")");
    133140
    134141    AffineTransform animatedLocalTransform = clipPathElement().animatedLocalTransform();
    135142
    136     if (shouldCreateClipperMaskImage && pathOnlyClipping(context, animatedLocalTransform, objectBoundingBox))
     143    if (!clipperData.imageBuffer && pathOnlyClipping(context, animatedLocalTransform, objectBoundingBox))
    137144        return true;
    138145
    139146    AffineTransform absoluteTransform = SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem(renderer);
    140 
    141     if (shouldCreateClipperMaskImage && !repaintRect.isEmpty()) {
     147    if (!clipperData.isValidForGeometry(objectBoundingBox, absoluteTransform)) {
    142148        // FIXME (149469): This image buffer should not be unconditionally unaccelerated. Making it match the context breaks nested clipping, though.
    143         clipperMaskImage = SVGRenderingContext::createImageBuffer(repaintRect, absoluteTransform, ColorSpace::SRGB, RenderingMode::Unaccelerated, &context);
    144         if (!clipperMaskImage)
    145             return false;
    146 
    147         GraphicsContext& maskContext = clipperMaskImage->context();
     149        auto maskImage = SVGRenderingContext::createImageBuffer(objectBoundingBox, absoluteTransform, ColorSpace::SRGB, RenderingMode::Unaccelerated, &context);
     150        if (!maskImage)
     151            return false;
     152
     153        clipperData = { WTFMove(maskImage), objectBoundingBox, absoluteTransform };
     154
     155        GraphicsContext& maskContext = clipperData.imageBuffer->context();
    148156        maskContext.concatCTM(animatedLocalTransform);
    149157
    … …  
    155163            GraphicsContextStateSaver stateSaver(maskContext);
    156164
    157             if (!clipper->applyClippingToContext(*this, objectBoundingBox, repaintRect, maskContext))
     165            if (!clipper->applyClippingToContext(*this, objectBoundingBox, maskContext))
    158166                return false;
    159167
    160             succeeded = drawContentIntoMaskImage(clipperMaskImage, objectBoundingBox);
     168            succeeded = drawContentIntoMaskImage(*clipperData.imageBuffer, objectBoundingBox);
    161169            // The context restore applies the clipping on non-CG platforms.
    162170        } else
    163             succeeded = drawContentIntoMaskImage(clipperMaskImage, objectBoundingBox);
     171            succeeded = drawContentIntoMaskImage(*clipperData.imageBuffer, objectBoundingBox);
    164172
    165173        if (!succeeded)
    166             clipperMaskImage.reset();
    167     }
    168 
    169     if (!clipperMaskImage)
     174            clipperData = { };
     175    }
     176
     177    if (!clipperData.imageBuffer)
    170178        return false;
    171179
    172     SVGRenderingContext::clipToImageBuffer(context, absoluteTransform, repaintRect, clipperMaskImage, shouldCreateClipperMaskImage);
     180    SVGRenderingContext::clipToImageBuffer(context, absoluteTransform, objectBoundingBox, clipperData.imageBuffer, true);
    173181    return true;
    174182}
    175183
    176 bool RenderSVGResourceClipper::drawContentIntoMaskImage(const ClipperMaskImage& clipperMaskImage, const FloatRect& objectBoundingBox)
    177 {
    178     ASSERT(clipperMaskImage);
    179 
    180     GraphicsContext& maskContext = clipperMaskImage->context();
     184bool RenderSVGResourceClipper::drawContentIntoMaskImage(ImageBuffer& maskImageBuffer, const FloatRect& objectBoundingBox)
     185{
     186    GraphicsContext& maskContext = maskImageBuffer.context();
    181187
    182188    AffineTransform maskContentTransformation;
    … …  
    228234        // We have to pass the <use> renderer itself to renderSubtreeToImageBuffer() to apply it's x/y/transform/etc. values when rendering.
    229235        // So if isUseElement is true, refetch the childNode->renderer(), as renderer got overridden above.
    230         SVGRenderingContext::renderSubtreeToImageBuffer(clipperMaskImage.get(), isUseElement ? *child.renderer() : *renderer, maskContentTransformation);
     236        SVGRenderingContext::renderSubtreeToImageBuffer(&maskImageBuffer, isUseElement ? *child.renderer() : *renderer, maskContentTransformation);
    231237    }
    232238
    … …  
    252258}
    253259
    254 ClipperMaskImage& RenderSVGResourceClipper::addRendererToClipper(const RenderObject& object)
    255 {
    256     return m_clipper.add(&object, ClipperMaskImage()).iterator->value;
     260RenderSVGResourceClipper::ClipperData& RenderSVGResourceClipper::addRendererToClipper(const RenderObject& object)
     261{
     262    return m_clipper.add(&object, ClipperData()).iterator->value;
    257263}
    258264
    … …  
    302308    // Resource was not layouted yet. Give back the boundingBox of the object.
    303309    if (selfNeedsLayout()) {
    304         addRendererToClipper(object);
     310        addRendererToClipper(object); // For selfNeedsClientInvalidation().
    305311        return object.objectBoundingBox();
    306312    }

  • trunk/Source/WebCore/rendering/svg/RenderSVGResourceClipper.h

    r224537 r259137  
    3131class ImageBuffer;
    3232
    33 typedef std::unique_ptr<ImageBuffer> ClipperMaskImage;
    34 
    3533class RenderSVGResourceClipper final : public RenderSVGResourceContainer {
    3634    WTF_MAKE_ISO_ALLOCATED(RenderSVGResourceClipper);
    … …  
    4846    // applyResource directly and use the rects from the object, since they are empty for RenderSVGResources
    4947    // FIXME: We made applyClippingToContext public because we cannot call applyResource on HTML elements (it asserts on RenderObject::objectBoundingBox)
    50     bool applyClippingToContext(RenderElement&, const FloatRect&, const FloatRect&, GraphicsContext&);
     48    bool applyClippingToContext(RenderElement&, const FloatRect&, GraphicsContext&);
    5149    FloatRect resourceBoundingBox(const RenderObject&) override;
    5250
    … …  
    6159
    6260private:
     61    struct ClipperData {
     62        FloatRect objectBoundingBox;
     63        AffineTransform absoluteTransform;
     64        std::unique_ptr<ImageBuffer> imageBuffer;
     65       
     66        ClipperData() = default;
     67        ClipperData(std::unique_ptr<ImageBuffer>&& buffer, const FloatRect& boundingBox, const AffineTransform& transform)
     68            : objectBoundingBox(boundingBox)
     69            , absoluteTransform(transform)
     70            , imageBuffer(WTFMove(buffer))
     71        {
     72        }
     73
     74        bool isValidForGeometry(const FloatRect& boundingBox, const AffineTransform& transform) const
     75        {
     76            return imageBuffer && objectBoundingBox == boundingBox && absoluteTransform == transform;
     77        }
     78    };
     79
    6380    void element() const = delete;
    6481
    … …  
    6784
    6885    bool pathOnlyClipping(GraphicsContext&, const AffineTransform&, const FloatRect&);
    69     bool drawContentIntoMaskImage(const ClipperMaskImage&, const FloatRect& objectBoundingBox);
     86    bool drawContentIntoMaskImage(ImageBuffer&, const FloatRect& objectBoundingBox);
    7087    void calculateClipContentRepaintRect();
    71     ClipperMaskImage& addRendererToClipper(const RenderObject&);
     88    ClipperData& addRendererToClipper(const RenderObject&);
    7289
    7390    FloatRect m_clipBoundaries;
    74     HashMap<const RenderObject*, ClipperMaskImage> m_clipper;
     91    HashMap<const RenderObject*, ClipperData> m_clipper;
    7592};
    7693
Note: See TracChangeset for help on using the changeset viewer.