Gradient SVG animation demonstrates tearing at animation extremes

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Gradient animation demonstrates tearing at animation extremes

http://srufaculty.sru.edu/david.dailey/svg/Sfour.svg

Animate the center of the radial gradient, when the radius is not particularly large to notice the "tearing" (parts of the triangle will disappear).   The same tearing is not evident in Firefox.

I'm not sure if this is a bug or not.  It may be a rounding error.  It may be a CG error (or implementation choice).

Firefox does not demonstrate such "tearing"

[Nikolas Zimmermann]

Bug, is still there, even on feature-branch. Just a reminder.

[Dirk Schulze]

It looks the same on gtkwebkit as on firefox. Is this bug still present on cg?

[Dirk Schulze]

The bug seems to be in SVGRadialGradientElement line 103-107. WebKitGtk/Cairo has the same problem after the platform independent SVG gradient patch.
We made a differnt check on cairo: http://trac.webkit.org/browser/trunk/WebCore/svg/graphics/cairo/SVGPaintServerGradientCairo.cpp?rev=38500#L96
I think this should fix the problem.

[Dirk Schulze]

hm. I was wrong. The only difference at the end is, that we used double's instead of float's to draw the gradients. I'll check this.

[Dirk Schulze]

Created attachment 44088 [details]
SVG Radial Gradient with focus outside of the circle

I'm a bit confused that our calculation (that moves the focus into the circle, if the user position it outside) doesn't work here while it does on http://www.w3.org/Graphics/SVG/Test/20061213/svggen/pservers-grad-13-b.svg . Independent of the gradientUnits.

[Dirk Schulze]

Created attachment 44103 [details]
SVG fix for broken radial gradient

This is the fix for the broken radial gradient. The adjusted focalPoint must be moved by the centralPoint, because the base (the focalPoint) was substracted by the centralPoint before. This issue was fixed in the cairo specific code, but got lost with the platform independent code. It is also neccessary to make a deviation of maximum 0.2% to get around the fixed point values in cairo.

I'm sure this patch will change some pixeltests. I also added a new pixeltest but can't produce the results at the moment.

[Dirk Schulze]

Created attachment 44153 [details]
SVG fix for broken radial gradient

See above. With test updates as well as results for new test this time.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44153 [details] without any errors.

[Dirk Schulze]

Created attachment 44155 [details]
SVG fix for broken radial gradient

Mixed up Changelogs of WebCore and LayoutTests.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44155 [details] without any errors.

[Nikolas Zimmermann]

Comment on attachment 44155 [details]
SVG fix for broken radial gradient

Good morning Dirk,

I see some issues with the patch:
- ChangeLogs are not using the correct style: bug url should sit above description, without surrounding brackets (best idea is to use "prepare-ChangeLog --bug=<bug-id>", this takes care of using the right style for you)
- The patch can't go in without updated test results (I can help with that, if you don't have a mac around)

> -    float fdx = focalPoint.x() - centerPoint.x();
> -    float fdy = focalPoint.y() - centerPoint.y();
> +    FloatPoint adjustedFocalPoint = focalPoint;
> +    adjustedFocalPoint.move(-centerPoint.x(), -centerPoint.y());
I agree with the first change, using 'FloatPoint adjustedFocalPoint' instead of the two adjustedFocusX/adjustedFocusY variables, though the "fdx" ones are useful, as they save function call overhead in the sqrt() below, just the name is bad. (Re-)Introducing helper variables, also saves you from having to mve back the adjustFocalPoint by the centerPoint below the first if() statement.


> +        // The maximum deviation of 0.2% is needed for Cairo, since Cairo
> +        // is working with fixed point numbers.
> +        if (focalPoint.x() < centerPoint.x())
> +            adjustedFocalPoint.setX(cosf(angle) * radius + 0.002f);
> +        else
> +            adjustedFocalPoint.setX(cosf(angle) * radius - 0.002f);
> +        if (focalPoint.y() < centerPoint.y())
> +            adjustedFocalPoint.setY(sinf(angle) * radius + 0.002f);
> +        else
> +            adjustedFocalPoint.setY(sinf(angle) * radius - 0.002f);

This really worries me. If you really need a Cairo specific workaround, it can't go directly into SVGRadialGradientElement. Not sure about the root of your problem, and how to fix it in another way, offhand. Please elaborate on that.

Have a nice day,
Niko

