563488 - Make pixman support self-copy operation

Status: RESOLVED DUPLICATE of bug 635035

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Pixman should be able to support self-copy operations and preferably without needing to make a temporary copy of the surface.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Version 0.01 Alpha attempt.

This appears to make self copy work flawlessly in the simple cases.

I still need to confirm that the functions with implemented self copy support (general and sse2 rgba->rgba) can handle tiny shifts.

Obviously needs a style cleanup before it's complete.

Comment 2

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

+    int sum_x = dest_x - src_x;
+    int sum_y = dest_y - src_y;

These are more often called "delta"

+    /* Same dest as source, positive scroll */
+    pixman_bool_t forwardY = !((dest == src) && (sum_y > 0));
+    pixman_bool_t forwardX = !((dest == src) && (sum_x > 0));

canScanForwardX/Y?

Something needs to happen here to handle generic cases like when we rotate a buffer by 90 degrees in-place. We definitely need to be creating a temporary surface sometimes.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Fixed handling of translations

Added creation of a temporary surface when a translation is required.
Added X translation support to the SSE2 backend function.
Tidied naming/comments.

Tested with various self copies including forward X/Y (both large numbers and 1px), and rotation.

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: same patch but with -p -U8 ctx

Comment 5

[Oleg Romashin (:romaxa)]

Comment on attachment 443561 [details] [diff] [review]
same patch but with -p -U8 ctx

oh, wrong patch

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: same patch but with -p -U8 ctx

Comment 7

[Oleg Romashin (:romaxa)]

I think it is a bit problematic to apply this patch on current central repo
bug 562087 need to be landed first

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Also, this should go upstream before we land it in central. I don't want to have to maintain it separately.

Comment 10

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

Suggest blocking fennec-final, because this is an optimization transparent to
gecko code that should be a big win for scrolling perf, when that's tuned.

Comment 11

[Siarhei Siamashka]

(In reply to comment #3)
> Created attachment 443249 [details] [diff] [review]
> Fixed handling of translations
> 
> Added creation of a temporary surface when a translation is required.
> Added X translation support to the SSE2 backend function.
> Tidied naming/comments.
> 
> Tested with various self copies including forward X/Y (both large numbers and
> 1px), and rotation.

As was discussed on #cairo irc channel a long time ago, everyone agreed (most importantly Søren Sandmann) that there are no strong objections and self-copy support can be added to pixman. But the patch is just a bit intrusive in the sense that it touches almost *every* possible way of using of pixman and needs to be tested really well. And an automated test for pixman test suite which would stress self-copy is a must.

The statement "It doesn't have to be complex" [1] is not very convincing at least to me without having a test program which can stress many possible self-copy related corner cases of pixman usage to support it. In particular, I feel that having complex clipping may (or may not) expose some bugs. Being pedantic, lots of other ways of using pixman (setting custom accessors, etc.) should be tested too.

The other relatively minor thing (but important for getting the patch accepted) is that some coding style [2] issues need to be fixed.

1. http://lists.cairographics.org/archives/cairo/2010-May/019816.html
2. http://cgit.freedesktop.org/pixman/tree/CODING_STYLE

Comment 12

[Doug Turner (:dougt)]

oleg, do you have tests for this?

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #10)
> Suggest blocking fennec-final, because this is an optimization transparent to
> gecko code that should be a big win for scrolling perf, when that's tuned.

Is still needed?

Comment 14

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

It's not important for scrolling perf in Gecko anymore; we wrote a very simple self-copy impl for the simple case we care most about (bug 635035).

Comment 15

[Matt Woodrow (:mattwoodrow)]

Fixed by bug 635035.