Merge pull request #25 from glyptodon/terminal-packet

GUAC-803: Make terminal updates more efficient

src/terminal/Makefile.am

@@ -34,6 +34,7 @@ noinst_HEADERS =                \
     cursor.h                    \
     display.h                   \
     ibar.h                      \
+    packet.h                    \
     pointer.h                   \
     scrollbar.h                 \
     terminal.h                  \
@@ -48,6 +49,7 @@ libguac_terminal_la_SOURCES =   \
     cursor.c                    \
     display.c                   \
     ibar.c                      \
+    packet.c                    \
     pointer.c                   \
     scrollbar.c                 \
     terminal.c                  \

src/terminal/common.c

@@ -108,3 +108,23 @@ int guac_terminal_write_all(int fd, const char* buffer, int size) {
 
 }
 
+int guac_terminal_fill_buffer(int fd, char* buffer, int size) {
+
+    int remaining = size;
+    while (remaining > 0) {
+
+        /* Attempt to read data */
+        int ret_val = read(fd, buffer, remaining);
+        if (ret_val <= 0)
+            return -1;
+
+        /* If successful, continue with what space remains (if any) */
+        remaining -= ret_val;
+        buffer += ret_val;
+
+    }
+
+    return size;
+
+}
+

src/terminal/common.h

@@ -52,5 +52,26 @@ bool guac_terminal_has_glyph(int codepoint);
  */
 int guac_terminal_write_all(int fd, const char* buffer, int size);
 
+/**
+ * Similar to read, but automatically retries the read until an error occurs,
+ * filling all available space within the buffer. Unless it is known that the
+ * given amount of space is available on the file descriptor, there is a good
+ * chance this function will block.
+ *
+ * @param fd
+ *     The file descriptor to read data from.
+ *
+ * @param buffer
+ *     The buffer to store data within.
+ *
+ * @param size
+ *     The number of bytes available within the buffer.
+ *
+ * @return
+ *     The number of bytes read if successful, or a negative value if an error
+ *     occurs.
+ */
+int guac_terminal_fill_buffer(int fd, char* buffer, int size);
+
 #endif
 

src/terminal/packet.c

@@ -0,0 +1,67 @@
+/*
+ * Copyright (C) 2015 Glyptodon LLC
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "common.h"
+#include "packet.h"
+
+#include <string.h>
+
+int guac_terminal_packet_write(int fd, const void* data, int length) {
+
+    guac_terminal_packet out;
+
+    /* Do not attempt to write packets beyond maximum size */
+    if (length > GUAC_TERMINAL_PACKET_SIZE)
+        return -1;
+
+    /* Calculate final packet length */
+    int packet_length = sizeof(int) + length;
+
+    /* Copy data into packet */
+    out.length = length;
+    memcpy(out.data, data, length);
+
+    /* Write packet */
+    return guac_terminal_write_all(fd, (const char*) &out, packet_length);
+
+}
+
+int guac_terminal_packet_read(int fd, void* data, int length) {
+
+    int bytes;
+
+    /* Read buffers MUST be at least GUAC_TERMINAL_PACKET_SIZE */
+    if (length < GUAC_TERMINAL_PACKET_SIZE)
+        return -1;
+
+    /* Read length */
+    if (guac_terminal_fill_buffer(fd, (char*) &bytes, sizeof(int)) < 0)
+        return -1;
+
+    /* Read body */
+    if (guac_terminal_fill_buffer(fd, (char*) data, bytes) < 0)
+        return -1;
+
+    return bytes;
+
+}
+

src/terminal/packet.h

