[v1,weston,06/11] tests: Add screenshot recording to weston-test

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

Details

Message ID 1416438386-4783-7-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.
From: Derek Foreman <derekf@osg.samsung.com>

Adds wl_test_record_screenshot() to weston test.  This commit also
adds a dependency on cairo to weston-test to use it for writing PNG
files.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981
Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
---
 Makefile.am               |  4 +--
 protocol/wayland-test.xml |  3 +++
 tests/weston-test.c       | 68 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/Makefile.am b/Makefile.am
index 1e7cc81..26dd473 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -881,7 +881,7 @@  noinst_PROGRAMS +=			\
 	matrix-test
 
 test_module_ldflags = \
-	-module -avoid-version -rpath $(libdir) $(COMPOSITOR_LIBS)
+	-module -avoid-version -rpath $(libdir) $(COMPOSITOR_LIBS) $(CAIRO_LIBS)
 
 surface_global_test_la_SOURCES = tests/surface-global-test.c
 surface_global_test_la_LDFLAGS = $(test_module_ldflags)
@@ -893,7 +893,7 @@  surface_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS)
 
 weston_test_la_LIBADD = $(COMPOSITOR_LIBS) libshared.la
 weston_test_la_LDFLAGS = $(test_module_ldflags)
-weston_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS)
+weston_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(CAIRO_CFLAGS)
 weston_test_la_SOURCES = tests/weston-test.c
 nodist_weston_test_la_SOURCES =			\
 	protocol/wayland-test-protocol.c	\
diff --git a/protocol/wayland-test.xml b/protocol/wayland-test.xml
index 18b6625..a22a6ac 100644
--- a/protocol/wayland-test.xml
+++ b/protocol/wayland-test.xml
@@ -58,5 +58,8 @@ 
     <event name="n_egl_buffers">
       <arg name="n" type="uint"/>
     </event>
+    <request name="record_screenshot">
+      <arg name="basename" type="string"/>
+    </request>
   </interface>
 </protocol>
diff --git a/tests/weston-test.c b/tests/weston-test.c
index f1e45c1..16f20c6 100644
--- a/tests/weston-test.c
+++ b/tests/weston-test.c
@@ -35,6 +35,8 @@ 
 #include <EGL/eglext.h>
 #endif /* ENABLE_EGL */
 
+#include <cairo.h>
+
 struct weston_test {
 	struct weston_compositor *compositor;
 	struct weston_layer layer;
@@ -235,6 +237,71 @@  get_n_buffers(struct wl_client *client, struct wl_resource *resource)
 	wl_test_send_n_egl_buffers(resource, n_buffers);
 }
 
+static void
+dump_image(const char *filename, int x, int y, uint32_t *image)
+{
+	cairo_surface_t *surface, *flipped;
+	cairo_t *cr;
+
+	surface = cairo_image_surface_create_for_data((unsigned char *)image,
+						      CAIRO_FORMAT_ARGB32,
+						      x, y, x * 4);
+	flipped = cairo_surface_create_similar_image(surface, CAIRO_FORMAT_ARGB32, x, y);
+
+	cr = cairo_create(flipped);
+	cairo_translate(cr, 0.0, y);
+	cairo_scale(cr, 1.0, -1.0);
+	cairo_set_source_surface(cr, surface, 0, 0);
+	cairo_paint(cr);
+	cairo_destroy(cr);
+	cairo_surface_destroy(surface);
+
+	cairo_surface_write_to_png(flipped, filename);
+	cairo_surface_destroy(flipped);
+}
+
+static void
+record_screenshot(struct wl_client *client, struct wl_resource *resource,
+		  const char *basename)
+{
+	struct weston_output *o;
+	struct weston_test *test = wl_resource_get_user_data(resource);
+	char *filename;
+	uint32_t *buffer;
+	int w, h, head = 0;
+
+	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;
+			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);
+
+		if (asprintf(&filename, "%s-%d.png", basename, head) < 0)
+			return;
+
+		dump_image(filename, w, h, buffer);
+		free(filename);
+		free(buffer);
+		head++;
+	}
+}
+
 static const struct wl_test_interface test_implementation = {
 	move_surface,
 	move_pointer,
@@ -242,6 +309,7 @@  static const struct wl_test_interface test_implementation = {
 	activate_surface,
 	send_key,
 	get_n_buffers,
+	record_screenshot
 };
 
 static void

Comments

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

> From: Derek Foreman <derekf@osg.samsung.com>
> 
> Adds wl_test_record_screenshot() to weston test.  This commit also
> adds a dependency on cairo to weston-test to use it for writing PNG
> files.
> 
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981
> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>

Hi,

could we use a wl_shm-based wl_buffer instead of a file, please?

That way we can simply relay the pixels as is to the test client, which
can then compare without compressing and decompressing from PNG first.

For an example how to do this, see Weston's screenshooter protocol and
implementation.

I think you also want to specify in the record_screenshot request:
- which output to capture (in case there are multiple)
- the rectangular sub-region that must be contained in the output
  (which you already do with the clip coords in a later patch, so that
  should be squashed here)

Synchronization cannot be done with wl_display_roundtrip, so you need
an explicit event for that. The best would be to create a new protocol
object on record_screenshot request, which then delivers the event and
self-destructs, like wl_callback. You could just use wl_callback, but
in principle we should avoid new use cases for wl_callback due to
the design issues.

Please also document carefully in which orientation the screenshot is
taken and how the clip coords are interpreted, in case some
output_scale/transform are in effect. Or if you rely on the renderer's
read_pixels() convention, I hope that is documented somewhere...

Btw. there is a small danger in linking Cairo to the compositor.
GL-renderer uses GLESv2, but if CAIRO_LIBS contains cairo-gl.so linking
to libGL, it might explode... or maybe not because of no RTLD_GLOBAL...
or maybe it could, I don't know. So I'd just avoid that. :-)

