Closed
Bug 579985
Opened 14 years ago
Closed 14 years ago
Black bar drawn and flickers across Google.com footer
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: aaronmt, Assigned: roc)
References
Details
(Keywords: regression)
Attachments
(6 files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Updated•14 years ago
|
Keywords: regression,
regressionwindow-wanted
Comment 2•14 years ago
|
||
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
blocking2.0: --- → ?
Reporter | ||
Comment 5•14 years ago
|
||
(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.
Reporter | ||
Updated•14 years ago
|
Keywords: regressionwindow-wanted
Updated•14 years ago
|
blocking2.0: ? → betaN+
Comment 6•14 years ago
|
||
Aaron, you never said - are you using accelerated layers?
Reporter | ||
Comment 7•14 years ago
|
||
(In reply to comment #6)
> Aaron, you never said - are you using accelerated layers?
D2D? Nope. Mac - new profile.
Comment 8•14 years ago
|
||
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.
Component: Graphics → Layout
QA Contact: thebes → layout
Updated•14 years ago
|
Component: Layout → Graphics
Comment 9•14 years ago
|
||
Aaron: why'd you remove the request for a regression window? Do you have one?
Reporter | ||
Comment 10•14 years ago
|
||
(In reply to comment #9)
> Aaron: why'd you remove the request for a regression window? Do you have one?
Mistake
Keywords: regressionwindow-wanted
Updated•14 years ago
|
QA Contact: layout → thebes
Comment 11•14 years ago
|
||
Timothy tells me he didn't mean to move this back to the Graphics component.
Component: Graphics → Layout
QA Contact: thebes → layout
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → roc
Assignee | ||
Comment 12•14 years ago
|
||
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.
Assignee: roc → tnikkel
Comment 13•14 years ago
|
||
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?
Assignee: tnikkel → roc
Assignee | ||
Comment 14•14 years ago
|
||
I'm guessing not.
Assignee | ||
Comment 15•14 years ago
|
||
(I do see it on Mac)
Assignee | ||
Comment 16•14 years ago
|
||
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.
Attachment #477850 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 17•14 years ago
|
||
This lets cairo-quartz optimize and is probably a better idea all around. We shouldn't be doing this optimization at this level.
Attachment #477852 -
Flags: review?(vladimir)
Assignee | ||
Comment 18•14 years ago
|
||
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).
Attachment #477855 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 19•14 years ago
|
||
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.
Attachment #477856 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 20•14 years ago
|
||
Note: part 4 depends on the patches in bug 593270, to avoid rotting those patches while they wait for review.
Attachment #477852 -
Flags: review?(vladimir) → review+
Assignee | ||
Comment 21•14 years ago
|
||
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.
Assignee | ||
Comment 22•14 years ago
|
||
Actually that alternative is tricky because we have to ensure that the drawing of the source surface would paint over the entire clip extents.
Assignee | ||
Comment 23•14 years ago
|
||
So let's go with the above patches instead.
Assignee | ||
Comment 24•14 years ago
|
||
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.
Attachment #478178 -
Flags: review?(vladimir)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review]
Comment 27•14 years ago
|
||
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.
Attachment #477850 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review] → [needs review][needs landing]
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.
Attachment #478178 -
Flags: review?(vladimir) → review+
Assignee | ||
Comment 29•14 years ago
|
||
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.
Assignee | ||
Comment 30•14 years ago
|
||
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•14 years ago
|
||
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()
Attachment #477855 -
Flags: review?(jmuizelaar) → review+
Comment 32•14 years ago
|
||
Comment on attachment 477856 [details] [diff] [review]
Part 4: Paint opaque surfaces using kPrivateCGCompositeCopy when possible
Quartz won't do this optimization itself?
Assignee | ||
Comment 33•14 years ago
|
||
How can it? Quartz doesn't know that the alpha values in the CGLayer are all 1.0.
Comment 34•14 years ago
|
||
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
Attachment #477856 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review][needs landing] → [needs landing]
Assignee | ||
Comment 35•14 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Keywords: regressionwindow-wanted
Resolution: --- → FIXED
Whiteboard: [needs landing]
Assignee | ||
Comment 36•14 years ago
|
||
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•