From c4693009413905ebf77c94cdba94d4e42dcc57f5 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 5 Jul 2021 16:54:31 -0700 Subject: [PATCH 01/15] GUACAMOLE-377: Support for RDPGFX. --- src/protocols/rdp/Makefile.am | 2 + src/protocols/rdp/channels/rdpgfx.c | 122 ++++++++++++++++++++++++++++ src/protocols/rdp/channels/rdpgfx.h | 49 +++++++++++ src/protocols/rdp/rdp.c | 22 +++-- src/protocols/rdp/settings.c | 11 +++ 5 files changed, 197 insertions(+), 9 deletions(-) create mode 100644 src/protocols/rdp/channels/rdpgfx.c create mode 100644 src/protocols/rdp/channels/rdpgfx.h diff --git a/src/protocols/rdp/Makefile.am b/src/protocols/rdp/Makefile.am index 839cc9af..90404b71 100644 --- a/src/protocols/rdp/Makefile.am +++ b/src/protocols/rdp/Makefile.am @@ -57,6 +57,7 @@ libguac_client_rdp_la_SOURCES = \ channels/rdpdr/rdpdr-printer.c \ channels/rdpdr/rdpdr.c \ channels/rdpei.c \ + channels/rdpgfx.c \ channels/rdpsnd/rdpsnd-messages.c \ channels/rdpsnd/rdpsnd.c \ client.c \ @@ -103,6 +104,7 @@ noinst_HEADERS = \ channels/rdpdr/rdpdr-printer.h \ channels/rdpdr/rdpdr.h \ channels/rdpei.h \ + channels/rdpgfx.h \ channels/rdpsnd/rdpsnd-messages.h \ channels/rdpsnd/rdpsnd.h \ client.h \ diff --git a/src/protocols/rdp/channels/rdpgfx.c b/src/protocols/rdp/channels/rdpgfx.c new file mode 100644 index 00000000..f22ffbf3 --- /dev/null +++ b/src/protocols/rdp/channels/rdpgfx.c @@ -0,0 +1,122 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "channels/rdpgfx.h" +#include "plugins/channels.h" +#include "rdp.h" +#include "settings.h" + +#include +#include +#include +#include +#include + +#include +#include + +/** + * Callback which associates handlers specific to Guacamole with the + * RdpgfxClientContext instance allocated by FreeRDP to deal with received + * RDPGFX (Graphics Pipeline) messages. + * + * This function is called whenever a channel connects via the PubSub event + * system within FreeRDP, but only has any effect if the connected channel is + * the RDPGFX channel. This specific callback is registered with the + * PubSub system of the relevant rdpContext when guac_rdp_rdpgfx_load_plugin() is + * called. + * + * @param context + * The rdpContext associated with the active RDP session. + * + * @param e + * Event-specific arguments, mainly the name of the channel, and a + * reference to the associated plugin loaded for that channel by FreeRDP. + */ +static void guac_rdp_rdpgfx_channel_connected(rdpContext* context, + ChannelConnectedEventArgs* e) { + + guac_client* client = ((rdp_freerdp_context*) context)->client; + + /* Ignore connection event if it's not for the RDPGFX channel */ + if (strcmp(e->name, RDPGFX_DVC_CHANNEL_NAME) != 0) + return; + + /* Init GDI-backed support for the Graphics Pipeline */ + RdpgfxClientContext* rdpgfx = (RdpgfxClientContext*) e->pInterface; + rdpGdi* gdi = context->gdi; + + if (!gdi_graphics_pipeline_init(gdi, rdpgfx)) + guac_client_log(client, GUAC_LOG_WARNING, "Rendering backend for RDPGFX " + "channel could not be loaded. Graphics may not render at all!"); + else + guac_client_log(client, GUAC_LOG_DEBUG, "RDPGFX channel will be used for " + "the RDP Graphics Pipeline Extension."); + +} + +/** + * Callback which handles any RDPGFX cleanup specific to Guacamole. + * + * This function is called whenever a channel disconnects via the PubSub event + * system within FreeRDP, but only has any effect if the disconnected channel + * is the RDPGFX channel. This specific callback is registered with the PubSub + * system of the relevant rdpContext when guac_rdp_rdpgfx_load_plugin() is + * called. + * + * @param context + * The rdpContext associated with the active RDP session. + * + * @param e + * Event-specific arguments, mainly the name of the channel, and a + * reference to the associated plugin loaded for that channel by FreeRDP. + */ +static void guac_rdp_rdpgfx_channel_disconnected(rdpContext* context, + ChannelDisconnectedEventArgs* e) { + + guac_client* client = ((rdp_freerdp_context*) context)->client; + + /* Ignore disconnection event if it's not for the RDPGFX channel */ + if (strcmp(e->name, RDPGFX_DVC_CHANNEL_NAME) != 0) + return; + + /* Un-init GDI-backed support for the Graphics Pipeline */ + RdpgfxClientContext* rdpgfx = (RdpgfxClientContext*) e->pInterface; + rdpGdi* gdi = context->gdi; + gdi_graphics_pipeline_uninit(gdi, rdpgfx); + + guac_client_log(client, GUAC_LOG_DEBUG, "RDPGFX channel support unloaded."); + +} + +void guac_rdp_rdpgfx_load_plugin(rdpContext* context) { + + /* Subscribe to and handle channel connected events */ + PubSub_SubscribeChannelConnected(context->pubSub, + (pChannelConnectedEventHandler) guac_rdp_rdpgfx_channel_connected); + + /* Subscribe to and handle channel disconnected events */ + PubSub_SubscribeChannelDisconnected(context->pubSub, + (pChannelDisconnectedEventHandler) guac_rdp_rdpgfx_channel_disconnected); + + /* Add "rdpgfx" channel */ + guac_freerdp_dynamic_channel_collection_add(context->settings, "rdpgfx", NULL); + +} + diff --git a/src/protocols/rdp/channels/rdpgfx.h b/src/protocols/rdp/channels/rdpgfx.h new file mode 100644 index 00000000..baeec876 --- /dev/null +++ b/src/protocols/rdp/channels/rdpgfx.h @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#ifndef GUAC_RDP_CHANNELS_RDPGFX_H +#define GUAC_RDP_CHANNELS_RDPGFX_H + +#include "settings.h" + +#include +#include +#include + +/** + * Adds FreeRDP's "rdpgfx" plugin to the list of dynamic virtual channel plugins + * to be loaded by FreeRDP's "drdynvc" plugin. The context of the plugin will + * automatically be assicated with the guac_rdp_rdpgfx instance pointed to by the + * current guac_rdp_client. The plugin will only be loaded once the "drdynvc" + * plugin is loaded. The "rdpgfx" plugin ultimately adds support for the RDP + * Graphics Pipeline Extension. + * + * If failures occur, messages noting the specifics of those failures will be + * logged. + * + * This MUST be called within the PreConnect callback of the freerdp instance + * for Graphics Pipeline support to be loaded. + * + * @param context + * The rdpContext associated with the active RDP session. + */ +void guac_rdp_rdpgfx_load_plugin(rdpContext* context); + +#endif + diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c index 5905f8e6..b37e2633 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -28,6 +28,7 @@ #include "channels/rail.h" #include "channels/rdpdr/rdpdr.h" #include "channels/rdpei.h" +#include "channels/rdpgfx.h" #include "channels/rdpsnd/rdpsnd.h" #include "client.h" #include "color.h" @@ -137,15 +138,6 @@ BOOL rdp_freerdp_pre_connect(freerdp* instance) { } - /* Load plugin providing Dynamic Virtual Channel support, if required */ - if (instance->settings->SupportDynamicChannels && - guac_freerdp_channels_load_plugin(context, "drdynvc", - instance->settings)) { - guac_client_log(client, GUAC_LOG_WARNING, - "Failed to load drdynvc plugin. Display update and audio " - "input support will be disabled."); - } - /* Init FreeRDP internal GDI implementation */ if (!gdi_init(instance, guac_rdp_get_native_pixel_format(FALSE))) return FALSE; @@ -204,6 +196,18 @@ BOOL rdp_freerdp_pre_connect(freerdp* instance) { offscreen_cache_register_callbacks(instance->update); palette_cache_register_callbacks(instance->update); + /* Load "rdpgfx" plugin for Graphics Pipeline Extension */ + guac_rdp_rdpgfx_load_plugin(context); + + /* Load plugin providing Dynamic Virtual Channel support, if required */ + if (instance->settings->SupportDynamicChannels && + guac_freerdp_channels_load_plugin(context, "drdynvc", + instance->settings)) { + guac_client_log(client, GUAC_LOG_WARNING, + "Failed to load drdynvc plugin. Display update and audio " + "input support will be disabled."); + } + return TRUE; } diff --git a/src/protocols/rdp/settings.c b/src/protocols/rdp/settings.c index 81dfedf9..feb6a00a 100644 --- a/src/protocols/rdp/settings.c +++ b/src/protocols/rdp/settings.c @@ -1397,6 +1397,17 @@ void guac_rdp_push_settings(guac_client* client, /* Explicitly set flag value */ rdp_settings->PerformanceFlags = guac_rdp_get_performance_flags(guac_settings); + rdp_settings->SupportGraphicsPipeline = TRUE; + rdp_settings->RemoteFxCodec = TRUE; + + /* Required for RemoteFX / Graphics Pipeline */ + rdp_settings->FastPathOutput = TRUE; + rdp_settings->FrameMarkerCommandEnabled = TRUE; + rdp_settings->ColorDepth = 32; + rdp_settings->SoftwareGdi = TRUE; + /*rdp_settings->GfxH264 = TRUE; + rdp_settings->GfxAVC444 = TRUE;*/ + /* Set individual flags - some FreeRDP versions overwrite the above */ rdp_settings->AllowFontSmoothing = guac_settings->font_smoothing_enabled; rdp_settings->DisableWallpaper = !guac_settings->wallpaper_enabled; From c795bf9e4a8060ceae00fe1f4efd3016033a91ee Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 2 Sep 2021 17:31:30 -0700 Subject: [PATCH 02/15] GUACAMOLE-377: Control RemoteFX / GFX support with "enable-gfx" parameter. --- src/protocols/rdp/rdp.c | 3 ++- src/protocols/rdp/settings.c | 39 +++++++++++++++++++++++++++--------- src/protocols/rdp/settings.h | 5 +++++ 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c index b37e2633..322465ac 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -197,7 +197,8 @@ BOOL rdp_freerdp_pre_connect(freerdp* instance) { palette_cache_register_callbacks(instance->update); /* Load "rdpgfx" plugin for Graphics Pipeline Extension */ - guac_rdp_rdpgfx_load_plugin(context); + if (settings->enable_gfx) + guac_rdp_rdpgfx_load_plugin(context); /* Load plugin providing Dynamic Virtual Channel support, if required */ if (instance->settings->SupportDynamicChannels && diff --git a/src/protocols/rdp/settings.c b/src/protocols/rdp/settings.c index feb6a00a..39089562 100644 --- a/src/protocols/rdp/settings.c +++ b/src/protocols/rdp/settings.c @@ -109,6 +109,7 @@ const char* GUAC_RDP_CLIENT_ARGS[] = { "create-recording-path", "resize-method", "enable-audio-input", + "enable-gfx", "enable-touch", "read-only", @@ -539,6 +540,12 @@ enum RDP_ARGS_IDX { */ IDX_ENABLE_AUDIO_INPUT, + /** + * "true" if the RDP Graphics Pipeline Extension should be used, "false" or + * blank if traditional RDP graphics should be used instead. + */ + IDX_ENABLE_GFX, + /** * "true" if multi-touch support should be enabled for the RDP connection, * "false" or blank otherwise. @@ -1129,6 +1136,11 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user, settings->resize_method = GUAC_RESIZE_NONE; } + /* RDP Graphics Pipeline enable/disable */ + settings->enable_gfx = + guac_user_parse_args_boolean(user, GUAC_RDP_CLIENT_ARGS, argv, + IDX_ENABLE_GFX, 0); + /* Multi-touch input enable/disable */ settings->enable_touch = guac_user_parse_args_boolean(user, GUAC_RDP_CLIENT_ARGS, argv, @@ -1397,16 +1409,25 @@ void guac_rdp_push_settings(guac_client* client, /* Explicitly set flag value */ rdp_settings->PerformanceFlags = guac_rdp_get_performance_flags(guac_settings); - rdp_settings->SupportGraphicsPipeline = TRUE; - rdp_settings->RemoteFxCodec = TRUE; + /* Enable RemoteFX / Graphics Pipeline */ + if (guac_settings->enable_gfx) { - /* Required for RemoteFX / Graphics Pipeline */ - rdp_settings->FastPathOutput = TRUE; - rdp_settings->FrameMarkerCommandEnabled = TRUE; - rdp_settings->ColorDepth = 32; - rdp_settings->SoftwareGdi = TRUE; - /*rdp_settings->GfxH264 = TRUE; - rdp_settings->GfxAVC444 = TRUE;*/ + rdp_settings->SupportGraphicsPipeline = TRUE; + rdp_settings->RemoteFxCodec = TRUE; + + if (rdp_settings->ColorDepth != 32) { + guac_client_log(client, GUAC_LOG_WARNING, "Ignoring requested " + "color depth of %i bpp, as the RDP Graphics Pipeline " + "requires 32 bpp.", rdp_settings->ColorDepth); + } + + /* Required for RemoteFX / Graphics Pipeline */ + rdp_settings->FastPathOutput = TRUE; + rdp_settings->FrameMarkerCommandEnabled = TRUE; + rdp_settings->ColorDepth = 32; + rdp_settings->SoftwareGdi = TRUE; + + } /* Set individual flags - some FreeRDP versions overwrite the above */ rdp_settings->AllowFontSmoothing = guac_settings->font_smoothing_enabled; diff --git a/src/protocols/rdp/settings.h b/src/protocols/rdp/settings.h index daaf3e9c..61662c35 100644 --- a/src/protocols/rdp/settings.h +++ b/src/protocols/rdp/settings.h @@ -552,6 +552,11 @@ typedef struct guac_rdp_settings { */ int enable_audio_input; + /** + * Whether the RDP Graphics Pipeline Extension is enabled. + */ + int enable_gfx; + /** * Whether multi-touch support is enabled. */ From dd85c549615cb3c9699df0da83d24587e383eff0 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Tue, 13 Jul 2021 00:08:39 -0700 Subject: [PATCH 03/15] GUACAMOLE-377: Add handling for EndPaint required by software GDI implementation of RDPGFX. --- src/protocols/rdp/gdi.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/protocols/rdp/gdi.c b/src/protocols/rdp/gdi.c index 1cbee144..4ecbe396 100644 --- a/src/protocols/rdp/gdi.c +++ b/src/protocols/rdp/gdi.c @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -372,8 +373,37 @@ BOOL guac_rdp_gdi_set_bounds(rdpContext* context, const rdpBounds* bounds) { } BOOL guac_rdp_gdi_end_paint(rdpContext* context) { - /* IGNORE */ + + guac_client* client = ((rdp_freerdp_context*) context)->client; + guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + rdpGdi* gdi = context->gdi; + + /* Ignore paint if GDI output is suppressed */ + if (gdi->suppressOutput) + return TRUE; + + /* Ignore paint if nothing has been done (empty rect) */ + if (gdi->primary->hdc->hwnd->invalid->null) + return TRUE; + + INT32 x = gdi->primary->hdc->hwnd->invalid->x; + INT32 y = gdi->primary->hdc->hwnd->invalid->y; + UINT32 w = gdi->primary->hdc->hwnd->invalid->w; + UINT32 h = gdi->primary->hdc->hwnd->invalid->h; + + /* Create surface from image data */ + cairo_surface_t* surface = cairo_image_surface_create_for_data( + gdi->primary_buffer + 4*x + y*gdi->stride, + CAIRO_FORMAT_RGB24, w, h, gdi->stride); + + /* Send surface to buffer */ + guac_common_surface_draw(rdp_client->display->default_surface, x, y, surface); + + /* Free surface */ + cairo_surface_destroy(surface); + return TRUE; + } BOOL guac_rdp_gdi_desktop_resize(rdpContext* context) { From c19eab9691cd7c048a56171db401712ab0b7c1c8 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Tue, 31 Aug 2021 10:47:35 -0700 Subject: [PATCH 04/15] GUACAMOLE-377: Revise processing lag calculations to consider cumulative processing lag. --- src/libguac/user-handlers.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/libguac/user-handlers.c b/src/libguac/user-handlers.c index c5cad9df..156b039e 100644 --- a/src/libguac/user-handlers.c +++ b/src/libguac/user-handlers.c @@ -120,31 +120,39 @@ int __guac_handle_sync(guac_user* user, int argc, char** argv) { /* Calculate length of frame, including network and processing lag */ frame_duration = current - timestamp; - /* Update lag statistics if at least one frame has been rendered */ + /* Calculate processing lag portion of length of frame */ + int frame_processing_lag = 0; if (user->last_frame_duration != 0) { /* Calculate lag using the previous frame as a baseline */ - int processing_lag = frame_duration - user->last_frame_duration; + frame_processing_lag = frame_duration - user->last_frame_duration; /* Adjust back to zero if cumulative error leads to a negative * value */ - if (processing_lag < 0) - processing_lag = 0; - - user->processing_lag = processing_lag; + if (frame_processing_lag < 0) + frame_processing_lag = 0; } - /* Record baseline duration of frame by excluding lag */ - user->last_frame_duration = frame_duration - user->processing_lag; + /* Record baseline duration of frame by excluding lag (this is the + * network round-trip time) */ + int estimated_rtt = frame_duration - frame_processing_lag; + user->last_frame_duration = estimated_rtt; + + /* Calculate cumulative accumulated processing lag relative to server timeline */ + int processing_lag = current - user->last_received_timestamp - estimated_rtt; + if (processing_lag < 0) + processing_lag = 0; + + user->processing_lag = processing_lag; } /* Log received timestamp and calculated lag (at TRACE level only) */ guac_user_log(user, GUAC_LOG_TRACE, "User confirmation of frame %" PRIu64 "ms received " - "at %" PRIu64 "ms (processing_lag=%ims)", - timestamp, current, user->processing_lag); + "at %" PRIu64 "ms (processing_lag=%ims, estimated_rtt=%ims)", + timestamp, current, user->processing_lag, user->last_frame_duration); if (user->sync_handler) return user->sync_handler(user, timestamp); From 52c8683bcf35a74edd1bcefbe5005746821df36c Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 2 Sep 2021 16:53:55 -0700 Subject: [PATCH 05/15] GUACAMOLE-377: Add protocol-level support for reporting remote frame statistics. --- src/libguac/client.c | 8 +++++-- src/libguac/guacamole/client.h | 39 ++++++++++++++++++++++++++++---- src/libguac/guacamole/protocol.h | 19 ++++++++++++---- src/libguac/protocol.c | 5 +++- 4 files changed, 59 insertions(+), 12 deletions(-) diff --git a/src/libguac/client.c b/src/libguac/client.c index 9738c0c2..80ba1741 100644 --- a/src/libguac/client.c +++ b/src/libguac/client.c @@ -413,15 +413,19 @@ void* guac_client_for_user(guac_client* client, guac_user* user, } int guac_client_end_frame(guac_client* client) { + return guac_client_end_multiple_frames(client, 0); +} + +int guac_client_end_multiple_frames(guac_client* client, int frames) { /* Update and send timestamp */ client->last_sent_timestamp = guac_timestamp_current(); /* Log received timestamp and calculated lag (at TRACE level only) */ guac_client_log(client, GUAC_LOG_TRACE, "Server completed " - "frame %" PRIu64 "ms.", client->last_sent_timestamp); + "frame %" PRIu64 "ms (%i logical frames)", client->last_sent_timestamp, frames); - return guac_protocol_send_sync(client->socket, client->last_sent_timestamp); + return guac_protocol_send_sync(client->socket, client->last_sent_timestamp, frames); } diff --git a/src/libguac/guacamole/client.h b/src/libguac/guacamole/client.h index 4ffe5cf4..b94f8418 100644 --- a/src/libguac/guacamole/client.h +++ b/src/libguac/guacamole/client.h @@ -509,18 +509,47 @@ void* guac_client_for_user(guac_client* client, guac_user* user, guac_user_callback* callback, void* data); /** - * Marks the end of the current frame by sending a "sync" instruction to - * all connected users. This instruction will contain the current timestamp. - * The last_sent_timestamp member of guac_client will be updated accordingly. + * Marks the end of the current frame by sending a "sync" instruction to all + * connected users, where the number of input frames that were considered in + * creating this frame is either unknown or inapplicable. This instruction will + * contain the current timestamp. The last_sent_timestamp member of guac_client + * will be updated accordingly. * * If an error occurs sending the instruction, a non-zero value is * returned, and guac_error is set appropriately. * - * @param client The guac_client which has finished a frame. - * @return Zero on success, non-zero on error. + * @param client + * The guac_client which has finished a frame. + * + * @return + * Zero on success, non-zero on error. */ int guac_client_end_frame(guac_client* client); +/** + * Marks the end of the current frame by sending a "sync" instruction to all + * connected users, where that frame may combine or otherwise represent the + * changes of an arbitrary number of input frames. This instruction will + * contain the current timestamp, as well as the number of frames that were + * considered in creating that frame. The last_sent_timestamp member of + * guac_client will be updated accordingly. + * + * If an error occurs sending the instruction, a non-zero value is + * returned, and guac_error is set appropriately. + * + * @param client + * The guac_client which has finished a frame. + * + * @param frames + * The number of distinct frames that were considered or combined when + * generating the current frame, or zero if the boundaries of relevant + * frames are unknown. + * + * @return + * Zero on success, non-zero on error. + */ +int guac_client_end_multiple_frames(guac_client* client, int frames); + /** * Initializes the given guac_client using the initialization routine provided * by the plugin corresponding to the named protocol. This will automatically diff --git a/src/libguac/guacamole/protocol.h b/src/libguac/guacamole/protocol.h index 362351c5..1d3c883e 100644 --- a/src/libguac/guacamole/protocol.h +++ b/src/libguac/guacamole/protocol.h @@ -363,11 +363,22 @@ int guac_protocol_send_select(guac_socket* socket, const char* protocol); * If an error occurs sending the instruction, a non-zero value is * returned, and guac_error is set appropriately. * - * @param socket The guac_socket connection to use. - * @param timestamp The current timestamp (in milliseconds). - * @return Zero on success, non-zero on error. + * @param socket + * The guac_socket connection to use. + * + * @param timestamp + * The current timestamp (in milliseconds). + * + * @param frames + * The number of distinct frames that were considered or combined when + * generating the frame terminated by this instruction, or zero if the + * boundaries of relevant frames are unknown. + * + * @return + * Zero on success, non-zero on error. */ -int guac_protocol_send_sync(guac_socket* socket, guac_timestamp timestamp); +int guac_protocol_send_sync(guac_socket* socket, guac_timestamp timestamp, + int frames); /* OBJECT INSTRUCTIONS */ diff --git a/src/libguac/protocol.c b/src/libguac/protocol.c index 1c53c200..b7cd5e41 100644 --- a/src/libguac/protocol.c +++ b/src/libguac/protocol.c @@ -1181,7 +1181,8 @@ int guac_protocol_send_start(guac_socket* socket, const guac_layer* layer, } -int guac_protocol_send_sync(guac_socket* socket, guac_timestamp timestamp) { +int guac_protocol_send_sync(guac_socket* socket, guac_timestamp timestamp, + int frames) { int ret_val; @@ -1189,6 +1190,8 @@ int guac_protocol_send_sync(guac_socket* socket, guac_timestamp timestamp) { ret_val = guac_socket_write_string(socket, "4.sync,") || __guac_socket_write_length_int(socket, timestamp) + || guac_socket_write_string(socket, ",") + || __guac_socket_write_length_int(socket, frames) || guac_socket_write_string(socket, ";"); guac_socket_instruction_end(socket); From 669e02b4dc15d8e4b14139e94eb10de08fefffbb Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 2 Sep 2021 17:07:58 -0700 Subject: [PATCH 06/15] GUACAMOLE-377: Leverage RDPGFX to report remote frame statistics to the client. --- src/protocols/rdp/gdi.c | 33 +++++++++++++++++++++++++++++++++ src/protocols/rdp/gdi.h | 18 ++++++++++++++++-- src/protocols/rdp/rdp.c | 21 ++++++++++++++++++--- src/protocols/rdp/rdp.h | 21 +++++++++++++++++++++ 4 files changed, 88 insertions(+), 5 deletions(-) diff --git a/src/protocols/rdp/gdi.c b/src/protocols/rdp/gdi.c index 4ecbe396..61bb5476 100644 --- a/src/protocols/rdp/gdi.c +++ b/src/protocols/rdp/gdi.c @@ -372,12 +372,27 @@ BOOL guac_rdp_gdi_set_bounds(rdpContext* context, const rdpBounds* bounds) { } +BOOL guac_rdp_gdi_begin_paint(rdpContext* context) { + + guac_client* client = ((rdp_freerdp_context*) context)->client; + guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + + /* A new frame is beginning */ + rdp_client->in_frame = 1; + + return TRUE; + +} + BOOL guac_rdp_gdi_end_paint(rdpContext* context) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; rdpGdi* gdi = context->gdi; + /* The current frame has ended */ + rdp_client->in_frame = 0; + /* Ignore paint if GDI output is suppressed */ if (gdi->suppressOutput) return TRUE; @@ -396,12 +411,30 @@ BOOL guac_rdp_gdi_end_paint(rdpContext* context) { gdi->primary_buffer + 4*x + y*gdi->stride, CAIRO_FORMAT_RGB24, w, h, gdi->stride); + guac_timestamp frame_end = guac_timestamp_current(); + int time_elapsed = frame_end - rdp_client->frame_start; + /* Send surface to buffer */ guac_common_surface_draw(rdp_client->display->default_surface, x, y, surface); /* Free surface */ cairo_surface_destroy(surface); + /* A new frame has been received from the RDP server and processed */ + rdp_client->frames_received++; + + /* Flush a new frame if the client is ready for it */ + if (time_elapsed >= guac_client_get_processing_lag(client)) { + + guac_common_display_flush(rdp_client->display); + guac_client_end_multiple_frames(client, rdp_client->frames_received); + guac_socket_flush(client->socket); + + rdp_client->frame_start = frame_end; + rdp_client->frames_received = 0; + + } + return TRUE; } diff --git a/src/protocols/rdp/gdi.h b/src/protocols/rdp/gdi.h index 76735aa5..1cb23952 100644 --- a/src/protocols/rdp/gdi.h +++ b/src/protocols/rdp/gdi.h @@ -156,8 +156,22 @@ BOOL guac_rdp_gdi_opaquerect(rdpContext* context, BOOL guac_rdp_gdi_set_bounds(rdpContext* context, const rdpBounds* bounds); /** - * Handler called when a paint operation is complete. We don't actually - * use this, but FreeRDP requires it. Calling this function has no effect. + * Handler called when a paint operation is beginning. This function is + * expected to be called by the FreeRDP GDI implementation of RemoteFX when a + * new frame is beginning. + * + * @param context + * The rdpContext associated with the current RDP session. + * + * @return + * TRUE if successful, FALSE otherwise. + */ +BOOL guac_rdp_gdi_begin_paint(rdpContext* context); + +/** + * Handler called when a paint operation is complete. This function is + * expected to be called by the FreeRDP GDI implementation of RemoteFX when a + * new frame has been completed. * * @param context * The rdpContext associated with the current RDP session. diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c index 322465ac..3fc4468b 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -179,6 +179,7 @@ BOOL rdp_freerdp_pre_connect(freerdp* instance) { /* Set up GDI */ instance->update->DesktopResize = guac_rdp_gdi_desktop_resize; + instance->update->BeginPaint = guac_rdp_gdi_begin_paint; instance->update->EndPaint = guac_rdp_gdi_end_paint; instance->update->SetBounds = guac_rdp_gdi_set_bounds; @@ -530,6 +531,7 @@ static int guac_rdp_handle_connection(guac_client* client) { rdp_client->rdp_inst = rdp_inst; guac_timestamp last_frame_end = guac_timestamp_current(); + rdp_client->frame_start = guac_timestamp_current(); /* Signal that reconnect has been completed */ guac_rdp_disp_reconnect_complete(rdp_client->disp); @@ -569,6 +571,13 @@ static int guac_rdp_handle_connection(guac_client* client) { break; } + /* Continue handling inbound data if we are in the middle of an RDP frame */ + if (rdp_client->in_frame) { + wait_result = rdp_guac_client_wait_for_messages(client, GUAC_RDP_FRAME_START_TIMEOUT); + if (wait_result >= 0) + continue; + } + /* Calculate time remaining in frame */ frame_end = guac_timestamp_current(); frame_remaining = frame_start + GUAC_RDP_FRAME_DURATION @@ -612,11 +621,17 @@ static int guac_rdp_handle_connection(guac_client* client) { guac_client_abort(client, GUAC_PROTOCOL_STATUS_UPSTREAM_UNAVAILABLE, "Connection closed."); - /* Flush frame only if successful */ - else { + /* Flush frame only if successful and an RDP frame is not known to be + * in progress */ + else if (rdp_client->frames_received) { + guac_common_display_flush(rdp_client->display); - guac_client_end_frame(client); + guac_client_end_multiple_frames(client, rdp_client->frames_received); guac_socket_flush(client->socket); + + rdp_client->frame_start = guac_timestamp_current(); + rdp_client->frames_received = 0; + } } diff --git a/src/protocols/rdp/rdp.h b/src/protocols/rdp/rdp.h index 9daef80f..fe571c8b 100644 --- a/src/protocols/rdp/rdp.h +++ b/src/protocols/rdp/rdp.h @@ -91,6 +91,27 @@ typedef struct guac_rdp_client { */ guac_common_surface* current_surface; + /** + * Whether the RDP server has reported that a new frame is in progress, and + * we are now receiving updates relevant to that frame. + */ + int in_frame; + + /** + * The number of distinct frames received from the RDP server since last + * flush, if the RDP server supports reporting frame boundaries. If the RDP + * server does not support tracking frames, this will be zero. + */ + int frames_received; + + /** + * The server timestamp of the end of the last frame received from the RDP + * server, as returned by guac_timestamp_current(), if the RDP server + * supports reporting frame boundaries. If the RDP server does not support + * tracking frames, this will be zero. + */ + guac_timestamp frame_start; + /** * The current state of the keyboard with respect to the RDP session. */ From bde8cdee4666840e25bf1910fbccf4dcf947679c Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 2 Sep 2021 19:05:21 -0700 Subject: [PATCH 07/15] GUACAMOLE-377: Add general RDP support for frame markers. --- src/protocols/rdp/gdi.c | 83 +++++++++++++++++++++++++++--------- src/protocols/rdp/gdi.h | 48 ++++++++++++++++++++- src/protocols/rdp/rdp.c | 7 +-- src/protocols/rdp/rdp.h | 5 +++ src/protocols/rdp/settings.c | 5 ++- 5 files changed, 123 insertions(+), 25 deletions(-) diff --git a/src/protocols/rdp/gdi.c b/src/protocols/rdp/gdi.c index 61bb5476..7e39cc4a 100644 --- a/src/protocols/rdp/gdi.c +++ b/src/protocols/rdp/gdi.c @@ -372,13 +372,67 @@ BOOL guac_rdp_gdi_set_bounds(rdpContext* context, const rdpBounds* bounds) { } +void guac_rdp_gdi_mark_frame(rdpContext* context, int starting) { + + guac_client* client = ((rdp_freerdp_context*) context)->client; + guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + + /* The server supports defining explicit frames */ + rdp_client->frames_supported = 1; + + /* A new frame is beginning */ + if (starting) { + rdp_client->in_frame = 1; + return; + } + + /* The current frame has ended */ + guac_timestamp frame_end = guac_timestamp_current(); + int time_elapsed = frame_end - rdp_client->frame_start; + rdp_client->in_frame = 0; + + /* A new frame has been received from the RDP server and processed */ + rdp_client->frames_received++; + + /* Flush a new frame if the client is ready for it */ + if (time_elapsed >= guac_client_get_processing_lag(client)) { + + guac_common_display_flush(rdp_client->display); + guac_client_end_multiple_frames(client, rdp_client->frames_received); + guac_socket_flush(client->socket); + + rdp_client->frame_start = frame_end; + rdp_client->frames_received = 0; + + } + +} + +BOOL guac_rdp_gdi_frame_marker(rdpContext* context, const FRAME_MARKER_ORDER* frame_marker) { + guac_rdp_gdi_mark_frame(context, frame_marker->action == FRAME_START); + return TRUE; +} + +BOOL guac_rdp_gdi_surface_frame_marker(rdpContext* context, const SURFACE_FRAME_MARKER* surface_frame_marker) { + + guac_rdp_gdi_mark_frame(context, surface_frame_marker->frameAction == SURFACECMD_FRAMEACTION_END); + + if (context->settings->FrameAcknowledge > 0) + IFCALL(context->update->SurfaceFrameAcknowledge, context, + surface_frame_marker->frameId); + + return TRUE; + +} + BOOL guac_rdp_gdi_begin_paint(rdpContext* context) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; - /* A new frame is beginning */ - rdp_client->in_frame = 1; + /* Leverage BeginPaint handler to detect start of frame for RDPGFX channel */ + if (rdp_client->settings->enable_gfx) + guac_rdp_gdi_mark_frame(context, 1); return TRUE; @@ -390,8 +444,10 @@ BOOL guac_rdp_gdi_end_paint(rdpContext* context) { guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; rdpGdi* gdi = context->gdi; - /* The current frame has ended */ - rdp_client->in_frame = 0; + /* Leverage EndPaint handler to detect end of frame for RDPGFX channel + * (ignore otherwise) */ + if (!rdp_client->settings->enable_gfx) + return TRUE; /* Ignore paint if GDI output is suppressed */ if (gdi->suppressOutput) @@ -411,28 +467,15 @@ BOOL guac_rdp_gdi_end_paint(rdpContext* context) { gdi->primary_buffer + 4*x + y*gdi->stride, CAIRO_FORMAT_RGB24, w, h, gdi->stride); - guac_timestamp frame_end = guac_timestamp_current(); - int time_elapsed = frame_end - rdp_client->frame_start; - /* Send surface to buffer */ guac_common_surface_draw(rdp_client->display->default_surface, x, y, surface); /* Free surface */ cairo_surface_destroy(surface); - /* A new frame has been received from the RDP server and processed */ - rdp_client->frames_received++; - - /* Flush a new frame if the client is ready for it */ - if (time_elapsed >= guac_client_get_processing_lag(client)) { - - guac_common_display_flush(rdp_client->display); - guac_client_end_multiple_frames(client, rdp_client->frames_received); - guac_socket_flush(client->socket); - - rdp_client->frame_start = frame_end; - rdp_client->frames_received = 0; - + /* Next frame */ + if (gdi->inGfxFrame) { + guac_rdp_gdi_mark_frame(context, 0); } return TRUE; diff --git a/src/protocols/rdp/gdi.h b/src/protocols/rdp/gdi.h index 1cb23952..955bf7bd 100644 --- a/src/protocols/rdp/gdi.h +++ b/src/protocols/rdp/gdi.h @@ -155,10 +155,56 @@ BOOL guac_rdp_gdi_opaquerect(rdpContext* context, */ BOOL guac_rdp_gdi_set_bounds(rdpContext* context, const rdpBounds* bounds); +/** + * Notifies the internal GDI implementation that a frame is either starting or + * ending. If the frame is ending and the connected client is ready to receive + * a new frame, a new frame will be flushed to the client. + * + * @param context + * The rdpContext associated with the current RDP session. + * + * @param starting + * Non-zero if the frame in question is starting, zero if the frame is + * ending. + */ +void guac_rdp_gdi_mark_frame(rdpContext* context, int starting); + +/** + * Handler called when a frame boundary is received from the RDP server in the + * form of a frame marker command. Each frame boundary may be the beginning or + * the end of a frame. + * + * @param context + * The rdpContext associated with the current RDP session. + * + * @param frame_marker + * The received frame marker. + * + * @return + * TRUE if successful, FALSE otherwise. + */ +BOOL guac_rdp_gdi_frame_marker(rdpContext* context, const FRAME_MARKER_ORDER* frame_marker); + +/** + * Handler called when a frame boundary is received from the RDP server in the + * form of a surface frame marker. Each frame boundary may be the beginning or + * the end of a frame. + * + * @param context + * The rdpContext associated with the current RDP session. + * + * @param surface_frame_marker + * The received frame marker. + * + * @return + * TRUE if successful, FALSE otherwise. + */ +BOOL guac_rdp_gdi_surface_frame_marker(rdpContext* context, const SURFACE_FRAME_MARKER* surface_frame_marker); + /** * Handler called when a paint operation is beginning. This function is * expected to be called by the FreeRDP GDI implementation of RemoteFX when a - * new frame is beginning. + * new frame has started. * * @param context * The rdpContext associated with the current RDP session. diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c index 3fc4468b..f2a121b5 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -183,6 +183,9 @@ BOOL rdp_freerdp_pre_connect(freerdp* instance) { instance->update->EndPaint = guac_rdp_gdi_end_paint; instance->update->SetBounds = guac_rdp_gdi_set_bounds; + instance->update->SurfaceFrameMarker = guac_rdp_gdi_surface_frame_marker; + instance->update->altsec->FrameMarker = guac_rdp_gdi_frame_marker; + rdpPrimaryUpdate* primary = instance->update->primary; primary->DstBlt = guac_rdp_gdi_dstblt; primary->PatBlt = guac_rdp_gdi_patblt; @@ -623,15 +626,13 @@ static int guac_rdp_handle_connection(guac_client* client) { /* Flush frame only if successful and an RDP frame is not known to be * in progress */ - else if (rdp_client->frames_received) { - + else if (!rdp_client->frames_supported || rdp_client->frames_received) { guac_common_display_flush(rdp_client->display); guac_client_end_multiple_frames(client, rdp_client->frames_received); guac_socket_flush(client->socket); rdp_client->frame_start = guac_timestamp_current(); rdp_client->frames_received = 0; - } } diff --git a/src/protocols/rdp/rdp.h b/src/protocols/rdp/rdp.h index fe571c8b..509fa1ac 100644 --- a/src/protocols/rdp/rdp.h +++ b/src/protocols/rdp/rdp.h @@ -91,6 +91,11 @@ typedef struct guac_rdp_client { */ guac_common_surface* current_surface; + /** + * Whether the RDP server supports defining explicit frame boundaries. + */ + int frames_supported; + /** * Whether the RDP server has reported that a new frame is in progress, and * we are now receiving updates relevant to that frame. diff --git a/src/protocols/rdp/settings.c b/src/protocols/rdp/settings.c index 39089562..911bc75d 100644 --- a/src/protocols/rdp/settings.c +++ b/src/protocols/rdp/settings.c @@ -1409,6 +1409,10 @@ void guac_rdp_push_settings(guac_client* client, /* Explicitly set flag value */ rdp_settings->PerformanceFlags = guac_rdp_get_performance_flags(guac_settings); + /* Always request frame markers */ + rdp_settings->FrameMarkerCommandEnabled = TRUE; + rdp_settings->SurfaceFrameMarkerEnabled = TRUE; + /* Enable RemoteFX / Graphics Pipeline */ if (guac_settings->enable_gfx) { @@ -1423,7 +1427,6 @@ void guac_rdp_push_settings(guac_client* client, /* Required for RemoteFX / Graphics Pipeline */ rdp_settings->FastPathOutput = TRUE; - rdp_settings->FrameMarkerCommandEnabled = TRUE; rdp_settings->ColorDepth = 32; rdp_settings->SoftwareGdi = TRUE; From a0e9f6ed9ba7729d28d04d53c6a1d390abf17a23 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 2 Sep 2021 20:53:52 -0700 Subject: [PATCH 08/15] GUACAMOLE-377: Leverage client timestamp tracking for RDP frame duration. --- src/protocols/rdp/gdi.c | 6 +----- src/protocols/rdp/rdp.c | 11 +---------- src/protocols/rdp/rdp.h | 8 -------- 3 files changed, 2 insertions(+), 23 deletions(-) diff --git a/src/protocols/rdp/gdi.c b/src/protocols/rdp/gdi.c index 7e39cc4a..88aea7f9 100644 --- a/src/protocols/rdp/gdi.c +++ b/src/protocols/rdp/gdi.c @@ -388,7 +388,7 @@ void guac_rdp_gdi_mark_frame(rdpContext* context, int starting) { /* The current frame has ended */ guac_timestamp frame_end = guac_timestamp_current(); - int time_elapsed = frame_end - rdp_client->frame_start; + int time_elapsed = frame_end - client->last_sent_timestamp; rdp_client->in_frame = 0; /* A new frame has been received from the RDP server and processed */ @@ -396,14 +396,10 @@ void guac_rdp_gdi_mark_frame(rdpContext* context, int starting) { /* Flush a new frame if the client is ready for it */ if (time_elapsed >= guac_client_get_processing_lag(client)) { - guac_common_display_flush(rdp_client->display); guac_client_end_multiple_frames(client, rdp_client->frames_received); guac_socket_flush(client->socket); - - rdp_client->frame_start = frame_end; rdp_client->frames_received = 0; - } } diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c index f2a121b5..e7ec71c5 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -534,7 +534,6 @@ static int guac_rdp_handle_connection(guac_client* client) { rdp_client->rdp_inst = rdp_inst; guac_timestamp last_frame_end = guac_timestamp_current(); - rdp_client->frame_start = guac_timestamp_current(); /* Signal that reconnect has been completed */ guac_rdp_disp_reconnect_complete(rdp_client->disp); @@ -554,7 +553,6 @@ static int guac_rdp_handle_connection(guac_client* client) { if (wait_result > 0) { int processing_lag = guac_client_get_processing_lag(client); - guac_timestamp frame_start = guac_timestamp_current(); /* Read server messages until frame is built */ do { @@ -582,6 +580,7 @@ static int guac_rdp_handle_connection(guac_client* client) { } /* Calculate time remaining in frame */ + guac_timestamp frame_start = client->last_sent_timestamp; frame_end = guac_timestamp_current(); frame_remaining = frame_start + GUAC_RDP_FRAME_DURATION - frame_end; @@ -604,12 +603,6 @@ static int guac_rdp_handle_connection(guac_client* client) { } while (wait_result > 0); - /* Record end of frame, excluding server-side rendering time (we - * assume server-side rendering time will be consistent between any - * two subsequent frames, and that this time should thus be - * excluded from the required wait period of the next frame). */ - last_frame_end = frame_start; - } /* Test whether the RDP server is closing the connection */ @@ -630,8 +623,6 @@ static int guac_rdp_handle_connection(guac_client* client) { guac_common_display_flush(rdp_client->display); guac_client_end_multiple_frames(client, rdp_client->frames_received); guac_socket_flush(client->socket); - - rdp_client->frame_start = guac_timestamp_current(); rdp_client->frames_received = 0; } diff --git a/src/protocols/rdp/rdp.h b/src/protocols/rdp/rdp.h index 509fa1ac..9e3d3fe0 100644 --- a/src/protocols/rdp/rdp.h +++ b/src/protocols/rdp/rdp.h @@ -109,14 +109,6 @@ typedef struct guac_rdp_client { */ int frames_received; - /** - * The server timestamp of the end of the last frame received from the RDP - * server, as returned by guac_timestamp_current(), if the RDP server - * supports reporting frame boundaries. If the RDP server does not support - * tracking frames, this will be zero. - */ - guac_timestamp frame_start; - /** * The current state of the keyboard with respect to the RDP session. */ From 28396ae3455b6c362018ad43f0231d0b6e21e376 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 18 May 2022 11:58:30 -0700 Subject: [PATCH 09/15] GUACAMOLE-377: Expect explicit RDP frame boundaries only after at least one frame boundary has been received. --- src/protocols/rdp/gdi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/protocols/rdp/gdi.c b/src/protocols/rdp/gdi.c index 88aea7f9..075a382a 100644 --- a/src/protocols/rdp/gdi.c +++ b/src/protocols/rdp/gdi.c @@ -427,7 +427,7 @@ BOOL guac_rdp_gdi_begin_paint(rdpContext* context) { guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; /* Leverage BeginPaint handler to detect start of frame for RDPGFX channel */ - if (rdp_client->settings->enable_gfx) + if (rdp_client->settings->enable_gfx && rdp_client->frames_supported) guac_rdp_gdi_mark_frame(context, 1); return TRUE; From da80163e24dbf728f5c2e1245c23ded5f629917e Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 18 May 2022 15:35:15 -0700 Subject: [PATCH 10/15] GUACAMOLE-377: Enable graphics pipeline extension by default. --- src/protocols/rdp/settings.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/protocols/rdp/settings.c b/src/protocols/rdp/settings.c index 911bc75d..6a3e6407 100644 --- a/src/protocols/rdp/settings.c +++ b/src/protocols/rdp/settings.c @@ -80,6 +80,7 @@ const char* GUAC_RDP_CLIENT_ARGS[] = { "disable-bitmap-caching", "disable-offscreen-caching", "disable-glyph-caching", + "disable-gfx", "preconnection-id", "preconnection-blob", "timezone", @@ -109,7 +110,6 @@ const char* GUAC_RDP_CLIENT_ARGS[] = { "create-recording-path", "resize-method", "enable-audio-input", - "enable-gfx", "enable-touch", "read-only", @@ -372,6 +372,13 @@ enum RDP_ARGS_IDX { */ IDX_DISABLE_GLYPH_CACHING, + /** + * "true" if the RDP Graphics Pipeline Extension should not be used, and + * traditional RDP graphics should be used instead, "false" or blank if the + * Graphics Pipeline Extension should be used if available. + */ + IDX_DISABLE_GFX, + /** * The preconnection ID to send within the preconnection PDU when * initiating an RDP connection, if any. @@ -540,12 +547,6 @@ enum RDP_ARGS_IDX { */ IDX_ENABLE_AUDIO_INPUT, - /** - * "true" if the RDP Graphics Pipeline Extension should be used, "false" or - * blank if traditional RDP graphics should be used instead. - */ - IDX_ENABLE_GFX, - /** * "true" if multi-touch support should be enabled for the RDP connection, * "false" or blank otherwise. @@ -1138,8 +1139,8 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user, /* RDP Graphics Pipeline enable/disable */ settings->enable_gfx = - guac_user_parse_args_boolean(user, GUAC_RDP_CLIENT_ARGS, argv, - IDX_ENABLE_GFX, 0); + !guac_user_parse_args_boolean(user, GUAC_RDP_CLIENT_ARGS, argv, + IDX_DISABLE_GFX, 0); /* Multi-touch input enable/disable */ settings->enable_touch = From b26f9d64d6b8f0d12ec35531616ae48cda2b268b Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 18 May 2022 15:44:47 -0700 Subject: [PATCH 11/15] GUACAMOLE-377: Clarify usage of EndPaint to detect frames. --- src/protocols/rdp/gdi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/protocols/rdp/gdi.c b/src/protocols/rdp/gdi.c index 075a382a..393bb734 100644 --- a/src/protocols/rdp/gdi.c +++ b/src/protocols/rdp/gdi.c @@ -440,8 +440,8 @@ BOOL guac_rdp_gdi_end_paint(rdpContext* context) { guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; rdpGdi* gdi = context->gdi; - /* Leverage EndPaint handler to detect end of frame for RDPGFX channel - * (ignore otherwise) */ + /* Ignore EndPaint handler unless needed to detect end of frame for RDPGFX + * channel */ if (!rdp_client->settings->enable_gfx) return TRUE; From d5761ad6250ba720a7ccd728369f03317259bf7f Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 18 May 2022 16:04:02 -0700 Subject: [PATCH 12/15] GUACAMOLE-377: Warn about required color depth only if actually overridden. --- src/protocols/rdp/settings.c | 16 ++++++++-------- src/protocols/rdp/settings.h | 5 +++++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/protocols/rdp/settings.c b/src/protocols/rdp/settings.c index 6a3e6407..acd00446 100644 --- a/src/protocols/rdp/settings.c +++ b/src/protocols/rdp/settings.c @@ -916,11 +916,6 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user, GUAC_RDP_CLIENT_ARGS[IDX_DISABLE_GLYPH_CACHING]); } - /* Session color depth */ - settings->color_depth = - guac_user_parse_args_int(user, GUAC_RDP_CLIENT_ARGS, argv, - IDX_COLOR_DEPTH, RDP_DEFAULT_DEPTH); - /* Preconnection ID */ settings->preconnection_id = -1; if (argv[IDX_PRECONNECTION_ID][0] != '\0') { @@ -1142,6 +1137,11 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user, !guac_user_parse_args_boolean(user, GUAC_RDP_CLIENT_ARGS, argv, IDX_DISABLE_GFX, 0); + /* Session color depth */ + settings->color_depth = + guac_user_parse_args_int(user, GUAC_RDP_CLIENT_ARGS, argv, + IDX_COLOR_DEPTH, settings->enable_gfx ? RDP_GFX_REQUIRED_DEPTH : RDP_DEFAULT_DEPTH); + /* Multi-touch input enable/disable */ settings->enable_touch = guac_user_parse_args_boolean(user, GUAC_RDP_CLIENT_ARGS, argv, @@ -1420,15 +1420,15 @@ void guac_rdp_push_settings(guac_client* client, rdp_settings->SupportGraphicsPipeline = TRUE; rdp_settings->RemoteFxCodec = TRUE; - if (rdp_settings->ColorDepth != 32) { + if (rdp_settings->ColorDepth != RDP_GFX_REQUIRED_DEPTH) { guac_client_log(client, GUAC_LOG_WARNING, "Ignoring requested " "color depth of %i bpp, as the RDP Graphics Pipeline " - "requires 32 bpp.", rdp_settings->ColorDepth); + "requires %i bpp.", rdp_settings->ColorDepth, RDP_GFX_REQUIRED_DEPTH); } /* Required for RemoteFX / Graphics Pipeline */ rdp_settings->FastPathOutput = TRUE; - rdp_settings->ColorDepth = 32; + rdp_settings->ColorDepth = RDP_GFX_REQUIRED_DEPTH; rdp_settings->SoftwareGdi = TRUE; } diff --git a/src/protocols/rdp/settings.h b/src/protocols/rdp/settings.h index 61662c35..3fe9d002 100644 --- a/src/protocols/rdp/settings.h +++ b/src/protocols/rdp/settings.h @@ -58,6 +58,11 @@ */ #define RDP_DEFAULT_DEPTH 16 +/** + * The color depth required by the RDPGFX channel, in bits. + */ +#define RDP_GFX_REQUIRED_DEPTH 32 + /** * The filename to use for the screen recording, if not specified. */ From b7f05b9e4f0523f82681d78f5e57563ebb695172 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 6 Jun 2022 09:26:01 -0700 Subject: [PATCH 13/15] GUACAMOLE-377: Ensure backing surface of underlying FreeRDP GDI implementation is resized when desktop is resized. --- src/protocols/rdp/gdi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/protocols/rdp/gdi.c b/src/protocols/rdp/gdi.c index 393bb734..f0ba4881 100644 --- a/src/protocols/rdp/gdi.c +++ b/src/protocols/rdp/gdi.c @@ -493,7 +493,8 @@ BOOL guac_rdp_gdi_desktop_resize(rdpContext* context) { guac_rdp_get_width(context->instance), guac_rdp_get_height(context->instance)); - return TRUE; + return gdi_resize(context->gdi, guac_rdp_get_width(context->instance), + guac_rdp_get_height(context->instance)); } From ce27936ed539872d55039b6a1b9aa54d8388659e Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 6 Jun 2022 09:32:36 -0700 Subject: [PATCH 14/15] GUACAMOLE-377: Add frame boundaries around cursor set operations if otherwise absent. --- src/protocols/rdp/pointer.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/protocols/rdp/pointer.c b/src/protocols/rdp/pointer.c index 5a72c787..7f4901e3 100644 --- a/src/protocols/rdp/pointer.c +++ b/src/protocols/rdp/pointer.c @@ -21,6 +21,7 @@ #include "common/cursor.h" #include "common/display.h" #include "common/surface.h" +#include "gdi.h" #include "pointer.h" #include "rdp.h" @@ -78,11 +79,22 @@ BOOL guac_rdp_pointer_set(rdpContext* context, const rdpPointer* pointer) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + /* Add explicit frame boundaries around cursor set operation if not already + * in a frame (the RDP protocol does not send nor expect frame boundaries + * for cursor changes, but Guacamole does expect this) */ + int in_frame = rdp_client->in_frame; + + if (rdp_client->frames_supported && !in_frame) + guac_rdp_gdi_mark_frame(context, 1); + /* Set cursor */ guac_common_cursor_set_surface(rdp_client->display->cursor, pointer->xPos, pointer->yPos, ((guac_rdp_pointer*) pointer)->layer->surface); + if (rdp_client->frames_supported && !in_frame) + guac_rdp_gdi_mark_frame(context, 0); + return TRUE; } @@ -106,9 +118,20 @@ BOOL guac_rdp_pointer_set_null(rdpContext* context) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + /* Add explicit frame boundaries around cursor set operation if not already + * in a frame (the RDP protocol does not send nor expect frame boundaries + * for cursor changes, but Guacamole does expect this) */ + int in_frame = rdp_client->in_frame; + + if (rdp_client->frames_supported && !in_frame) + guac_rdp_gdi_mark_frame(context, 1); + /* Set cursor to empty/blank graphic */ guac_common_cursor_set_blank(rdp_client->display->cursor); + if (rdp_client->frames_supported && !in_frame) + guac_rdp_gdi_mark_frame(context, 0); + return TRUE; } @@ -118,9 +141,20 @@ BOOL guac_rdp_pointer_set_default(rdpContext* context) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + /* Add explicit frame boundaries around cursor set operation if not already + * in a frame (the RDP protocol does not send nor expect frame boundaries + * for cursor changes, but Guacamole does expect this) */ + int in_frame = rdp_client->in_frame; + + if (rdp_client->frames_supported && !in_frame) + guac_rdp_gdi_mark_frame(context, 1); + /* Set cursor to embedded pointer */ guac_common_cursor_set_pointer(rdp_client->display->cursor); + if (rdp_client->frames_supported && !in_frame) + guac_rdp_gdi_mark_frame(context, 0); + return TRUE; } From 31f1b2c7c488c7ace756d7adecc2933311bbd5b3 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 9 Jun 2022 09:01:51 -0700 Subject: [PATCH 15/15] GUACAMOLE-377: Rename single-letter "e" event arguments variable to "args" for readability. --- src/protocols/rdp/channels/cliprdr.c | 14 +++++++------- src/protocols/rdp/channels/disp.c | 14 +++++++------- src/protocols/rdp/channels/rail.c | 8 ++++---- src/protocols/rdp/channels/rdpei.c | 14 +++++++------- src/protocols/rdp/channels/rdpgfx.c | 16 ++++++++-------- 5 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/protocols/rdp/channels/cliprdr.c b/src/protocols/rdp/channels/cliprdr.c index 4845577e..f16b41ca 100644 --- a/src/protocols/rdp/channels/cliprdr.c +++ b/src/protocols/rdp/channels/cliprdr.c @@ -509,12 +509,12 @@ static UINT guac_rdp_cliprdr_format_data_response(CliprdrClientContext* cliprdr, * @param context * The rdpContext associated with the active RDP session. * - * @param e + * @param args * Event-specific arguments, mainly the name of the channel, and a * reference to the associated plugin loaded for that channel by FreeRDP. */ static void guac_rdp_cliprdr_channel_connected(rdpContext* context, - ChannelConnectedEventArgs* e) { + ChannelConnectedEventArgs* args) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; @@ -526,12 +526,12 @@ static void guac_rdp_cliprdr_channel_connected(rdpContext* context, assert(clipboard != NULL); /* Ignore connection event if it's not for the CLIPRDR channel */ - if (strcmp(e->name, CLIPRDR_SVC_CHANNEL_NAME) != 0) + if (strcmp(args->name, CLIPRDR_SVC_CHANNEL_NAME) != 0) return; /* The structure pointed to by pInterface is guaranteed to be a * CliprdrClientContext if the channel is CLIPRDR */ - CliprdrClientContext* cliprdr = (CliprdrClientContext*) e->pInterface; + CliprdrClientContext* cliprdr = (CliprdrClientContext*) args->pInterface; /* Associate FreeRDP CLIPRDR context and its Guacamole counterpart with * eachother */ @@ -562,12 +562,12 @@ static void guac_rdp_cliprdr_channel_connected(rdpContext* context, * @param context * The rdpContext associated with the active RDP session. * - * @param e + * @param args * Event-specific arguments, mainly the name of the channel, and a * reference to the associated plugin loaded for that channel by FreeRDP. */ static void guac_rdp_cliprdr_channel_disconnected(rdpContext* context, - ChannelDisconnectedEventArgs* e) { + ChannelDisconnectedEventArgs* args) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; @@ -579,7 +579,7 @@ static void guac_rdp_cliprdr_channel_disconnected(rdpContext* context, assert(clipboard != NULL); /* Ignore disconnection event if it's not for the CLIPRDR channel */ - if (strcmp(e->name, CLIPRDR_SVC_CHANNEL_NAME) != 0) + if (strcmp(args->name, CLIPRDR_SVC_CHANNEL_NAME) != 0) return; /* Channel is no longer connected */ diff --git a/src/protocols/rdp/channels/disp.c b/src/protocols/rdp/channels/disp.c index 70342ebd..918676b9 100644 --- a/src/protocols/rdp/channels/disp.c +++ b/src/protocols/rdp/channels/disp.c @@ -68,19 +68,19 @@ void guac_rdp_disp_free(guac_rdp_disp* disp) { * @param context * The rdpContext associated with the active RDP session. * - * @param e + * @param args * Event-specific arguments, mainly the name of the channel, and a * reference to the associated plugin loaded for that channel by FreeRDP. */ static void guac_rdp_disp_channel_connected(rdpContext* context, - ChannelConnectedEventArgs* e) { + ChannelConnectedEventArgs* args) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; guac_rdp_disp* guac_disp = rdp_client->disp; /* Ignore connection event if it's not for the Display Update channel */ - if (strcmp(e->name, DISP_DVC_CHANNEL_NAME) != 0) + if (strcmp(args->name, DISP_DVC_CHANNEL_NAME) != 0) return; /* Init module with current display size */ @@ -89,7 +89,7 @@ static void guac_rdp_disp_channel_connected(rdpContext* context, guac_rdp_get_height(context->instance)); /* Store reference to the display update plugin once it's connected */ - DispClientContext* disp = (DispClientContext*) e->pInterface; + DispClientContext* disp = (DispClientContext*) args->pInterface; guac_disp->disp = disp; guac_client_log(client, GUAC_LOG_DEBUG, "Display update channel " @@ -110,19 +110,19 @@ static void guac_rdp_disp_channel_connected(rdpContext* context, * @param context * The rdpContext associated with the active RDP session. * - * @param e + * @param args * Event-specific arguments, mainly the name of the channel, and a * reference to the associated plugin loaded for that channel by FreeRDP. */ static void guac_rdp_disp_channel_disconnected(rdpContext* context, - ChannelDisconnectedEventArgs* e) { + ChannelDisconnectedEventArgs* args) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; guac_rdp_disp* guac_disp = rdp_client->disp; /* Ignore disconnection event if it's not for the Display Update channel */ - if (strcmp(e->name, DISP_DVC_CHANNEL_NAME) != 0) + if (strcmp(args->name, DISP_DVC_CHANNEL_NAME) != 0) return; /* Channel is no longer connected */ diff --git a/src/protocols/rdp/channels/rail.c b/src/protocols/rdp/channels/rail.c index 6f8d7f7f..e8ac1698 100644 --- a/src/protocols/rdp/channels/rail.c +++ b/src/protocols/rdp/channels/rail.c @@ -230,22 +230,22 @@ static UINT guac_rdp_rail_handshake_ex(RailClientContext* rail, * @param context * The rdpContext associated with the active RDP session. * - * @param e + * @param args * Event-specific arguments, mainly the name of the channel, and a * reference to the associated plugin loaded for that channel by FreeRDP. */ static void guac_rdp_rail_channel_connected(rdpContext* context, - ChannelConnectedEventArgs* e) { + ChannelConnectedEventArgs* args) { guac_client* client = ((rdp_freerdp_context*) context)->client; /* Ignore connection event if it's not for the RAIL channel */ - if (strcmp(e->name, RAIL_SVC_CHANNEL_NAME) != 0) + if (strcmp(args->name, RAIL_SVC_CHANNEL_NAME) != 0) return; /* The structure pointed to by pInterface is guaranteed to be a * RailClientContext if the channel is RAIL */ - RailClientContext* rail = (RailClientContext*) e->pInterface; + RailClientContext* rail = (RailClientContext*) args->pInterface; /* Init FreeRDP RAIL context, ensuring the guac_client can be accessed from * within any RAIL-specific callbacks */ diff --git a/src/protocols/rdp/channels/rdpei.c b/src/protocols/rdp/channels/rdpei.c index 06d2ba4c..a7e6f349 100644 --- a/src/protocols/rdp/channels/rdpei.c +++ b/src/protocols/rdp/channels/rdpei.c @@ -66,23 +66,23 @@ void guac_rdp_rdpei_free(guac_rdp_rdpei* rdpei) { * @param context * The rdpContext associated with the active RDP session. * - * @param e + * @param args * Event-specific arguments, mainly the name of the channel, and a * reference to the associated plugin loaded for that channel by FreeRDP. */ static void guac_rdp_rdpei_channel_connected(rdpContext* context, - ChannelConnectedEventArgs* e) { + ChannelConnectedEventArgs* args) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; guac_rdp_rdpei* guac_rdpei = rdp_client->rdpei; /* Ignore connection event if it's not for the RDPEI channel */ - if (strcmp(e->name, RDPEI_DVC_CHANNEL_NAME) != 0) + if (strcmp(args->name, RDPEI_DVC_CHANNEL_NAME) != 0) return; /* Store reference to the RDPEI plugin once it's connected */ - RdpeiClientContext* rdpei = (RdpeiClientContext*) e->pInterface; + RdpeiClientContext* rdpei = (RdpeiClientContext*) args->pInterface; guac_rdpei->rdpei = rdpei; /* Declare level of multi-touch support */ @@ -107,19 +107,19 @@ static void guac_rdp_rdpei_channel_connected(rdpContext* context, * @param context * The rdpContext associated with the active RDP session. * - * @param e + * @param args * Event-specific arguments, mainly the name of the channel, and a * reference to the associated plugin loaded for that channel by FreeRDP. */ static void guac_rdp_rdpei_channel_disconnected(rdpContext* context, - ChannelDisconnectedEventArgs* e) { + ChannelDisconnectedEventArgs* args) { guac_client* client = ((rdp_freerdp_context*) context)->client; guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; guac_rdp_rdpei* guac_rdpei = rdp_client->rdpei; /* Ignore disconnection event if it's not for the RDPEI channel */ - if (strcmp(e->name, RDPEI_DVC_CHANNEL_NAME) != 0) + if (strcmp(args->name, RDPEI_DVC_CHANNEL_NAME) != 0) return; /* Channel is no longer connected */ diff --git a/src/protocols/rdp/channels/rdpgfx.c b/src/protocols/rdp/channels/rdpgfx.c index f22ffbf3..0fae972f 100644 --- a/src/protocols/rdp/channels/rdpgfx.c +++ b/src/protocols/rdp/channels/rdpgfx.c @@ -45,21 +45,21 @@ * @param context * The rdpContext associated with the active RDP session. * - * @param e + * @param args * Event-specific arguments, mainly the name of the channel, and a * reference to the associated plugin loaded for that channel by FreeRDP. */ static void guac_rdp_rdpgfx_channel_connected(rdpContext* context, - ChannelConnectedEventArgs* e) { + ChannelConnectedEventArgs* args) { guac_client* client = ((rdp_freerdp_context*) context)->client; /* Ignore connection event if it's not for the RDPGFX channel */ - if (strcmp(e->name, RDPGFX_DVC_CHANNEL_NAME) != 0) + if (strcmp(args->name, RDPGFX_DVC_CHANNEL_NAME) != 0) return; /* Init GDI-backed support for the Graphics Pipeline */ - RdpgfxClientContext* rdpgfx = (RdpgfxClientContext*) e->pInterface; + RdpgfxClientContext* rdpgfx = (RdpgfxClientContext*) args->pInterface; rdpGdi* gdi = context->gdi; if (!gdi_graphics_pipeline_init(gdi, rdpgfx)) @@ -83,21 +83,21 @@ static void guac_rdp_rdpgfx_channel_connected(rdpContext* context, * @param context * The rdpContext associated with the active RDP session. * - * @param e + * @param args * Event-specific arguments, mainly the name of the channel, and a * reference to the associated plugin loaded for that channel by FreeRDP. */ static void guac_rdp_rdpgfx_channel_disconnected(rdpContext* context, - ChannelDisconnectedEventArgs* e) { + ChannelDisconnectedEventArgs* args) { guac_client* client = ((rdp_freerdp_context*) context)->client; /* Ignore disconnection event if it's not for the RDPGFX channel */ - if (strcmp(e->name, RDPGFX_DVC_CHANNEL_NAME) != 0) + if (strcmp(args->name, RDPGFX_DVC_CHANNEL_NAME) != 0) return; /* Un-init GDI-backed support for the Graphics Pipeline */ - RdpgfxClientContext* rdpgfx = (RdpgfxClientContext*) e->pInterface; + RdpgfxClientContext* rdpgfx = (RdpgfxClientContext*) args->pInterface; rdpGdi* gdi = context->gdi; gdi_graphics_pipeline_uninit(gdi, rdpgfx);