579985 - Black bar drawn and flickers across Google.com footer

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Aaron Train [:aaronmt]]

A black bar is drawn and flickers across Google.com's footer most likely a result of changes in retained layers on trunk.

STR:
1. Visit http://www.google.com
2. Click in the search box
3. See black box drawn and flicker across the footer

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100719 Minefield/4.0b2pre

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Black bar drawn across footer

Comment 2

[Drew Willcoxon :adw]

Surely this is a dupe of some well known bug (which I could not find)...  Requesting blocking because this is super visible and super not cool.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b6pre) Gecko/20100904 Firefox/4.0b6pre

Comment 5

[Aaron Train [:aaronmt]]

(In reply to comment #2)
> Surely this is a dupe of some well known bug (which I could not find)... 
> Requesting blocking because this is super visible and super not cool.
> 
> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b6pre) Gecko/20100904
> Firefox/4.0b6pre

Could be bug 580756, but I'm not certain. Thanks for the block because this is incredibly ugly.

Comment 6

[Joe Drew (not getting mail)]

Aaron, you never said - are you using accelerated layers?

Comment 7

[Aaron Train [:aaronmt]]

(In reply to comment #6)
> Aaron, you never said - are you using accelerated layers?

D2D? Nope. Mac - new profile.

Comment 8

[Joe Drew (not getting mail)]

Actually, I meant OpenGL accelerated layers, but those aren't enabled by default in a new profile, so this is probably a retained layers or some-other-related-thing regression.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Aaron: why'd you remove the request for a regression window? Do you have one?

Comment 10

[Aaron Train [:aaronmt]]

(In reply to comment #9)
> Aaron: why'd you remove the request for a regression window? Do you have one?

Mistake

Comment 11

[Joe Drew (not getting mail)]

Timothy tells me he didn't mean to move this back to the Graphics component.

Comment 12

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

Timothy, can you reproduce this on one of your machines? If not, please assign it back to me. It's pretty bad, we should fix it ASAP.

Comment 13

[Timothy Nikkel (:tnikkel)]

I couldn't reproduce. I didn't try Windows 7 because I can't reboot that machine right now, but I feel like I would have seen this before if I could reproduce somewhere.

Has anyone seen this on something besides Mac?

Comment 14

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

I'm guessing not.

Comment 15

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

(I do see it on Mac)

Comment 16

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

Attachment: fix

The problem is that _cairo_quartz_surface_mask tries to be fast by calling CGContextSetAlpha to mask with a solid color. Unfortunately that doesn't work with some non-OVER operators. In particular, Quartz seems to treat the alpha as part of the source rather than a mask (in cairo terms), so when using OPERATOR_SOURCE, the destination pixels are completely wiped out, whereas we want 1-maskalpha of the destination pixels to be retained.

This patch fixes the problem by restricting the optimization to OPERATOR_OVER. However, this introduces a potential performance regression, because BasicLayers is currently using OPERATOR_SOURCE to composite opaque surfaces into the backbuffer. So in particular, testcases involving opaque surfaces with CSS opacity set on them will require us to create a temporary mask surface in cairo-quartz now. I'll fix that in subsequent patches.

Comment 17

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

Attachment: Part 2: use OVER instead of SOURCe to composite ThebesLayers

This lets cairo-quartz optimize and is probably a better idea all around. We shouldn't be doing this optimization at this level.

Comment 18

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

Attachment: Part 3: support opaque CGLayer surfaces

We want to internally be able to optimize for the fact that a CGLayer cairo-quartz surface is opaque, even though Quartz itself doesn't know. That means we need to be able to pass in a cairo_content_t when creating one. This patch does that. That means we need to always recreate the surface if the layer changes from opaque to transparent or vice versa, so there is no need to have AreSimilarSurfacesSensitiveToContentType (it would now return true for all surface types).

Comment 19

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

Attachment: Part 4: Paint opaque surfaces using kPrivateCGCompositeCopy when possible

This automatically turns OVER into SOURCE when painting any kind of opaque surface in the cairo-quartz backend, as long as no-one's trying to use CGContextSetAlpha to do masking.

Comment 20

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

Note: part 4 depends on the patches in bug 593270, to avoid rotting those patches while they wait for review.

Comment 21

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

We may not actually want parts 2 3 and 4 for FF4, they're a bit risky since part 2 affects all backends.

An alternative would be to change SOURCE to OVER in _cairo_quartz_surface_mask when possible.

Comment 22

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

Actually that alternative is tricky because we have to ensure that the drawing of the source surface would paint over the entire clip extents.

Comment 23

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

So let's go with the above patches instead.

Comment 24

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

Attachment: Part 2 alternative: use OVER instead of SOURCE when we have opacity

This is a safer version of part 2. When we have aOpacity < 1 we're going to have to do some OVER compositing anyway, so disabling the SOURCE optimization in that case should be fine for performance on all platforms. And it will actually let us go faster on Mac given patch #1.

If we go with this version, we don't need patches 3 and 4, although they're still valid optimizations.

Comment 27

[Jeff Muizelaar [:jrmuizel]]

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

It would be good to have "Using CGContextSetAlpha to implement mask alpha doesn't work for some operators." as a comment near the conditional.

Comment 28

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

Comment on attachment 478178 [details] [diff] [review]
Part 2 alternative: use OVER instead of SOURCE when we have opacity

So I'll r+ this, but the problem is that for some backends, SOURCE is slower than OVER.  So ideally we'd want the previous patch (always use OVER, let backends convert that to SOURCE if the source is opaque), but this shouldn't hurt.

Comment 29

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

I actually think part 2 *will* hurt on Mac without parts 3 and 4 also being landed.

Looks like D2D optimizes SOURCE to OVER when compositing an opaque surface, so we should be OK there. And pixman should be OK.

Comment 30

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

Oh, your comment was for "part 2 alternative".

D2D wants "OVER". I believe pixman can optimize OVER to SOURCE for opaque surfaces already. Hopefully XRender drivers can do the same (or it doesn't matter because their compositing is fast anyway). So I think parts 2, 3 and 4 together would actually be good. So I'll hold out for Jeff's review on parts 3 and 4.

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 477855 [details] [diff] [review]
Part 3: support opaque CGLayer surfaces

You should update the documentation for cairo_quartz_surface_create_cg_layer()

Comment 32

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 477856 [details] [diff] [review]
Part 4: Paint opaque surfaces using kPrivateCGCompositeCopy when possible

Quartz won't do this optimization itself?

Comment 33

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

How can it? Quartz doesn't know that the alpha values in the CGLayer are all 1.0.

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 477856 [details] [diff] [review]
Part 4: Paint opaque surfaces using kPrivateCGCompositeCopy when possible

It's probably worth adding a comment that this optimization is for layers that a format with an alpha channel but have a content type of CAIRO_CONTENT_COLOR

Comment 35

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

http://hg.mozilla.org/mozilla-central/rev/965528190a2d
http://hg.mozilla.org/mozilla-central/rev/b58d66cea46e
http://hg.mozilla.org/mozilla-central/rev/aacc4ca15cad
http://hg.mozilla.org/mozilla-central/rev/19744c96dffd
http://hg.mozilla.org/mozilla-central/rev/deecd1437c71

The test was broken, so it was disabled:
http://hg.mozilla.org/mozilla-central/rev/d2d645506534

I'll check in a fixed test later.

Comment 36

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

Fixed test.
http://hg.mozilla.org/mozilla-central/rev/dca9da098880