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