[PATCH weston 09/10] tests: Add internal test for the weston test screenshot capability

[Pekka Paalanen]

On Wed,  6 May 2015 17:44:28 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> This also serves as a proof of concept of the screen capture
> functionality and as a demo for snapshot-based rendering verification.
> Implements screenshot saving clientside in the test itself.
> 
> This also demonstrates use of test-specific configuration files, in this
> case to disable fadein animations and background images.
> 
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
>  Makefile.am                                |  19 +++-
>  tests/internal-screenshot-test.c           | 151 +++++++++++++++++++++++++++++
>  tests/internal-screenshot.ini              |   3 +
>  tests/reference/internal-screenshot-00.png | Bin 0 -> 5149 bytes
>  4 files changed, 172 insertions(+), 1 deletion(-)
>  create mode 100644 tests/internal-screenshot-test.c
>  create mode 100644 tests/internal-screenshot.ini
>  create mode 100644 tests/reference/internal-screenshot-00.png
> 
> diff --git a/Makefile.am b/Makefile.am
> index 023b7e1..5bca9c2 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -941,7 +941,10 @@ libshared_cairo_la_SOURCES =			\
>  # tests subdirectory
>  #
>  
> -TESTS = $(shared_tests) $(module_tests) $(weston_tests) $(ivi_tests)
> +TESTS = $(internal_tests) $(shared_tests) $(module_tests) $(weston_tests) $(ivi_tests)

Chop.

> +
> +internal_tests = 				\
> +	internal-screenshot.weston
>  
>  shared_tests =					\
>  	config-parser.test			\
> @@ -1041,6 +1044,20 @@ nodist_libtest_client_la_SOURCES =		\
>  libtest_client_la_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
>  libtest_client_la_LIBADD = $(TEST_CLIENT_LIBS) libshared.la libtest-runner.la
>  
> +
> +#
> +# Internal tests - tests functionality of the testsuite itself
> +#
> +
> +internal_screenshot_weston_SOURCES = tests/internal-screenshot-test.c
> +internal_screenshot_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS) $(CAIRO_CFLAGS)
> +internal_screenshot_weston_LDADD = libtest-client.la  $(CAIRO_LIBS)

Will be getting Cairo via TEST_CLIENT.

> +
> +
> +#
> +# Weston Tests
> +#
> +
>  bad_buffer_weston_SOURCES = tests/bad-buffer-test.c
>  bad_buffer_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS) $(CAIRO_CFLAGS)
>  bad_buffer_weston_LDADD = libtest-client.la $(CAIRO_LIBS)
> diff --git a/tests/internal-screenshot-test.c b/tests/internal-screenshot-test.c
> new file mode 100644
> index 0000000..1aea393
> --- /dev/null
> +++ b/tests/internal-screenshot-test.c
> @@ -0,0 +1,151 @@
> +/*
> + * Copyright © 2015 Samsung Electronics Co., Ltd
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission.  The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose.  It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include "config.h"
> +
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <string.h> // memcpy

Use /* */ everywhere.

