Last Comment Bug 330682 - [FIX] Radial gradients are broken when fx,fy is on or outside the circumference defined by cx, cy and r
: [FIX] Radial gradients are broken when fx,fy is on or outside the circumferen...
Status: RESOLVED FIXED
: fixed1.8.1
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jonathan Watt [:jwatt] (back in October - email directly if necessary)
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-16 07:13 PST by Jonathan Watt [:jwatt] (back in October - email directly if necessary)
Modified: 2006-04-21 02:19 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
clamp fx and fy (1.32 KB, patch)
2006-03-16 07:21 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
scootermorris: review+
Details | Diff | Splinter Review
testcase - fx on circumference (554 bytes, image/svg+xml)
2006-03-16 07:29 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
no flags Details
testcase - fx outside circumference (553 bytes, image/svg+xml)
2006-03-16 07:30 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
no flags Details
address scooter's review comments (1.78 KB, patch)
2006-03-17 11:02 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
tor: superreview+
tor: approval‑branch‑1.8.1+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-03-16 07:13:21 PST
 
Comment 1 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-03-16 07:15:24 PST
Requesting blocking1.8.1. I have a very safe fix for this.
Comment 2 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-03-16 07:21:41 PST
Created attachment 215263 [details] [diff] [review]
clamp fx and fy

Make sure we clamp fx and fx as required by the paragraph a little further down the definition of the xlink:href behavour:

  http://www.w3.org/TR/SVG11/pservers.html#RadialGradientElementHrefAttribute

  If the point defined by fx and fy lies outside the circle defined by
  cx, cy and r, then the user agent shall set the focal point to the
  intersection of the line from (cx, cy) to (fx, fy) with the circle
  defined by cx, cy and r.
Comment 3 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-03-16 07:29:38 PST
Created attachment 215264 [details]
testcase - fx on circumference
Comment 4 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-03-16 07:30:25 PST
Created attachment 215265 [details]
testcase - fx outside circumference
Comment 5 Scooter Morris 2006-03-16 09:52:09 PST
Comment on attachment 215263 [details] [diff] [review]
clamp fx and fy