I see the test interface has been extended before without bumping the
revision... but we probably should bump it, to not give a bad example.


Thanks,
pq

> ---
>  Makefile.am               |  4 +--
>  protocol/wayland-test.xml |  3 +++
>  tests/weston-test.c       | 68 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 1e7cc81..26dd473 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -881,7 +881,7 @@ noinst_PROGRAMS +=			\
>  	matrix-test
>  
>  test_module_ldflags = \
> -	-module -avoid-version -rpath $(libdir) $(COMPOSITOR_LIBS)
> +	-module -avoid-version -rpath $(libdir) $(COMPOSITOR_LIBS) $(CAIRO_LIBS)
>  
>  surface_global_test_la_SOURCES = tests/surface-global-test.c
>  surface_global_test_la_LDFLAGS = $(test_module_ldflags)
> @@ -893,7 +893,7 @@ surface_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS)
>  
>  weston_test_la_LIBADD = $(COMPOSITOR_LIBS) libshared.la
>  weston_test_la_LDFLAGS = $(test_module_ldflags)
> -weston_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS)
> +weston_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(CAIRO_CFLAGS)
>  weston_test_la_SOURCES = tests/weston-test.c
>  nodist_weston_test_la_SOURCES =			\
>  	protocol/wayland-test-protocol.c	\
> diff --git a/protocol/wayland-test.xml b/protocol/wayland-test.xml
> index 18b6625..a22a6ac 100644
> --- a/protocol/wayland-test.xml
> +++ b/protocol/wayland-test.xml
> @@ -58,5 +58,8 @@
>      <event name="n_egl_buffers">
>        <arg name="n" type="uint"/>
>      </event>
> +    <request name="record_screenshot">
> +      <arg name="basename" type="string"/>
> +    </request>
>    </interface>
>  </protocol>
> diff --git a/tests/weston-test.c b/tests/weston-test.c
> index f1e45c1..16f20c6 100644
> --- a/tests/weston-test.c
> +++ b/tests/weston-test.c
> @@ -35,6 +35,8 @@
>  #include <EGL/eglext.h>
>  #endif /* ENABLE_EGL */
>  
> +#include <cairo.h>
> +
>  struct weston_test {
>  	struct weston_compositor *compositor;
>  	struct weston_layer layer;
> @@ -235,6 +237,71 @@ get_n_buffers(struct wl_client *client, struct wl_resource *resource)
>  	wl_test_send_n_egl_buffers(resource, n_buffers);
>  }
>  
> +static void
> +dump_image(const char *filename, int x, int y, uint32_t *image)
> +{
> +	cairo_surface_t *surface, *flipped;
> +	cairo_t *cr;
> +
> +	surface = cairo_image_surface_create_for_data((unsigned char *)image,
> +						      CAIRO_FORMAT_ARGB32,
> +						      x, y, x * 4);
> +	flipped = cairo_surface_create_similar_image(surface, CAIRO_FORMAT_ARGB32, x, y);
> +
> +	cr = cairo_create(flipped);
> +	cairo_translate(cr, 0.0, y);
> +	cairo_scale(cr, 1.0, -1.0);
> +	cairo_set_source_surface(cr, surface, 0, 0);
> +	cairo_paint(cr);
> +	cairo_destroy(cr);
> +	cairo_surface_destroy(surface);
> +
> +	cairo_surface_write_to_png(flipped, filename);
> +	cairo_surface_destroy(flipped);
> +}
> +
> +static void
> +record_screenshot(struct wl_client *client, struct wl_resource *resource,
> +		  const char *basename)
> +{
> +	struct weston_output *o;
> +	struct weston_test *test = wl_resource_get_user_data(resource);
> +	char *filename;
> +	uint32_t *buffer;
> +	int w, h, head = 0;
> +
> +	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;
> +			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);
> +
> +		if (asprintf(&filename, "%s-%d.png", basename, head) < 0)
> +			return;
> +
> +		dump_image(filename, w, h, buffer);
> +		free(filename);
> +		free(buffer);
> +		head++;
> +	}
> +}
> +
>  static const struct wl_test_interface test_implementation = {
>  	move_surface,
>  	move_pointer,
> @@ -242,6 +309,7 @@ static const struct wl_test_interface test_implementation = {
>  	activate_surface,
>  	send_key,
>  	get_n_buffers,
> +	record_screenshot
>  };
>  
>  static void
On 24/11/14 05:01 AM, Pekka Paalanen wrote:
> On Wed, 19 Nov 2014 15:06:21 -0800
> Bryce Harrington <bryce@osg.samsung.com> wrote:
> 
>> From: Derek Foreman <derekf@osg.samsung.com>
>>
>> Adds wl_test_record_screenshot() to weston test.  This commit also
>> adds a dependency on cairo to weston-test to use it for writing PNG
>> files.
>>
>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981
>> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> 
> Hi,
> 
> could we use a wl_shm-based wl_buffer instead of a file, please?
> 
> That way we can simply relay the pixels as is to the test client, which
> can then compare without compressing and decompressing from PNG first.

Ok - it (the client) will still need to be able to write and read files
though, so it can create or read the reference images.

> For an example how to do this, see Weston's screenshooter protocol and
> implementation.

Thanks - I'd actually considered dumping screenshots in screenshooter's
wcap format, but I figured it would be a bit of a pain to work with that.

> I think you also want to specify in the record_screenshot request:
> - which output to capture (in case there are multiple)
> - the rectangular sub-region that must be contained in the output
>   (which you already do with the clip coords in a later patch, so that
>   should be squashed here)

That makes sense too.

> Synchronization cannot be done with wl_display_roundtrip, so you need
> an explicit event for that. The best would be to create a new protocol
> object on record_screenshot request, which then delivers the event and
> self-destructs, like wl_callback. You could just use wl_callback, but
> in principle we should avoid new use cases for wl_callback due to
> the design issues.
> 
> Please also document carefully in which orientation the screenshot is
> taken and how the clip coords are interpreted, in case some
> output_scale/transform are in effect. Or if you rely on the renderer's
> read_pixels() convention, I hope that is documented somewhere...

Not yet ;)

