[PATCH weston v4 15/15] Add a screen sharing plugin

[Bryce W. Harrington]

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?

> > 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