Bug 25431 – Simplify how SVG containers paint (and likely fix bugs by doing so)

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

Simplify how SVG containers paint (and likely fix bugs by doing so)

See attached patch (with long ChangeLog entry).

[Eric Seidel (no email)]

Created attachment 29827 [details]
Simplify how SVG containers paint

 21 files changed, 302 insertions(+), 203 deletions(-)

[Dirk Schulze]

About  "FIXME: Should we still render filters?" in RenderSVGRoot. I think so, yes. I understand it that way, that you want to stop rendering process, if the root-element is empty. E.g. a empty <g>, right? The spec want to continue on empty elements for filters.

[Eric Seidel (no email)]

Yeah, there are likely lots of filter bugs in our tree right now that we can't test because we have filters off.  Filters still kinda need to be re-written some day anyway. :)

[Simon Fraser (smfr)]

Comment on attachment 29827 [details]
Simplify how SVG containers paint

>  bool RenderSVGContainer::selfWillPaint() const
>  {
>  #if ENABLE(SVG_FILTERS)
> @@ -121,29 +110,29 @@ void RenderSVGContainer::paint(PaintInfo& paintInfo, int, int)
>       // Spec: groups w/o children still may render filter content.
>      if (!firstChild() && !selfWillPaint())
>          return;
> -    
> -    paintInfo.context->save();
> -    applyContentTransforms(paintInfo);
>  
> -    SVGResourceFilter* filter = 0;
> -    PaintInfo savedInfo(paintInfo);
> +    PaintInfo childPaintInfo(paintInfo);
>  
> -    FloatRect boundingBox = repaintRectInLocalCoordinates();
> -    if (paintInfo.phase == PaintPhaseForeground)
> -        prepareToRenderSVGContent(this, paintInfo, boundingBox, filter); 
> +    childPaintInfo.context->save();
> +
> +    // Let the RenderSVGViewportContainer subclass clip if necessary
> +    applyViewportClip(childPaintInfo);
>  
> -    applyAdditionalTransforms(paintInfo);
> +    applyTransformToPaintInfo(childPaintInfo, localToParentTransform());
> +
> +    SVGResourceFilter* filter = 0;
> +    FloatRect boundingBox = repaintRectInLocalCoordinates();
> +    if (childPaintInfo.phase == PaintPhaseForeground)
> +        prepareToRenderSVGContent(this, childPaintInfo, boundingBox, filter);
>  
> -    // default implementation. Just pass paint through to the children
> -    PaintInfo childInfo(paintInfo);
> -    childInfo.paintingRoot = paintingRootForChildren(paintInfo);
> +    childPaintInfo.paintingRoot = paintingRootForChildren(childPaintInfo);
>      for (RenderObject* child = firstChild(); child; child = child->nextSibling())
> -        child->paint(childInfo, 0, 0);
> +        child->paint(childPaintInfo, 0, 0);
>  
>      if (paintInfo.phase == PaintPhaseForeground)
> -        finishRenderSVGContent(this, paintInfo, boundingBox, filter, savedInfo.context);
> +        finishRenderSVGContent(this, childPaintInfo, boundingBox, filter, paintInfo.context);

Can we start to optimize painting by skipping children that are entirely clipped out?

> +void RenderSVGImage::computeRectForRepaint(RenderBoxModelObject* repaintContainer, IntRect& repaintRect, bool fixed)
>  {
> -    // FIXME: handle non-root repaintContainer
> -    return m_absoluteBounds;
> +    // Translate to coords in our parent renderer, and then call computeRectForRepaint on our parent
> +    repaintRect = localToParentTransform().mapRect(repaintRect);
> +    parent()->computeRectForRepaint(repaintContainer, repaintRect, fixed);
>  }

It's a shame we can't just fall back on RenderBox::computeRectForRepaint(). If we fixed that to be more
HTML-agnostic, could we?

>  void RenderSVGRoot::calcViewport()
> @@ -204,35 +182,59 @@ void RenderSVGRoot::calcViewport()
>      if (!selfNeedsLayout() && !svg->hasRelativeValues())
>          return;
>  
> -    SVGLength width = svg->width();
> -    SVGLength height = svg->height();
>      if (!svg->hasSetContainerSize()) {
> -        m_viewport = FloatRect(0, 0, width.value(svg), height.value(svg));
> -        return;
> +        // In the normal case of <svg> being stand-alone or in a CSSBoxModel object we use
> +        // RenderBox::width()/height() (which pulls data from RenderStyle)
> +        m_viewportSize = FloatSize(width(), height());
> +    } else {
> +        // In the SVGImage case grab the SVGLength values off of SVGSVGElement and use
> +        // the special relativeWidthValue accessors which respect the specified containerSize
> +        SVGLength width = svg->width();
> +        SVGLength height = svg->height();
> +        float viewportWidth = (width.unitType() == LengthTypePercentage) ? svg->relativeWidthValue() : width.value(svg);
> +        float viewportHeight = (height.unitType() == LengthTypePercentage) ? svg->relativeHeightValue() : height.value(svg);
> +        m_viewportSize = FloatSize(viewportWidth, viewportHeight);

Does this logic deserve to be wrapped in its own method?

>  TransformationMatrix RenderSVGRoot::localToParentTransform() const
>  {
> -    TransformationMatrix offsetTranslation;
> -    offsetTranslation.translate(x(), y());
> -    return localToParentTransformWithoutCSSParentOffset() * offsetTranslation;
> +    IntSize parentToBorderBoxOffset = parentOriginToBorderBox();
> +
> +    TransformationMatrix borderBoxOriginToParentOrigin;
> +    borderBoxOriginToParentOrigin.translate(parentToBorderBoxOffset.width(), parentToBorderBoxOffset.height());
> +
> +    return localToBorderBoxTransform() * borderBoxOriginToParentOrigin;
>  }

Some tests with <svg> inside transformed HTML would be good to see if this all works.

> +void RenderSVGText::computeRectForRepaint(RenderBoxModelObject* repaintContainer, IntRect& repaintRect, bool fixed)
> +{
> +    // Translate to coords in our parent renderer, and then call computeRectForRepaint on our parent
> +    repaintRect = localToParentTransform().mapRect(repaintRect);
> +    parent()->computeRectForRepaint(repaintContainer, repaintRect, fixed);
>  }

Again, it's a shame we can't just drop into RenderBox::computeRectForRepaint()

> +    bool calculateLocalTransform();

What does the return value mean?

r=me

[Eric Seidel (no email)]

(In reply to comment #4)
> (From update of attachment 29827 [details] [review])
> Can we start to optimize painting by skipping children that are entirely
> clipped out?

Would we do that by clipping the damage rect and then checking against the clipped damage rect?

> It's a shame we can't just fall back on RenderBox::computeRectForRepaint(). If
> we fixed that to be more HTML-agnostic, could we?

We could, I think that's fodder for a separate bug though.  And actually, I'm not sure it will matter as the goal is to move all SVG renderers onto RenderSVGModelObject, and then they'll all share one computeRectForRepaint anyway.

> >  void RenderSVGRoot::calcViewport()
> Does this logic deserve to be wrapped in its own method?

Which logic?  calcViewport is the wrapper.  I do wonder if the SVGRoot for the SVGImage case shouldn't be a different renderer all together...

> Some tests with <svg> inside transformed HTML would be good to see if this all
> works.

I don't think there are any of those yet.  I've never dealt with transformed HTML, but yes, I expect much of this works.

> > +    bool calculateLocalTransform();
>
> What does the return value mean?

No clue.  That's all legacy code.  I'll remove the return value.

[Eric Seidel (no email)]

(In reply to comment #5)
> > > +    bool calculateLocalTransform();
> >
> > What does the return value mean?
> 
> No clue.  That's all legacy code.  I'll remove the return value.

Actually, I still need to remove localTransform() from RenderObject.  I'll fix this all in a later patch.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/mac/svg/custom/circle-move-invalidation-expected.txt
	M	LayoutTests/platform/mac/svg/custom/svg-float-border-padding-expected.txt
	M	LayoutTests/platform/mac/svg/custom/viewport-update2-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderPath.cpp
	M	WebCore/rendering/RenderSVGContainer.cpp
	M	WebCore/rendering/RenderSVGContainer.h
	M	WebCore/rendering/RenderSVGImage.cpp
	M	WebCore/rendering/RenderSVGImage.h
	M	WebCore/rendering/RenderSVGModelObject.cpp
	M	WebCore/rendering/RenderSVGModelObject.h
	M	WebCore/rendering/RenderSVGRoot.cpp
	M	WebCore/rendering/RenderSVGRoot.h
	M	WebCore/rendering/RenderSVGText.cpp
	M	WebCore/rendering/RenderSVGText.h
	M	WebCore/rendering/RenderSVGViewportContainer.cpp
	M	WebCore/rendering/RenderSVGViewportContainer.h
	M	WebCore/rendering/SVGRenderSupport.cpp
	M	WebCore/rendering/SVGRenderSupport.h
	M	WebCore/rendering/SVGRootInlineBox.cpp
Committed r42950

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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