604533 - "Tearing"/"Shearing" while panning on local pages/error console in Fennec

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Michael Wu [:mwu]]

Attachment: An example

While panning, sometimes things go crazy and draw wrong. Example attached. It fixes itself a few seconds later.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is reminiscent of bug 579736 and bug 583115, but obviously the underlying cause would be different.  It's also strange that this doesn't appear on remote pages.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I was able to reproduce this on desktop and device using steps similar to those in bug 599876.  Still not sure what's up though.

Comment 4

[Robert Sayre]

I made a movie: http://www.youtube.com/watch?v=7fklbzqDmzY

Comment 6

[(not currently active) Ted Mielczarek]

This is 100% reproducible with the following steps:
1) Load fennec
2) Kill the plugin-container process via adb shell
3) Reload content process, load about:crashes
4) Click on the crash report in the list to submit it
5) While the throbber is spinning (it spins forever currently), pan the page

Comment 7

[(not currently active) Ted Mielczarek]

I'm guessing the animation of the throbber is what makes this so easy to reproduce in this case.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I suspect the problem here is that cairo image surface don't really support self-copies.  I suspect the origin of the rendering artifacts is here http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ThebesLayerBuffer.cpp#250, so we would want to not attempt that optimization.  There are various levels of cleanliness in a disabling patch; dirtiest hack would be skipping this #if !mobile, cleanest would probably be adding gfxASurface::CanCopyFromSelf(), false for gfxImageSurface, and adding a fallback path in thebeslayerbuffer.  I can look at this late tonight if no one else has in the meantime.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Should add that comment 8 refers to a temporary workaround for b3.  The real fix would obviously be to make cairo image surfaces support self-copies to the extent that the impl can be performant.)

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 492799 [details] [diff] [review]
patch

I'm worried about the negative performance impact that this will have but I'm also not a fan of self-copy and would rather we fix the performance problem without using self-copy. Still, correctness is better than performance here, so r+. 

I've asked roc for review here because he likely has an opinion, but I think it should be ok to land without waiting for him.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 492799 [details] [diff] [review]
patch

No, you can't force this workaround on desktop too, where it's a valid and correct optimization.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Rather, on the gfx surfaces we use on desktop, i.e. not gfxImageSurface.)

Comment 14

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

(In reply to comment #11)
> I'm worried about the negative performance impact that this will have but I'm
> also not a fan of self-copy and would rather we fix the performance problem
> without using self-copy. Still, correctness is better than performance here, so
> r+.

We can't avoid the self-copy without allocating a buffer. It seems unlikely to me that we can avoid self-copy without some kind of performance hit.

The cairo approach of saying that self-copies are undefined is a bad decision. Maybe we should hack around it by adding a method to gfxASurface that tells us whether self-copies are supported?

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 492888 [details] [diff] [review]
patch

(superfluous r+ from me, though you I guess could r=cjones sr=roc for gfx API change.)

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/9c3babe383e1 with s/bool/PRBool

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Jeff/Rob: the correct followup to this bug would be, "make cairo image surfaces support self-copies to the extent that the impl can be performant", right?

Comment 19

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

Yes. Actually Matt Woodrow did some work on that.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Turns out we had bug 563488 sitting in reserve all along.  Ah well.