[PATCH weston 04/10] tests: Add surface checks

[Pekka Paalanen]

On Sat, 9 May 2015 14:52:38 +0100
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi Bryce,
> A couple of nitpicks follow ...
> 
> On 7 May 2015 at 01:44, Bryce Harrington <bryce at osg.samsung.com> wrote:
> > @@ -39,7 +39,7 @@ tests/weston-ivi.ini : $(srcdir)/ivi-shell/weston.ini.in
> >
> >  all-local : weston.ini ivi-shell/weston.ini
> >
> > -AM_CFLAGS = $(GCC_CFLAGS)
> > +AM_CFLAGS = $(GCC_CFLAGS)  $(CAIRO_CFLAGS)

Hi,

we don't want Cairo in everything. I'm definitely in favour of adding
cairo to TEST_CLIENT.

> >
> >  AM_CPPFLAGS =                                  \
> >         -I$(top_srcdir)/src                     \
> > @@ -987,6 +987,7 @@ noinst_LTLIBRARIES +=                       \
> >
> >  noinst_PROGRAMS +=                     \
> >         $(setbacklight)                 \
> > +       $(internal_tests)               \
> >         $(shared_tests)                 \
> >         $(weston_tests)                 \
> >         $(ivi_tests)                    \
> > @@ -1040,59 +1041,59 @@ libtest_client_la_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> >  libtest_client_la_LIBADD = $(TEST_CLIENT_LIBS) libshared.la libtest-runner.la
> >
> >  bad_buffer_weston_SOURCES = tests/bad-buffer-test.c
> > -bad_buffer_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> > -bad_buffer_weston_LDADD = libtest-client.la
> > +bad_buffer_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS) $(CAIRO_CFLAGS)
> > +bad_buffer_weston_LDADD = libtest-client.la $(CAIRO_LIBS)
> 
> Just making Cairo part of the TEST_CLIENT checks would avoid the need
> for all of these, and people tripping up later when forgetting to add
> them. It should also make the AM_CFLAGS change redundant (do we really
> want to link _everything_ in the build against Cairo?).
> 
> > +/**
> > + * 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, cairo_surface_t *b)
> > +{
> > +       if (a == NULL || b == NULL)
> > +               return false;
> > +       if (a->width != cairo_image_surface_get_width(b) ||
> > +           a->height != cairo_image_surface_get_height(b))
> > +               return false;
> > +
> > +       if (memcmp(a->data, cairo_image_surface_get_data(b), a->width * a->height) != 0)
> > +               return false;
> > +
> > +       return true;
> > +}
> 
> This will break on different strides, and also the bpp seems to be
> missing when calculating offsets and lengths:
>     {
>         unsigned int a_stride = a->width * a->bpp; /* bytes */
>         unsigned int b_stride = cairo_image_surface_get_stride(b); /* bytes */
>         unsigned char *b_data = cairo_image_surface_get_data(b);
> 
>         if (a_stride == b_stride) {
>             return (memcmp(a->data, b_data, a_stride * a->height) == 0);
>         }
>         else {
>             int y;
>             for (y = 0; y < a->height; y++) {
>                 if (memcmp(a->data + (y * a_stride), b_data + (y *
> b_stride), a_stride) != 0)
>                     return false;
>             }
>             return true;
>         }

Should we be comparing the stride-contents (the data in the stride part
that is not valid pixel content on a row, i.e. not included in width) at
all?

I'd rather not, because it might be undefined.

The same if we happen to have XRGB instead of ARGB, the contents of the
X channel probably shouldn't be compared.

> > +/**
> > + * check_surfaces_match_in_clip() - tests if a given region within two
> > + * surfaces are pixel-identical.
> > + *
> > + * Returns true if the two surfaces have the same byte values within the
> > + * given clipping region, or false if they don't match or the surfaces
> > + * can't be compared.
> > + */
> > +bool
> > +check_surfaces_match_in_clip(const struct surface *a, cairo_surface_t *b, const struct rectangle *clip_rect)
> 
> This one also doesn't take bpp into account when doing offsets.
> Testing with non-solid surfaces (e.g. the simple-shm test is a pretty
> excellently bonkers pattern) should show this up I think?

Yeah, using pattern like something that simple-shm computes would be
a much more strong validation.

I agree with Daniel's points. And, even if we always happen to use the
same RGBA32 variant, it would be good to write the tests to be general
or explicitly test the assumptions.


Thanks,
pq