(fwiw it's done so an unrotated display's screenshot looks normal in an
image viewer - so the opposite of gl convention :)

> Btw. there is a small danger in linking Cairo to the compositor.
> GL-renderer uses GLESv2, but if CAIRO_LIBS contains cairo-gl.so linking
> to libGL, it might explode... or maybe not because of no RTLD_GLOBAL...
> or maybe it could, I don't know. So I'd just avoid that. :-)

Siiiigh, I was afraid someone would come up with a reason to actually
deal with libpng directly.  Oh, wait, only the client has to deal with
png files - I think it's safe to link cairo there? :)

> I see the test interface has been extended before without bumping the
> revision... but we probably should bump it, to not give a bad example.

Hehe, that's why I thought it was ok.  I'll bump it in the next revision.

Thanks for the review!

>> ---
>>  Makefile.am               |  4 +--
>>  protocol/wayland-test.xml |  3 +++
>>  tests/weston-test.c       | 68 +++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 73 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 1e7cc81..26dd473 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -881,7 +881,7 @@ noinst_PROGRAMS +=			\
>>  	matrix-test
>>  
>>  test_module_ldflags = \
>> -	-module -avoid-version -rpath $(libdir) $(COMPOSITOR_LIBS)
>> +	-module -avoid-version -rpath $(libdir) $(COMPOSITOR_LIBS) $(CAIRO_LIBS)
>>  
>>  surface_global_test_la_SOURCES = tests/surface-global-test.c
>>  surface_global_test_la_LDFLAGS = $(test_module_ldflags)
>> @@ -893,7 +893,7 @@ surface_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS)
>>  
>>  weston_test_la_LIBADD = $(COMPOSITOR_LIBS) libshared.la
>>  weston_test_la_LDFLAGS = $(test_module_ldflags)
>> -weston_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS)
>> +weston_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(CAIRO_CFLAGS)
>>  weston_test_la_SOURCES = tests/weston-test.c
>>  nodist_weston_test_la_SOURCES =			\
>>  	protocol/wayland-test-protocol.c	\
>> diff --git a/protocol/wayland-test.xml b/protocol/wayland-test.xml
>> index 18b6625..a22a6ac 100644
>> --- a/protocol/wayland-test.xml
>> +++ b/protocol/wayland-test.xml
>> @@ -58,5 +58,8 @@
>>      <event name="n_egl_buffers">
>>        <arg name="n" type="uint"/>
>>      </event>
>> +    <request name="record_screenshot">
>> +      <arg name="basename" type="string"/>
>> +    </request>
>>    </interface>
>>  </protocol>
>> diff --git a/tests/weston-test.c b/tests/weston-test.c
>> index f1e45c1..16f20c6 100644
>> --- a/tests/weston-test.c
>> +++ b/tests/weston-test.c
>> @@ -35,6 +35,8 @@
>>  #include <EGL/eglext.h>
>>  #endif /* ENABLE_EGL */
>>  
>> +#include <cairo.h>
>> +
>>  struct weston_test {
>>  	struct weston_compositor *compositor;
>>  	struct weston_layer layer;
>> @@ -235,6 +237,71 @@ get_n_buffers(struct wl_client *client, struct wl_resource *resource)
>>  	wl_test_send_n_egl_buffers(resource, n_buffers);
>>  }
>>  
>> +static void
>> +dump_image(const char *filename, int x, int y, uint32_t *image)
>> +{
>> +	cairo_surface_t *surface, *flipped;
>> +	cairo_t *cr;
>> +
>> +	surface = cairo_image_surface_create_for_data((unsigned char *)image,
>> +						      CAIRO_FORMAT_ARGB32,
>> +						      x, y, x * 4);
>> +	flipped = cairo_surface_create_similar_image(surface, CAIRO_FORMAT_ARGB32, x, y);
>> +
>> +	cr = cairo_create(flipped);
>> +	cairo_translate(cr, 0.0, y);
>> +	cairo_scale(cr, 1.0, -1.0);
>> +	cairo_set_source_surface(cr, surface, 0, 0);
>> +	cairo_paint(cr);
>> +	cairo_destroy(cr);
>> +	cairo_surface_destroy(surface);
>> +
>> +	cairo_surface_write_to_png(flipped, filename);
>> +	cairo_surface_destroy(flipped);
>> +}
>> +
>> +static void
>> +record_screenshot(struct wl_client *client, struct wl_resource *resource,
>> +		  const char *basename)
>> +{
>> +	struct weston_output *o;
>> +	struct weston_test *test = wl_resource_get_user_data(resource);
>> +	char *filename;
>> +	uint32_t *buffer;
>> +	int w, h, head = 0;
>> +
>> +	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;
>> +			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);
>> +
>> +		if (asprintf(&filename, "%s-%d.png", basename, head) < 0)
>> +			return;
>> +
>> +		dump_image(filename, w, h, buffer);
>> +		free(filename);
>> +		free(buffer);
>> +		head++;
>> +	}
>> +}
>> +
>>  static const struct wl_test_interface test_implementation = {
>>  	move_surface,
>>  	move_pointer,
>> @@ -242,6 +309,7 @@ static const struct wl_test_interface test_implementation = {
>>  	activate_surface,
>>  	send_key,
>>  	get_n_buffers,
>> +	record_screenshot
>>  };
>>  
>>  static void
>
On Mon, Nov 24, 2014 at 04:31:01PM -0600, Derek Foreman wrote:
> On 24/11/14 05:01 AM, Pekka Paalanen wrote:
> > On Wed, 19 Nov 2014 15:06:21 -0800
> > Bryce Harrington <bryce@osg.samsung.com> wrote:
> > 
> >> From: Derek Foreman <derekf@osg.samsung.com>
> >>
> >> Adds wl_test_record_screenshot() to weston test.  This commit also
> >> adds a dependency on cairo to weston-test to use it for writing PNG
> >> files.
> >>
> >> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981
> >> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> > 
> > Hi,
> > 
> > could we use a wl_shm-based wl_buffer instead of a file, please?
> > 
> > That way we can simply relay the pixels as is to the test client, which
> > can then compare without compressing and decompressing from PNG first.
> 
> Ok - it (the client) will still need to be able to write and read files
> though, so it can create or read the reference images.

I agree it will run faster and save on disk space if we can skip writing
the images to files.  Yes, we'll still need to read the reference
images, but reading is faster than writing so we'll be ahead.  Most of
the time we don't care what the images look like anyway.

That said, it can be beneficial to write the output out to files for
troubleshooting purposes.  So we still need a write mode.  But perhaps
it should be off by default with some env var or switch to make it show
its work.
 
> > Synchronization cannot be done with wl_display_roundtrip, so you need
> > an explicit event for that. The best would be to create a new protocol
> > object on record_screenshot request, which then delivers the event and
> > self-destructs, like wl_callback. You could just use wl_callback, but
> > in principle we should avoid new use cases for wl_callback due to
> > the design issues.
> > 
> > Please also document carefully in which orientation the screenshot is
> > taken and how the clip coords are interpreted, in case some
> > output_scale/transform are in effect. Or if you rely on the renderer's
> > read_pixels() convention, I hope that is documented somewhere...
> 
> Not yet ;)
> 
> (fwiw it's done so an unrotated display's screenshot looks normal in an
> image viewer - so the opposite of gl convention :)
> 
> > Btw. there is a small danger in linking Cairo to the compositor.
> > GL-renderer uses GLESv2, but if CAIRO_LIBS contains cairo-gl.so linking
> > to libGL, it might explode... or maybe not because of no RTLD_GLOBAL...
> > or maybe it could, I don't know. So I'd just avoid that. :-)
> 
> Siiiigh, I was afraid someone would come up with a reason to actually
> deal with libpng directly.  Oh, wait, only the client has to deal with
> png files - I think it's safe to link cairo there? :)