@@ -0,0 +1,92 @@
+/*
+ * Copyright (C) 2015 Glyptodon LLC
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef GUAC_TERMINAL_PACKET_H
+#define GUAC_TERMINAL_PACKET_H
+
+/**
+ * The maximum size of a packet written or read by the
+ * guac_terminal_packet_write() or guac_terminal_packet_read() functions.
+ */
+#define GUAC_TERMINAL_PACKET_SIZE 4096
+
+/**
+ * An arbitrary data packet with minimal framing.
+ */
+typedef struct guac_terminal_packet {
+
+    /**
+     * The number of bytes in the data portion of this packet.
+     */
+    int length;
+
+    /**
+     * Arbitrary data.
+     */
+    char data[GUAC_TERMINAL_PACKET_SIZE];
+
+} guac_terminal_packet;
+
+/**
+ * Writes a single packet of data to the given file descriptor. The provided
+ * length MUST be no greater than GUAC_TERMINAL_PACKET_SIZE. Zero-length
+ * writes are legal and do result in a packet being written to the file
+ * descriptor.
+ *
+ * @param fd
+ *     The file descriptor to write to.
+ *
+ * @param data
+ *     A buffer containing the data to write.
+ *
+ * @param length
+ *     The number of bytes to write to the file descriptor.
+ *
+ * @return
+ *     The number of bytes written on success, which may be zero if the data
+ *     length is zero, or a negative value on error.
+ */
+int guac_terminal_packet_write(int fd, const void* data, int length);
+
+/**
+ * Reads a single packet of data from the given file descriptor. The provided
+ * length MUST be at least GUAC_TERMINAL_PACKET_SIZE to ensure any packet
+ * read will fit in the buffer. Zero-length reads are possible if a zero-length
+ * packet was written.
+ *
+ * @param fd
+ *     The file descriptor to read from.
+ *
+ * @param data
+ *     The buffer to store data within.
+ *
+ * @param length
+ *     The number of bytes available within the buffer.
+ *
+ * @return
+ *     The number of bytes read on success, which may be zero if the read
+ *     packet had a length of zero, or a negative value on error.
+ */
+int guac_terminal_packet_read(int fd, void* data, int length);
+
+#endif
+

src/terminal/terminal.c

@@ -29,6 +29,7 @@
 #include "display.h"
 #include "ibar.h"
 #include "guac_clipboard.h"
+#include "packet.h"
 #include "pointer.h"
 #include "scrollbar.h"
 #include "terminal.h"
@@ -49,6 +50,7 @@
 #include <guacamole/error.h>
 #include <guacamole/protocol.h>
 #include <guacamole/socket.h>
