Merge pull request #4 from glyptodon/limit-resize-freq

GUAC-935: Only update display size if it's been longer than 250ms.

configure.ac

@@ -488,6 +488,15 @@ then
                       #include <winpr/collections.h>])
 fi
 
+# Support for "PubSub" event system
+if test "x${have_freerdp}" = "xyes"
+then
+    AC_CHECK_DECL([PubSub_SubscribeChannelConnected],
+        [AC_DEFINE([HAVE_FREERDP_EVENT_PUBSUB],,
+                   [Whether this version of FreeRDP provides the PubSub event system])],,
+        [#include <freerdp/event.h>])
+fi
+
 # Addin registration variations
 if test "x${have_freerdp}" = "xyes"
 then

src/protocols/rdp/client.c

@@ -57,6 +57,14 @@
 #include "compat/client-cliprdr.h"
 #endif
 
+#ifdef HAVE_FREERDP_CLIENT_DISP_H
+#include <freerdp/client/disp.h>
+#endif
+
+#ifdef HAVE_FREERDP_EVENT_PUBSUB
+#include <freerdp/event.h>
+#endif
+
 #ifdef ENABLE_WINPR
 #include <winpr/wtypes.h>
 #else
@@ -146,6 +154,45 @@ int __guac_receive_channel_data(freerdp* rdp_inst, int channelId, UINT8* data, i
     return freerdp_channels_data(rdp_inst, channelId, data, size, flags, total_size);
 }
 
+#ifdef HAVE_FREERDP_EVENT_PUBSUB
+/**
+ * Called whenever a channel connects via the PubSub event system within
+ * FreeRDP.
+ *
+ * @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_channel_connected(rdpContext* context,
+        ChannelConnectedEventArgs* e) {
+
+#ifdef HAVE_RDPSETTINGS_SUPPORTDISPLAYCONTROL
+    /* Store reference to the display update plugin once it's connected */
+    if (strcmp(e->name, DISP_DVC_CHANNEL_NAME) == 0) {
+
+        DispClientContext* disp = (DispClientContext*) e->pInterface;
+
+        guac_client* client = ((rdp_freerdp_context*) context)->client;
+        rdp_guac_client_data* guac_client_data =
+            (rdp_guac_client_data*) client->data;
+
+        /* Init module with current display size */
+        guac_rdp_disp_set_size(guac_client_data->disp, context,
+                guac_rdp_get_width(context->instance),
+                guac_rdp_get_height(context->instance));
+
+        /* Store connected channel */
+        guac_rdp_disp_connect(guac_client_data->disp, disp);
+        guac_client_log(client, GUAC_LOG_DEBUG,
+                "Display update channel connected.");
+
+    }
+#endif
+
+}
+#endif
+
 BOOL rdp_freerdp_pre_connect(freerdp* instance) {
 
     rdpContext* context = instance->context;
@@ -171,9 +218,15 @@ BOOL rdp_freerdp_pre_connect(freerdp* instance) {
         guac_client_log(client, GUAC_LOG_WARNING,
                 "Failed to load drdynvc plugin.");
 
+#ifdef HAVE_FREERDP_EVENT_PUBSUB
+    /* Subscribe to and handle channel connected events */
+    PubSub_SubscribeChannelConnected(context->pubSub,
+            (pChannelConnectedEventHandler) guac_rdp_channel_connected);
+#endif
+
 #ifdef HAVE_FREERDP_DISPLAY_UPDATE_SUPPORT
     /* Init display update plugin */
-    guac_client_data->disp = NULL;
+    guac_client_data->disp = guac_rdp_disp_alloc();
     guac_rdp_disp_load_plugin(instance->context);
 #endif
 

src/protocols/rdp/client.h

@@ -33,15 +33,15 @@
 #include "rdp_keymap.h"
 #include "rdp_settings.h"
 
+#ifdef HAVE_FREERDP_DISPLAY_UPDATE_SUPPORT
+#include "rdp_disp.h"
+#endif
+
 #include <freerdp/freerdp.h>
 #include <freerdp/codec/color.h>
 #include <guacamole/audio.h>
 #include <guacamole/client.h>
 
-#ifdef HAVE_FREERDP_CLIENT_DISP_H
-#include <freerdp/client/disp.h>
-#endif
-
 #include <pthread.h>
 #include <stdint.h>
 
@@ -159,9 +159,9 @@ typedef struct rdp_guac_client_data {
 
 #ifdef HAVE_FREERDP_DISPLAY_UPDATE_SUPPORT
     /**
-     * Display control interface.
+     * Display size update module.
      */
-    DispClientContext* disp;
+    guac_rdp_disp* disp;
 #endif
 
     /**

src/protocols/rdp/guac_handlers.c

@@ -89,6 +89,11 @@ int rdp_guac_client_free_handler(guac_client* client) {
     if (guac_client_data->filesystem != NULL)
         guac_rdp_fs_free(guac_client_data->filesystem);
 
+#ifdef HAVE_FREERDP_DISPLAY_UPDATE_SUPPORT
+    /* Free display update module */
+    guac_rdp_disp_free(guac_client_data->disp);
+#endif
+
     /* Free SVC list */
     guac_common_list_free(guac_client_data->available_svc);
 
@@ -188,6 +193,13 @@ int rdp_guac_client_handle_messages(guac_client* client) {
     rdpChannels* channels = rdp_inst->context->channels;
     wMessage* event;
 
+#ifdef HAVE_FREERDP_DISPLAY_UPDATE_SUPPORT
+    /* Update remote display size */
+    pthread_mutex_lock(&(guac_client_data->rdp_lock));
+    guac_rdp_disp_update_size(guac_client_data->disp, rdp_inst->context);
+    pthread_mutex_unlock(&(guac_client_data->rdp_lock));
+#endif
+
     /* Wait for messages */
     int wait_result = rdp_guac_client_wait_for_messages(client, 250000);
     guac_timestamp frame_start = guac_timestamp_current();
@@ -470,7 +482,8 @@ int rdp_guac_client_size_handler(guac_client* client, int width, int height) {
 
     /* Send display update */
     pthread_mutex_lock(&(guac_client_data->rdp_lock));
-    guac_rdp_disp_send_size(rdp_inst->context, width, height);
+    guac_rdp_disp_set_size(guac_client_data->disp, rdp_inst->context,
+            width, height);
     pthread_mutex_unlock(&(guac_client_data->rdp_lock));
 #endif
 

src/protocols/rdp/rdp_disp.c

@@ -26,39 +26,30 @@
 #include <freerdp/freerdp.h>
 #include <freerdp/client/disp.h>
 #include <guacamole/client.h>
+#include <guacamole/timestamp.h>
 
-/**
+guac_rdp_disp* guac_rdp_disp_alloc() {
- * Called whenever a channel connects. If that channel happens to be the
- * display update channel, a reference to that channel will be stored within
- * the guac_client data.
- */
-static void guac_rdp_disp_channel_connected(rdpContext* context,
-        ChannelConnectedEventArgs* e) {
 
-    /* Store reference to the display update plugin once it's connected */
+    guac_rdp_disp* disp = malloc(sizeof(guac_rdp_disp));
-    if (strcmp(e->name, DISP_DVC_CHANNEL_NAME) == 0) {
 
-        DispClientContext* disp = (DispClientContext*) e->pInterface;
+    /* Not yet connected */
+    disp->disp = NULL;
 
-        guac_client* client = ((rdp_freerdp_context*) context)->client;
+    /* No requests have been made */
-        rdp_guac_client_data* guac_client_data =
+    disp->last_request = 0;
-            (rdp_guac_client_data*) client->data;
+    disp->requested_width  = 0;
+    disp->requested_height = 0;
 
-        guac_client_data->disp = disp;
+    return disp;
-
-        guac_client_log(client, GUAC_LOG_DEBUG,
-                "Display update channel connected.");
 
 }
 
+void guac_rdp_disp_free(guac_rdp_disp* disp) {
+    free(disp);
 }
 
 void guac_rdp_disp_load_plugin(rdpContext* context) {
 
-    /* Subscribe to and handle channel connected events */
-    PubSub_SubscribeChannelConnected(context->pubSub,
-            (pChannelConnectedEventHandler) guac_rdp_disp_channel_connected);
-
 #ifdef HAVE_RDPSETTINGS_SUPPORTDISPLAYCONTROL
     context->settings->SupportDisplayControl = TRUE;
 #endif
@@ -72,19 +63,52 @@ void guac_rdp_disp_load_plugin(rdpContext* context) {
 
 }
 
-void guac_rdp_disp_send_size(rdpContext* context, int width, int height) {
+void guac_rdp_disp_connect(guac_rdp_disp* guac_disp, DispClientContext* disp) {
+    guac_disp->disp = disp;
+}
+
+void guac_rdp_disp_set_size(guac_rdp_disp* disp, rdpContext* context,
+        int width, int height) {
+
+    /* Width must be at least 200 pixels */
+    if (width < 200)
+        width = 200;
+
+    /* Width may be no more than 8192 pixels */
+    else if (width > 8192)
+        width = 8192;
+
+    /* Width must be even */
+    else if (width % 2 == 1)
+        width -= 1;
+
+    /* Height must be at least 200 pixels */
+    if (height < 200)
+        height = 200;
+
+    /* Height may be no more than 8192 pixels */
+    else if (height > 8192)
+        height = 8192;
+
+    /* Store deferred size */
+    disp->requested_width = width;
+    disp->requested_height = height;
+
+    /* Send display update notification if possible */
+    guac_rdp_disp_update_size(disp, context);
+
+}
+
+void guac_rdp_disp_update_size(guac_rdp_disp* disp, rdpContext* context) {
 
     guac_client* client = ((rdp_freerdp_context*) context)->client;
 
-    rdp_guac_client_data* guac_client_data =
-        (rdp_guac_client_data*) client->data;
-
     /* Send display update notification if display channel is connected */
-    if (guac_client_data->disp != NULL) {
+    if (disp->disp == NULL)
+        return;
 
-        guac_client_log(client, GUAC_LOG_DEBUG,
+    int width = disp->requested_width;
-                "Resizing remote display to %ix%i",
+    int height = disp->requested_height;
-                width, height);
 
     DISPLAY_CONTROL_MONITOR_LAYOUT monitors[1] = {{
         .Flags  = 0x1, /* DISPLAYCONTROL_MONITOR_PRIMARY */
@@ -99,10 +123,24 @@ void guac_rdp_disp_send_size(rdpContext* context, int width, int height) {
         .DeviceScaleFactor = 0
     }};
 
-        guac_client_data->disp->SendMonitorLayout(guac_client_data->disp, 1,
+    guac_timestamp now = guac_timestamp_current();
-                monitors);
+
-
+    /* Limit display update frequency */
-    }
+    if (disp->last_request != 0
+            && now - disp->last_request <= GUAC_RDP_DISP_UPDATE_INTERVAL)
+        return;
+
+    /* Do NOT send requests unless the size will change */
+    if (width == guac_rdp_get_width(context->instance)
+            && height == guac_rdp_get_height(context->instance))
+        return;
+
+    guac_client_log(client, GUAC_LOG_DEBUG,
+            "Resizing remote display to %ix%i",
+            width, height);
+
+    disp->last_request = now;
+    disp->disp->SendMonitorLayout(disp->disp, 1, monitors);
 
 }
 

src/protocols/rdp/rdp_disp.h

@@ -23,20 +23,111 @@
 #ifndef GUAC_RDP_DISP_H
 #define GUAC_RDP_DISP_H
 
+#include <freerdp/client/disp.h>
 #include <freerdp/freerdp.h>
 
+
 /**
- * Loads the "disp" plugin for FreeRDP. If successfully loaded, it will be
+ * The minimum amount of time that must elapse between display size updates,
- * stored within the guac_client data.
+ * in milliseconds.
+ */
+#define GUAC_RDP_DISP_UPDATE_INTERVAL 250
+
+/**
+ * Display size update module.
+ */
+typedef struct guac_rdp_disp {
+
+    /**
+     * Display control interface.
+     */
+    DispClientContext* disp;
+
+    /**
+     * The timestamp of the last display update request, or 0 if no request
+     * has been sent yet.
+     */
+    guac_timestamp last_request;
+
+    /**
+     * The last requested screen width, in pixels.
+     */
+    int requested_width;
+
+    /**
+     * The last requested screen height, in pixels.
+     */
+    int requested_height;
+
+} guac_rdp_disp;
+
+/**
+ * Allocates a new display update module, which will ultimately control the
+ * display update channel once conected.
+ *
+ * @return A new display update module.
+ */
+guac_rdp_disp* guac_rdp_disp_alloc();
+
+/**
+ * Frees the given display update module.
+ *
+ * @param disp The display update module to free.
+ */
+void guac_rdp_disp_free(guac_rdp_disp* disp);
+
+/**
+ * Loads the "disp" plugin for FreeRDP. It is still up to external code to
+ * detect when the "disp" channel is connected, and update the guac_rdp_disp
+ * with a call to guac_rdp_disp_connect().
+ *
+ * @param context The rdpContext associated with the active RDP session.
  */
 void guac_rdp_disp_load_plugin(rdpContext* context);
 
 /**
- * Sends a display update message to the RDP server, notifying that the
+ * Stores the given DispClientContext within the given guac_rdp_disp, such that
- * monitor layout has changed to a single monitor of the given width and
+ * display updates can be properly sent. Until this is called, changes to the
- * height (in pixels).
+ * display size will be deferred.
+ *
+ * @param guac_disp The display update module to associate with the connected
+ *                  display update channel.
+ * @param disp The DispClientContext associated by FreeRDP with the connected
+ *             display update channel.
  */
-void guac_rdp_disp_send_size(rdpContext* context, int width, int height);
+void guac_rdp_disp_connect(guac_rdp_disp* guac_disp, DispClientContext* disp);
+
+/**
+ * Requests a display size update, which may then be sent immediately to the
+ * RDP server. If an update was recently sent, this update may be delayed until
+ * the RDP server has had time to settle. The width/height values provided may
+ * be automatically altered to comply with the restrictions imposed by the
+ * display update channel.
+ *
+ * @param disp The display update module which should maintain the requested
+ *             size, sending the corresponding display update request when
+ *             appropriate.
+ * @param context The rdpContext associated with the active RDP session.
+ * @param width The desired display width, in pixels. Due to the restrictions
+ *              of the RDP display update channel, this will be contrained to
+ *              the range of 200 through 8192 inclusive, and rounded down to
+ *              the nearest even number.
+ * @param height The desired display height, in pixels. Due to the restrictions
+ *               of the RDP display update channel, this will be contrained to
+ *               the range of 200 through 8192 inclusive.
+ */
+void guac_rdp_disp_set_size(guac_rdp_disp* disp, rdpContext* context,
+        int width, int height);
+
+/**
+ * Sends an actual display update request to the RDP server based on previous
+ * calls to guac_rdp_disp_set_size(). If an update was recently sent, the
+ * update may be delayed until a future call to this function.
+ *
+ * @param disp The display update module which should track the update request.
+ * @param context The rdpContext associated with the active RDP session.
+ */
+void guac_rdp_disp_update_size(guac_rdp_disp* disp, rdpContext* context);
 
 #endif
 

src/protocols/rdp/rdp_gdi.c

@@ -421,6 +421,10 @@ void guac_rdp_gdi_desktop_resize(rdpContext* context) {
 
     guac_common_surface_reset_clip(data->default_surface);
 
+    guac_client_log(client, GUAC_LOG_DEBUG, "Server resized display to %ix%i",
+            guac_rdp_get_width(context->instance),
+            guac_rdp_get_height(context->instance));
+
 }