Most distros don't ship cairo with gl enabled anyway.  You'd have to do
that intentionally.

Even with it enabled, if we keep png generation client-side then we're
not going to risk interfering with the compositor.  Although, maybe some
tests link in some compositor code and could hit the issue... not sure.
At least here the scope of the problem will be limited to the one test.
And there's probably a few paths to solve it.

Bryce
On Mon, 24 Nov 2014 15:20:35 -0800
Bryce Harrington <bryce@osg.samsung.com> wrote:

> On Mon, Nov 24, 2014 at 04:31:01PM -0600, Derek Foreman wrote:
> > On 24/11/14 05:01 AM, Pekka Paalanen wrote:
> > > On Wed, 19 Nov 2014 15:06:21 -0800
> > > Bryce Harrington <bryce@osg.samsung.com> wrote:
> > > 
> > >> From: Derek Foreman <derekf@osg.samsung.com>
> > >>
> > >> Adds wl_test_record_screenshot() to weston test.  This commit also
> > >> adds a dependency on cairo to weston-test to use it for writing PNG
> > >> files.
> > >>
> > >> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981
> > >> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> > > 
> > > Hi,
> > > 
> > > could we use a wl_shm-based wl_buffer instead of a file, please?
> > > 
> > > That way we can simply relay the pixels as is to the test client, which
> > > can then compare without compressing and decompressing from PNG first.
> > 
> > Ok - it (the client) will still need to be able to write and read files
> > though, so it can create or read the reference images.
> 
> I agree it will run faster and save on disk space if we can skip writing
> the images to files.  Yes, we'll still need to read the reference
> images, but reading is faster than writing so we'll be ahead.  Most of
> the time we don't care what the images look like anyway.
> 
> That said, it can be beneficial to write the output out to files for
> troubleshooting purposes.  So we still need a write mode.  But perhaps
> it should be off by default with some env var or switch to make it show
> its work.

