651469 - Scrolling up renders page unreadable (X11 with GL layers)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Here on linux x86-64, proprietary NVIDIA driver, with layers.acceleration.force-enabled=true, scrolling is completely broken on this page:

http://zrusin.blogspot.com/

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

To be more precise, the problem seems to be only/mostly with scrolling up, as opposed to scrolling down.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

The timing makes it plausible that this might be a regression from bug 640082, but I haven't verified this.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Add MovePixels support to gfxXlibSurface

Interestingly this happens when scrolling up the page (moving pixels down on the backing surface). With Image surfaces copying up is the problem, so I guess this means our internal data is Y-inverted.

If this ends up being a bottleneck we could try detect the flip (is it always the case for X surface?) and only allocate a temporary if we need one.

XCopyArea can handle self copies without manual intervention, but it is apparently rather slow. See http://lists.freedesktop.org/archives/cairo/2009-June/017290.html

Comment 6

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

I actually think that gfxASurface::MovePixels should use the safe, copying version by default, and a "fast" self-copy should be used in each backend where we know it works. I think gfxWindowsSurface and gfxQuartzSurface can use the fast implementation; gfxImageSurface already has its own implementation.

Comment 7

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

Oh, I guess D2D can use the fast version too.

A protected helper method in gfxASurface could implement the direct self-copy so each subclass only needs a few lines of code to use it.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Add FastMovePixels to gfxASurface and use it where appropriate

Sure, works for me.

Comment 9

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

Comment on attachment 529911 [details] [diff] [review]
Add FastMovePixels to gfxASurface and use it where appropriate

Review of attachment 529911 [details] [diff] [review]:

Comment 10

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

Do you have a testcase for this that could be turned into a reftest?

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Add new scrolling reftest that scrolls backwards

Fails without the patch, passes with.

Comment 12

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

Comment on attachment 529922 [details] [diff] [review]
Add new scrolling reftest that scrolls backwards

Review of attachment 529922 [details] [diff] [review]:

lovely!

Comment 13

[Matt Woodrow (:mattwoodrow)]

http://hg.mozilla.org/mozilla-central/rev/a0fd33b098f6
http://hg.mozilla.org/mozilla-central/rev/e861aabf1631

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> XCopyArea can handle self copies without manual intervention, but it is
> apparently rather slow. See
> http://lists.freedesktop.org/archives/cairo/2009-June/017290.html

Quoting that post:
> When an overlapped XCopyArea is issued - as is typical for a short
> scroll in any direction - XCopyArea breaks up the request into
> individual scanline fetches and puts.  This produces correct results,
> but because each scanline is done via two full Pixman call chains, it is
> hideously slow.  By that I mean that it can be about a third of the
> speed of a full copy-out followed by copy-back.

FWIW it looks like this is describing the fb implementation, the one used when the ddx graphics driver doesn't provide it's own implementation.