402527 - canvas gradients broken on mac

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect, P2)

Description

[John Daggett (:jtd)]

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.

Comment 1

[John Daggett (:jtd)]

Attachment: another testcase

Should see an orange curve, as in FF2.

Tested on: Mac OS X 10.4.10 (8R218) PPC

Comment 2

[Brian Crowder]

Vlad:  If you've got any suggestions on this, I'll take the bug.  I am able to reproduce it.

Comment 3

[John Daggett (:jtd)]

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).

Comment 4

[Brian Crowder]

Wondering if this is related to bug 379321, Brian (Ewins) any ideas?

Comment 5

[Brian Ewins]

(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.

Comment 6

[Brian Ewins]

# 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.  

Comment 7

[John Daggett (:jtd)]

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.

Comment 8

[Brian Ewins]

(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.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 10

[John Daggett (:jtd)]

Attachment: 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.

Comment 11

[Brian Ewins]

(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.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Fixed by upgrade.

Comment 13

[Carsten Book [:Tomcat]]

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