Agreed on all accounts. One will be interested on the output image only
when the test fails (hey, you could write it automatically in that
case) or when bootstrapping the test (creating the reference image to
begin with).

And if a missing reference image automatically causes a test to fail,
then all one needs is to 'mv && git add' after checking the result is
as expected.

Btw. I would imagine that it would be useful to have a check function
like:
	bool check_solid_color(image, rectangle, color, fuzz?)
which would check that the rectangle sub-region of the image is of that
solid color (with appropriate fuzz). That way we don't need reference
images in git for everything.

> > > Synchronization cannot be done with wl_display_roundtrip, so you need
> > > an explicit event for that. The best would be to create a new protocol
> > > object on record_screenshot request, which then delivers the event and
> > > self-destructs, like wl_callback. You could just use wl_callback, but
> > > in principle we should avoid new use cases for wl_callback due to
> > > the design issues.
> > > 
> > > Please also document carefully in which orientation the screenshot is
> > > taken and how the clip coords are interpreted, in case some
> > > output_scale/transform are in effect. Or if you rely on the renderer's
> > > read_pixels() convention, I hope that is documented somewhere...
> > 
> > Not yet ;)
> > 
> > (fwiw it's done so an unrotated display's screenshot looks normal in an
> > image viewer - so the opposite of gl convention :)
> > 
> > > Btw. there is a small danger in linking Cairo to the compositor.
> > > GL-renderer uses GLESv2, but if CAIRO_LIBS contains cairo-gl.so linking
> > > to libGL, it might explode... or maybe not because of no RTLD_GLOBAL...
> > > or maybe it could, I don't know. So I'd just avoid that. :-)
> > 
> > Siiiigh, I was afraid someone would come up with a reason to actually
> > deal with libpng directly.  Oh, wait, only the client has to deal with
> > png files - I think it's safe to link cairo there? :)
> 
> Most distros don't ship cairo with gl enabled anyway.  You'd have to do
> that intentionally.

Yeah. Also GL vs. GLESv2 must be chosen during Cairo build time and
they are mutually exclusive.

Relying on cairo-image is ok, cairo-gl not so much.

