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

[Pekka Paalanen]

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

> +		}
> +}
> +
>  TEST(internal_screenshot)
>  {
> +	struct wl_buffer *buf;
>  	struct client *client;
> +	struct wl_surface *surface;
>  	struct surface *screenshot = NULL;
> -	struct surface *reference = NULL;
> +	struct surface *reference_good = NULL;
> +	struct surface *reference_bad = NULL;
>  	struct rectangle clip;
>  	const char *fname;
>  	bool match = false;
>  	bool dump_all_images = true;
> +	void *pixels;
>  
>  	/* Create the client */
>  	printf("Creating client for test\n");
>  	client = create_client_and_test_surface(100, 100, 100, 100);
>  	assert(client);
> +	surface = client->surface->wl_surface;
> +
> +	buf = create_shm_buffer(client, 100, 100, &pixels);
> +	draw_stuff(pixels, 100, 100);
> +	wl_surface_attach(surface, buf, 0, 0);
> +	wl_surface_damage(surface, 0, 0, 100, 100);
> +	wl_surface_commit(surface);
>  
>  	/* Take a snapshot.  Result will be in screenshot->wl_buffer. */
>  	printf("Taking a screenshot\n");
>  	screenshot = capture_screenshot_of_output(client);
>  	assert(screenshot);
>  
> -	/* Load reference image */
> -	fname = screenshot_reference_filename("internal-screenshot", 0);
> -	printf("Loading reference image %s\n", fname);
> -	reference = load_surface_from_png(fname);
> -	assert(reference);
> +	/* Load good reference image */
> +	fname = screenshot_reference_filename("internal-screenshot-good", 0);
> +	printf("Loading good reference image %s\n", fname);
> +	reference_good = load_surface_from_png(fname);
> +	assert(reference_good);
> +
> +	/* Load bad reference image */
> +	fname = screenshot_reference_filename("internal-screenshot-bad", 0);
> +	printf("Loading bad reference image %s\n", fname);
> +	reference_bad = load_surface_from_png(fname);
> +	assert(reference_bad);
>  
>  	/* Test check_surfaces_equal()
> -	 * We expect this to fail since the clock will differ from when we made the reference image
> +	 * We expect this to fail since we use a bad reference image
>  	 */
> -	match = check_surfaces_equal(screenshot, reference);
> +	match = check_surfaces_equal(screenshot, reference_bad);
>  	printf("Screenshot %s reference image\n", match? "equal to" : "different from");
>  	assert(!match);
> +	free(reference_bad->data);
> +	free(reference_bad);
>  
>  	/* Test check_surfaces_match_in_clip()
>  	 * Alpha-blending and other effects can cause irrelevant discrepancies, so look only
>  	 * at a small portion of the solid-colored background
>  	 */
> -	clip.x = 50;
> -	clip.y = 50;
> -	clip.width = 101;
> -	clip.height = 101;
> +	clip.x = 100;
> +	clip.y = 100;
> +	clip.width = 100;
> +	clip.height = 100;
>  	printf("Clip: %d,%d %d x %d\n", clip.x, clip.y, clip.width, clip.height);
> -	match = check_surfaces_match_in_clip(screenshot, reference, &clip);
> +	match = check_surfaces_match_in_clip(screenshot, reference_good,
> +					     &clip);
>  	printf("Screenshot %s reference image in clipped area\n", match? "matches" : "doesn't match");
> -	free(reference);
> +	free(reference_good->data);
> +	free(reference_good);
>  
>  	/* Test dumping of non-matching images */
>  	if (!match || dump_all_images) {
> diff --git a/tests/reference/internal-screenshot-00.png b/tests/reference/internal-screenshot-00.png
> deleted file mode 100644
> index 5dc79576345241122f9aba308c93e5e9c7797fbd..0000000000000000000000000000000000000000
> GIT binary patch

> 
> diff --git a/tests/reference/internal-screenshot-bad-00.png b/tests/reference/internal-screenshot-bad-00.png
> new file mode 100644
> index 0000000000000000000000000000000000000000..5dc79576345241122f9aba308c93e5e9c7797fbd
> GIT binary patch

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


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.


Thanks,
pq