648266 - SVG animate path d subpath of zero length rendering residuals

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[Dr. Olaf Hoffmann]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0

The path d attribute is animated with several combinations of the commands M, m, Z, z producing zero length subpaths with round linecaps, to be displayed therefore as blue circles.
Apart from the problem that several suppaths are not displayed (maybe bug 584623 ? -if this is not covered by 584623, this is another problem available with this sample), there is a remaining trace of residuals from the one blue subpath, that is rendered. All subpaths should be displayed as blue circles covering always the red circles completely (the blue circles are larger than the red simulations). Gray circles indicate initial and final positions.

Reproducible: Always

Steps to Reproduce:
1. Try the example given at the URI above
2. Compare with desc(ription) and specification
3. Compare with behaviour of Batik/Squiggle, Adobe plugin, WebKit
Actual Results:  
a) Only one blue circle visible at all (maybe due to bug 584623, maybe for other reasons)

b) There remain residuals from the animated circle at previous positions
(this can be cleared for example by switching to another tab and back again)

c) The appearance of the residuals depends on the transformed red circles as
well, if this group is removed from the file, there are less residuals, but
still at most only one blue circle or some residuals from this circle.
Therefore parts of the residuals seem to be produced by the transformed group
of red circles below, but not all.

Expected Results:  
Blue circles should always cover the red circles, animating between the initial
and final positions indicated as gray circles. 
Everything else but circles indicate an error (trace residuals).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yep, looks like an invalidation issue.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: The testcase

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 607224 [details] [diff] [review]
patch

That's a lot of semi-duplicate code, and having to traverse the path data twice is overhead that would be good to avoid.

It seems like this could be fixed more simply by having the ConstructPath methods take an argument that indicates whether the call is for painting, hit-testing or obtaining bounds, and then changing the |capsAreSquare| bool in SVGPathData::ConstructPath to something like:

  // When stroke-linecap="square" cairo neither paints zero length subpaths nor
  // include them in the path extents. When stroke-linecap="round" cairo does
  // paint zero length subpaths correctly, but it still doesn't include them in
  // the path extents.
  bool needToFakeZeroLengthPaths =
    aCtx->CurrentLineCap() == gfxContext::LINE_CAP_SQUARE ||
    (aFor == eCreatePathForBounds &&
     aCtx->CurrentLineCap() == gfxContext::LINE_CAP_ROUND);

Comment 5

[Robert Longson [:longsonr]]

Attachment: updated patch

This implements most of your suggestion. If I change the signature of ConstructPath it means changing it in lots of places however only paths can have zero length subpaths so that didn't seem ideal.

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 609363 [details] [diff] [review]
updated patch

Review of attachment 609363 [details] [diff] [review]:
-----------------------------------------------------------------

r- based on the bustage for stroke extents for non-<path> (see below).