> Even with it enabled, if we keep png generation client-side then we're
> not going to risk interfering with the compositor.  Although, maybe some
> tests link in some compositor code and could hit the issue... not sure.
> At least here the scope of the problem will be limited to the one test.
> And there's probably a few paths to solve it.

Linking to Cairo in the test clients is fine. By definition, that is
client code.

We have some tests that include server code IIRC, but those are not
test clients, and do not run with an actual server, so you wouldn't be
screenshooting there anyway.


Thanks,
pq
On 25/11/14 03:53 AM, Pekka Paalanen wrote:
> On Mon, 24 Nov 2014 15:20:35 -0800
> Bryce Harrington <bryce@osg.samsung.com> wrote:
> 
>> On Mon, Nov 24, 2014 at 04:31:01PM -0600, Derek Foreman wrote:
>>> On 24/11/14 05:01 AM, Pekka Paalanen wrote:
>>>> On Wed, 19 Nov 2014 15:06:21 -0800
>>>> Bryce Harrington <bryce@osg.samsung.com> wrote:
>>>>
>>>>> From: Derek Foreman <derekf@osg.samsung.com>
>>>>>
>>>>> Adds wl_test_record_screenshot() to weston test.  This commit also
>>>>> adds a dependency on cairo to weston-test to use it for writing PNG
>>>>> files.
>>>>>
>>>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981
>>>>> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
>>>>
>>>> Hi,
>>>>
>>>> could we use a wl_shm-based wl_buffer instead of a file, please?
>>>>
>>>> That way we can simply relay the pixels as is to the test client, which
>>>> can then compare without compressing and decompressing from PNG first.
>>>
>>> Ok - it (the client) will still need to be able to write and read files
>>> though, so it can create or read the reference images.
>>
>> I agree it will run faster and save on disk space if we can skip writing
>> the images to files.  Yes, we'll still need to read the reference
>> images, but reading is faster than writing so we'll be ahead.  Most of
>> the time we don't care what the images look like anyway.
>>
>> That said, it can be beneficial to write the output out to files for
>> troubleshooting purposes.  So we still need a write mode.  But perhaps
>> it should be off by default with some env var or switch to make it show
>> its work.
> 
> Agreed on all accounts. One will be interested on the output image only
> when the test fails (hey, you could write it automatically in that
> case) or when bootstrapping the test (creating the reference image to
> begin with).

Easy enough, I think missing reference image should be a failure so
these become the same case.

> And if a missing reference image automatically causes a test to fail,
> then all one needs is to 'mv && git add' after checking the result is
> as expected.
>
> Btw. I would imagine that it would be useful to have a check function
> like:
> 	bool check_solid_color(image, rectangle, color, fuzz?)
> which would check that the rectangle sub-region of the image is of that
> solid color (with appropriate fuzz). That way we don't need reference
> images in git for everything.

What's fuzz exactly?  Each pixel can be within +-fuzz/2 on each color
component and still be a match?  a fuzz of 256 would match everything?

Or would fuzz be to allow a certain percentage of pixels to deviate (ie:
the bar with the clock and the icons on it being tested against red...)

>>>> Synchronization cannot be done with wl_display_roundtrip, so you need
>>>> an explicit event for that. The best would be to create a new protocol
>>>> object on record_screenshot request, which then delivers the event and
>>>> self-destructs, like wl_callback. You could just use wl_callback, but
>>>> in principle we should avoid new use cases for wl_callback due to
>>>> the design issues.
>>>>
>>>> Please also document carefully in which orientation the screenshot is
>>>> taken and how the clip coords are interpreted, in case some
>>>> output_scale/transform are in effect. Or if you rely on the renderer's
>>>> read_pixels() convention, I hope that is documented somewhere...
>>>
>>> Not yet ;)
>>>
>>> (fwiw it's done so an unrotated display's screenshot looks normal in an
>>> image viewer - so the opposite of gl convention :)
>>>
>>>> Btw. there is a small danger in linking Cairo to the compositor.
>>>> GL-renderer uses GLESv2, but if CAIRO_LIBS contains cairo-gl.so linking
>>>> to libGL, it might explode... or maybe not because of no RTLD_GLOBAL...
>>>> or maybe it could, I don't know. So I'd just avoid that. :-)
>>>
>>> Siiiigh, I was afraid someone would come up with a reason to actually
>>> deal with libpng directly.  Oh, wait, only the client has to deal with
>>> png files - I think it's safe to link cairo there? :)
>>
>> Most distros don't ship cairo with gl enabled anyway.  You'd have to do
>> that intentionally.
> 
> Yeah. Also GL vs. GLESv2 must be chosen during Cairo build time and
> they are mutually exclusive.
> 
> Relying on cairo-image is ok, cairo-gl not so much.

I'll rewrite this patch so it makes no use of cairo in weston-test, or
in any server code.

