<div class="gmail_quote">On Wed, Apr 4, 2012 at 6:37 AM, Kristian Hoegsberg <<a href="mailto:hoegsberg@gmail.com">hoegsberg@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. <div class="im"> > 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. </div>I see the drm glitches without this, it's a different problem. <div><div class="h5"> > > 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; </div></div>Can we call this screenshooter_output instead? </blockquote><div> I named it screenshooter_output to begin with but felt it made the code too messy. It can be renamed back. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > 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; > + } > + } </div>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); </blockquote><div> Yes, I didn't realize you can do this. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > } > > 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; > + } > + } </div>Same here. </blockquote><div> Right. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > } > > 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); </div>Pass output instead of NULL as the last arg here. </blockquote><div> I see.. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > } 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; > + } </div></div>I'd just set ss_area_width/height to 0 initially. </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > + 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); </div>You need wl_list_for_each_safe here since you're freeing the structs that holds the prev/next pointers. </blockquote><div> Ah, I was wondering about that. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > + } > > 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; </div></div>Group this assignment with the other function pointer assignments (repaint, destroy etc, at the end of the function). </blockquote><div> Got it. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > 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; </div></div>Just add output->x * 4 term to d here instead of inside the loop. </blockquote><div> Good idea. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > + 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 > </div>> _______________________________________________ > wayland-devel mailing list > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote><div> Thanks for your review, I will write a new patch considering what you've mentioned here. Scott </div></div>