507939 - Areas not painted when scrolling radial gradient

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

Scrolling the attached testcase on Mac, I see that some parts of the gradient aren't repainted.

Michael, can you reproduce this on Linux?

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we can't reproduce this on Linux or Windows, I'm calling this a Cairo/Quartz bug.

Comment 2

[philippe (part-time)]

Fwiw, I see this on OS X, but not on Ubuntu904 running in a VM

Comment 3

[Michael Ventnor]

WFM on Ubuntu 9.04.

Comment 5

[Timothy Nikkel (:tnikkel)]

Could not reproduce on Windows.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase 2

Click on "Redraw". Every single time the element becomes visible, a small band at the bottom of the element is not painted. It only happens with repeating radial gradients --- not linear gradients, not non-repeating radial gradients. It's something to do with the dirty area though, since if you force repainting of the whole window, it paints OK. So I suspect a cairo/quartz bug with repeating radial gradients being clipped.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, but we actually fall back to pixman for repeating radial gradients:
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-quartz-surface.c#1169
I don't understand why we have to, but anyway, it seems this bug must be in cairo somewhere.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We seem to be consistently chopping 18 rows off the bottom of the rendered gradient.

There must be something wrong in the fallback path; _cairo_quartz_setup_fallback_source is only used for repeating radial gradients. However, the image data looks correct to me before it goes into _cairo_surface_to_cgimage, the extra lines are all there. I'm stumped for now.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm. The 18 is the height of the title bar. I think the problem is here:

    // get the Y flip right -- the CTM will always have a Y flip in place
    clipBox.origin.y = surface->extents.height - (clipBox.origin.y + clipBox.size.height);

Here (in a modified testcase) clipBox starts at x=0,y=0,width=200,height=200. The CTM (inverse) is a=1,b=0,c=0,d=-1,tx=0,ty=612. surface->extents is x=0,y=0,width=1038,height=594. So we compute a clipBox of x=0,y=-18,width=200,height=200.

I'm still not 100% sure what's going on here but clearly surface->extents being different from the 612 in the CTM is a problem.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, 18 is the height of the status bar.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes it.

The CTM we keep in our CGContext is always a mapping from cairo "device space" to CGContext's native space (i.e., a flip). The comment "// the clipBox is in userspace, so:" is misleading; it's actually already in cairo device space, so we shouldn't be doing anything at all with it here.

I also happened to notice that in _cairo_quartz_surface_paint with action DO_SHADING we aren't saving and restoring the CGContext state properly.

Unfortunately we can't really test this; I think drawing to a canvas won't exercise the problem here.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I filed bug 508227 on avoiding fallback for repeated radial gradients. I think there will be degenerate cases where we still have to fall back, though.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 392459 [details] [diff] [review]
fix

Maybe Jeff can review this if he gets to it first?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Guys, we really need review here. We're not really able to talk about our gradient support until it's not completely broken on Mac.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 392459 [details] [diff] [review]
fix

Vlad said that he thought that some of this code might be for 10.4 but he couldn't remember anything. He didn't put a comment in, so we agreed to delete it. As long as we pass tests this looks fine to me.

Comment 16

[Markus Stange [:mstange]]

I tested this on 10.4, it works.

Landed: http://hg.mozilla.org/mozilla-central/rev/cc6bebbd93bb

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does this need to be added to a list of local patches we have to cairo?  And does it need to be moved upstream?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes and yes.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We also need to get this on branch...

Comment 20

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1a6603cd7841

Comment 21

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a2pre) Gecko/20090911 Namoroka/3.6a2pre

Probably should this to litmus FFT

Comment 22

[Tracy Walker [:tracy]]

removing in-litmus flag, it no longer exists