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

[Pekka Paalanen]

On Fri, 15 May 2015 16:13:32 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

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

> > > +       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.  ;-)

If bytes per pixel differs, then necessarily the pixel formats differ.
If pixel formats differ, you just cannot do direct byte by byte
comparison. You have to convert from the more accurate format to the
less accurate format, and compare only values that are in the same
format. Or convert in the opposite direction, depending on which way is
correct for the case at hand.

It's like you're trying to compare wl_fixed_from_int(1) == 1 and
expect true when the pixel formats are not the same.

The easiest thing here is to just make sure the pixel formats are
always the same, and then there will never be a difference in
bytes-per-pixel. Implementing the general case for differing formats is
not worth it here, IMHO.

Comparing RGBA (4 bytespp) to RGB (3 bytespp) is really a special case,
and considering that 3 bytespp formats are rare in use (Cairo does not
have them at all), it's not really worth considering. (The latter
branch in your code really only tests the first byte of each pixel,
even.)


Thanks,
pq