[PATCH] screenshooter: Properly handle multiple outputs.
Scott Moreau
oreaus at gmail.com
Wed Apr 4 09:30:48 PDT 2012
On Wed, Apr 4, 2012 at 6:37 AM, Kristian Hoegsberg <hoegsberg at gmail.com>wrote:
> On Wed, Apr 04, 2012 at 01:51:08AM -0600, Scott Moreau wrote:
> > ---
>
> Looks pretty good. There are a few small comments below, but the
> overall approach is good.
>
> > Tested: Side-by-side and stacked output configurations, in x11 and drm.
> > Shooting under drm glitches sometimes and either or both output data
> > contains garbage.
>
> I see the drm glitches without this, it's a different problem.
>
> >
> > clients/screenshot.c | 69
> ++++++++++++++++++++++++++++++++++++++++------
> > src/compositor-drm.c | 14 +++++++++
> > src/compositor-wayland.c | 14 +++++++++
> > src/compositor-x11.c | 14 +++++++++
> > src/compositor.h | 1 +
> > src/screenshooter.c | 28 +++++++++---------
> > 6 files changed, 117 insertions(+), 23 deletions(-)
> >
> > diff --git a/clients/screenshot.c b/clients/screenshot.c
> > index 6cc2ebb..40f0af4 100644
> > --- a/clients/screenshot.c
> > +++ b/clients/screenshot.c
> > @@ -32,14 +32,24 @@
> > #include <wayland-client.h>
> > #include "screenshooter-client-protocol.h"
> >
> > +#define MAX(X,Y) ((X) > (Y) ? (X) : (Y))
> > +
> > /* The screenshooter is a good example of a custom object exposed by
> > * the compositor and serves as a test bed for implementing client
> > * side marshalling outside libwayland.so */
> >
> > -static struct wl_output *output;
> > static struct wl_shm *shm;
> > static struct screenshooter *screenshooter;
> > -static int output_width, output_height;
> > +static struct wl_list output_list;
> > +
> > +struct ss_output {
> > + struct wl_output *output;
> > + struct wl_buffer *buffer;
> > + int width, height, offset_x, offset_y;
> > + struct wl_list link;
> > +};
> > +
> > +static struct ss_output *output;
>
> Can we call this screenshooter_output instead?
>
I named it screenshooter_output to begin with but felt it made the code too
messy.
It can be renamed back.
>
> > static void
> > display_handle_geometry(void *data,
> > @@ -52,6 +62,14 @@ display_handle_geometry(void *data,
> > const char *make,
> > const char *model)
> > {
> > + struct ss_output *ss_output;
> > +
> > + wl_list_for_each(ss_output, &output_list, link) {
> > + if (wl_output == ss_output->output) {
> > + ss_output->offset_x = x;
> > + ss_output->offset_y = y;
> > + }
> > + }
>
> Instead of this loop, you can just set the user data on the wl_output
> object (the data arg to add_listener) and then get it here using
>
> ss_output = wl_output_get_user_data(wl_output);
>
Yes, I didn't realize you can do this.
>
>
> > }
> >
> > static void
> > @@ -62,8 +80,15 @@ display_handle_mode(void *data,
> > int height,
> > int refresh)
> > {
> > - output_width = width;
> > - output_height = height;
> > + struct ss_output *ss_output;
> > +
> > + wl_list_for_each(ss_output, &output_list, link) {
> > + if (wl_output == ss_output->output &&
> > + (flags & WL_OUTPUT_MODE_CURRENT)) {
> > + ss_output->width = width;
> > + ss_output->height = height;
> > + }
> > + }
>
> Same here.
>
Right.
>
> > }
> >
> > static const struct wl_output_listener output_listener = {
> > @@ -76,8 +101,10 @@ handle_global(struct wl_display *display, uint32_t
> id,
> > const char *interface, uint32_t version, void *data)
> > {
> > if (strcmp(interface, "wl_output") == 0) {
> > - output = wl_display_bind(display, id,
> &wl_output_interface);
> > - wl_output_add_listener(output, &output_listener, NULL);
> > + output = malloc(sizeof *output);
> > + output->output = wl_display_bind(display, id,
> &wl_output_interface);
> > + wl_list_insert(&output_list, &output->link);
> > + wl_output_add_listener(output->output, &output_listener,
> NULL);
>
> Pass output instead of NULL as the last arg here.
>
I see..
>
> > } else if (strcmp(interface, "wl_shm") == 0) {
> > shm = wl_display_bind(display, id, &wl_shm_interface);
> > } else if (strcmp(interface, "screenshooter") == 0) {
> > @@ -144,6 +171,8 @@ int main(int argc, char *argv[])
> > struct wl_display *display;
> > struct wl_buffer *buffer;
> > void *data = NULL;
> > + struct ss_output *ss_output;
> > + int ss_area_width = 0, ss_area_height = 0, num_outputs = 0;
> >
> > display = wl_display_connect(NULL);
> > if (display == NULL) {
> > @@ -151,6 +180,7 @@ int main(int argc, char *argv[])
> > return -1;
> > }
> >
> > + wl_list_init(&output_list);
> > wl_display_add_global_listener(display, handle_global,
> &screenshooter);
> > wl_display_iterate(display, WL_DISPLAY_READABLE);
> > wl_display_roundtrip(display);
> > @@ -159,11 +189,32 @@ int main(int argc, char *argv[])
> > return -1;
> > }
> >
> > - buffer = create_shm_buffer(output_width, output_height, &data);
> > - screenshooter_shoot(screenshooter, output, buffer);
> > + wl_list_for_each(ss_output, &output_list, link) {
> > +
> > + if (!num_outputs) {
> > + ss_area_width = ss_output->width +
> ss_output->offset_x;
> > + ss_area_height = ss_output->height +
> ss_output->offset_y;
> > + }
>
> I'd just set ss_area_width/height to 0 initially.
>
num_outputs must be initialised since it checks !num_outputs on the first
pass.
Unless there is another way to do this, we need to set num_outputs to begin
with.
>
>
> > + else {
> > + ss_area_width = MAX(ss_area_width,
> ss_output->offset_x + ss_output->width);
> > + ss_area_height = MAX(ss_area_height,
> ss_output->offset_y + ss_output->height);
> > + }
> > + num_outputs++;
> > + }
> > +
> > + buffer = create_shm_buffer(ss_area_width, ss_area_height, &data);
> > +
> > + wl_list_for_each(ss_output, &output_list, link) {
> > + screenshooter_shoot(screenshooter, ss_output->output,
> buffer);
> > + }
> > +
> > wl_display_roundtrip(display);
> >
> > - write_png(output_width, output_height, data);
> > + write_png(ss_area_width, ss_area_height, data);
> > +
> > + wl_list_for_each(ss_output, &output_list, link) {
> > + free(ss_output);
>
> You need wl_list_for_each_safe here since you're freeing the structs
> that holds the prev/next pointers.
>
Ah, I was wondering about that.
>
> > + }
> >
> > return 0;
> > }
> > diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> > index d428c82..7f2886e 100644
> > --- a/src/compositor-drm.c
> > +++ b/src/compositor-drm.c
> > @@ -1047,6 +1047,19 @@ drm_set_dpms(struct weston_output *output_base,
> enum dpms_enum level)
> > drmModeFreeConnector(connector);
> > }
> >
> > +static void
> > +drm_output_read_pixels(struct weston_output *output_base, void *data) {
> > + struct drm_output *output = (struct drm_output *) output_base;
> > + struct drm_compositor *compositor =
> > + (struct drm_compositor *) output->base.compositor;
> > +
> > + eglMakeCurrent(compositor->base.display, output->egl_surface,
> > + output->egl_surface, compositor->base.context);
> > +
> > + glReadPixels(0, 0, output_base->current->width,
> output_base->current->height,
> > + GL_BGRA_EXT, GL_UNSIGNED_BYTE, data);
> > +}
> > +
> > static int
> > create_output_for_connector(struct drm_compositor *ec,
> > drmModeRes *resources,
> > @@ -1085,6 +1098,7 @@ create_output_for_connector(struct drm_compositor
> *ec,
> > output->base.subpixel =
> drm_subpixel_to_wayland(connector->subpixel);
> > output->base.make = "unknown";
> > output->base.model = "unknown";
> > + output->base.read_pixels = drm_output_read_pixels;
>
> Group this assignment with the other function pointer assignments
> (repaint, destroy etc, at the end of the function).
>
Got it.
>
> > wl_list_init(&output->base.mode_list);
> >
> > output->crtc_id = resources->crtcs[i];
> > diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> > index 593e272..8c87721 100644
> > --- a/src/compositor-wayland.c
> > +++ b/src/compositor-wayland.c
> > @@ -377,6 +377,19 @@ wayland_output_destroy(struct weston_output
> *output_base)
> > return;
> > }
> >
> > +static void
> > +wayland_output_read_pixels(struct weston_output *output_base, void
> *data) {
> > + struct wayland_output *output = (struct wayland_output *)
> output_base;
> > + struct wayland_compositor *compositor =
> > + (struct wayland_compositor *) output->base.compositor;
> > +
> > + eglMakeCurrent(compositor->base.display, output->egl_surface,
> > + output->egl_surface, compositor->base.context);
> > +
> > + glReadPixels(0, 0, output_base->current->width,
> output_base->current->height,
> > + GL_BGRA_EXT, GL_UNSIGNED_BYTE, data);
> > +}
> > +
> > static int
> > wayland_compositor_create_output(struct wayland_compositor *c,
> > int width, int height)
> > @@ -393,6 +406,7 @@ wayland_compositor_create_output(struct
> wayland_compositor *c,
> > output->mode.width = width;
> > output->mode.height = height;
> > output->mode.refresh = 60;
> > + output->base.read_pixels = wayland_output_read_pixels;
> > wl_list_init(&output->base.mode_list);
> > wl_list_insert(&output->base.mode_list, &output->mode.link);
> >
> > diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> > index d0203ca..09aa117 100644
> > --- a/src/compositor-x11.c
> > +++ b/src/compositor-x11.c
> > @@ -344,6 +344,19 @@ x11_output_set_icon(struct x11_compositor *c,
> > pixman_image_unref(image);
> > }
> >
> > +static void
> > +x11_output_read_pixels(struct weston_output *output_base, void *data) {
> > + struct x11_output *output = (struct x11_output *) output_base;
> > + struct x11_compositor *compositor =
> > + (struct x11_compositor *) output->base.compositor;
> > +
> > + eglMakeCurrent(compositor->base.display, output->egl_surface,
> > + output->egl_surface, compositor->base.context);
> > +
> > + glReadPixels(0, 0, output_base->current->width,
> output_base->current->height,
> > + GL_BGRA_EXT, GL_UNSIGNED_BYTE, data);
> > +}
> > +
> > static int
> > x11_compositor_create_output(struct x11_compositor *c, int x, int y,
> > int width, int height, int fullscreen)
> > @@ -381,6 +394,7 @@ x11_compositor_create_output(struct x11_compositor
> *c, int x, int y,
> > output->mode.width = width;
> > output->mode.height = height;
> > output->mode.refresh = 60;
> > + output->base.read_pixels = x11_output_read_pixels;
> > wl_list_init(&output->base.mode_list);
> > wl_list_insert(&output->base.mode_list, &output->mode.link);
> >
> > diff --git a/src/compositor.h b/src/compositor.h
> > index d1cd7c8..ea18b7b 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -96,6 +96,7 @@ struct weston_output {
> > pixman_region32_t *damage);
> > void (*destroy)(struct weston_output *output);
> > void (*assign_planes)(struct weston_output *output);
> > + void (*read_pixels)(struct weston_output *output, void *data);
> >
> > /* backlight values are on 0-255 range, where higher is brighter */
> > uint32_t backlight_current;
> > diff --git a/src/screenshooter.c b/src/screenshooter.c
> > index f9497f7..3fe7bc3 100644
> > --- a/src/screenshooter.c
> > +++ b/src/screenshooter.c
> > @@ -32,7 +32,7 @@ struct screenshooter {
> > struct weston_compositor *ec;
> > struct wl_global *global;
> > struct wl_client *client;
> > - struct weston_process screenshooter_process;
> > + struct weston_process process;
> > };
> >
> > static void
> > @@ -44,7 +44,7 @@ screenshooter_shoot(struct wl_client *client,
> > struct weston_output *output = output_resource->data;
> > struct wl_buffer *buffer = buffer_resource->data;
> > uint8_t *tmp, *d, *s;
> > - int32_t stride, i;
> > + int32_t buffer_stride, output_stride, i;
> >
> > if (!wl_buffer_is_shm(buffer))
> > return;
> > @@ -53,24 +53,24 @@ screenshooter_shoot(struct wl_client *client,
> > buffer->height < output->current->height)
> > return;
> >
> > - stride = wl_shm_buffer_get_stride(buffer);
> > - tmp = malloc(stride * buffer->height);
> > + buffer_stride = wl_shm_buffer_get_stride(buffer);
> > + output_stride = output->current->width * 4;
> > + tmp = malloc(output_stride * output->current->height);
> > if (tmp == NULL) {
> > wl_resource_post_no_memory(resource);
> > return;
> > }
> >
> > glPixelStorei(GL_PACK_ALIGNMENT, 1);
> > - glReadPixels(0, 0, output->current->width, output->current->height,
> > - GL_BGRA_EXT, GL_UNSIGNED_BYTE, tmp);
> > + output->read_pixels(output, tmp);
> >
> > - d = wl_shm_buffer_get_data(buffer);
> > - s = tmp + stride * (buffer->height - 1);
> > + d = wl_shm_buffer_get_data(buffer) + output->y * buffer_stride;
>
> Just add output->x * 4 term to d here instead of inside the loop.
>
Good idea.
>
> > + s = tmp + output_stride * (output->current->height - 1);
> >
> > - for (i = 0; i < buffer->height; i++) {
> > - memcpy(d, s, stride);
> > - d += stride;
> > - s -= stride;
> > + for (i = 0; i < output->current->height; i++) {
> > + memcpy(d + output->x * 4, s, output_stride);
> > + d += buffer_stride;
> > + s -= output_stride;
> > }
> >
> > free(tmp);
> > @@ -101,7 +101,7 @@ static void
> > screenshooter_sigchld(struct weston_process *process, int status)
> > {
> > struct screenshooter *shooter =
> > - container_of(process, struct screenshooter,
> screenshooter_process);
> > + container_of(process, struct screenshooter, process);
> >
> > shooter->client = NULL;
> > }
> > @@ -116,7 +116,7 @@ screenshooter_binding(struct wl_input_device
> *device, uint32_t time,
> >
> > if (!shooter->client)
> > shooter->client = weston_client_launch(shooter->ec,
> > - &shooter->screenshooter_process,
> > + &shooter->process,
> > screenshooter_exe,
> screenshooter_sigchld);
> > }
> >
> > --
> > 1.7.4.1
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
Thanks for your review, I will write a new patch considering what you've
mentioned here.
Scott
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20120404/4106a845/attachment-0001.html>
More information about the wayland-devel
mailing list