Bug 11929 - Gradient SVG animation demonstrates tearing at animation extremes
: Gradient SVG animation demonstrates tearing at animation extremes
Status: RESOLVED FIXED
: WebKit
SVG
: 420+
: Macintosh Mac OS X 10.4
: P4 Normal
Assigned To:
: http://srufaculty.sru.edu/david.daile...
: NeedsReduction
:
:
  Show dependency treegraph
 
Reported: 2006-12-22 07:28 PST by
Modified: 2009-12-04 13:05 PST (History)


Attachments
SVG Radial Gradient with focus outside of the circle (1.10 KB, image/svg+xml)
2009-12-01 11:18 PST, Dirk Schulze
no flags Details
SVG fix for broken radial gradient (5.46 KB, patch)
2009-12-01 14:36 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
SVG fix for broken radial gradient (242.77 KB, patch)
2009-12-02 08:30 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
SVG fix for broken radial gradient (242.03 KB, patch)
2009-12-02 09:13 PST, Dirk Schulze
zimmermann: review-
Review Patch | Details | Formatted Diff | Diff
SVG fix for broken radial gradient (241.98 KB, patch)
2009-12-03 01:12 PST, Dirk Schulze
zimmermann: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2006-12-22 07:28:45 PST
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"
------- Comment #1 From 2007-07-16 12:29:56 PST -------
Bug, is still there, even on feature-branch. Just a reminder.
------- Comment #2 From 2008-11-21 11:24:07 PST -------
It looks the same on gtkwebkit as on firefox. Is this bug still present on cg?
------- Comment #3 From 2009-03-03 11:04:36 PST -------
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.
------- Comment #4 From 2009-03-03 11:45:53 PST -------
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.
------- Comment #5 From 2009-12-01 11:18:06 PST -------
Created an attachment (id=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.
------- Comment #6 From 2009-12-01 14:36:29 PST -------
Created an attachment (id=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.
------- Comment #7 From 2009-12-02 08:30:01 PST -------
Created an attachment (id=44153) [details]
SVG fix for broken radial gradient

See above. With test updates as well as results for new test this time.
------- Comment #8 From 2009-12-02 08:31:14 PST -------
style-queue ran check-webkit-style on attachment 44153 [details] without any errors.
------- Comment #9 From 2009-12-02 09:13:47 PST -------
Created an attachment (id=44155) [details]
SVG fix for broken radial gradient

Mixed up Changelogs of WebCore and LayoutTests.
------- Comment #10 From 2009-12-02 09:17:28 PST -------
style-queue ran check-webkit-style on attachment 44155 [details] without any errors.
------- Comment #11 From 2009-12-02 18:37:50 PST -------
(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)
- 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 @@
> +2009-12-02  Dirk Schulze  <krit@webkit.org>
> +
> +        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  <ossy@webkit.org>G
------- Comment #12 From 2009-12-03 00:24:44 PST -------
(In reply to comment #11)
> (From update of attachment 44155 [details] [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).
------- Comment #13 From 2009-12-03 01:12:57 PST -------
Created an attachment (id=44214) [details]
SVG fix for broken radial gradient

See comment above.
------- Comment #14 From 2009-12-03 01:13:50 PST -------
style-queue ran check-webkit-style on attachment 44214 [details] without any errors.
------- Comment #15 From 2009-12-03 14:57:07 PST -------
(From update of attachment 44214 [details])
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.
------- Comment #16 From 2009-12-04 13:05:24 PST -------
landed in r51708. Closing bug.