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

[Derek Foreman]

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.

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

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

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.

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.

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