Closed Bug 330682 Opened 18 years ago Closed 18 years ago

[FIX] Radial gradients are broken when fx,fy is on or outside the circumference defined by cx, cy and r

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: jwatt)

Details

(Keywords: fixed1.8.1)

Attachments

(3 files, 1 obsolete file)

 
Requesting blocking1.8.1. I have a very safe fix for this.
Status: NEW → ASSIGNED
Flags: blocking1.8.1?
Attached patch clamp fx and fy (obsolete) — Splinter Review
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.
Attachment #215263 - Flags: superreview?(tor)
Attachment #215263 - Flags: review?(scootermorris)
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)
> {
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 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
Attachment #215263 - Flags: review?(scootermorris) → review+
as discussed on IRC
Attachment #215263 - Attachment is obsolete: true
Attachment #215425 - Flags: superreview?(tor)
Attachment #215263 - Flags: superreview?(tor)
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
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.)
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?
Yes.
Attachment #215425 - Flags: superreview?(tor) → superreview+
Attachment #215425 - Flags: approval-branch-1.8.1+
Checked in with the magic number as discussed on IRC.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Flags: blocking1.8.1?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: