[v1,weston,11/11] tests: Construct the filename external to wl_test_record_screenshot

Submitted by Bryce Harrington on Nov. 19, 2014, 11:06 p.m.

Details

Message ID 1416438386-4783-12-git-send-email-bryce@osg.samsung.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Bryce Harrington Nov. 19, 2014, 11:06 p.m.
We need to use the output filename when we do a comparison with the
appropriate reference image, so move the filename construction to the
caller of record_screenshot() instead.

Doing this also requires externalizing the output iteration loop since
we want the output number to be recorded in the filename.  In most cases
this won't matter since we will just have a single head anyway; plus, in
cases where we do have multiple heads we could well be caring only about
the contents of the Nth head rather than all of them.

The fadein test doesn't care about multi-head so no need to traverse the
list; we'll look just at head #0.

Fixes:  https://bugs.freedesktop.org/show_bug.cgi?id=83987
Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
---
 protocol/wayland-test.xml         |  3 +-
 tests/fadein-test.c               | 40 ++++++-------------------
 tests/weston-test-client-helper.c | 34 +++++++++++++++++++++
 tests/weston-test-client-helper.h |  6 ++++
 tests/weston-test.c               | 62 ++++++++++++++++++++-------------------
 5 files changed, 83 insertions(+), 62 deletions(-)

Patch hide | download patch | download mbox

diff --git a/protocol/wayland-test.xml b/protocol/wayland-test.xml
index 93fbc16..4c289a6 100644
--- a/protocol/wayland-test.xml
+++ b/protocol/wayland-test.xml
@@ -59,7 +59,8 @@ 
       <arg name="n" type="uint"/>
     </event>
     <request name="record_screenshot">
-      <arg name="basename" type="string"/>
+      <arg name="filename" type="string"/>
+      <arg name="head" type="uint"/>
       <arg name="clip_x" type="uint"/>
       <arg name="clip_y" type="uint"/>
       <arg name="clip_width" type="uint"/>
diff --git a/tests/fadein-test.c b/tests/fadein-test.c
index a359bbc..4be9887 100644
--- a/tests/fadein-test.c
+++ b/tests/fadein-test.c
@@ -33,47 +33,24 @@ 
 
 char *server_parameters="--use-pixman --width=320 --height=240";
 