+#include <guacamole/timestamp.h>
 
 /**
  * Sets the given range of columns to the given character.
@@ -321,37 +323,68 @@ void guac_terminal_free(guac_terminal* term) {
 
 }
 
-int guac_terminal_render_frame(guac_terminal* terminal) {
+/**
-
+ * Waits for data to become available on the given file descriptor.
-    guac_client* client = terminal->client;
+ *
-    char buffer[8192];
+ * @param fd
-
+ *    The file descriptor to wait on.
-    int ret_val;
+ *
-    int fd = terminal->stdout_pipe_fd[0];
+ * @param msec_timeout
-    struct timeval timeout;
+ *    The maximum amount of time to wait, in milliseconds.
-    fd_set fds;
+ *
+ * @return
+ *    A positive if data is available, zero if the timeout has elapsed without
+ *    data becoming available, or negative if an error occurred.
+ */
+static int guac_terminal_wait_for_data(int fd, int msec_timeout) {
 
     /* Build fd_set */
+    fd_set fds;
     FD_ZERO(&fds);
     FD_SET(fd, &fds);
 
-    /* Time to wait */
+    /* Split millisecond timeout into seconds and microseconds */
-    timeout.tv_sec =  1;
+    struct timeval timeout = {
-    timeout.tv_usec = 0;
+        .tv_sec  =  msec_timeout / 1000,
+        .tv_usec = (msec_timeout % 1000) * 1000
+    };
+
+    /* Wait for data */
+    return select(fd+1, &fds, NULL, NULL, &timeout);
+
+}
+
+int guac_terminal_render_frame(guac_terminal* terminal) {
+
+    guac_client* client = terminal->client;
+    char buffer[GUAC_TERMINAL_PACKET_SIZE];
+
+    int wait_result;
+    int fd = terminal->stdout_pipe_fd[0];
 
     /* Wait for data to be available */
-    ret_val = select(fd+1, &fds, NULL, NULL, &timeout);
+    wait_result = guac_terminal_wait_for_data(fd, 1000);
-    if (ret_val > 0) {
+    if (wait_result > 0) {
-
-        int bytes_read = 0;
 
         guac_terminal_lock(terminal);
+        guac_timestamp frame_start = guac_timestamp_current();
+
+        do {
+
+            guac_timestamp frame_end;
+            int frame_remaining;
+
+            int bytes_read;
 
             /* Read data, write to terminal */
-        if ((bytes_read = read(fd, buffer, sizeof(buffer))) > 0) {
+            if ((bytes_read = guac_terminal_packet_read(fd,
+                            buffer, sizeof(buffer))) > 0) {
 
                 if (guac_terminal_write(terminal, buffer, bytes_read)) {
-                guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error writing data");
+                    guac_client_abort(client,
+                            GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+                            "Error writing data");
+                    guac_terminal_unlock(terminal);
                     return 1;
                 }
 
@@ -359,17 +392,36 @@ int guac_terminal_render_frame(guac_terminal* terminal) {
 
             /* Notify on error */
             if (bytes_read < 0) {
-            guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error reading data");
+                guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+                        "Error reading data");
+                guac_terminal_unlock(terminal);
                 return 1;
             }
 
+            /* Calculate time remaining in frame */
+            frame_end = guac_timestamp_current();
+            frame_remaining = frame_start + GUAC_TERMINAL_FRAME_DURATION
+                            - frame_end;
+
+            /* Wait again if frame remaining */
+            if (frame_remaining > 0)
+                wait_result = guac_terminal_wait_for_data(fd,
+                        GUAC_TERMINAL_FRAME_TIMEOUT);
+            else
+                break;
+
+        } while (wait_result > 0);
+
         /* Flush terminal */
         guac_terminal_flush(terminal);
         guac_terminal_unlock(terminal);
 
     }
-    else if (ret_val < 0) {
+
-        guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error waiting for data");
+    /* Notify of any errors */
+    if (wait_result < 0) {
+        guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR,
+                "Error waiting for data");
         return 1;
     }
 
@@ -382,8 +434,28 @@ int guac_terminal_read_stdin(guac_terminal* terminal, char* c, int size) {
     return read(stdin_fd, c, size);
 }
 
-int guac_terminal_write_stdout(guac_terminal* terminal, const char* c, int size) {
+int guac_terminal_write_stdout(guac_terminal* terminal, const char* c,
-    return guac_terminal_write_all(terminal->stdout_pipe_fd[1], c, size);
+        int size) {
+
+    /* Write maximally-sized packets until only one packet remains */
+    while (size > GUAC_TERMINAL_PACKET_SIZE) {
+
+        /* Write maximally-sized packet */
+        if (guac_terminal_packet_write(terminal->stdout_pipe_fd[1], c,
+                GUAC_TERMINAL_PACKET_SIZE) < 0)
+            return -1;
+
+        /* Advance to next packet */
+        c    += GUAC_TERMINAL_PACKET_SIZE;
+        size -= GUAC_TERMINAL_PACKET_SIZE;
+
+    }
+
+    return guac_terminal_packet_write(terminal->stdout_pipe_fd[1], c, size);
+}
+
+int guac_terminal_notify(guac_terminal* terminal) {
+    return guac_terminal_packet_write(terminal->stdout_pipe_fd[1], NULL, 0);
 }
 
 int guac_terminal_printf(guac_terminal* terminal, const char* format, ...) {
@@ -693,12 +765,7 @@ void guac_terminal_scroll_display_down(guac_terminal* terminal,
 
     }
 
-    guac_terminal_display_flush(terminal->display);
+    guac_terminal_notify(terminal);
-    guac_terminal_scrollbar_flush(terminal->scrollbar);
-
-    guac_protocol_send_sync(terminal->client->socket,
-            terminal->client->last_sent_timestamp);
-    guac_socket_flush(terminal->client->socket);
 
 }
 
@@ -760,12 +827,7 @@ void guac_terminal_scroll_display_up(guac_terminal* terminal,
 
     }
 
-    guac_terminal_display_flush(terminal->display);
+    guac_terminal_notify(terminal);
-    guac_terminal_scrollbar_flush(terminal->scrollbar);
-
-    guac_protocol_send_sync(terminal->client->socket,
-            terminal->client->last_sent_timestamp);
-    guac_socket_flush(terminal->client->socket);
 
 }
 
@@ -1186,16 +1248,9 @@ int guac_terminal_resize(guac_terminal* terminal, int width, int height) {
         /* Reset scroll region */
         terminal->scroll_end = rows - 1;
 
-        guac_terminal_flush(terminal);
-    }
-
-    /* If terminal size hasn't changed, still need to flush */
-    else {
-        guac_terminal_scrollbar_flush(terminal->scrollbar);
-        guac_protocol_send_sync(socket, client->last_sent_timestamp);
-        guac_socket_flush(socket);
     }
 
+    guac_terminal_notify(terminal);
     return 0;
 
 }
@@ -1228,7 +1283,7 @@ static int __guac_terminal_send_key(guac_terminal* term, int keysym, int pressed
     if (term->current_cursor != term->blank_cursor) {
         term->current_cursor = term->blank_cursor;
         guac_terminal_set_cursor(term->client, term->blank_cursor);
-        guac_socket_flush(term->client->socket);
+        guac_terminal_notify(term);
     }
 
     /* Track modifiers */
@@ -1402,11 +1457,7 @@ static int __guac_terminal_send_mouse(guac_terminal* term, int x, int y, int mas
             guac_terminal_set_cursor(client, term->pointer_cursor);
         }
 
-        /* Flush scrollbar */
+        guac_terminal_notify(term);
-        guac_terminal_scrollbar_flush(term->scrollbar);
-        guac_protocol_send_sync(socket, client->last_sent_timestamp);
-        guac_socket_flush(socket);
-
         return 0;
 
     }
@@ -1417,8 +1468,7 @@ static int __guac_terminal_send_mouse(guac_terminal* term, int x, int y, int mas
     if (term->current_cursor != term->ibar_cursor) {
         term->current_cursor = term->ibar_cursor;
         guac_terminal_set_cursor(client, term->ibar_cursor);
-        guac_protocol_send_sync(socket, client->last_sent_timestamp);
+        guac_terminal_notify(term);
-        guac_socket_flush(socket);
     }
 
     /* Paste contents of clipboard on right or middle mouse button up */

src/terminal/terminal.h

@@ -39,6 +39,17 @@
 #include <guacamole/client.h>
 #include <guacamole/stream.h>
 
+/**
+ * The maximum duration of a single frame, in milliseconds.
+ */
+#define GUAC_TERMINAL_FRAME_DURATION 40
+
+/**
+ * The maximum amount of time to wait for more data before declaring a frame
+ * complete, in milliseconds.
+ */
+#define GUAC_TERMINAL_FRAME_TIMEOUT 10
+
 /**
  * The maximum number of custom tab stops.
  */
@@ -367,6 +378,19 @@ int guac_terminal_read_stdin(guac_terminal* terminal, char* c, int size);
  */
 int guac_terminal_write_stdout(guac_terminal* terminal, const char* c, int size);
 
+/**
+ * Notifies the terminal that an event has occurred and the terminal should
+ * flush itself when reasonable.
+ *
+ * @param terminal
+ *     The terminal to notify.
+ *
+ * @return
+ *     Zero if notification succeeded, non-zero if an error occurred while
+ *     notifying the terminal.
+ */
+int guac_terminal_notify(guac_terminal* terminal);
+
 /**
  * Reads a single line from this terminal's STDIN. Input is retrieved in
  * the same manner as guac_terminal_read_stdin() and the same restrictions