>Index: mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp,v
>retrieving revision 1.10
>diff -u -8 -p -r1.10 nsSVGCairoGradient.cpp
>--- mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp	18 Jan 2006 16:15:02 -0000	1.10
>+++ mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp	16 Mar 2006 15:17:04 -0000
>@@ -119,16 +119,29 @@ CairoRadialGradient(cairo_t *ctx, nsISVG
>   NS_ASSERTION(aRgrad, "error gradient did not provide a Linear Gradient interface");
> 
>   aRgrad->GetCx(&fCx);
>   aRgrad->GetCy(&fCy);
>   aRgrad->GetR(&fR);
>   aRgrad->GetFx(&fFx);
>   aRgrad->GetFy(&fFy);
> 
>+  if (fFx != fCx || fFy != fCy) {
>+    // clamp fx and fy to be within r
>+    float dx = fFx - fCx;
>+    float dy = fFy - fCy;
>+    double d = sqrt((dx * dx) + (dy * dy));
>+    double dMax = 0.995 * fR;  // we require < r, or the maths goes funny

Where does the math go funny?  Is it in cairo_pattern_create_radial?  I'm concerned that 0.995 is not close enough to 1.0, which might result in a "fringe" outside of the clamped fx and fy.

>+    if (d > dMax) {
>+      double angle = atan2(dy, dx);
>+      fFx = (float)(dMax * cos(angle)) + fCx;
>+      fFy = (float)(dMax * sin(angle)) + fCy;
>+    }
>+  }
>+
>   return cairo_pattern_create_radial(fFx, fFy, 0, fCx, fCy, fR);
> }
> 
> 
> cairo_pattern_t *
> CairoGradient(cairo_t *ctx, nsISVGGradient *aGrad,
>               nsISVGGeometrySource *aSource)
> {
Comment 6 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-03-16 10:33:19 PST
When I say the math goes funny, I mean I believe the algorithm for radial gradients only holds for a focal point inside the circumference. Certainly it's clear it doesn't hold for a focal point *on* the circumference, and the spec doesn't allow focal points outside it.

My reason for choosing 0.995 was arbitrary. Too close to 1 and rounding error will mean we end up with invalid result as if we actually had used 1. Batik uses a value of 0.97, and apparently nobody noticed until I did during my testing. Thomas is considering changing this to 0.999, and I'm happy enough to use that value too. Worst case scenario is that we might get some rare edge cases where we get the current funny rendering, but we'd have around 2 significant digits to play with so hopefully not. (If only we used double instead of float for our internal calculations as recommented by the spec.)
Comment 7 Scooter Morris 2006-03-17 09:45:53 PST
Comment on attachment 215263 [details] [diff] [review]
clamp fx and fy

>Index: mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp,v
>retrieving revision 1.10
>diff -u -8 -p -r1.10 nsSVGCairoGradient.cpp
>--- mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp	18 Jan 2006 16:15:02 -0000	1.10
>+++ mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGradient.cpp	16 Mar 2006 15:17:04 -0000
>@@ -119,16 +119,29 @@ CairoRadialGradient(cairo_t *ctx, nsISVG
>   NS_ASSERTION(aRgrad, "error gradient did not provide a Linear Gradient interface");
> 
>   aRgrad->GetCx(&fCx);
>   aRgrad->GetCy(&fCy);
>   aRgrad->GetR(&fR);
>   aRgrad->GetFx(&fFx);
>   aRgrad->GetFy(&fFy);
> 
>+  if (fFx != fCx || fFy != fCy) {
>+    // clamp fx and fy to be within r
>+    float dx = fFx - fCx;
>+    float dy = fFy - fCy;
>+    double d = sqrt((dx * dx) + (dy * dy));

OK, let's comment this more completely, and change the value 0.999 so that we can avoid the fringe as much as possible without getting too close to 1.0.  How about something like:

  // If dMax is set to fR, then we put the focal point on the circumference and cairo_pattern_create_radial
  // fails.  We need to set dMax to be different enough from fR to avoid it rounding to fR, but close enough
  // to avoid leaving a fringe around the outside.
>+    double dMax = 0.995 * fR;  // we require < r, or the maths goes funny
>+    if (d > dMax) {
>+      double angle = atan2(dy, dx);
>+      fFx = (float)(dMax * cos(angle)) + fCx;
>+      fFy = (float)(dMax * sin(angle)) + fCy;
>+    }
>+  }
>+
>   return cairo_pattern_create_radial(fFx, fFy, 0, fCx, fCy, fR);
> }
> 
> 
> cairo_pattern_t *
> CairoGradient(cairo_t *ctx, nsISVGGradient *aGrad,
>               nsISVGGeometrySource *aSource)
> {

With that, (or something similar) r=me
Comment 8 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-03-17 11:02:42 PST
Created attachment 215425 [details] [diff] [review]
address scooter's review comments

as discussed on IRC
Comment 9 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-03-31 18:15:40 PST
Filed a but with cairo to see what they think and if they will fix this from their end.

  https://bugs.freedesktop.org/show_bug.cgi?id=6308
Comment 10 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-04-13 05:02:42 PDT
Another bug that is relevant:

  https://bugs.freedesktop.org/show_bug.cgi?id=5681

And some relevant threads:

  http://lists.freedesktop.org/archives/cairo/2005-October/thread.html#5485
  http://lists.freedesktop.org/archives/cairo/2006-January/thread.html#5927

And some cworth comments on the above:

Some of those [patches] got applied, (which may be where the regressions started), and some did not, (the thread should have details). I stopped applying patches when I heard about X server crashes when switching to X-server-based gradients. But there is one patch before that which just touches pixman, which David claims helps correctness, and which has not been applied. Best thing to do would be to first come up with some test cases and see what happens with that patch. (As the bug report I linked to above indicates, there's a current regression with respect to some 1.0 releases, so that release could be used for generating reference images for a test case.)
Comment 11 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-04-19 14:31:42 PDT
So given http://lists.freedesktop.org/archives/cairo/2006-April/006706.html can you give this superreview and approval&#8209;branch&#8209;1.8.1 if I review the magic number tor?
Comment 12 tor 2006-04-19 14:33:44 PDT
Yes.
Comment 13 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2006-04-19 15:36:32 PDT
Checked in with the magic number as discussed on IRC.

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