-static char*
-output_filename(const char* basename, int head) {
-	static const char *path = "./";
-	char *filename;
-
-        if (asprintf(&filename, "%s%s-%d.png", path, basename, head) < 0)
-		filename = NULL;
-
-	return filename;
-}
-
-static char*
-reference_filename(const char* basename, int head) {
-        static const char *path = "./tests/reference/";
-        char *filename;
-
-        if (asprintf(&filename, "%s%s-%d.png", path, basename, head) < 0)
-                filename = NULL;
-
-        return filename;
-}
-
 TEST(fadein)
 {
 	struct client *client;
-	char basename[32];
 	char *out_path;
 	char *ref_path;
 	int i;
+	uint32_t head_number = 0;
 
 	client = client_create(100, 100, 100, 100);
 	assert(client);
 
 	for (i = 0; i < 6; i++) {
-		snprintf(basename, sizeof basename, "fadein-%02d", i);
-		// FIXME: Iterate over all heads
-		out_path = output_filename(basename, 0);
+		out_path = screenshot_output_filename(testfadein.name,
+						      i, head_number);
 
 		/* Use a thin 40xN vertical strip for comparison */
-		// FIXME: Would be preferred to pass in out_path rather than basename here...
-		wl_test_record_screenshot(client->test->wl_test, basename,
+		wl_test_record_screenshot(client->test->wl_test,
+					  out_path, head_number,
 					  80, 40, 40, DISPLAY_HEIGHT-80);
 		client_roundtrip(client);
 
@@ -82,11 +59,12 @@  TEST(fadein)
 		if (i == 2) {
 			// At t=0.5, the screen fade colors are changing rapidly,
 			// so just verify the screen is different than at t=0.25.
-			snprintf(basename, sizeof basename, "fadein-%02d", 1);
-			ref_path = reference_filename(basename, 0);
+			ref_path = screenshot_reference_filename(testfadein.name,
+								 1, head_number);
 			assert(! files_equal(out_path, ref_path));
 		} else {
-			ref_path = reference_filename(basename, 0);
+			ref_path = screenshot_reference_filename(testfadein.name,
+								 i, head_number);
 			assert(files_equal(out_path, ref_path));
 		}
 
diff --git a/tests/weston-test-client-helper.c b/tests/weston-test-client-helper.c
index 72834e4..3c9a1ac 100644
--- a/tests/weston-test-client-helper.c
+++ b/tests/weston-test-client-helper.c
@@ -657,3 +657,37 @@  files_equal(const char *test_filename, const char* ref_filename)
 
         return t == p;  /* both EOF */
 }
+
+static const char*
+_output_path(void) {
+	char *path = getenv("WAYLAND_TEST_OUTPUT_PATH");
+	if (!path)
+		return ".";
+	return path;
+}
+
+char*
+screenshot_output_filename(const char* basename, uint32_t seq, uint32_t head_number) {
+        char *filename;
+        if (asprintf(&filename, "%s/%s-%02d-%d.png",
+		     _output_path(), basename, seq, head_number) < 0)
+		return NULL;
+        return filename;
+}
+
+static const char*
+_reference_path(void) {
+	char *path = getenv("WAYLAND_TEST_REFERENCE_PATH");
+	if (!path)
+		return "./tests/reference";
+	return path;
+}
+
+char*
+screenshot_reference_filename(const char* basename, uint32_t seq, uint32_t head) {
+        char *filename;
+        if (asprintf(&filename, "%s/%s-%02d-%d.png",
+		     _reference_path(), basename, seq, head) < 0)
+                return NULL;
+        return filename;
+}
diff --git a/tests/weston-test-client-helper.h b/tests/weston-test-client-helper.h
index 20e08b1..4316d62 100644
--- a/tests/weston-test-client-helper.h
+++ b/tests/weston-test-client-helper.h
@@ -140,4 +140,10 @@  expect_protocol_error(struct client *client,
 bool
 files_equal(const char *file_1, const char *file_2);
 
+char*
+screenshot_output_filename(const char* basename, uint32_t seq, uint32_t head_number);
+
+char*
+screenshot_reference_filename(const char* basename, uint32_t seq, uint32_t head);
+
 #endif
diff --git a/tests/weston-test.c b/tests/weston-test.c
index 5c65624..5931f00 100644
--- a/tests/weston-test.c
+++ b/tests/weston-test.c
@@ -278,46 +278,48 @@  dump_image(const char *filename, int x, int y, uint32_t *image,
 
 static void
 record_screenshot(struct wl_client *client, struct wl_resource *resource,
-		  const char *basename, unsigned int clip_x, unsigned int clip_y,
+		  const char *filename, unsigned int output_number,
+		  unsigned int clip_x, unsigned int clip_y,
 		  unsigned int clip_width, unsigned int clip_height)
 {
 	struct weston_output *o;
 	struct weston_test *test = wl_resource_get_user_data(resource);
-	char *filename;
 	uint32_t *buffer;
-	int w, h, head = 0;
+	int w, h = 0;
+	unsigned int i = 0;
 
+	assert(output_number < (unsigned int)wl_list_length(&test->compositor->output_list));
 	wl_list_for_each(o, &test->compositor->output_list, link) {
-		switch (o->transform) {
-		case WL_OUTPUT_TRANSFORM_90:
-		case WL_OUTPUT_TRANSFORM_270:
-		case WL_OUTPUT_TRANSFORM_FLIPPED_90:
-		case WL_OUTPUT_TRANSFORM_FLIPPED_270:
-			w = o->height;
-			h = o->width;
+		if (i == output_number)
 			break;
-		default:
-			w = o->width;
-			h = o->height;
-			break;
-		}
-		buffer = malloc(w * h * 4);
-		if (!buffer)
-			return;
-
-		test->compositor->renderer->read_pixels(o,
-					      o->compositor->read_format,
-					      buffer, 0, 0, w, h);
+		i++;
+	}
+	assert(i == output_number);
+
+	switch (o->transform) {
+	case WL_OUTPUT_TRANSFORM_90:
+	case WL_OUTPUT_TRANSFORM_270:
+	case WL_OUTPUT_TRANSFORM_FLIPPED_90:
+	case WL_OUTPUT_TRANSFORM_FLIPPED_270:
+		w = o->height;
+		h = o->width;
+		break;
+	default:
+		w = o->width;
+		h = o->height;
+		break;
+	}
+	buffer = malloc(w * h * 4);
+	if (!buffer)
+		return;
 
-		if (asprintf(&filename, "%s-%d.png", basename, head) < 0)
-			return;
+	test->compositor->renderer->read_pixels(o,
+						o->compositor->read_format,
+						buffer, 0, 0, w, h);
 
-		dump_image(filename, w, h, buffer,
-			   clip_x, clip_y, clip_width, clip_height);
-		free(filename);
-		free(buffer);
-		head++;
-	}
+	dump_image(filename, w, h, buffer,
+		   clip_x, clip_y, clip_width, clip_height);
+	free(buffer);
 }
 
 static const struct wl_test_interface test_implementation = {

Comments

On Wed, 19 Nov 2014 15:06:26 -0800
Bryce Harrington <bryce@osg.samsung.com> wrote:

> We need to use the output filename when we do a comparison with the
> appropriate reference image, so move the filename construction to the
> caller of record_screenshot() instead.
> 
> Doing this also requires externalizing the output iteration loop since
> we want the output number to be recorded in the filename.  In most cases
> this won't matter since we will just have a single head anyway; plus, in
> cases where we do have multiple heads we could well be caring only about
> the contents of the Nth head rather than all of them.
> 
> The fadein test doesn't care about multi-head so no need to traverse the
> list; we'll look just at head #0.
> 
> Fixes:  https://bugs.freedesktop.org/show_bug.cgi?id=83987
> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> ---
>  protocol/wayland-test.xml         |  3 +-
>  tests/fadein-test.c               | 40 ++++++-------------------
>  tests/weston-test-client-helper.c | 34 +++++++++++++++++++++
>  tests/weston-test-client-helper.h |  6 ++++
>  tests/weston-test.c               | 62 ++++++++++++++++++++-------------------
>  5 files changed, 83 insertions(+), 62 deletions(-)
> 
> diff --git a/protocol/wayland-test.xml b/protocol/wayland-test.xml
> index 93fbc16..4c289a6 100644
> --- a/protocol/wayland-test.xml
> +++ b/protocol/wayland-test.xml
> @@ -59,7 +59,8 @@
>        <arg name="n" type="uint"/>
>      </event>
>      <request name="record_screenshot">
> -      <arg name="basename" type="string"/>
> +      <arg name="filename" type="string"/>
> +      <arg name="head" type="uint"/>

I wonder if using the head index is stable enough. How about using a
wl_output object instead?

That would be more predictable, if we one day start testing output
hotplug, with protocol in the test interface to add and remove outputs.


Thanks,
pq


>        <arg name="clip_x" type="uint"/>
>        <arg name="clip_y" type="uint"/>
>        <arg name="clip_width" type="uint"/>