User Agent: Mozilla/5.0 (Windows NT 5.1; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release) Build ID: 20140515030202 Steps to reproduce: Create a document filled with lots of text. Style it with `background-repeat: repeat` and some `background-image`. Scroll down. Actual results: There are weird visual issues. Expected results: There shouldn't be weird visual issues.

Could you paste "Graphics" section of Help > "Troubleshooting Information"?

I can repro in safemode with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140515030202 CSet: e4843f4f08a7 Dupe of Bug 671302?

Here is my Graphics info: > Adapter Description NVIDIA GeForce 6150SE nForce 430 > Adapter Drivers nv4_disp > Adapter RAM Unknown > Device ID 0x03d0 > DirectWrite Enabled false (0.0.0.0) > Driver Date 1-31-2013 > Driver Version 6.14.13.783 > GPU #2 Active false > GPU Accelerated Windows 0/1 Basic Blocked for your graphics card because of unresolved driver issues. > Vendor ID 0x10de > WebGL Renderer Google Inc. -- ANGLE (NVIDIA GeForce 6150SE nForce 430 Direct3D9 vs_3_0 ps_3_0) > windowLayerManagerRemote false > AzureCanvasBackend skia > AzureContentBackend cairo > AzureFallbackCanvasBackend cairo > AzureSkiaAccelerated 0

XtC4UaLL, probably both bugs are related, because they appear at the same scrolling point. But I think they are not exactly the same, because bug 671302 only affects the background, while this bug also affects the text.

The problem appeared first in 2014-05-06-03-02-04-mozilla-central. In 2014-05-05-03-02-02-mozilla-central I can only reproduce bug 671302.

I can reproduce the text layout problem if HWA off. (Background image problem is a different bug(bug 671302)). Regression window(m-i) Good: https://hg.mozilla.org/integration/mozilla-inbound/rev/50116088c244 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140505010736 Bad: https://hg.mozilla.org/integration/mozilla-inbound/rev/fd797f07d0db Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140505012936 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=50116088c244&tochange=fd797f07d0db Regressed by: Bug 1000875 - Opaque fixed background repainted when starting to scroll

And I can reproduce on ubuntu 12.04 without HWA. https://hg.mozilla.org/mozilla-central/rev/e4843f4f08a7 Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140515030202

:mstange If you do not fix this, you should back Bug 1000875 out.

I am going to fix it. I'll back out if the fix hasn't landed within two weeks.

Bug 1012797 fixed the specific gradient case as used in bugzilla, but did not fix attachment 8423343 [details] or bug 671302. I'm going to leave this bug open for fixing gradients in general. The fix for repeating raster images will happen in bug 671302.

Markus I think we need an update here :-)

Comment on attachment 8455319 [details] [diff] [review] patch Oops, I attached this patch to the wrong bug. This patch doesn't fix the gradient case, only the repeated image case. I'll move it to bug 671302.

oops

Comment on attachment 8458726 [details] [diff] [review] v1 Review of attachment 8458726 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.cpp @@ +2241,5 @@ > + // magnitude from the gradient line points into the pattern matrix allows > + // more gradients to render. > + double scale = 1.0 / (1 << 8); > + *aLineStart = *aLineStart * scale; > + *aLineEnd = *aLineEnd * scale; Use *=

Comment on attachment 8458726 [details] [diff] [review] v1 Review of attachment 8458726 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.cpp @@ +2241,5 @@ > + // magnitude from the gradient line points into the pattern matrix allows > + // more gradients to render. > + double scale = 1.0 / (1 << 8); > + *aLineStart = *aLineStart * scale; > + *aLineEnd = *aLineEnd * scale; Oh I guess gfxPoint doesn't have it. Whatever.

Is the patch good to land? If so, can we get this on m-c and get uplift requests for Aurora and Beta?

It's not, unfortunately. Here's a try push with lots of test failures: https://tbpl.mozilla.org/?tree=Try&rev=588b894ed6e6 I'm still working on it.

Two more try pushes with different experimental changes: https://tbpl.mozilla.org/?tree=Try&rev=8a494cc68d68 https://tbpl.mozilla.org/?tree=Try&rev=3fa95876fb41

