[PATCH weston v2 04/14] tests: Add surface checks

[Daniel Stone]

Hi,

On 15 May 2015 at 09:21, Bryce Harrington <bryce at osg.samsung.com> wrote:
> +/**
> + * check_surfaces_equal() - tests if two surfaces are pixel-identical
> + *
> + * Returns true if surface buffers have all the same byte values,
> + * false if the surfaces don't match or can't be compared due to
> + * different dimensions.
> + */
> +bool
> +check_surfaces_equal(const struct surface *a, const struct surface *b)
> +{
> +       int y;
> +       void *p, *q;
> +
> +       if (a == NULL || b == NULL)
> +               return false;
> +       if (a->width != b->width || a->height != a->height)
> +               return false;

I realise you're going to hate me here, but I have led you astray a bit ...

> +       if (a->stride == b->stride) {

/* Second comparison needed to avoid checking potentially garbage
values in the padding. */
if (a->stride == b->stride && a->stride == (a->width * a->bpp))

> +               printf("Checking data for equivalent strides\n");
> +               return (memcmp(a->data, b->data, a->stride * a->height) == 0);
> +       } else {
> +               printf("Manually comparing due to differing strides\n");
> +               for (y = 0; y < a->height; y++) {
> +                       p = a->data + (y * a->stride);
> +                       q = b->data + (y * b->stride);
> +                       if (memcmp(p, q, a->stride) != 0)

if (memcmp(p, q, a->width * a->bpp) != 0) /* don't check the garbage
values in the stride */

So sorry.

If you just fix these up (and a->bpp is actually a real thing which is
bytes per pixel), then I'm happy for it to get merged without a third
pass through the list.

> +       printf("Bytewise comparison inside clip\n");
> +       a_bpp = a->stride / a->width;
> +       b_bpp = b->stride / b->width;

This isn't necessarily true, e.g. if you have a 1366x768 buffer padded
out to 1344px to meet a 32px alignment requirement, at 32bpp, then
deriving bpp from stride/width would give you a bpp of 3.9355.
Probably enough to get away with, but really bpp needs to come from
the format.

> +       for (i=y0; i<y1; i++) {
> +               p = a->data + i * a->stride + x0 * a_bpp;
> +               q = b->data + i * b->stride + x0 * b_bpp;
> +               if (a->stride == b->stride) {
> +                       if (memcmp(p, q, (x1-x0)*a_bpp) != 0) {
> +                               // Dump the bad row

Stray C++ comment.

> +                               printf("Mismatched image on row %d\n", i);
> +                               for (j=0; j<(x1-x0)*a_bpp; j++) {
> +                                       a_char = *((char*)(p+j*a_bpp));
> +                                       b_char = *((char*)(q+j*b_bpp));
> +                                       printf("%d,%d: %8x %8x %s\n", i, j, a_char, b_char,
> +                                              (a_char != b_char)? " <---": "");
> +                               }
> +                               return false;
> +                       }
> +               } else {
> +                       /* account for bpp differences */
> +                       for (j=0; j<x1-x0; j++) {
> +                               a_char = *((char*)(p+j*a_bpp));
> +                               b_char = *((char*)(q+j*b_bpp));
> +                               if (a_char != b_char) {
> +                                       printf("%d,%d: %8x %8x %s\n", i, j, a_char, b_char,
> +                                              (a_char != b_char)? " <---": "");
> +                                       return false;
> +                               }
> +                       }
> +               }

Hmm, maybe a stupid question, but don't these branches fundamentally
do the same thing ... ? Except that in the 'account for bpp
differences' branch, it only iterates for as many bytes as there are
pixels to compare, which probably isn't what you want. I think the
memcmp should be perfectly sufficient, and you don't need to test the
stride at all.

Cheers,
Daniel