On Mar 10, 2014 9:29 PM, "Bryce W. Harrington" <<a href="mailto:b.harrington@samsung.com">b.harrington@samsung.com</a>> wrote: > > On Mon, Mar 10, 2014 at 08:42:41PM -0500, Jason Ekstrand wrote: > > > > +static void > > > > +ss_seat_handle_pointer_enter(void *data, struct wl_pointer *pointer, > > > > + uint32_t serial, struct wl_surface *surface, > > > > + wl_fixed_t x, wl_fixed_t y) > > > > +{ > > > > + struct ss_seat *seat = data; > > > > + > > > > + /* We make the tacit assumption here that we are always recieving > > > > > > receiving > > > > > > > + * input in output coordinates. > > > > + weston_output_transform_coordinate(&seat->output->base, x, y, &x, > > &y); > > > > + */ > > > > > > Is that function call supposed to be commented out? > > > If so, prefix with "*". > > > > Yes, it's supposed to be commented out. However, the comment needs more > > explanation. In the wayland backend we have to transform the input into > > output coordinates. Because the screen-share plugin is untransforming the > > output before sending it to the RDP backend, this isn't needed. Leaving > > the function there but commented out was mostly a note to me that it needs > > to go back in if this ever changes. That said, I should leave a far better > > comment than I did. I'll fix it. > > Cool, makes sense. > > > > > > > I know this function is cribbed from an analogous routine from > > > compositor-wayland.c, but I'm curious why perror() is used here, when > > > weston_log() is used elsewhere? > > > > No good reason. We should probably change it in compositor-wayland.c too > > Shall I do the honors or do you want to change it to keep it consistent > with this patch? Go ahead. This is going to sit on the list for a little while (I've got changes to make). No sense making compositor-wayland wait on it. > > > > Also since there is some similarity, could any of this be refactored > > > into shared code to avoid the redundancy? > > > > Yes, it could. However, this really only shares the shm buffer > > implementation with compositor-wayland.c and even there it's not > > identical. I thought about trying to make them share this, input code, and > > another thing or two. However, there turned out to be enough differences > > that it wasn't worth it. > > *Nod* > > > > > + > > > > + sb = zalloc(sizeof *sb); > > > > + > > > > + sb->output = so; > > > > + wl_list_init(&sb->free_link); > > > > + wl_list_insert(&so->shm.buffers, &sb->link); > > > > + > > > > + pixman_region32_init_rect(&sb->damage, 0, 0, width, height); > > > > + > > > > + sb->data = data; > > > > + sb->size = height * stride; > > > > + > > > > + pool = wl_shm_create_pool(so->parent.shm, fd, sb->size); > > > > + > > > > + sb->buffer = wl_shm_pool_create_buffer(pool, 0, > > > > + width, height, stride, > > > > + WL_SHM_FORMAT_ARGB8888); > > > > + wl_buffer_add_listener(sb->buffer, &buffer_listener, sb); > > > > + wl_shm_pool_destroy(pool); > > > > + close(fd); > > > > + > > > > + memset(data, 0, sb->size); > > > > + > > > > + sb->pm_image = > > > > + pixman_image_create_bits(PIXMAN_a8r8g8b8, width, height, > > > > + (uint32_t *)data, stride); > > > > + > > > > + return sb; > > > > +} > > > > + > > > > +static void > > > > +output_compute_transform(struct weston_output *output, > > > > + pixman_transform_t *transform) > > > > +{ > > > > + pixman_fixed_t fw, fh; > > > > + > > > > + pixman_transform_init_identity(transform); > > > > + > > > > + fw = pixman_int_to_fixed(output->width); > > > > + fh = pixman_int_to_fixed(output->height); > > > > + > > > > + switch (output->transform) { > > > > + case WL_OUTPUT_TRANSFORM_FLIPPED: > > > > + case WL_OUTPUT_TRANSFORM_FLIPPED_90: > > > > + case WL_OUTPUT_TRANSFORM_FLIPPED_180: > > > > + case WL_OUTPUT_TRANSFORM_FLIPPED_270: > > > > + pixman_transform_scale(transform, NULL, > > > > + pixman_int_to_fixed (-1), > > > > + pixman_int_to_fixed (1)); > > > > + pixman_transform_translate(transform, NULL, fw, 0); > > > > + } > > > > + > > > > + switch (output->transform) { > > > > + default: > > > > + case WL_OUTPUT_TRANSFORM_NORMAL: > > > > + case WL_OUTPUT_TRANSFORM_FLIPPED: > > > > + break; > > > > + case WL_OUTPUT_TRANSFORM_90: > > > > + case WL_OUTPUT_TRANSFORM_FLIPPED_90: > > > > + pixman_transform_rotate(transform, NULL, 0, > > pixman_fixed_1); > > > > + pixman_transform_translate(transform, NULL, fh, 0); > > > > + break; > > > > + case WL_OUTPUT_TRANSFORM_180: > > > > + case WL_OUTPUT_TRANSFORM_FLIPPED_180: > > > > + pixman_transform_rotate(transform, NULL, -pixman_fixed_1, > > 0); > > > > + pixman_transform_translate(transform, NULL, fw, fh); > > > > + break; > > > > + case WL_OUTPUT_TRANSFORM_270: > > > > + case WL_OUTPUT_TRANSFORM_FLIPPED_270: > > > > + pixman_transform_rotate(transform, NULL, 0, > > -pixman_fixed_1); > > > > + pixman_transform_translate(transform, NULL, 0, fw); > > > > + break; > > > > + } > > > > + > > > > + pixman_transform_scale(transform, NULL, > > > > + pixman_fixed_1 * output->current_scale, > > > > + pixman_fixed_1 * output->current_scale); > > > > +} > > > > + > > > > +static void > > > > +shared_output_destroy(struct shared_output *so); > > > > + > > > > +static int > > > > +shared_output_ensure_tmp_data(struct shared_output *so, > > > > + pixman_region32_t *region) > > > > +{ > > > > + pixman_box32_t *ext; > > > > + int32_t area; > > > > + size_t size; > > > > + > > > > + if (pixman_region32_not_empty(region)) { > > > > + ext = pixman_region32_extents(region); > > > > + area = (ext->x2 - ext->x1) * (ext->y2 - ext->y1); > > > > + } else { > > > > + return 0; > > > > + } > > > > + > > > > + /* Damage is in buffer coordinates */ > > > > + area *= so->output->current_scale * so->output->current_scale; > > > > + > > > > + size = area * 4; > > > > > > The area temporary variable can be factored out, and the code simplified > > > to: > > > > > > pixman_box32_t *ext; > > > size_t size; > > > > > > if (!pixman_region32_not_empty(region)) > > > return; > > I made an error here - it should be 'return 0;' > > > > ext = pixman_region32_extents(region); > > > > > > /* Damage is in buffer coordinates */ > > > size = 4 * (ext->x2 - ext->x1) * (ext->y2 - ext->y1) > > > * so->output->current_scale * so->output->current_scale; > > > > > > Might also be worth adding a comment as to why we're multiplying by 4. > > > > Yeah, that's more compact. I was originally being verbose to try and make > > it more clear what was going on and just trusting the compiler to optimize > > for me. However, I think what you've written there is clear enough. I'll > > make that change. > > > > Bryce