[PATCH weston v4 15/15] Add a screen sharing plugin
Jason Ekstrand
jason at jlekstrand.net
Tue Mar 11 08:57:00 PDT 2014
On Mar 10, 2014 9:29 PM, "Bryce W. Harrington" <b.harrington at samsung.com>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140311/eb01a606/attachment.html>
More information about the wayland-devel
mailing list