[PATCH] Last updates for the RDP compositor
Pekka Paalanen
ppaalanen at gmail.com
Tue May 21 23:32:18 PDT 2013
On Tue, 21 May 2013 23:53:53 +0200
Hardening <rdp.effort at gmail.com> wrote:
> This patch fixes the compilation of the RDP compositor with the head of the
> FreeRDP project. It also brings the following improvements/fixes:
> * the fake seat as been dropped as now a compositor can be safely started
> without any seat
> * fixed a wrong initialisation of the NSC encoder context
> * the first screen update is now sent on postConnect, not on Synchronize packets
> as not all clients send then (this is the case for the last version of FreeRDP).
> In the specs, Synchronize packets are described as old artifacts that SHOULD be
> ignored.
> * send frame markers when using raw surfaces
> * reworked raw surfaces sending so that the subtile and the image flip are done
> in one step (instead of computing the subtile and then flip it)
> * we now send all the sub-rectangles instead of sending the full bounding box
> * the negociated size for the fragmentation buffer is honored when sending raw
> surfaces PDUs
> * always send using the preferred codec without caring about the size
Hi,
by the description, sounds like this patch should actually be split
into 8 separate patches, each doing one logical change. If you cannot
write a commit title, that identifies the exact changes a patch makes,
and instead you have to blurt out like "lotsa changes" or "A, B, C, and
D", that's an indication for a need to split.
Granted, this patch is not huge, and the individual patches would be
tiny, but size is rarely a reason for a certain split or squash.
Just a thing worth keeping in mind.
A few nitpicks below, to show that I at least looked through this
patch. ;-)
> ---
> src/compositor-rdp.c | 184 ++++++++++++++++++++++++++++++---------------------
> 1 file changed, 108 insertions(+), 76 deletions(-)
>
> diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
> index 0dae963..7eec273 100644
> --- a/src/compositor-rdp.c
> +++ b/src/compositor-rdp.c
> @@ -59,7 +59,6 @@ struct rdp_output;
>
> struct rdp_compositor {
> struct weston_compositor base;
> - struct weston_seat main_seat;
>
> freerdp_listener *listener;
> struct wl_event_source *listener_events[MAX_FREERDP_FDS];
> @@ -133,8 +132,8 @@ rdp_peer_refresh_rfx(pixman_region32_t *damage, pixman_image_t *image, freerdp_p
> SURFACE_BITS_COMMAND *cmd = &update->surface_bits_command;
> RdpPeerContext *context = (RdpPeerContext *)peer->context;
>
> - stream_clear(context->encode_stream);
> - stream_set_pos(context->encode_stream, 0);
> + Stream_Clear(context->encode_stream);
> + Stream_SetPosition(context->encode_stream, 0);
>
> width = (damage->extents.x2 - damage->extents.x1);
> height = (damage->extents.y2 - damage->extents.y1);
> @@ -169,8 +168,8 @@ rdp_peer_refresh_rfx(pixman_region32_t *damage, pixman_image_t *image, freerdp_p
> pixman_image_get_stride(image)
> );
>
> - cmd->bitmapDataLength = stream_get_length(context->encode_stream);
> - cmd->bitmapData = stream_get_head(context->encode_stream);
> + cmd->bitmapDataLength = Stream_GetPosition(context->encode_stream);
> + cmd->bitmapData = Stream_Buffer(context->encode_stream);
>
> update->SurfaceBits(update->context, cmd);
> }
> @@ -185,8 +184,8 @@ rdp_peer_refresh_nsc(pixman_region32_t *damage, pixman_image_t *image, freerdp_p
> SURFACE_BITS_COMMAND *cmd = &update->surface_bits_command;
> RdpPeerContext *context = (RdpPeerContext *)peer->context;
>
> - stream_clear(context->encode_stream);
> - stream_set_pos(context->encode_stream, 0);
> + Stream_Clear(context->encode_stream);
> + Stream_SetPosition(context->encode_stream, 0);
>
> width = (damage->extents.x2 - damage->extents.x1);
> height = (damage->extents.y2 - damage->extents.y1);
> @@ -206,42 +205,79 @@ rdp_peer_refresh_nsc(pixman_region32_t *damage, pixman_image_t *image, freerdp_p
> nsc_compose_message(context->nsc_context, context->encode_stream, (BYTE *)ptr,
> cmd->width, cmd->height,
> pixman_image_get_stride(image));
> - cmd->bitmapDataLength = stream_get_length(context->encode_stream);
> - cmd->bitmapData = stream_get_head(context->encode_stream);
> + cmd->bitmapDataLength = Stream_GetPosition(context->encode_stream);
> + cmd->bitmapData = Stream_Buffer(context->encode_stream);
> update->SurfaceBits(update->context, cmd);
> }
>
> static void
> +pixman_image_flipped_subrect(const pixman_box32_t *rect, pixman_image_t *img, BYTE *dest) {
> + int stride = pixman_image_get_stride(img);
> + int h;
> + int toCopy = (rect->x2 - rect->x1) * 4;
> + int height = (rect->y2 - rect->y1);
> + const BYTE *src = (const BYTE *)pixman_image_get_data(img);
> + src += ((rect->y2-1) * stride) + (rect->x1 * 4);
> +
> + for(h = 0; h < height; h++, src -= stride, dest += toCopy)
Missing space.
> + memcpy(dest, src, toCopy);
> +}
I wonder if there was a pixman subrect copy function, that could
achieve the same, using negative stride, maybe? Well, might be more
hassle to set it up than do manually.
> +static void
> rdp_peer_refresh_raw(pixman_region32_t *region, pixman_image_t *image, freerdp_peer *peer)
> {
> - pixman_image_t *tile;
> rdpUpdate *update = peer->update;
> SURFACE_BITS_COMMAND *cmd = &update->surface_bits_command;
> - pixman_box32_t *extends = pixman_region32_extents(region);
> + SURFACE_FRAME_MARKER *marker = &update->surface_frame_marker;
> + pixman_box32_t *rect, subrect;
> + int nrects, i;
> + int heightIncrement, remainingHeight, top;
> +
> + rect = pixman_region32_rectangles(region, &nrects);
> + if(!nrects)
Missing space.
> + return;
> +
> + marker->frameId++;
> + marker->frameAction = SURFACECMD_FRAMEACTION_BEGIN;
> + update->SurfaceFrameMarker(peer->context, marker);
>
> cmd->bpp = 32;
> cmd->codecID = 0;
> - cmd->width = (extends->x2 - extends->x1);
> - cmd->height = (extends->y2 - extends->y1);;
> - cmd->bitmapDataLength = cmd->width * cmd->height * 4;
> - tile = pixman_image_create_bits(PIXMAN_x8r8g8b8, cmd->width, cmd->height, 0, cmd->width * 4);
> - pixman_image_composite32(PIXMAN_OP_SRC, image, NULL, /* op, src, mask */
> - tile, extends->x1, extends->y1, /* dest, src_x, src_y */
> - 0, 0, /* mask_x, mask_y */
> - 0, 0, /* dest_x, dest_y */
> - cmd->width, cmd->height /* width, height */
> - );
> - freerdp_image_flip((BYTE *)pixman_image_get_data(tile),
> - (BYTE *)pixman_image_get_data(tile),
> - cmd->width, cmd->height, cmd->bpp
> - );
> - cmd->bitmapData = (BYTE *)pixman_image_get_data(tile);
> - cmd->destLeft = extends->x1;
> - cmd->destTop = extends->y1;
> - cmd->destRight = extends->x2;
> - cmd->destBottom = extends->y2;
> - update->SurfaceBits(peer->context, cmd);
> - pixman_image_unref(tile);
> +
> + for(i = 0; i < nrects; i++, rect++) {
Missing space.
> + /*weston_log("rect(%d,%d, %d,%d)\n", rect->x1, rect->y1, rect->x2, rect->y2);*/
> + cmd->destLeft = rect->x1;
> + cmd->destRight = rect->x2;
> + cmd->width = (rect->x2 - rect->x1);
Parentheses not needed.
> +
> + heightIncrement = peer->settings->MultifragMaxRequestSize / (16 + cmd->width * 4);
> + remainingHeight = (rect->y2 - rect->y1);
Parentheses not needed.
> + top = rect->y1;
> +
> + subrect.x1 = rect->x1;
> + subrect.x2 = rect->x2;
> +
> + while(remainingHeight) {
Missing space.
> + cmd->height = (remainingHeight > heightIncrement) ? heightIncrement : remainingHeight;
> + cmd->destTop = top;
> + cmd->destBottom = top + cmd->height;
> + cmd->bitmapDataLength = cmd->width * cmd->height * 4;
> + cmd->bitmapData = (BYTE *)realloc(cmd->bitmapData, cmd->bitmapDataLength);
> +
> + subrect.y1 = top;
> + subrect.y2 = top + cmd->height;
> + pixman_image_flipped_subrect(&subrect, image, cmd->bitmapData);
> +
> + /*weston_log("* sending (%d,%d, %d,%d)\n", subrect.x1, subrect.y1, subrect.x2, subrect.y2); */
> + update->SurfaceBits(peer->context, cmd);
> +
> + remainingHeight -= cmd->height;
> + top += cmd->height;
> + }
> + }
> +
> + marker->frameAction = SURFACECMD_FRAMEACTION_END;
> + update->SurfaceFrameMarker(peer->context, marker);
> }
>
> static void
> @@ -250,20 +286,13 @@ rdp_peer_refresh_region(pixman_region32_t *region, freerdp_peer *peer)
> RdpPeerContext *context = (RdpPeerContext *)peer->context;
> struct rdp_output *output = context->rdpCompositor->output;
> rdpSettings *settings = peer->settings;
> - pixman_box32_t *extents = pixman_region32_extents(region);
>
> - int regionSz = (extents->x2 - extents->x1) * (extents->y2 - extents->y1);
> -
> - if(regionSz > 64 * 64) {
> - if(settings->RemoteFxCodec)
> - rdp_peer_refresh_rfx(region, output->shadow_surface, peer);
> - else if(settings->NSCodec)
> - rdp_peer_refresh_nsc(region, output->shadow_surface, peer);
> - else
> - rdp_peer_refresh_raw(region, output->shadow_surface, peer);
> - } else {
> + if(settings->RemoteFxCodec)
> + rdp_peer_refresh_rfx(region, output->shadow_surface, peer);
> + else if(settings->NSCodec)
Missing spaces.
> + rdp_peer_refresh_nsc(region, output->shadow_surface, peer);
> + else
> rdp_peer_refresh_raw(region, output->shadow_surface, peer);
> - }
> }
>
> static void
> @@ -452,7 +481,7 @@ rdp_compositor_create_output(struct rdp_compositor *c, int width, int height,
> width, height,
> NULL,
> width * 4);
> - if (output->shadow_surface == NULL) {
> + if (!output->shadow_surface) {
> weston_log("Failed to create surface for frame buffer.\n");
> goto out_output;
> }
> @@ -498,9 +527,7 @@ rdp_restore(struct weston_compositor *ec)
> static void
> rdp_destroy(struct weston_compositor *ec)
> {
> - struct rdp_compositor *c = (struct rdp_compositor *) ec;
> -
> - weston_seat_release(&c->main_seat);
> + //struct rdp_compositor *c = (struct rdp_compositor *) ec;
Does some commented-out debug code need this commented-out line? If
not, can be removed.
>
> ec->renderer->destroy(ec);
> weston_compositor_shutdown(ec);
> @@ -519,6 +546,7 @@ int rdp_listener_activity(int fd, uint32_t mask, void *data) {
> weston_log("failed to check FreeRDP file descriptor\n");
> return -1;
> }
> +
> return 0;
> }
>
> @@ -560,9 +588,10 @@ rdp_peer_context_new(freerdp_peer* client, RdpPeerContext* context)
> rfx_context_set_pixel_format(context->rfx_context, RDP_PIXEL_FORMAT_B8G8R8A8);
>
> context->nsc_context = nsc_context_new();
> - rfx_context_set_pixel_format(context->rfx_context, RDP_PIXEL_FORMAT_B8G8R8A8);
> + nsc_context_set_pixel_format(context->nsc_context, RDP_PIXEL_FORMAT_B8G8R8A8);
>
> - context->encode_stream = stream_new(65536);
> + context->encode_stream = Stream_New(NULL, 65536);
> + Stream_Clear(context->encode_stream);
> }
>
> static void
> @@ -580,7 +609,8 @@ rdp_peer_context_free(freerdp_peer* client, RdpPeerContext* context)
>
> if(context->item.flags & RDP_PEER_ACTIVATED)
> weston_seat_release(&context->item.seat);
> - stream_free(context->encode_stream);
> +
> + Stream_Free(context->encode_stream, TRUE);
> nsc_context_free(context->nsc_context);
> rfx_context_free(context->rfx_context);
> free(context->rfx_rects);
> @@ -598,6 +628,7 @@ rdp_client_activity(int fd, uint32_t mask, void *data) {
> return 0;
>
> out_clean:
> + client->Disconnect(client);
> freerdp_peer_context_free(client);
> freerdp_peer_free(client);
> return 0;
> @@ -649,6 +680,9 @@ xf_peer_post_connect(freerdp_peer* client)
> struct xkb_rule_names xkbRuleNames;
> struct xkb_keymap *keymap;
> int i;
> + rdpPointerUpdate *pointer;
> + pixman_box32_t box;
> + pixman_region32_t damage;
>
> peerCtx = (RdpPeerContext *)client->context;
> c = peerCtx->rdpCompositor;
> @@ -702,12 +736,32 @@ xf_peer_post_connect(freerdp_peer* client)
> weston_seat_init_pointer(&peerCtx->item.seat);
>
> peerCtx->item.flags |= RDP_PEER_ACTIVATED;
> +
> +
> +
> + /* disable pointer on the client side */
> + pointer = client->update->pointer;
> + pointer->pointer_system.type = SYSPTR_NULL;
> + pointer->PointerSystem(client->context, &pointer->pointer_system);
> +
> + /* sends a full refresh */
> + box.x1 = 0;
> + box.y1 = 0;
> + box.x2 = output->base.width;
> + box.y2 = output->base.height;
> + pixman_region32_init_with_extents(&damage, &box);
> +
> + rdp_peer_refresh_region(&damage, client);
> + pixman_region32_fini(&damage);
> +
> return TRUE;
> }
>
> static BOOL
> xf_peer_activate(freerdp_peer *client)
> {
> + RdpPeerContext *context = (RdpPeerContext *)client->context;
> + rfx_context_reset(context->rfx_context);
> return TRUE;
> }
>
> @@ -761,27 +815,6 @@ xf_extendedMouseEvent(rdpInput *input, UINT16 flags, UINT16 x, UINT16 y) {
> static void
> xf_input_synchronize_event(rdpInput *input, UINT32 flags)
> {
> - freerdp_peer *client = input->context->peer;
> - rdpPointerUpdate *pointer = client->update->pointer;
> - RdpPeerContext *peerCtx = (RdpPeerContext *)input->context;
> - struct rdp_output *output = peerCtx->rdpCompositor->output;
> - pixman_box32_t box;
> - pixman_region32_t damage;
> -
> - /* disable pointer on the client side */
> - pointer->pointer_system.type = SYSPTR_NULL;
> - pointer->PointerSystem(client->context, &pointer->pointer_system);
> -
> - /* sends a full refresh */
> - box.x1 = 0;
> - box.y1 = 0;
> - box.x2 = output->base.width;
> - box.y2 = output->base.height;
> - pixman_region32_init_with_extents(&damage, &box);
> -
> - rdp_peer_refresh_region(&damage, client);
> -
> - pixman_region32_fini(&damage);
> }
>
> extern DWORD KEYCODE_TO_VKCODE_EVDEV[];
> @@ -944,9 +977,9 @@ rdp_compositor_create(struct wl_display *display,
> config_fd) < 0)
> goto err_free;
>
> - weston_seat_init(&c->main_seat, &c->base);
> c->base.destroy = rdp_destroy;
> c->base.restore = rdp_restore;
> + c->base.focus = 1;
> c->rdp_key = config->rdp_key ? strdup(config->rdp_key) : NULL;
>
> /* activate TLS only if certificate/key are available */
> @@ -1004,7 +1037,6 @@ err_free_strings:
> free(c->server_cert);
> if(c->server_key)
> free(c->server_key);
> - weston_seat_release(&c->main_seat);
> err_free:
> free(c);
> return NULL;
> @@ -1012,7 +1044,7 @@ err_free:
>
> WL_EXPORT struct weston_compositor *
> backend_init(struct wl_display *display, int *argc, char *argv[],
> - const char *config_file)
> + int config_fd)
> {
> struct rdp_compositor_config config;
> rdp_compositor_config_init(&config);
> @@ -1035,5 +1067,5 @@ backend_init(struct wl_display *display, int *argc, char *argv[],
> };
>
> parse_options(rdp_options, ARRAY_LENGTH(rdp_options), argc, argv);
> - return rdp_compositor_create(display, &config, argc, argv, config_file);
> + return rdp_compositor_create(display, &config, argc, argv, config_fd);
> }
Thanks,
pq
More information about the wayland-devel
mailing list