[PATCH weston 07/10] tests: Add screenshot recording capability to weston-test

[Pekka Paalanen]

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

> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
>  Makefile.am         |   4 +-
>  tests/weston-test.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 245 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index fb3152e..023b7e1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -995,7 +995,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)
> @@ -1007,7 +1007,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)

This patch is not adding any Cairo calls, so CAIRO_LIBS and
CAIRO_CFLAGS should not be added here.

>  weston_test_la_SOURCES = tests/weston-test.c
>  nodist_weston_test_la_SOURCES =			\
>  	protocol/weston-test-protocol.c	\
> diff --git a/tests/weston-test.c b/tests/weston-test.c
> index 9f1f49b..1d6d264 100644
> --- a/tests/weston-test.c
> +++ b/tests/weston-test.c
> @@ -27,6 +27,7 @@
>  #include <signal.h>
>  #include <unistd.h>
>  #include <string.h>
> +#include <cairo.h>

I can't see anywhere actually using Cairo, so don't add th include.

>  
>  #include "../src/compositor.h"
>  #include "weston-test-server-protocol.h"
> @@ -269,6 +270,247 @@ get_n_buffers(struct wl_client *client, struct wl_resource *resource)
>  	weston_test_send_n_egl_buffers(resource, n_buffers);
>  }
>  
> +enum weston_test_screenshot_outcome {
> +	WESTON_TEST_SCREENSHOT_SUCCESS,
> +	WESTON_TEST_SCREENSHOT_NO_MEMORY,
> +	WESTON_TEST_SCREENSHOT_BAD_BUFFER
> +	};
> +
> +typedef void (*weston_test_screenshot_done_func_t)(void *data,
> +						   enum weston_test_screenshot_outcome outcome);
> +
> +struct test_screenshot {
> +	struct weston_compositor *ec;

Please, use 'compositor'. 'ec' is the cargo-culted name.

> +	struct wl_global *global;
> +	struct wl_client *client;
> +	struct weston_process process;
> +	struct wl_listener destroy_listener;
> +};
> +
> +struct test_screenshot_frame_listener {
> +	struct wl_listener listener;
> +	struct weston_buffer *buffer;
> +	weston_test_screenshot_done_func_t done;

Is there a reason for the indirection through the 'done' function
pointer? It's not obvious to me, so I'd suggest just dropping that
indirection.

> +	void *data;
> +};

