[PATCH weston 2/2] internal-screenshot-test: Fix test so it doesn't expect shell surfaces

[Pekka Paalanen]

On Tue, 26 May 2015 12:10:07 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 26/05/15 03:30 AM, Pekka Paalanen wrote:
> > On Mon, 25 May 2015 15:19:38 -0500
> > Derek Foreman <derekf at osg.samsung.com> wrote:
> > 
> >> We no longer have a race with shell startup because we create our own
> >> colored surface and check that it's properly drawn.
> >>
> >> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> >> ---
> >>  tests/internal-screenshot-test.c                |  63 ++++++++++++++++++------
> >>  tests/reference/internal-screenshot-00.png      | Bin 5149 -> 0 bytes
> >>  tests/reference/internal-screenshot-bad-00.png  | Bin 0 -> 5149 bytes
> >>  tests/reference/internal-screenshot-good-00.png | Bin 0 -> 970 bytes
> >>  4 files changed, 49 insertions(+), 14 deletions(-)
> >>  delete mode 100644 tests/reference/internal-screenshot-00.png
> >>  create mode 100644 tests/reference/internal-screenshot-bad-00.png
> >>  create mode 100644 tests/reference/internal-screenshot-good-00.png
> >>
> >> diff --git a/tests/internal-screenshot-test.c b/tests/internal-screenshot-test.c
> >> index 43aabf6..991761f 100644
> >> --- a/tests/internal-screenshot-test.c
> >> +++ b/tests/internal-screenshot-test.c
> >> @@ -184,51 +184,86 @@ capture_screenshot_of_output(struct client *client) {
> >>  	return screenshot;
> >>  }
> >>  
> >> +static void
> >> +draw_stuff(char *pixels, int w, int h)
> >> +{
> >> +	int x, y;
> >> +
> >> +	for (x = 0; x < w; x++)
> >> +		for (y = 0; y < h; y++) {
> >> +			pixels[y * w * 4 + x * 4] = x;
> >> +			pixels[y * w * 4 + x * 4 + 1] = x + y;
> >> +			pixels[y * w * 4 + x * 4 + 2] = y;
> >> +			pixels[y * w * 4 + x * 4 + 3] = 255;
> > 
> > FYI, this is endian-dependent code. Assuming pixel format
> > WL_SHM_FORMAT_ARGB8888 (PIXMAN_a8r8g8b8, CAIRO_FORMAT_ARGB32), pixels
> > should be handled as uint32_t type covering a whole pixel, and channels
> > set as:
> > 
> > 	uint8_t a, r, g, b;
> > 	int stride; /* bytes */
> > 	uint32_t *pixel = (uint32_t *)data + y * stride / 4 + x;
> > 
> > 	*pixel = (a << 24) | (r << 16) | (g << 8) | b;
> > 
> > See
> > http://cairographics.org/manual/cairo-Image-Surfaces.html#cairo-format-t
> 
> Sigh, a personal anti-pattern.  I know better.  I just sent a new patch
> for that.

It's just a format definition so you'd simply have to know how it works
to get it right. For instance, IIRC GL formats are defined the other
way, as arrays of bytes (GL_UNSIGNED_BYTE). Except the GL formats that
are not (the other value types).

> I left stride out since create_shm_buffer makes us an ARGB8888 buffer
> with stride == width every time.

Yup.

> >> diff --git a/tests/reference/internal-screenshot-good-00.png b/tests/reference/internal-screenshot-good-00.png
> >> new file mode 100644
> >> index 0000000000000000000000000000000000000000..7cf8045f4d205ee6dcf4b1e0f71a21a87bbbdb60
> >> GIT binary patch
> > 
> > Since we are procedurally generating these particular reference images,
> > I would not save them as PNGs in git to avoid growing the size.
> > However, it is valuable to see not only the failed image but also the
> > expected reference image, so when the failed screencapture is saved as
> > a PNG, the reference image should be too.
> > 
> > OTOH, saving even generated reference images as PNG in git has the
> > benefit that it will expose also (unintentional) changes in the
> > generator. But, that could be protected against by using a CRC (see
> > e.g. Pixman).
> > 
> > What do you think about not saving trivially generated reference images
> > in git as PNG files?
> 
> I'd like to minimize the number of binary files in git, and generating
> trivial images at build time seems like a good way to do that.  Since
> these files are ending up in the distributed tarballs, those could get
> annoyingly large.
> 
> I think that requires a little more work than we'd like to get into
> before 1.8 is out the door.

Yeah, none of this is required for 1.8.

> Added Jon to the CC list, since this is test related.
> 
> I think we'd need a way to compare a reference image to part of a
> screenshot (sizes don't match but clip sizes do?)  This is similar if
> not exactly what you suggested previously about fixing the screenshot
> test, but I took a shortcut to save time
> 
> Not sure exactly how all the build infrastructure for generated
> reference images needs to be implemented.

Why would there be any (non-trivial) infrastructure needed?

You draw into a wl_buffer in a test to set up the test, you can use the
same buffer as the reference image.

All you have to do is to define where the PNG files will be saved when
asked for, and how to name them.

If you want to guarantee, that the drawn image remains the same over
development, you could compute a CRC and hardcode that in a check, like
the Pixman test suite does. You could also use that to detect the
unlikely case of the compositor accidentally overwriting the client
buffer.

> > All my comments here are just for future work, and the patch is good for
> > now.
> > 
> > This one patch pushed:
> >    1223fa4..35b7f25  master -> master
> > 
> > This one is not enough alone to completely fix the screenshot test
> > race, one more patch is needed.
> 
> That's the one you just wrote and landed?

Yes. The replacement for the 1/2 of this series.


Thanks,
pq