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

[Bryce Harrington]

On Fri, May 15, 2015 at 11:12:45AM +0100, Daniel Stone wrote:
> 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))

There is actually no bpp member to the surface structure.  Adding that
(and putting in the relevant numbers) is going to be a fair bit of
plumbing.  I actually wondered about that, but throughout the existing
code I see assumptions that hardcode it to just '4' anyway, so it didn't
seem worth worrying about.  But I guess it's possible it'll be different
some day.
 
> > +               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.

Well, if the bpp for p and q differ, then comparing an N pixel row from
each is not going to result in a correct match, right?

The latter branch is more generalized and would work correctly for both,
but lacks the optimizations that the memset function has and so would
likely be slower in execution.  Maybe that matters less in test code,
especially if we continue not writing test cases.  ;-)

Bryce