> +static void
> +test_screenshot_frame_notify(struct wl_listener *listener, void *data)
> +{
> +	struct test_screenshot_frame_listener *l =
> +		container_of(listener,
> +			     struct test_screenshot_frame_listener, listener);
> +	struct weston_output *output = data;
> +	struct weston_compositor *compositor = output->compositor;
> +	int32_t stride;
> +	uint8_t *pixels, *d, *s;
> +
> +	output->disable_planes--;
> +	wl_list_remove(&listener->link);
> +	stride = l->buffer->width * (PIXMAN_FORMAT_BPP(compositor->read_format) / 8);
> +	pixels = malloc(stride * l->buffer->height);
> +
> +	if (pixels == NULL) {
> +		l->done(l->data, WESTON_TEST_SCREENSHOT_NO_MEMORY);
> +		free(l);
> +		return;
> +	}
> +
> +	// FIXME: Needs to handle output transformations

Should it handle output transformations, or should it just record the
output transformation parameters and deliver them to the client?

In fact, if there is an output scale > 1, and the test client uses the
same buffer scale, one would expect to receive "unscaled" pixels, i.e.
the same pixels as the client posted but maybe in a different
orientation.

If we handle output transformations in the test plugin, that means the
client would receive images in the "global coordinate space", which in
the above example is then downscaled. And that downscaling is something
that the compositor would not normally do, because it would be done in
the test plugin.

So I'd propose to define the shooting protocol such, that the produced
image is the final image on the output. Do not undo the output
transformation in the test plugin, but let the test client handle it.
This way the test plugin does not cause information loss by e.g.
downscaling.

wl_output modes are raw hardware modes too, which I assume you are
using to pick the size the for wl_buffer in the first place in the
client.

> +
> +	compositor->renderer->read_pixels(output,
> +					  compositor->read_format,
> +					  pixels,
> +					  0, 0,
> +					  output->current_mode->width,
> +					  output->current_mode->height);
> +
> +	stride = wl_shm_buffer_get_stride(l->buffer->shm_buffer);
> +
> +	d = wl_shm_buffer_get_data(l->buffer->shm_buffer);
> +	s = pixels + stride * (l->buffer->height - 1);
> +
> +	wl_shm_buffer_begin_access(l->buffer->shm_buffer);
> +
> +	switch (compositor->read_format) {
> +	case PIXMAN_a8r8g8b8:
> +	case PIXMAN_x8r8g8b8:
> +		if (compositor->capabilities & WESTON_CAP_CAPTURE_YFLIP)
> +			copy_bgra_yflip(d, s, output->current_mode->height, stride);
> +		else
> +			copy_bgra(d, pixels, output->current_mode->height, stride);
> +		break;
> +	case PIXMAN_x8b8g8r8:
> +	case PIXMAN_a8b8g8r8:
> +		if (compositor->capabilities & WESTON_CAP_CAPTURE_YFLIP)
> +			copy_rgba_yflip(d, s, output->current_mode->height, stride);
> +		else
> +			copy_rgba(d, pixels, output->current_mode->height, stride);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	wl_shm_buffer_end_access(l->buffer->shm_buffer);
> +
> +	l->done(l->data, WESTON_TEST_SCREENSHOT_SUCCESS);
> +	free(pixels);
> +	free(l);
> +}
> +
> +static bool
> +weston_test_screenshot_shoot(struct weston_output *output,
> +			     struct weston_buffer *buffer,
> +			     weston_test_screenshot_done_func_t done,
> +			     void *data)
> +{
> +	struct test_screenshot_frame_listener *l;
> +
> +	/* Get the shm buffer resource the client created */
> +	if (!wl_shm_buffer_get(buffer->resource)) {
> +		done(data, WESTON_TEST_SCREENSHOT_BAD_BUFFER);
> +		return false;
> +	}
> +
> +	buffer->shm_buffer = wl_shm_buffer_get(buffer->resource);
> +	buffer->width = wl_shm_buffer_get_width(buffer->shm_buffer);
> +	buffer->height = wl_shm_buffer_get_height(buffer->shm_buffer);
> +
> +	/* Verify buffer is big enough */
> +	if (buffer->width < output->current_mode->width ||
> +		buffer->height < output->current_mode->height) {
> +		done(data, WESTON_TEST_SCREENSHOT_BAD_BUFFER);
> +		return false;
> +	}
> +
> +	/* allocate the frame listener */
> +	l = malloc(sizeof *l);
> +	if (l == NULL) {
> +		done(data, WESTON_TEST_SCREENSHOT_NO_MEMORY);
> +		return false;
> +	}
> +
> +	/* Set up the listener */
> +	l->buffer = buffer;

To be completely correct, you would need to set a destroy_signal
listener on the weston_buffer. If the client destroys the wl_buffer
before the compositor has shot anything, the weston_buffer will get
freed, and the compositor hits freed memory when it tries to shoot.

> +	l->done = done;
> +	l->data = data;
> +	l->listener.notify = test_screenshot_frame_notify;
> +	wl_signal_add(&output->frame_signal, &l->listener);
> +
> +	/* Fire off a repaint */
> +	output->disable_planes++;
> +	weston_output_schedule_repaint(output);
> +
> +	return true;
> +}

Looks like some of my comments also apply to the existing
src/screenshooter.c.

Plus all the comments from Daniel.


Thanks,
pq