Closed Bug 402527 Opened 14 years ago Closed 14 years ago

canvas gradients broken on mac

Categories

(Core :: Canvas: 2D, defect, P2)

PowerPC
macOS
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jtd, Assigned: jtd)

References

Details

(Keywords: testcase)

Attachments

(2 files)

The testcase URL renders fine in FF 2.0.0.9 but displays a blank white box in trunk latest.  Opera 9.5alpha renders it but Safari 2.04 shows a blank black box.
Flags: blocking1.9?
Attached file another testcase
Should see an orange curve, as in FF2.

Tested on: Mac OS X 10.4.10 (8R218) PPC
Flags: blocking1.9? → blocking1.9+
Target Milestone: --- → mozilla1.9 M10
Priority: -- → P2
Target Milestone: mozilla1.9 M10 → ---
Vlad:  If you've got any suggestions on this, I'll take the bug.  I am able to reproduce it.
Brian, I traced this down to the Cairo update that was done late PM 8/1 (builds before work, after don't).  

Bonsai query for the list of changes:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=%2Fmozilla%2Fgfx&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-08-01&maxdate=2007-08-02+12%3A00&cvsroot=%2Fcvsroot

Since this is Mac-only, my first guess would be something in the Quartz surface code:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/gfx/cairo/cairo/src&command=DIFF_FRAMESET&file=cairo-quartz-surface.c&rev1=1.25&rev2=1.26&root=/cvsroot

I would probably be a good thing to set up simple reftests to flag obvious regressions like this (e.g. canvas w/ gradient should not match about:blank).
Wondering if this is related to bug 379321, Brian (Ewins) any ideas?
(In reply to comment #4)
> Wondering if this is related to bug 379321, Brian (Ewins) any ideas?
> 

I dont think its related. Those patches landed in time for 1.4.10, which renders the relevant tests ok. 

I'd mentioned to Vlad on IRC that I've been seeing blanks for gradient tests all week; I thought it might have been something environmental with me because I recently upgraded to Leopard and Vlad wasn't seeing it; but I've since tested on an older Tiger machine and its the same.

The problems seem to have started between 1.4.10 and 1.5.2, but that period of time is difficult to git-bisect because the pixman api wasn't stable (so you need a different build of pixman for each build of cairo!)

The problem is triggered if you hit the call to CGContextDrawShading in _cairo_quartz_surface_paint, but I think the source of the problem must lie elsewhere - not sure if its in the creation of the CGShading or in what we do with the painted surface. 

There were a couple of changes to gradients at that time but they don't look suspicious at all, I'm trying to bisect manually now.
# good: [e5fdacae1c5b7005b95db8f9065cce51ef19bf20] [fixpt] fix up image surface to translate to 16.16 for pixman
git-bisect good e5fdacae1c5b7005b95db8f9065cce51ef19bf20
# bad: [b719592428907d2010645303fb65d38dcb8b30c0] [fixpt] Fix up compilation post pixman merge
git-bisect bad b719592428907d2010645303fb65d38dcb8b30c0

In between these two the code doesn't compile (or rather, fails to compile so badly you might as well apply b71959.) That narrows down the offending cairo commits to:
b719592428907d2010645303fb65d38dcb8b30c0 [fixpt] Fix up compilation post pixman merge
866b485314bfd5d8bbf865d19f6a589d08292e2a [fixpt] Let the compiler calculate the magic double-to-fixed value
0abe5324a5b03149630a5b6496c980f83be4fd75 [fixpt] Create cairo_region wrapper around pixman_region16_t
dc035fecda0070e18a68e06f567f268fc39483f1 [fixpt] Fix xcb surface to handle conversion to 16.16
58d9664702308639ead888c7167e71ca605a8fe3 [fixpt] Fix xlib surface to handle conversion to 16.16
aaf94ef6c4656d7e836e52c2a71db214a1c01b57 [fixpt] remove dependency on some pixman types

We can rule out the xcb, xlib patches, but there's still a lot of lines in here.  
I stepped through this and it looks like we're setting up the call to CGShadingCreateAxial and calling through to CGContextDrawShading, so the code isn't wildly wrong.  But setting a break in ComputeGradientValue, I'm seeing some really weird values.  The calculation looks like:

 out[0] = (grad->stops[i-1].color.red / 65535.) * ap + (grad->stops[i].color.red / 65535.) * bp;

But color.red looks like it's already a value between 0 and 1.  So the out color will *always* be essentially (0,0,0,0).  With alpha = 0, nothing will get drawn.  There's a color.red_short that's [0,65536).  Guessing that trimming out that '/ 65535' may fix things.  Give it a whirl.
(In reply to comment #7)
> But color.red looks like it's already a value between 0 and 1.  So the out
> color will *always* be essentially (0,0,0,0).  With alpha = 0, nothing will get
> drawn.  There's a color.red_short that's [0,65536).  Guessing that trimming out
> that '/ 65535' may fix things.  Give it a whirl.

Good catch John, thats almost certainly it. You can see the corresponding fix in cairo-pattern here: 
http://gitweb.freedesktop.org/?p=cairo;a=blobdiff;h=a4c4682f2fc5964f1a6dca7afd9f94d5eb21d82e;hp=00c427e89eff6186659145dde68406309240f982;hb=aaf94ef6c4656d7e836e52c2a71db214a1c01b57;f=src/cairo-pattern.c

... previous to these patches, .red/.green/.blue were shorts, afterwards they were doubles (oh, that change is just *evil*), and no corresponding fix got applied to cairo-quartz.

I'll get this tested and pushed tonight. We're getting other blanks appearing in tests from the CGContextClipToMask patch onwards, but I'm hoping they're related to this; that patch worked fine prior to the changeset above.
Ah yes, sorry about that!  I expected some warnings, but in this case it was being used in a double context :(  That's almost certainly the problem. Those should just be direct assignments from .red/.green/.blue instead of dividing by 65535.
Not sure whether this is getting fixed on the cairo side of things but I tested this patch and it fixes the problem.
Assignee: nobody → jdaggett
(In reply to comment #10)
> Created an attachment (id=289292) [details]
> patch to remove unnecessary division
> 
> Not sure whether this is getting fixed on the cairo side of things but I tested
> this patch and it fixes the problem.
> 

Vlad pushed a fix to cairo on 15 Nov
http://gitweb.freedesktop.org/?p=cairo;a=commit;h=50d5f5a4e6d7424694b0b27fc0c3a00c9eb203bb

... so I assume he's picking that up as part of bug 404092.
Depends on: 404092
Fixed by upgrade.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre and the testcase

--> Verified fixed
Status: RESOLVED → VERIFIED
Keywords: testcase
You need to log in before you can comment on or make changes to this bug.