[cairo] pixman "Simplify clipping rule" problems

[Soeren Sandmann]

Hi,

First, thanks for testing and reporting this. It shows that we really
need a regression test suite beyond what scaling-test, as great as
that is, provides.

> The final crc32 checksum is also different, but it is to be expected because
> source clipping behavior changed.
> 
> The main source of problem are invalid memory accesses. The test deliberately
> uses x, y, width, height parameters to sometimes refer to pixels outside of
> source and destination images to check how pixman handles clipping. Before
> "Simplify clipping rule" commit, it had no problems with this.
> 
> Is it a pixman bug or we have some new rules for pixman clients regarding the
> valid range of input values for x, y, width, height?

No, the x, y, width, height may fall outside the pixel grids. This is
not supposed to result in invalid memory accesses.

The issue here is more involved though. Before the clipping change in
pixman, there was a bug in pixman where if you set a clip region for
an image, it would not clip to the destination, but instead simply use
the clip region. This means that if you set a clip region outside of
an image, you would get out-of-bounds accesses. I'm not sure why this
didn't show up with scaling-test before.

Unfortunately, current X servers depend on this bug, because the X
server would sometimes create a drawable at the wrong position in
memory, and then set a clip region at the right position. Due to the
pixman bug, this would work.

Since I am not willing to break shipping X servers I ended up doing
this:

    /* Some X servers rely on an old bug, where pixman would just believe the
     * set clip_region and not clip against the destination geometry. So, 
     * since only X servers set "source clip", we don't clip against
     * destination geometry when that is set.
     */

See pixman-utils.c.

Basically, if you set source clipping to true and a clip region
outside of destination drawables, then out-of-bound accesses to the
destination image should be expected. Other ideas welcome, but
fundamentally, existing X servers depend on what from pixman's
perspective are invalid memory accesses.

The X bug was fixed by this commit:

http://cgit.freedesktop.org/xorg/xserver/commit/?id=ebfd6688d1927288155221e7a78fbca9f9293952

That said, even if you disable source clipping in scaling-test.c,
there are still invalid accesses, and this is very likely a pixman bug
that needs to be fixed.

> Also I still wonder how many clients can be affected by this change and if the
> old problems like http://bugs.freedesktop.org/show_bug.cgi?id=11620 could be
> reincarnated.

There are two separate changes to clippping behavior:

1. Transformed or repeated sources now obey the clip, where before the
   clip would be ignored for those images.

2. Pixels outside the bounds of untransformed source images are now
   considered 0, where before those pixels would be clipped away.

I don't think 1. affects any clients because since the old clip was
just ignored for transformed sources, there was no point setting it.

For untransformed sources with a source clip, the new clip rules are
unchanged. In particular, Qt does not use transformed Render images as
far as I can tell from its sources, and so far I have only seen one
case of source clipping actually being used: the one in bug 11620.

The behavior in 2. is what Render has always specified and always
implemented for transformed sources. The change is that it now also
implements it for untransformed ones. I'm not very concerned about
this one because

        - Applications that rely on the old behavior are relying on a
          bug.

        - It is only an issue for operators that don't treat a source
          0 as 'leave the destination in place'

However, if there are a lot of buggy applications, then we may need to
look at this one again.



Soren