>      }
>  
> +    adjustedFocalPoint.move(centerPoint.x(), centerPoint.y());
> +
>      RefPtr<Gradient> gradient = Gradient::create(
> -        FloatPoint(adjustedFocusX, adjustedFocusY),
> +        adjustedFocalPoint,
>          0.f, // SVG does not support a "focus radius"
>          centerPoint,
>          radius);
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 51600)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> [phone]  Dirk Schulze  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        In SVG a focalPoint must be inside the radius of a radial gradient.
> +        It this isn't the case, we have to move the focalPoint into the radius.
> +        This checks the correct behavior of WebKit on false values for fx, fy.      
> +
> +        * platform/mac/svg/W3C-SVG-1.1/pservers-grad-13-b-expected.checksum:
> +        * platform/mac/svg/W3C-SVG-1.1/pservers-grad-13-b-expected.png:
> +        * platform/mac/svg/custom/radial-gradient-with-outstanding-focalPoint-expected.checksum: Added.
> +        * platform/mac/svg/custom/radial-gradient-with-outstanding-focalPoint-expected.png: Added.

> +        * platform/mac/svg/custom/radial-gradient-with-outstanding-focalPoint-expected.txt: Added.
> +        * svg/custom/radial-gradient-with-outstanding-focalPoint.svg: Added.
> +
>  2009-12-02  Csaba Osztrogonác  <[e-mail]>G

[Dirk Schulze]

(In reply to comment #11)
> (From update of attachment 44155 [details])
> Good morning Dirk,
> 
> I see some issues with the patch:
> - ChangeLogs are not using the correct style: bug url should sit above
> description, without surrounding brackets (best idea is to use
> "prepare-ChangeLog --bug=<bug-id>", this takes care of using the right style
> for you)

I take care of it in the next patch.

> - The patch can't go in without updated test results (I can help with that, if
> you don't have a mac around)

I updated the results already.

> 
> > -    float fdx = focalPoint.x() - centerPoint.x();
> > -    float fdy = focalPoint.y() - centerPoint.y();
> > +    FloatPoint adjustedFocalPoint = focalPoint;
> > +    adjustedFocalPoint.move(-centerPoint.x(), -centerPoint.y());
> I agree with the first change, using 'FloatPoint adjustedFocalPoint' instead of
> the two adjustedFocusX/adjustedFocusY variables, though the "fdx" ones are
> useful, as they save function call overhead in the sqrt() below, just the name
> is bad. (Re-)Introducing helper variables, also saves you from having to mve
> back the adjustFocalPoint by the centerPoint below the first if() statement.

Well fdx/fdy can be renamed dfx/dfy (d stands for delta). Is this ok for you?
 
> > +        // The maximum deviation of 0.2% is needed for Cairo, since Cairo
> > +        // is working with fixed point numbers.
> > +        if (focalPoint.x() < centerPoint.x())
> > +            adjustedFocalPoint.setX(cosf(angle) * radius + 0.002f);
> > +        else
> > +            adjustedFocalPoint.setX(cosf(angle) * radius - 0.002f);
> > +        if (focalPoint.y() < centerPoint.y())
> > +            adjustedFocalPoint.setY(sinf(angle) * radius + 0.002f);
> > +        else
> > +            adjustedFocalPoint.setY(sinf(angle) * radius - 0.002f);
> 
> This really worries me. If you really need a Cairo specific workaround, it
> can't go directly into SVGRadialGradientElement. Not sure about the root of
> your problem, and how to fix it in another way, offhand. Please elaborate on
> that.

We do platform specific things everywhere in WebCore (mostly for CG), also for SVG (see SVGPaintServerGradient as example). The deviation of maximal 0.2% is not noticable, even if you zoom as far as possible into the document, you won't see a difference.
I'm fine to make #if PLATFORM(Cairo) around this code, but it is not possible to move this deviation into the platform specific code in platform/graphics/cairo. That would mean to move the complete calculation of adjustedFocalPoint into platform/graphics/* (and replication of the same code multiple times).

[Dirk Schulze]

Created attachment 44214 [details]
SVG fix for broken radial gradient

See comment above.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44214 [details] without any errors.

[Nikolas Zimmermann]

Comment on attachment 44214 [details]
SVG fix for broken radial gradient

Please move the 0.002f hacks in a PLATFORM(CAIRO) block. Not optimal, but as we can't share this code in cairo-specific Gradient implementation (as <canvas> allows any focal point, as example), it's fine for now, to stay in SVG*Gradient.cpp, r=me.

[Dirk Schulze]

landed in r51708. Closing bug.