[cairo] pixman "Simplify clipping rule" problems

[Siarhei Siamashka]

On Wednesday 24 June 2009, Soeren Sandmann wrote:
> Hi,
>
> First, thanks for testing and reporting this.

You are welcome.

> It shows that we really 
> need a regression test suite beyond what scaling-test, as great as
> that is, provides.

Yes, sure. I only made this test to get a safe environment for the experiments
with optimizing transform/scaling code (the test can be trivially extended to
the BILINEAR case too). It got tuned to the point when I could hardly
intentionally add a bug to the code on the common patch so that it is
unnoticed. Surely corner cases like the potential fixed point arithmetic
overflow issues and the likes must be treated and tested in a special way
and have their own tests.

I intend to make more tests which would aim to provide full coverage for all
the pixman fastpath functions. To be honest, I already have some of such tests
developed long ago, but they are in a bit messy state :-)

The one thing that worries me a bit is that apparently (almost) nobody else is
using 'scaling-test' program. As limited as it is, it could have prevented a
handful of broken commits already. And you can't rely on me monitoring this
mailing list forever, effectively working as a trained monkey who just runs
the test program occasionally and complains when something gets obviously
broken. Seriously, should we add some 'make test' target with an easily
interpretable result and ask commiters to reject any patches which make it
fail unless this failure is investigated and properly explained?

> > 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.

OK, thanks for this information (and for the fix too).

> 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.

It has shown before. I just modified the test to never set clip region
outside of the image after encountering these crashes. I was not sure if
it was a bug or feature and did not dare to ask :)

> 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:

[...]

OK

> > 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.

I did some experiments (without setting source clipping) and it really seems
like only change 2 is visible in these tests, I did not find any other
differences when compared to old pixman versions. Looks like it is time to
update 'scaling-test' program with a new reference crc32 value. Patch is
attached.

Thanks again for timely responding and providing bugfixes.

-- 
Best regards,
Siarhei Siamashka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Update-of-expected-crc32-value-for-scaling-test-prog.patch
Type: text/x-diff
Size: 1061 bytes
Desc: not available
Url : http://lists.cairographics.org/archives/cairo/attachments/20090628/9b34db67/attachment.patch