Bug 25431 - Simplify how SVG containers paint (and likely fix bugs by doing so)
Summary: Simplify how SVG containers paint (and likely fix bugs by doing so)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 19366 24633 (view as bug list)
Depends on:
Blocks: 25403 25432
  Show dependency treegraph
 
Reported: 2009-04-27 14:17 PDT by Eric Seidel (no email)
Modified: 2009-04-28 17:10 PDT (History)
4 users (show)

See Also:


Attachments
Simplify how SVG containers paint (44.26 KB, patch)
2009-04-27 14:18 PDT, Eric Seidel (no email)
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-04-27 14:17:29 PDT
Simplify how SVG containers paint (and likely fix bugs by doing so)

See attached patch (with long ChangeLog entry).
Comment 1 Eric Seidel (no email) 2009-04-27 14:18:32 PDT
Created attachment 29827 [details]
Simplify how SVG containers paint

 21 files changed, 302 insertions(+), 203 deletions(-)
Comment 2 Dirk Schulze 2009-04-28 03:28:12 PDT
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.
Comment 3 Eric Seidel (no email) 2009-04-28 10:44:02 PDT
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. :)
Comment 4 Simon Fraser (smfr) 2009-04-28 11:16:42 PDT
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
Comment 5 Eric Seidel (no email) 2009-04-28 12:04:13 PDT
(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.
Comment 6 Eric Seidel (no email) 2009-04-28 12:34:43 PDT
(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.
Comment 7 Eric Seidel (no email) 2009-04-28 12:37:43 PDT
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
Comment 8 Eric Seidel (no email) 2009-04-28 14:41:58 PDT
*** Bug 19366 has been marked as a duplicate of this bug. ***
Comment 9 Eric Seidel (no email) 2009-04-28 17:10:42 PDT
*** Bug 24633 has been marked as a duplicate of this bug. ***