>> Even with it enabled, if we keep png generation client-side then we're
>> not going to risk interfering with the compositor.  Although, maybe some
>> tests link in some compositor code and could hit the issue... not sure.
>> At least here the scope of the problem will be limited to the one test.
>> And there's probably a few paths to solve it.
> 
> Linking to Cairo in the test clients is fine. By definition, that is
> client code.
> 
> We have some tests that include server code IIRC, but those are not
> test clients, and do not run with an actual server, so you wouldn't be
> screenshooting there anyway.

As eager as I am to not use libpng directly it's not that big deal, and
I have done it before.  I think I can see wanting to have a test client
that uses GL for its own rendering, and if using cairo for screenshots
makes that unworkable I'll look silly later for being lazy now.

We're probably better off just ditching cairo entirely here (like Bill
suggested in the first place.  yeah, hi, sorry.  ;)

Unfortunately, I think that necessitates changes to some of the later
patches too that use cairo for masking regions.
On 11/25/2014 07:11 AM, Derek Foreman wrote:

> What's fuzz exactly?  Each pixel can be within +-fuzz/2 on each color
> component and still be a match?  a fuzz of 256 would match everything?

The cairo project has done some work on a fuzzy comparison for their 
tests, may be able to reuse that.
On Tue, Nov 25, 2014 at 11:46:05AM -0800, Bill Spitzak wrote:
> On 11/25/2014 07:11 AM, Derek Foreman wrote:
> 
> >What's fuzz exactly?  Each pixel can be within +-fuzz/2 on each color
> >component and still be a match?  a fuzz of 256 would match everything?
> 
> The cairo project has done some work on a fuzzy comparison for their
> tests, may be able to reuse that.

I've messed with this in Cairo (and in Inkscape previously), in writing
and debugging rendering errors.  There's several different ways things
tend to fail.

One is simply caused by rounding during antialiasing or whatever.  So,
instead of a pixel being RGB(100,100,100) you get RGB(99,99,99).  For
most purposes that's good enough.  So the fuzz is to make the comparison
routine allow colors to be +/- by N.  That should be a straightforward
mod of our existing code.

Second is caused by algorithmic differences.  We saw this with the
different pixman filtering algorithms used in downscaling images in
Cairo.  So, one algorithm might make two pixels 50% grey, another
algorithm might make one pixel 75% the other 25%.  Stuff like that.
Antialiasing can also produce variances like this.  In Cairo we work
around this by letting different backends have their own reference
images.  But I think a better approach is to be more precise in
selecting what to test; crop out a rectangular box where you expect it
must be blue and verify it is.

A third case I've seen is where alpha blending produces different
resultant colors for shapes.  Although none of the times this was seen
was it considered anything other than a bug...

A common fourth case is that a given backend simply hasn't implemented
XYZ (e.g. alpha transparency in Postscript).  XFail or custom reference
images are used here too, but I wonder if this could be handled better.

Anyway, Cairo's pdiff stuff doesn't really differentiate between these
different types of rendering failures - it just sums up how many pixels
differ and returns a percentage.  (Maybe it weights the tally based on
how different the color is, I forget).  As a result, Cairo's testsuite
emits false positives and false negatives a lot, and tests end up
requiring more maintenance than they should.  So I'm game to start from
what Cairo does but we shouldn't be shy about shooting for something a
bit more clever where feasible.

Bryce
On Tue, 25 Nov 2014 09:11:39 -0600
Derek Foreman <derekf@osg.samsung.com> wrote:

> On 25/11/14 03:53 AM, Pekka Paalanen wrote:
> > On Mon, 24 Nov 2014 15:20:35 -0800
> > Bryce Harrington <bryce@osg.samsung.com> wrote:
> > 
> >> On Mon, Nov 24, 2014 at 04:31:01PM -0600, Derek Foreman wrote:
> >>> On 24/11/14 05:01 AM, Pekka Paalanen wrote:
> >>>> On Wed, 19 Nov 2014 15:06:21 -0800
> >>>> Bryce Harrington <bryce@osg.samsung.com> wrote:
> >>>>
> >>>>> From: Derek Foreman <derekf@osg.samsung.com>
> >>>>>
> >>>>> Adds wl_test_record_screenshot() to weston test.  This commit also
> >>>>> adds a dependency on cairo to weston-test to use it for writing PNG
> >>>>> files.
> >>>>>
> >>>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981
> >>>>> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> >>>>
> >>>> Hi,
> >>>>
> >>>> could we use a wl_shm-based wl_buffer instead of a file, please?

...

> > Btw. I would imagine that it would be useful to have a check function
> > like:
> > 	bool check_solid_color(image, rectangle, color, fuzz?)
> > which would check that the rectangle sub-region of the image is of that
> > solid color (with appropriate fuzz). That way we don't need reference
> > images in git for everything.
> 
> What's fuzz exactly?  Each pixel can be within +-fuzz/2 on each color
> component and still be a match?  a fuzz of 256 would match everything?
> 
> Or would fuzz be to allow a certain percentage of pixels to deviate (ie:
> the bar with the clock and the icons on it being tested against red...)

