Areas not painted when scrolling radial gradient

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

({verified1.9.2})

Trunk
mozilla1.9.3a1
x86
Mac OS X
verified1.9.2
Points:
---
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(3 attachments)

Created attachment 392182 [details]
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?
If we can't reproduce this on Linux or Windows, I'm calling this a Cairo/Quartz bug.

Comment 2

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

Comment 3

9 years ago
WFM on Ubuntu 9.04.
Duplicate of this bug: 508116
Could not reproduce on Windows.
Created attachment 392417 [details]
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.
Assignee: nobody → roc
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.
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.
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.
Actually, 18 is the height of the status bar.
Created attachment 392459 [details] [diff] [review]
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.
Attachment #392459 - Flags: review?(vladimir)
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.
Flags: blocking1.9.2?
Whiteboard: [needs review]
Attachment #392459 - Flags: review?(jmuizelaar)
Comment on attachment 392459 [details] [diff] [review]
fix

Maybe Jeff can review this if he gets to it first?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
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 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.
Attachment #392459 - Flags: review?(jmuizelaar) → review+
I tested this on 10.4, it works.

Landed: http://hg.mozilla.org/mozilla-central/rev/cc6bebbd93bb
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla1.9.3
Does this need to be added to a list of local patches we have to cairo?  And does it need to be moved upstream?
We also need to get this on branch...
Keywords: checkin-needed
Whiteboard: [needs landing on 1.9.2]
Target Milestone: mozilla1.9.3 → mozilla1.9.3a1
Attachment #392459 - Flags: review?(vladimir) → approval1.9.2?
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1a6603cd7841
status1.9.2: --- → beta1-fixed
Keywords: checkin-needed
Whiteboard: [needs landing on 1.9.2]

Updated

9 years ago
Attachment #392459 - Flags: approval1.9.2?
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
Flags: in-litmus?
Keywords: verified1.9.2
removing in-litmus flag, it no longer exists
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.