The try pushes show two things: 1. double scale = 1.0 / (1 << 8) is misleading because it promises more than it gives us. It turns out that this trick only gains us one bit, i.e. it only allows us to double the gradient size from 2^15 pixels to 2^16 pixels. Using 1.0 / (1 << 1), or just 0.5, gives the same result. No matter the scale, after 2^16 pixels pixman gradients disappear. 2. Even with scale = 0.5, many gradient reftests render slightly differently, on all platforms. Most could be fixed with a bit of fuzz since the color values only differ by 1 or 2. There's one exception: With scale = 0.5, D2D fails the reftests layout/reftests/css-gradients/repeating-radial-1*.html completely by scaling and placing the radial gradient incorrectly. So I'm going to do two things: First, I'm going to change scale to 0.5 and explicitly call out in the comment that we only double the possible gradient size, and I'm going to deactivate the workaround if the gradient vector is small enough that the workaround is not necessary. I should probably also file a bug about the D2D issue.

This patch also adds a few tests: one that's fixed by this patch (large-gradient-1.html), one that's not (large-gradient-3.html) because the gradient size exceeds 2^16 pixels, and two for bug 1012797 (-2 and -4).

bug 1049499 + this: https://tbpl.mozilla.org/?tree=Try&rev=6f59ff87fa68

(In reply to Markus Stange [:mstange] from comment #23) > The try pushes show two things: > 1. double scale = 1.0 / (1 << 8) is misleading because it promises more than > it gives us. It turns out that this trick only gains us one bit, i.e. it > only allows us to double the gradient size from 2^15 pixels to 2^16 pixels. > Using 1.0 / (1 << 1), or just 0.5, gives the same result. No matter the > scale, after 2^16 pixels pixman gradients disappear. Why is this?

Comment on attachment 8468596 [details] [diff] [review] v2 Thanks for asking the hard question. Now that I know the answer, I was able to find a much better fix. The reason for that was that the value for ty that's calculated here was too big: http://hg.mozilla.org/mozilla-central/annotate/71497ed2e0db/gfx/cairo/cairo/src/cairo-image-surface.c#l1207 It needs to be such that fabs (ty + extents->y + extents->height) < PIXMAN_MAX_INT, otherwise the gradient won't render. I haven't tried to track down the reason for that.

This patch now lets us render gradients of all sizes. The function _pixman_image_for_gradient has a very similar approach to what I tried: If the gradient vector is too long, a scale is pulled into the pattern matrix and the gradient vector size is reduced by that scale. However, there are two bugs here. First, the scaled matrix that is calculated (|matrix|) is never used in the rest of the function. So I replaced all other instances of "pattern->base.matrix" with "matrix". Secondly, the scale is applied to the matrix in the wrong order. Changing the order around means that the translation components of the pattern matrix are also affected by the scale factor, which means that they're much less likely to overflow, which almost solves all of our problems. With the matrix stuff fixed, we only need to make sure that tx and ty don't get too large, and then everything works. Jeff, is this also the code that's used on Windows XP to draw gradients? Or does that use a different code path? Unfortunately the try server is closed at the moment.

Updating the bug summary to match what I'm fixing.

The cairo xlib backend gets its gradient rasterized by _cairo_pattern_acquire_surface_for_gradient in cairo-pattern.c. This patch now makes the same change there, and I hope that this will also fix Windows. https://tbpl.mozilla.org/?tree=Try&rev=48a409b40451

Same patch as before but with the declaration of max_x and max_y moved to the start of the block to satisfy stricter C compilers. Passes reftests on all platforms. :) Try push with Windows coverage: https://tbpl.mozilla.org/?tree=Try&rev=1cfa56e2fa74

Jeff - This bug has been waiting on review from you for 10 days and is tracked for beta. If we're going to take this on beta we really need this to land by Thu morning this week. Can you get to this review or delegate the review to someone else?

Milan - I *think* you told me that Jeff is on PTO. Is there someone else on your team who can do the review?

That's going to be tough. We can probably check if the code is "correct", but Jeff is the one that can tell us if the code is "complete" as well. Roc can always review everything, so if we really want to rush this, I'd say he's probably the one to do it.

Given tests pass, I say we rubber-stamp this and let Jeff deal with it when he gets back. It looks OK to me but I won't pretend to know this code.