I always thought the fuzz would be a threshold on color value
difference for fail/pass. Even one failing pixel would fail the whole
check.

Bryce listed lots of things related to fuzz and where the valid
inaccuracies may come from. I think we should be able to avoid geometry
rasterization differences by leaving a margin between the checked rect
and the edge of the (assumed) color region. The only thing we'd need to
account for is rounding in blending and color values.

That should get us pretty far. When we come up with tests where it's
not enough, we can think about it again.

Luckily Wayland does not include a drawing protocol. :-)

In the end, the fuzz can be defined any way you see appropriate.

> >> Even with it enabled, if we keep png generation client-side then we're
> >> not going to risk interfering with the compositor.  Although, maybe some
> >> tests link in some compositor code and could hit the issue... not sure.
> >> At least here the scope of the problem will be limited to the one test.
> >> And there's probably a few paths to solve it.
> > 
> > Linking to Cairo in the test clients is fine. By definition, that is
> > client code.
> > 
> > We have some tests that include server code IIRC, but those are not
> > test clients, and do not run with an actual server, so you wouldn't be
> > screenshooting there anyway.
> 
> As eager as I am to not use libpng directly it's not that big deal, and
> I have done it before.  I think I can see wanting to have a test client
> that uses GL for its own rendering, and if using cairo for screenshots
> makes that unworkable I'll look silly later for being lazy now.

Ah no, Cairo alone does not prevent using GL. I... uh, huh!

Hmm. libcairo.so when built with cairo-gl support does indeed link to
libGL.so. I didn't realize that, for some reason I imagined there was
cairo-gl.so somewhere.

But I still believe it should be ok to use cairo-image with any flavour
of GL.

Now I'm not completely sure why, though...

> We're probably better off just ditching cairo entirely here (like Bill
> suggested in the first place.  yeah, hi, sorry.  ;)

I'd like to think we don't need to ditch Cairo. I might suggest to just
stick with Cairo, use cairo-image as much as you want, and see if
anything breaks. We can fix it later, too.

Like Bryce said, most distros don't ship Cairo with GL enabled. If we
use Cairo and GL, and it explodes because someone built Cairo with a
differen GL flavour support, I think we could even report that as bug
in Cairo upstream.


Thanks,
pq
On Mon, Nov 24, 2014 at 01:01:29PM +0200, Pekka Paalanen wrote:
> could we use a wl_shm-based wl_buffer instead of a file, please?
> 
> That way we can simply relay the pixels as is to the test client, which
> can then compare without compressing and decompressing from PNG first.
> 
> For an example how to do this, see Weston's screenshooter protocol and
> implementation.
> 
> I think you also want to specify in the record_screenshot request:
> - which output to capture (in case there are multiple)
> - the rectangular sub-region that must be contained in the output
>   (which you already do with the clip coords in a later patch, so that
>   should be squashed here)

Actually, if we're not storing the screenshot to a file on the server
side, I wonder if we even need to communicate the sub-region coords to
the server?  We can just let it pass the full image to the client and
take care of clipping and whatnot all client-side.

It'd probably be more bandwidth efficient to just pass the bit of image
we're going to test, but I don't think we care about that much here.  By
doing it all client-side it'll keep the protocol and the server-side
implementation more minimal.

Bryce
On Wed, 10 Dec 2014 19:19:50 -0800
Bryce Harrington <bryce@osg.samsung.com> wrote:

> On Mon, Nov 24, 2014 at 01:01:29PM +0200, Pekka Paalanen wrote:
> > could we use a wl_shm-based wl_buffer instead of a file, please?
> > 
> > That way we can simply relay the pixels as is to the test client,
> > which can then compare without compressing and decompressing from
> > PNG first.
> > 
> > For an example how to do this, see Weston's screenshooter protocol
> > and implementation.
> > 
> > I think you also want to specify in the record_screenshot request:
> > - which output to capture (in case there are multiple)
> > - the rectangular sub-region that must be contained in the output
> >   (which you already do with the clip coords in a later patch, so
> > that should be squashed here)
> 
> Actually, if we're not storing the screenshot to a file on the server
> side, I wonder if we even need to communicate the sub-region coords to
> the server?  We can just let it pass the full image to the client and
> take care of clipping and whatnot all client-side.
> 
> It'd probably be more bandwidth efficient to just pass the bit of
> image we're going to test, but I don't think we care about that much
> here.  By doing it all client-side it'll keep the protocol and the
> server-side implementation more minimal.

True that. The only benefit would be copying less data when shooting,
but since this is testing, performance doesn't matter until someone
complains the tests take too long. :-)


Thanks,
pq