> +#include <cairo.h>
> +
> +#include "weston-test-client-helper.h"
> +
> +char *server_parameters="--use-pixman --width=320 --height=240";
> +
> +TEST(internal_screenshot)
> +{
> +	struct client *client;
> +	struct surface *screenshot = NULL;
> +	struct rectangle clip;
> +	const char *fname;
> +	cairo_surface_t *reference;
> +	cairo_status_t status;
> +	bool match = false;
> +	bool dump_all_images = true;
> +
> +	printf("Starting test\n");
> +
> +	/* Create the client */
> +	client = create_client_and_test_surface(100, 100, 100, 100);
> +	assert(client);
> +	printf("Client created\n");
> +
> +	/* Create a surface to hold the screenshot */
> +	screenshot = xzalloc(sizeof *screenshot);

I think you are leaking this. Could be allocated from stack just as
well.

> +	assert(screenshot);
> +	screenshot->wl_surface = wl_compositor_create_surface(client->wl_compositor);
> +	assert(screenshot->wl_surface);
> +	printf("Screenshot surface created\n");
> +
> +	/* Create and attach buffer to our surface */
> +	screenshot->wl_buffer = create_shm_buffer(client,
> +						  client->output->width,
> +						  client->output->height,
> +						  &screenshot->data);
> +	screenshot->height = client->output->height;
> +	screenshot->width = client->output->width;
> +	assert(screenshot->wl_buffer);
> +	printf("Screenshot buffer created and attached to surface\n");
> +
> +	/* Take a snapshot.  Result will be in screenshot->wl_buffer. */
> +	client->test->buffer_copy_done = 0;
> +	weston_test_capture_screenshot(client->test->weston_test,
> +				       client->output->wl_output,
> +				       screenshot->wl_buffer);
> +	printf("Capture request sent\n");
> +	while (client->test->buffer_copy_done == 0)
> +		client_roundtrip(client);

That's a pretty busy-loop-happy way to do it. You can spin lots of
roundtrips until the compositor goes repainting. If you would like to
block properly, look at frame_callback_wait().

> +	printf("Roundtrip done\n");
> +	assert(screenshot->wl_buffer);

Redundant assert.

> +
> +	// FIXME: Document somewhere the orientation the screenshot is taken
> +	// and how the clip coords are interpreted, in case of scaling/transform.
> +	// If we're using read_pixels() just make sure it is documented somewhere

Right. Protocol docs in the XML, comparison function docs in Doxygen
style.

> +	// Load reference image
> +	fname = screenshot_reference_filename("internal-screenshot", 0);
> +	printf("Loading reference image %s\n", fname);
> +	reference = cairo_image_surface_create_from_png(fname);
> +	status = cairo_surface_status(reference);
> +	if (status != CAIRO_STATUS_SUCCESS) {
> +		printf("Could not open %s: %s\n", fname, cairo_status_to_string(status));
> +		cairo_surface_destroy(reference);
> +		assert(status != CAIRO_STATUS_SUCCESS);
> +	}
> +
> +	// Test check_surfaces_equal()
> +	// We expect this to fail since the clock will differ from when we made the reference image
> +	match = check_surfaces_equal(screenshot, reference);
> +	printf("Screenshot %s reference image\n", match? "equal to" : "different from");
> +	assert(!match);

Heh, this is more like a test framework self-test for this particular
comparison function. I think such self-tests should be separated. (c.f.
for Pixman I recently wrote
http://lists.freedesktop.org/archives/pixman/2015-May/003643.html ).
There it would be easy to test stride and other differences properly.

But that's just for follow-up, not a blocker and not important in
this case because the correctness is fairly easy to see from the code.

> +
> +	// 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 = 20;
> +	clip.height = 20;
> +	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);
> +	printf("Screenshot %s reference image in clipped area\n", match? "matches" : "doesn't match");
> +	cairo_surface_destroy(reference);
> +
> +	// Test dumping of non-matching images
> +	if (!match || dump_all_images) {
> +		/* Write image to .png file */
> +		int output_stride, buffer_stride, i;
> +		cairo_surface_t *surface;
> +		void *data, *d, *s;
> +
> +		printf("Allocating memory for copying bad image to disk\n");
> +		buffer_stride = client->output->width * 4;
> +		data = xmalloc(buffer_stride * client->output->height);
> +		assert(data);
> +
> +		output_stride = client->output->width * 4;
> +		s = screenshot->data;
> +		d = data;
> +
> +		printf("Copying screenshot data\n");
> +		for (i = 0; i < client->output->height; i++) {
> +			memcpy(d, s, output_stride);
> +			d += buffer_stride;
> +			s += output_stride;
> +		}
> +
> +		printf("Creating Cairo image surface from returned data\n");
> +		surface = cairo_image_surface_create_for_data(data,

Why not simply pass screenshot->data here?

> +							      CAIRO_FORMAT_ARGB32,
> +							      client->output->width,
> +							      client->output->height,
> +							      buffer_stride);
> +
> +		printf("Writing PNG to disk\n");
> +		cairo_surface_write_to_png(surface, "clientside-screenshot.png");
> +		cairo_surface_destroy(surface);
> +		free(data);
> +	}
> +
> +	printf("Test complete\n");
> +	assert(match);
> +}

Could use splitting into functions. Lots of this could be reusable and
more structure woudln't hurt.


Thanks,
pq