Comment on attachment 8470088 [details] [diff] [review] better fix inside cairo Review of attachment 8470088 [details] [diff] [review]: ----------------------------------------------------------------- Rubber-stamp=me

Roc, thanks for the rubber stamp, which I think is fine for m-c in this case. However, I'm not filled with a lot of confidence in taking this fix in the final beta. milan/jet/roc - Are you OK shipping Firefox 32 with this regression? If not, can you help weigh the pros/cons of accepting this patch at the last minute vs shipping a known regression?

Given that this was caused by bug 1000875, we could just back that out on Beta. Explanation of why bug 1000875 made not rendering giant gradients worse: On the systems / gfx configurations that this bug affects, we've never rendered giant gradients. This is not a new issue. However, before bug 1000875, we didn't correctly pick up that opaque gradients in the background of a page are really opaque, so before painting the gradient, we cleared the background with the page's background color. Then we painted any text on top of that background color. After bug 1000875, we correctly pick up on the opaqueness of the gradients, and don't bother painting a background color under the gradient. However, that means that any text from the previous paint is still there in the graphics buffer. Then, if painting the gradient on top of the old buffer contents fails, the existing text from the last paint is not erased, and when we paint new text on top of it, we have new text on top of old text, and things get hard to read.

Sounds like the bug 1000875 was less bad than this regression - wrong background, correct text, vs. correct background, wrong looking text. We will probably see a 20% tscroll regression, so we need to have that story ready for the Beta and just allow it to get through.

Milan/Markus - Do you have a bug on file for backing our bug 1000875 or do you want to take over this bug? Can you get the backout completed in the next couple of hours?

I'll attach a backout patch to this bug, and will land it as soon as the tree reopens.

One thing I forgot to mention: Bug 1000875 improved our scrolling performance numbers on some platforms. Backing it out will mean that we'll get regression alerts when we drop back to the perf numbers from before.

Backed out bug 1000875 on Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/11a5306111d0 Tracking should move from 32 to 33, I suppose?

