[Pixman] [PATCH] test: Add new fuzz tester targeting solid images

[Pekka Paalanen]

On Thu, 07 May 2015 19:31:36 +0100
"Ben Avison" <bavison at riscosopen.org> wrote:

> On Wed, 06 May 2015 13:25:43 +0100, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > this is written based on blitters-test.c, right? That would have been
> > interesting to mention, so new reviewers like me can compare with
> > existing code you mirrored.
> 
> Well, it's a new addition the family of fuzz testers - the others are
>   affine-test
>   blitters-test
>   composite-traps-test
>   glyph-test
>   matrix-test
>   region-contains-test
>   rotate-test
>   scaling-test
> 
> I didn't consciously set out to borrow from blitters-test in particular,
> but out of that set it tests the widest range of Porter-Duff operators,
> and (jointly with glyph-test) the widest range of pixel formats, so
> looking back, that's obviously the one I lifted the operator and format
> arrays from.
> 
> Aside from the immediate bug that prompted the creation of solid-test, I
> wanted to increase the proportion of solid source and mask images over
> those tested elsewhere, because they represent a sizeable proportion of
> fast paths and therefore a larger "attack surface" for bugs than was
> represented to date. Although blitters-test already does a fair amount of
> testing of this, the proportions are low (1/8 for source images) and it
> doesn't exercise pixman_image_create_solid_fill images at all. The only
> two fuzz testers that do use pixman_image_create_solid_fill are
> composite-traps-test and glyph-test: they don't use the full range of
> Porter-Duff operators, and they don't use pixman_image_create_solid_fill
> for mask images at all.
> 
> I think a number of your comments arise from the expectation that the
> test was only to test what happens when you change the contents of a 1x1
> bits image - hopefully my aim to improve testing of both types of solid
> images in general helps explain things.

Hi Ben,

definitely. This should be explained in the commit message, or maybe
even in a summary comment in the beginning of solid-test.c, what things
this test aims to test. It's not done for any existing tests, and I find
it hard to reason what all things a test is intended to coved.

> > This test hits the case of fully transparent/opaque -> arbitrary color
> > change, but very rarely -> fully transparent/opaque, never starting
> > with an arbitrary color. I understand the two latter cases are not
> > really testable: setting the fast path flags for an arbitrary color
> > when you could use fully transparent/opaque is not detectable via
> > correctness tests. Are there any cases where more symmetric test case
> > selection would be useful? If not, nevermind.
> 
> My thinking was that as long as we ensured that information about a solid
> colour wasn't being cached, then we'd guard against my bug or any similar
> ones being reintroduced. The two cases which are most likely to be
> treated differently from the others, depending upon the operator, are
> fully-transparent and fully-opaque, so as long as each of those featured
> reasonably frequently on at least one side of the before/after divide, I
> think that should detect any caching.

Agreed.

> > Here's how I understand the code.
> >
> > Source image:
> > - 50% chance for multi-pixel bits image
> > - 25% chance for 1x1 bits image
> > - 25% chance for solid image
> >
> > Mask image:
> > - 50% chance to not be used
> > - 12.5% chance for multi-pixel bits image without CA
> > - 6.25% chance for 1x1 bits image without CA
> > - 6.25% chance for solid image without CA
> > - 12.5% chance for multi-pixel bits image with CA
> > - 6.25% chance for 1x1 bits image with CA
> > - 6.25% chance for solid image with CA
> >
> > Destination image:
> > - always multi-pixel
> >
> > Both source and mask, when they are 1x1 bits images, have:
> > - 50% chance to start fully opaque white
> > - 50% chance to start fully transparent
> 
> Subtle distinction here: because the 1x1 image may use any pixel format,
> the choices are between 0 and 2^bpp-1. Depending upon the format, there
> may not be any pixel value which corresponds to transparent, and the
> maximum value may not be opaque white (because the image may use a
> palette).

Ah, an important note. Can we guarantee we get at least the fully
opaque case always?

> > and then switch to arbitrary random color for the actual test.
> >
> > The problems this test attempts to reveal happen only with 1x1 bits
> > images, so shouldn't the chances for 1x1 bits images be higher?
> 
> Well, 34% of the tests feature at least one 1x1 bits image. If you
> include the solid images - to measure the number of tests which would
> select fast paths which feature at least one solid component, that figure
> rises to 63%. That includes 13% where both the source and mask are solid,
> which is silly (it'd make more sense for the caller to multiply the
> constant colours and use an operation with a solid source and no mask
> image). So by increasing the ratios, while you'd cut the number of tests
> with multi-pixel source and mask (which are redundant with blitters-test)
> you'd raise the number of tests of unrealistic combinations.

I'm not sure unrealistic combinations are that bad - redundancy with
blitter-test is almost just wasted time (assuming blitter-test covers
what it should) which we could use hitting the new cases here.

If an unrealistic case hits a bug, it's still a bug, right?

> There's a danger that a more sophisticated way to choose combinations of
> images and operators to better reflect the share of different fast paths
> would just obfuscate the code and could just as easily be achieved by
> increasing the number of tests, so I didn't feel it was worth spending
> too much brainpower on it.

I was mostly thinking just a simple reordering of the binary random
decisions. Simplicity is good.

> Perhaps you'd be happier with a three-way split between op_n_bits,
> op_n_bits_bits and op_bits_n_bits as in the version I'm just about to
> post? That guarantees 100% of tests involve a solid image, and 67% of
> tests involve at least one 1x1 bits image.

Sounds good.

> > Multi-pixel source or mask images are never written after the initial
> > use, so aren't those useless in this test?
> 
> If you only use single-pixel images, then you're limiting the cases
> you're testing to a subset of fast paths such as over_n_8888. If a fast
> path for (say) add_8888_n_8888 was cacheing information about the solid
> image, we'd want to pick that up too. The chances of anyone writing code
> that cached information about the pixel values in a multi-pixel image, on
> the other hand, feels remote, so I suspect the CPU cycles would be better
> spent on doing more iterations than scrambling the pixel array.

Makes sense. Let's say use-modify-use for multi-pixel images is out of
scope here.

> >> +static pixman_image_t *
> >> +create_image (const pixman_format_code_t *allowed_formats,
> >> +              uint32_t                   *buffer,
> >> +              pixman_format_code_t       *used_fmt)
> >
> > The 'buffer' argument here is kind of strange, because the caller does
> > not know what the image size or format will be. But, it does work here,
> > with the assumption that the size will be at most WIDTH * HEIGHT *
> > uint32_t and the caller knows to allocate with that.
> 
> Pixman typically treats the pixel buffer as uint32_t * internally
> irrespective of the colour depth - see struct bits_image, for example. It
> makes some sense considering that there is a requirement that the row
> stride is always an integer number of 32-bit words.
> 
> Since we have specified the list of possible pixel formats, we know that
> none of them exceed 32bpp, so that's OK. By using buffers of size
> WIDTH * HEIGHT * sizeof(uint32_t), we save ourselves the overhead of a
> pair of heap operations per image per iteration.

I'd say that is optimization where it doesn't matter, while it is
making the code a little bit harder to reason with.

But, fine by me in this case. Could also be solved with a simple
comment:
/* This requires 'buffer' to point to an array of
 * size WIDTH * HEIGHT * uint32_t
 */


Thanks,
pq