[cairo] Pixman sampling coordinates (Re: White seams (lines) appearing between objects
Bertram Felgenhauer
bertram.felgenhauer at googlemail.com
Sat Jan 19 07:45:47 PST 2008
Carl Worth wrote:
> > May I suggest that it's the rendering of the rectangle that's odd here,
> > and not the sampling of the image?
>
> Thanks for the clarification.
>
> > OFFSET=0.5 is a boundary case, it's hard to get this correct.
>
> They should still be consistent in this case. So I think both the
> rendering of the geometry (trapezoid sampling) and the rendering of
> the image (surface sampling) both have bugs.
Ok. I've investigate both issues and came up with patches for pixman
that fix them.
Patch 1: fix cairo's a1-image-sample test
The problem here is that the image source offset is subtracted from
the sample position, so an offset of 0.5 will end up as -0.5 in the
math and will then be rounded up, so the effective offset is 0.
The patch essentially adds an epsilon to get the desired rounding
behaviour. There's no good mathematical justification for it.
Patch 2: fix cairo's a1-traps-sample test
This patch actually fixes a problem with pixman's RenderSamplesX()
macro, which could assign a subpixel a coverage of over 100%. It also
fixes the targeted test case.
Note that this time the correct rounding falls out naturally from
the math.
Patch 3: move sampling point for gradients as well
This applies the epsilon hack from the image sampling to gradient
handling as well. I don't think it should be applied, but I'll include
it for completeness.
Test suite results (image backend only):
- new passes:
a1-image-sample
a1-traps-sample
The targeted testcases. Good.
- new failures:
rectangle-rounding-error
unantialiased-shapes
These tests involve non-antialiased trapezoids so that's expected.
rotate-image-surface-paint
This test uses an image source with FILTER_NEAREST, and hits the boundary
case. Again, this is expected.
- additional failures caused by the 3rd patch:
mask
text-pattern
These involve various gradients and apparently hit the boundary case.
All in all I think this looks good.
> I've just pushed two new tests (a1-traps-sample and a1-image-sample)
> that both fail which you can read here:
>
> http://gitweb.freedesktop.org/?p=cairo;a=commitdiff;h=50d0767c8bf4c738b86e10be09d5c4fd7e14a05f
>
> In both cases I'm trying to draw a grid of 100 single-pixel squares
> each slightly offset by a different amount on the sub-pixel grid so
> that you can watch and see the point where the offset changes which
> relative pixel sample position is considered "inside" the square.
I'd use
#define POINTS 11
#define STEP (1.0 / (POINTS - 1))
because that specifies a symmetric geometry. Of course, the output will
then be asymmetric in one way or another.
> What I believe I'd like to see in each case is the following
> image.
This image would then get an extra row of squares to the right
and bottom.
But in any case, I agree it's important to get trapezoid and image
sampling to agree, and this choice is as good as the other.
> Meanwhile, here's the result I'm getting in the case of image
> rendering:
>
> Here the break is visible on both axes, which is good, but it's below
> and to the right of where I expected it. I believe this corresponds to
> a sample position at the half-integer locations but with the "tie
> breaking" happening in the opposite direction compared to what I
> described above.
If you'd specify a symmetric geometry as I suggested above, that picture
would look equally asymmetric for both choices of rounding 0.5.
regards,
Bertram
-------------- next part --------------
commit 1d89bac5a7a5693911d8a74701bd1c0292160478
Author: Bertram Felgenhauer <int-e at gmx.de>
Date: Sat Jan 19 13:29:56 2008 +0100
fix cairo's a1-image-sample test
Move the sampling point for image surfaces very slightly so that it's in
the upper left quadrant of the pixel.
diff --git a/pixman/pixman-compose.c b/pixman/pixman-compose.c
index c3f50e2..e240850 100644
--- a/pixman/pixman-compose.c
+++ b/pixman/pixman-compose.c
@@ -3695,8 +3695,8 @@ static void fbFetchTransformed(bits_image_t * pict, int x, int y, int width, uin
stride = pict->rowstride;
/* reference point is the center of the pixel */
- v.vector[0] = pixman_int_to_fixed(x) + pixman_fixed_1 / 2;
- v.vector[1] = pixman_int_to_fixed(y) + pixman_fixed_1 / 2;
+ v.vector[0] = pixman_int_to_fixed(x) + pixman_fixed_1 / 2 - 1;
+ v.vector[1] = pixman_int_to_fixed(y) + pixman_fixed_1 / 2 - 1;
v.vector[2] = pixman_fixed_1;
/* when using convolution filters one might get here without a transform */
-------------- next part --------------
commit a614662f842e1ad0166c41b2911b80b40afdc065
Author: Bertram Felgenhauer <int-e at gmx.de>
Date: Sat Jan 19 14:35:13 2008 +0100
fix cairo's a1-traps-sample test
The left and right boundaries of edges were not rounded correctly.
Interestingly, the coverage calculation contained the required offset for
rounding in the RenderSamplesX() macro, but at this point it's bogus, as
it can lead to sub-pixels being attributed more than 100% coverage.
The right way to handle this offset is to pre-add it to the edge coordinates,
which incidentally also fixes the edge rounding problem.
diff --git a/pixman/pixman-edge-imp.h b/pixman/pixman-edge-imp.h
index 2cf6d73..aa0c88e 100644
--- a/pixman/pixman-edge-imp.h
+++ b/pixman/pixman-edge-imp.h
@@ -46,10 +46,10 @@ rasterizeEdges (pixman_image_t *image,
int rxi;
/* clip X */
- lx = l->x;
+ lx = l->x + X_FRAC_FIRST(N_BITS);
if (lx < 0)
lx = 0;
- rx = r->x;
+ rx = r->x + X_FRAC_FIRST(N_BITS);
if (pixman_fixed_to_int (rx) >= width)
rx = pixman_int_to_fixed (width);
diff --git a/pixman/pixman-edge.c b/pixman/pixman-edge.c
index 1ed6f10..9018770 100644
--- a/pixman/pixman-edge.c
+++ b/pixman/pixman-edge.c
@@ -142,10 +142,10 @@ fbRasterizeEdges8 (pixman_image_t *image,
int lxi, rxi;
/* clip X */
- lx = l->x;
+ lx = l->x + X_FRAC_FIRST(8);
if (lx < 0)
lx = 0;
- rx = r->x;
+ rx = r->x + X_FRAC_FIRST(8);
if (pixman_fixed_to_int (rx) >= width)
rx = pixman_int_to_fixed (width);
diff --git a/pixman/pixman-private.h b/pixman/pixman-private.h
index 33a9e7c..2e8229a 100644
--- a/pixman/pixman-private.h
+++ b/pixman/pixman-private.h
@@ -748,7 +748,7 @@ union pixman_image
#define MAX_ALPHA(n) ((1 << (n)) - 1)
#define N_Y_FRAC(n) ((n) == 1 ? 1 : (1 << ((n)/2)) - 1)
-#define N_X_FRAC(n) ((1 << ((n)/2)) + 1)
+#define N_X_FRAC(n) ((n) == 1 ? 1 : (1 << ((n)/2)) + 1)
#define STEP_Y_SMALL(n) (pixman_fixed_1 / N_Y_FRAC(n))
#define STEP_Y_BIG(n) (pixman_fixed_1 - (N_Y_FRAC(n) - 1) * STEP_Y_SMALL(n))
@@ -762,7 +762,7 @@ union pixman_image
#define X_FRAC_FIRST(n) (STEP_X_SMALL(n) / 2)
#define X_FRAC_LAST(n) (X_FRAC_FIRST(n) + (N_X_FRAC(n) - 1) * STEP_X_SMALL(n))
-#define RenderSamplesX(x,n) ((n) == 1 ? 0 : (pixman_fixed_frac (x) + X_FRAC_FIRST(n)) / STEP_X_SMALL(n))
+#define RenderSamplesX(x,n) ((n) == 1 ? 0 : (pixman_fixed_frac (x)) / STEP_X_SMALL(n))
/*
* Step across a small sample grid gap
-------------- next part --------------
commit 2c5e7ddcd75f4dd0cf4a9eec64b7b51f6be1d6ca
Author: Bertram Felgenhauer <int-e at gmx.de>
Date: Sat Jan 19 15:34:33 2008 +0100
move sampling point for gradients as well
This patch is a logical consequence of "fix cairo's a1-image-sample test",
but it breaks some cairo tests.
diff --git a/pixman/pixman-compose.c b/pixman/pixman-compose.c
index e240850..586749e 100644
--- a/pixman/pixman-compose.c
+++ b/pixman/pixman-compose.c
@@ -3282,8 +3282,8 @@ static void pixmanFetchSourcePict(source_image_t * pict, int x, int y, int width
linear_gradient_t *linear = (linear_gradient_t *)pict;
/* reference point is the center of the pixel */
- v.vector[0] = pixman_int_to_fixed(x) + pixman_fixed_1/2;
- v.vector[1] = pixman_int_to_fixed(y) + pixman_fixed_1/2;
+ v.vector[0] = pixman_int_to_fixed(x) + pixman_fixed_1 / 2 - 1;
+ v.vector[1] = pixman_int_to_fixed(y) + pixman_fixed_1 / 2 - 1;
v.vector[2] = pixman_fixed_1;
if (pict->common.transform) {
if (!pixman_transform_point_3d (pict->common.transform, &v))
@@ -3521,8 +3521,8 @@ static void pixmanFetchSourcePict(source_image_t * pict, int x, int y, int width
if (pict->common.transform) {
pixman_vector_t v;
/* reference point is the center of the pixel */
- v.vector[0] = pixman_int_to_fixed(x) + pixman_fixed_1/2;
- v.vector[1] = pixman_int_to_fixed(y) + pixman_fixed_1/2;
+ v.vector[0] = pixman_int_to_fixed(x) + pixman_fixed_1 / 2 - 1;
+ v.vector[1] = pixman_int_to_fixed(y) + pixman_fixed_1 / 2 - 1;
v.vector[2] = pixman_fixed_1;
if (!pixman_transform_point_3d (pict->common.transform, &v))
return;
More information about the cairo
mailing list