(In reply to Markus Stange [:mstange] from comment #44) > One thing I forgot to mention: Bug 1000875 improved our scrolling > performance numbers on some platforms. Backing it out will mean that we'll > get regression alerts when we drop back to the perf numbers from before. You didn't forget, it was mentioned in comment 41 :) - I expect 20% badness.

Huh. I did forget to completely read your comment then! But I'm not completely sure what you're saying. Do you agree that backout + perf regression is the right tradeoff?

Absolutely.

Tracking for 33 and 34. cc jmaher about the perf regression. See comment 41 and comment 47.

I reproduced the issue with Firefox 32 Beta 8 and Firefox 34 Nightly from August 13th & 21st, on Windows 7 x64 and Ubuntu 13.04 x64. I set Hardware Acceleration OFF, opened the demo page and scrolled to the bottom. When close to the bottom I got the mixed up text. Note that I could not reproduce the original issue on Mac OS X 10.8.5. The text issue no longer reproduced on: - Firefox 32 Beta 9 (Ubuntu 13.04 x64) - BuildID: 20140822024446 - Latest Nightly 34 (Win 7 x64, Ubuntu 13.04 x64) - BuildID: 20140822030201 Note that I can still see the background issue on Firefox 32 Beta 9 (bug 671302), which makes sense since it did not make it to 32. Leaving as fixed for 32, until I can verify it on Windows as well.

Also verified today on Win 7 x64 with Firefox 32 Beta 9, with same results as on Linux.

Markus, could you tell me what are you plan for 33? Should we uplift the patch which landed in m-c? Thanks

I was hoping to get a review from Jeff first, and then uplift the patch.

Jeff, could you help us here? I could like to see the fix uplifted soon to 33. Thanks

Markus, do you know someone else who could help here? Thanks

Comment on attachment 8470088 [details] [diff] [review] better fix inside cairo Review of attachment 8470088 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/cairo/cairo/src/cairo-image-surface.c @@ +1146,5 @@ > p2.x = _cairo_fixed_16_16_from_double (_cairo_fixed_to_double (linear->p2.x) * sf); > p2.y = _cairo_fixed_16_16_from_double (_cairo_fixed_to_double (linear->p2.y) * sf); > > + cairo_matrix_init_scale (&scale, sf, sf); > + cairo_matrix_multiply (&matrix, &matrix, &scale); What's the point of breaking this part out? @@ +1208,5 @@ > > x = _cairo_lround (inv.x0 / 2); > y = _cairo_lround (inv.y0 / 2); > + > + max_x = PIXMAN_MAX_INT - 1 - fabs (extents->x + extents->width); Where does the '1' come from? This collection ::: gfx/cairo/cairo/src/cairo-pattern.c @@ +1399,5 @@ > p2.x = _cairo_fixed_16_16_from_double (_cairo_fixed_to_double (linear->p2.x) * sf); > p2.y = _cairo_fixed_16_16_from_double (_cairo_fixed_to_double (linear->p2.y) * sf); > > + cairo_matrix_init_scale (&scale, sf, sf); > + cairo_matrix_multiply (&matrix, &matrix, &scale); What's the point of breaking this out?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #58) > Comment on attachment 8470088 [details] [diff] [review] > better fix inside cairo > > Review of attachment 8470088 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/cairo/cairo/src/cairo-image-surface.c > @@ +1146,5 @@ > > p2.x = _cairo_fixed_16_16_from_double (_cairo_fixed_to_double (linear->p2.x) * sf); > > p2.y = _cairo_fixed_16_16_from_double (_cairo_fixed_to_double (linear->p2.y) * sf); > > > > + cairo_matrix_init_scale (&scale, sf, sf); > > + cairo_matrix_multiply (&matrix, &matrix, &scale); > > What's the point of breaking this part out? It's not about breaking it out, it's about reversing the order of the multiplication. The existing code was doing a prescale, but it needs to be a postscale. > @@ +1208,5 @@ > > > > x = _cairo_lround (inv.x0 / 2); > > y = _cairo_lround (inv.y0 / 2); > > + > > + max_x = PIXMAN_MAX_INT - 1 - fabs (extents->x + extents->width); > > Where does the '1' come from? I no longer recall completely, but the comment I added above says we need to ensure that fabs (ty + extents->y + extents->height) < PIXMAN_MAX_INT and without the -1 we can get a == instead of a < there, I think. I haven't taken the time to completely prove it to me again. > This collection ?

(In reply to Markus Stange [:mstange] from comment #59) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #58) > > Comment on attachment 8470088 [details] [diff] [review] > > better fix inside cairo > > > > Review of attachment 8470088 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: gfx/cairo/cairo/src/cairo-image-surface.c > > @@ +1146,5 @@ > > > p2.x = _cairo_fixed_16_16_from_double (_cairo_fixed_to_double (linear->p2.x) * sf); > > > p2.y = _cairo_fixed_16_16_from_double (_cairo_fixed_to_double (linear->p2.y) * sf); > > > > > > + cairo_matrix_init_scale (&scale, sf, sf); > > > + cairo_matrix_multiply (&matrix, &matrix, &scale); > > > > What's the point of breaking this part out? > > It's not about breaking it out, it's about reversing the order of the > multiplication. The existing code was doing a prescale, but it needs to be a > postscale. Ah right. It might be worth adding a commend explaining that. > > @@ +1208,5 @@ > > > > > > x = _cairo_lround (inv.x0 / 2); > > > y = _cairo_lround (inv.y0 / 2); > > > + > > > + max_x = PIXMAN_MAX_INT - 1 - fabs (extents->x + extents->width); > > > > Where does the '1' come from? > > I no longer recall completely, but the comment I added above says we need to > ensure that > fabs (ty + extents->y + extents->height) < PIXMAN_MAX_INT > and without the -1 we can get a == instead of a < there, I think. I haven't > taken the time to completely prove it to me again. > > > This collection > This was supposed to say. This collection of statements would probably be easier to read with the use of a clamp function that explicitly had the upper and lower bounds passed in.

Jeff, Markus, we are going to build beta 8 today. If we want this to be fixed in 33, we will have to act fast. Thanks

Comment on attachment 8470088 [details] [diff] [review] better fix inside cairo Thanks for the reminder. Let's land this patch on Beta as-is. I'll address Jeff's review comments in a new patch that only needs to land on Nightly, since it won't change any functionality.

The text issue no longer reproduced on Firefox 33 Beta 8 (buildID: 20140929180120 ) on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5.