Make pixman support self-copy operation

RESOLVED DUPLICATE of bug 635035

Status

()

Core
Graphics
RESOLVED DUPLICATE of bug 635035
8 years ago
7 years ago

People

(Reporter: mattwoodrow, Unassigned)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Pixman should be able to support self-copy operations and preferably without needing to make a temporary copy of the surface.
(Reporter)

Comment 1

8 years ago
Created attachment 443203 [details] [diff] [review]
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.
Attachment #443203 - Flags: review?(roc)
+    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.
(Reporter)

Comment 3

8 years ago
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.
Attachment #443203 - Attachment is obsolete: true
Attachment #443203 - Flags: review?(roc)
Created attachment 443561 [details] [diff] [review]
same patch but with -p -U8 ctx
Comment on attachment 443561 [details] [diff] [review]
same patch but with -p -U8 ctx

oh, wrong patch
Attachment #443561 - Attachment is obsolete: true
Created attachment 443562 [details] [diff] [review]
same patch but with -p -U8 ctx
I think it is a bit problematic to apply this patch on current central repo
bug 562087 need to be landed first
Depends on: 562087
Also, this should go upstream before we land it in central. I don't want to have to maintain it separately.
Duplicate of this bug: 614736
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.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+

Comment 11

7 years ago
(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
oleg, do you have tests for this?

Updated

7 years ago
tracking-fennec: 2.0+ → 2.0-
(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?
tracking-fennec: 2.0- → ---
Whiteboard: [fennec-4.1?]
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).

Updated

7 years ago
Whiteboard: [fennec-4.1?]
(Reporter)

Comment 15

7 years ago
Fixed by bug 635035.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Resolution: FIXED → DUPLICATE
Duplicate of bug: 635035
You need to log in before you can comment on or make changes to this bug.