(In reply to Robert Longson from comment #5)
> If I change the signature of
> ConstructPath it means changing it in lots of places however only paths can
> have zero length subpaths so that didn't seem ideal.

Seems like it would make the code easier to follow and less quirky though, so it's still what I'd do. That said, see my comments below on your solution.

::: content/svg/content/src/nsSVGPathElement.cpp
@@ +407,5 @@
>  
> +void
> +nsSVGPathElement::GetZeroLengthSubpaths(gfxContext *aCtx, nsTArray<gfxPoint> *aPoints)
> +{
> +  mD.GetAnimValue().ConstructPath(aCtx, aPoints);

To avoid ending up with a doubly drawn path, you should probably call NewPath() on the context before calling ConstructPath. It may not matter in the current code, but it may catch someone out at a later date.

::: content/svg/content/test/bounds-helper.svg
@@ +25,5 @@
>      <rect id="rect4" x="150" y="50" width="50" height="50" fill="yellow"/>
>      <rect id="rect4a" x="150" y="50" width="50" height="50" fill="none" stroke-width="4" stroke="blue"/>
>    </g>
> +  <!-- multiple visible zero length subpaths -->
> +  <path id="path" d="M150 150Z M200 150z" stroke="blue" stroke-width="5" stroke-linecap="round" stroke-miterlimit="1"/>

Given the comment regarding bustage below, can you also add tests for:

 * <path> the same as above, but with no "d" attribute
 * <polyline>, both with "points" set to a zero length path not at {0,0}, and
   without the "points" attribute
 * <line> and <rect>, both with explicit zero size not at {0,0}, and without
   their four position attributes set

Seems like <path> and <polyline> without d/points should give rect(0,0,0,0) (since that will draw nothing), whereas <line> and <rect> should include stroke in their bounds in all four new tests requested for them (since they will paint the stroke).

::: layout/svg/base/src/nsSVGPathGeometryFrame.cpp
@@ +305,5 @@
>        ((aFlags & nsSVGUtils::eBBoxIgnoreStrokeIfNone) == 0 || HasStroke())) {
>      // We can't use context->GetUserStrokeExtent() since it doesn't work for
>      // device space extents. Instead we approximate the stroke extents from
>      // pathExtents using PathExtentsToMaxStrokeExtents.
> +    SetupCairoStrokeGeometry(context);

This call should go in the |if (mContent->Tag() == nsGkAtoms::path)| block.

@@ +306,5 @@
>      // We can't use context->GetUserStrokeExtent() since it doesn't work for
>      // device space extents. Instead we approximate the stroke extents from
>      // pathExtents using PathExtentsToMaxStrokeExtents.
> +    SetupCairoStrokeGeometry(context);
> +    if (pathExtents.Width() > 0 || pathExtents.Height() > 0) {

This isn't right, since it would make us stop taking account of stroke extents for zero length <line>, <polygon> and <polyline> with line caps, and zero size shapes that create a "dot" such as empty <rect>. Hence the request for extra tests above.

I think we probably need the existing code blocks to stay as they are, and to put something like your |if (mContent->Tag() == nsGkAtoms::path)| block before the |if (pathExtents.Width() <= 0 && pathExtents.Height() <= 0)| block, and put the existing code in an |else| block.

@@ +317,5 @@
> +    if (mContent->Tag() == nsGkAtoms::path) {
> +      nsTArray<gfxPoint> zeroLengthSubpaths;
> +      static_cast<nsSVGPathElement*>(mContent)->
> +        GetZeroLengthSubpaths(context, &zeroLengthSubpaths);
> +      const gfxSize tinyAdvance = context->DeviceToUser(gfxSize(2.0/256.0, 0.0));

It seems like we should avoid the quirky extra expansion that looks like small rounding error due to using tinyAdvance. Why not do something more like the following. I've also made the code avoid calling PathExtentsToMaxStrokeExtents in the loop.

    if (mContent->Tag() == nsGkAtoms::path) {
      // We need special handling for <path> since it can include zero length
      // subpaths.
      SetupCairoStrokeGeometry(context);
      nsTArray<gfxPoint> zeroLengthSubpaths;
      static_cast<nsSVGPathElement*>(mContent)->
        GetZeroLengthSubpaths(context, &zeroLengthSubpaths);
      if (!zeroLengthSubpaths.IsEmpty()) {
        if (pathExtents == gfxRect()) {
          // Since we need to use UnionEdges below to take account of the zero
          // size points, we need to overwrite the default gfxRect value so that
          // it is not included in the bounds.
          pathExtents =
            gfxRect(context->DeviceToUser(zeroLengthSubpaths[0]), gfxSize());
        }
        for (PRInt32 i = 0; i < zeroLengthSubpaths.Length(); i++) {
          pathExtents.UnionEdges(
            gfxRect(context->DeviceToUser(zeroLengthSubpaths[i]), gfxSize());
        }
        bbox = bbox.Union(
                 nsSVGUtils::PathExtentsToMaxStrokeExtents(pathExtents,
                                                           this,
                                                           aToBBoxUserspace));
      } else if (pathExtents != gfxRect()) {
        bbox = bbox.Union(
                 nsSVGUtils::PathExtentsToMaxStrokeExtents(pathExtents,
                                                           this,
                                                           aToBBoxUserspace));
      }
    } else {
      ...existing code
    }

Or something like that.

Comment 7

[Jonathan Watt [:jwatt]]

Some of the additional requested tests may not currently work, but those that do should continue to work after this patch. If you can fix the others too, that would be good.

Comment 8

[Robert Longson [:longsonr]]

Attachment: another testcase for zero length things

Comment 9

[Robert Longson [:longsonr]]

> This isn't right, since it would make us stop taking account of stroke extents for zero length <line>, <polygon> and <polyline> with line caps, and zero size shapes that create a "dot" such as empty <rect>. Hence the request for extra tests above.

We don't display line, polyline or polygons right. Empty rects are not dots though, they are not displayed no matter how you style them.

width = "<length>"
    The width of the rectangle.
    A negative value is an error (see Error processing). A value of zero disables rendering of the element.

Path with no d attribute or an empty d attribute is the same - not displayed as is polyline/polygon without points.

Comment 10

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 11

[Robert Longson [:longsonr]]

Comment on attachment 625735 [details] [diff] [review]
address review comments

I just realised we still need to draw the zero length paths in non-path cases.

Comment 12

[Robert Longson [:longsonr]]

Attachment: updated patch

Comment 13

[Robert Longson [:longsonr]]

Attachment: fix bitrot

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 633828 [details] [diff] [review]
fix bitrot

Can you update this again, Robert?

Comment 16

[Robert Longson [:longsonr]]

Testcases seem to be OK these days.