From 9535539bb2416797709e3f93997ea0cee37e4759 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 22 Sep 2016 10:12:26 +0200 Subject: [PATCH 1/4] Process entire WebSocket message at once setTimeout() causes too much delay to be useful. Also, we already handle all rects in a message at once, so this shouldn't be too much of a change. --- core/rfb.js | 20 +------------------- tests/test.rfb.js | 6 ++---- 2 files changed, 3 insertions(+), 23 deletions(-) diff --git a/core/rfb.js b/core/rfb.js index 7f8ae6d0..a0136f35 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -81,7 +81,6 @@ this._keyboard = null; // Keyboard input handler object this._mouse = null; // Mouse input handler object this._disconnTimer = null; // disconnection timer - this._msgTimer = null; // queued handle_msg timer this._supportsFence = false; @@ -415,11 +414,6 @@ }, _cleanup: function () { - if (this._msgTimer) { - clearInterval(this._msgTimer); - this._msgTimer = null; - } - if (this._display && this._display.get_context()) { if (!this._view_only) { this._keyboard.ungrab(); } if (!this._view_only) { this._mouse.ungrab(); } @@ -573,19 +567,7 @@ Util.Error("Got data while disconnected"); break; case 'connected': - if (this._normal_msg() && this._sock.rQlen() > 0) { - // true means we can continue processing - // Give other events a chance to run - if (this._msgTimer === null) { - Util.Debug("More data to process, creating timer"); - this._msgTimer = setTimeout(function () { - this._msgTimer = null; - this._handle_message(); - }.bind(this), 0); - } else { - Util.Debug("More data to process, existing timer"); - } - } + while (this._normal_msg() && this._sock.rQlen() > 0); break; default: this._init_msg(); diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 43633da2..b8bc9d22 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -1274,7 +1274,7 @@ describe('Remote Frame Buffer Protocol Client', function() { client._sock._websocket._receive_data(new Uint8Array([0, 0, 0, 3])); expect(client._sock._websocket._get_sent_data()).to.have.length(0); - client._framebufferUpdate = function () { return true; }; // we magically have enough data + client._framebufferUpdate = function () { this._sock.rQskip8(); return true; }; // we magically have enough data // 247 should *not* be used as the message type here client._sock._websocket._receive_data(new Uint8Array([247])); expect(client._sock).to.have.sent(expected_msg._sQ); @@ -2080,14 +2080,12 @@ describe('Remote Frame Buffer Protocol Client', function() { expect(client._init_msg).to.have.been.calledOnce; }); - it('should split up the handling of muplitle normal messages across 10ms intervals', function () { + it('should process all normal messages directly', function () { client.connect('host', 8675); client._sock._websocket._open(); client._rfb_connection_state = 'connected'; client.set_onBell(sinon.spy()); client._sock._websocket._receive_data(new Uint8Array([0x02, 0x02])); - expect(client.get_onBell()).to.have.been.calledOnce; - this.clock.tick(20); expect(client.get_onBell()).to.have.been.calledTwice; }); From 1578fa68ac754ed1ca29431a87586b2463d354db Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 22 Sep 2016 10:19:26 +0200 Subject: [PATCH 2/4] Hide image handling in display object The callers don't need to concern themselves with how images are rendered, so hide the details behind the API. This also avoids exposing the render queue. --- core/display.js | 41 ++++++++++++++++++++++++++--------------- core/rfb.js | 16 ++-------------- tests/test.display.js | 14 +++++++------- 3 files changed, 35 insertions(+), 36 deletions(-) diff --git a/core/display.js b/core/display.js index e6239f48..938bb895 100644 --- a/core/display.js +++ b/core/display.js @@ -365,7 +365,7 @@ fillRect: function (x, y, width, height, color, from_queue) { if (this._renderQ.length !== 0 && !from_queue) { - this.renderQ_push({ + this._renderQ_push({ 'type': 'fill', 'x': x, 'y': y, @@ -381,7 +381,7 @@ copyImage: function (old_x, old_y, new_x, new_y, w, h, from_queue) { if (this._renderQ.length !== 0 && !from_queue) { - this.renderQ_push({ + this._renderQ_push({ 'type': 'copy', 'old_x': old_x, 'old_y': old_y, @@ -400,6 +400,17 @@ } }, + imageRect: function(x, y, mime, arr) { + var img = new Image(); + img.src = "data: " + mime + ";base64," + Base64.encode(arr); + this._renderQ_push({ + 'type': 'img', + 'img': img, + 'x': x, + 'y': y + }); + }, + // start updating a tile startTile: function (x, y, width, height, color) { this._tile_x = x; @@ -480,7 +491,7 @@ // this probably isn't getting called *nearly* as much var new_arr = new Uint8Array(width * height * 4); new_arr.set(new Uint8Array(arr.buffer, 0, new_arr.length)); - this.renderQ_push({ + this._renderQ_push({ 'type': 'blit', 'data': new_arr, 'x': x, @@ -502,7 +513,7 @@ // this probably isn't getting called *nearly* as much var new_arr = new Uint8Array(width * height * 3); new_arr.set(new Uint8Array(arr.buffer, 0, new_arr.length)); - this.renderQ_push({ + this._renderQ_push({ 'type': 'blitRgb', 'data': new_arr, 'x': x, @@ -525,7 +536,7 @@ // this probably isn't getting called *nearly* as much var new_arr = new Uint8Array(width * height * 4); new_arr.set(new Uint8Array(arr.buffer, 0, new_arr.length)); - this.renderQ_push({ + this._renderQ_push({ 'type': 'blitRgbx', 'data': new_arr, 'x': x, @@ -552,16 +563,6 @@ this._drawCtx.drawImage(img, x - this._viewportLoc.x, y - this._viewportLoc.y); }, - renderQ_push: function (action) { - this._renderQ.push(action); - if (this._renderQ.length === 1) { - // If this can be rendered immediately it will be, otherwise - // the scanner will start polling the queue (every - // requestAnimationFrame interval) - this._scan_renderQ(); - } - }, - changeCursor: function (pixels, mask, hotx, hoty, w, h) { if (this._cursor_uri === false) { Util.Warn("changeCursor called but no cursor data URI support"); @@ -741,6 +742,16 @@ this._drawCtx.putImageData(img, x - vx, y - vy); }, + _renderQ_push: function (action) { + this._renderQ.push(action); + if (this._renderQ.length === 1) { + // If this can be rendered immediately it will be, otherwise + // the scanner will start polling the queue (every + // requestAnimationFrame interval) + this._scan_renderQ(); + } + }, + _scan_renderQ: function () { var ready = true; while (ready && this._renderQ.length > 0) { diff --git a/core/rfb.js b/core/rfb.js index a0136f35..7567519c 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -1692,10 +1692,6 @@ return (new DES(passwd)).encrypt(challenge); }; - RFB.extract_data_uri = function (arr) { - return ";base64," + Base64.encode(arr); - }; - RFB.encodingHandlers = { RAW: function () { if (this._FBU.lines === 0) { @@ -2198,16 +2194,8 @@ // We have everything, render it this._sock.rQskipBytes(1 + cl_header); // shift off clt + compact length - var img = new Image(); - img.src = "data: image/" + cmode + - RFB.extract_data_uri(this._sock.rQshiftBytes(cl_data)); - this._display.renderQ_push({ - 'type': 'img', - 'img': img, - 'x': this._FBU.x, - 'y': this._FBU.y - }); - img = null; + data = this._sock.rQshiftBytes(cl_data); + this._display.imageRect(this._FBU.x, this._FBU.y, "image/" + cmode, data); break; case "filter": var filterId = rQ[rQi + 1]; diff --git a/tests/test.display.js b/tests/test.display.js index 7c7c693b..64932b10 100644 --- a/tests/test.display.js +++ b/tests/test.display.js @@ -396,13 +396,13 @@ describe('Display/Canvas Helper', function () { }); it('should try to process an item when it is pushed on, if nothing else is on the queue', function () { - display.renderQ_push({ type: 'noop' }); // does nothing + display._renderQ_push({ type: 'noop' }); // does nothing expect(display._scan_renderQ).to.have.been.calledOnce; }); it('should not try to process an item when it is pushed on if we are waiting for other items', function () { display._renderQ.length = 2; - display.renderQ_push({ type: 'noop' }); + display._renderQ_push({ type: 'noop' }); expect(display._scan_renderQ).to.not.have.been.called; }); @@ -425,35 +425,35 @@ describe('Display/Canvas Helper', function () { it('should draw a blit image on type "blit"', function () { display.blitImage = sinon.spy(); - display.renderQ_push({ type: 'blit', x: 3, y: 4, width: 5, height: 6, data: [7, 8, 9] }); + display._renderQ_push({ type: 'blit', x: 3, y: 4, width: 5, height: 6, data: [7, 8, 9] }); expect(display.blitImage).to.have.been.calledOnce; expect(display.blitImage).to.have.been.calledWith(3, 4, 5, 6, [7, 8, 9], 0); }); it('should draw a blit RGB image on type "blitRgb"', function () { display.blitRgbImage = sinon.spy(); - display.renderQ_push({ type: 'blitRgb', x: 3, y: 4, width: 5, height: 6, data: [7, 8, 9] }); + display._renderQ_push({ type: 'blitRgb', x: 3, y: 4, width: 5, height: 6, data: [7, 8, 9] }); expect(display.blitRgbImage).to.have.been.calledOnce; expect(display.blitRgbImage).to.have.been.calledWith(3, 4, 5, 6, [7, 8, 9], 0); }); it('should copy a region on type "copy"', function () { display.copyImage = sinon.spy(); - display.renderQ_push({ type: 'copy', x: 3, y: 4, width: 5, height: 6, old_x: 7, old_y: 8 }); + display._renderQ_push({ type: 'copy', x: 3, y: 4, width: 5, height: 6, old_x: 7, old_y: 8 }); expect(display.copyImage).to.have.been.calledOnce; expect(display.copyImage).to.have.been.calledWith(7, 8, 3, 4, 5, 6); }); it('should fill a rect with a given color on type "fill"', function () { display.fillRect = sinon.spy(); - display.renderQ_push({ type: 'fill', x: 3, y: 4, width: 5, height: 6, color: [7, 8, 9]}); + display._renderQ_push({ type: 'fill', x: 3, y: 4, width: 5, height: 6, color: [7, 8, 9]}); expect(display.fillRect).to.have.been.calledOnce; expect(display.fillRect).to.have.been.calledWith(3, 4, 5, 6, [7, 8, 9]); }); it('should draw an image from an image object on type "img" (if complete)', function () { display.drawImage = sinon.spy(); - display.renderQ_push({ type: 'img', x: 3, y: 4, img: { complete: true } }); + display._renderQ_push({ type: 'img', x: 3, y: 4, img: { complete: true } }); expect(display.drawImage).to.have.been.calledOnce; expect(display.drawImage).to.have.been.calledWith({ complete: true }, 3, 4); }); From bb6965f2e60c2301dd638383bdc792d1f10af942 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 22 Sep 2016 10:28:35 +0200 Subject: [PATCH 3/4] Wait for proper image load event There is a specific event for when an image has finished loading, so trigger on that rather than polling. The polling interval of requestAnimationFrame() can also be very large. --- core/display.js | 16 ++++++++++------ tests/test.display.js | 11 ++++------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/core/display.js b/core/display.js index 938bb895..e17e2956 100644 --- a/core/display.js +++ b/core/display.js @@ -746,12 +746,18 @@ this._renderQ.push(action); if (this._renderQ.length === 1) { // If this can be rendered immediately it will be, otherwise - // the scanner will start polling the queue (every - // requestAnimationFrame interval) + // the scanner will wait for the relevant event this._scan_renderQ(); } }, + _resume_renderQ: function() { + // "this" is the object that is ready, not the + // display object + this.removeEventListener('load', this._noVNC_display._resume_renderQ); + this._noVNC_display._scan_renderQ(); + }, + _scan_renderQ: function () { var ready = true; while (ready && this._renderQ.length > 0) { @@ -776,6 +782,8 @@ if (a.img.complete) { this.drawImage(a.img, a.x, a.y); } else { + a.img._noVNC_display = this; + a.img.addEventListener('load', this._resume_renderQ); // We need to wait for this image to 'load' // to keep things in-order ready = false; @@ -787,10 +795,6 @@ this._renderQ.shift(); } } - - if (this._renderQ.length > 0) { - requestAnimationFrame(this._scan_renderQ.bind(this)); - } }, }; diff --git a/tests/test.display.js b/tests/test.display.js index 64932b10..3c7a28fc 100644 --- a/tests/test.display.js +++ b/tests/test.display.js @@ -384,11 +384,6 @@ describe('Display/Canvas Helper', function () { display = new Display({ target: document.createElement('canvas'), prefer_js: false }); display.resize(4, 4); sinon.spy(display, '_scan_renderQ'); - this.old_requestAnimationFrame = window.requestAnimationFrame; - window.requestAnimationFrame = function (cb) { - this.next_frame_cb = cb; - }.bind(this); - this.next_frame = function () { this.next_frame_cb(); }; }); afterEach(function () { @@ -407,7 +402,7 @@ describe('Display/Canvas Helper', function () { }); it('should wait until an image is loaded to attempt to draw it and the rest of the queue', function () { - var img = { complete: false }; + var img = { complete: false, addEventListener: sinon.spy() } display._renderQ = [{ type: 'img', x: 3, y: 4, img: img }, { type: 'fill', x: 1, y: 2, width: 3, height: 4, color: 5 }]; display.drawImage = sinon.spy(); @@ -416,11 +411,13 @@ describe('Display/Canvas Helper', function () { display._scan_renderQ(); expect(display.drawImage).to.not.have.been.called; expect(display.fillRect).to.not.have.been.called; + expect(img.addEventListener).to.have.been.calledOnce; display._renderQ[0].img.complete = true; - this.next_frame(); + display._scan_renderQ(); expect(display.drawImage).to.have.been.calledOnce; expect(display.fillRect).to.have.been.calledOnce; + expect(img.addEventListener).to.have.been.calledOnce; }); it('should draw a blit image on type "blit"', function () { From d9ca5e5b6b418bf9c9e8c6281fa2edfaa695ac02 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 22 Sep 2016 10:57:56 +0200 Subject: [PATCH 4/4] Don't allow more than one pending update --- core/display.js | 25 +++++++++++++++++++++++-- core/rfb.js | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/core/display.js b/core/display.js index e17e2956..d9f66d7f 100644 --- a/core/display.js +++ b/core/display.js @@ -20,6 +20,7 @@ this._c_forceCanvas = false; this._renderQ = []; // queue drawing actions for in-oder rendering + this._flushing = false; // the full frame buffer (logical canvas) size this._fb_width = 0; @@ -44,7 +45,8 @@ 'colourMap': [], 'scale': 1.0, 'viewport': false, - 'render_mode': '' + 'render_mode': '', + "onFlush": function () {}, }); Util.Debug(">> Display.constructor"); @@ -363,6 +365,18 @@ this._renderQ = []; }, + pending: function() { + return this._renderQ.length > 0; + }, + + flush: function() { + if (this._renderQ.length === 0) { + this._onFlush(); + } else { + this._flushing = true; + } + }, + fillRect: function (x, y, width, height, color, from_queue) { if (this._renderQ.length !== 0 && !from_queue) { this._renderQ_push({ @@ -795,6 +809,11 @@ this._renderQ.shift(); } } + + if (this._renderQ.length === 0 && this._flushing) { + this._flushing = false; + this._onFlush(); + } }, }; @@ -814,7 +833,9 @@ ['render_mode', 'ro', 'str'], // Canvas rendering mode (read-only) ['prefer_js', 'rw', 'str'], // Prefer Javascript over canvas methods - ['cursor_uri', 'rw', 'raw'] // Can we render cursor using data URI + ['cursor_uri', 'rw', 'raw'], // Can we render cursor using data URI + + ['onFlush', 'rw', 'func'], // onFlush(): A flush request has finished ]); // Class Methods diff --git a/core/rfb.js b/core/rfb.js index 7567519c..224e822b 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -78,6 +78,7 @@ this._sock = null; // Websock object this._display = null; // Display object + this._flushing = false; // Display flushing state this._keyboard = null; // Keyboard input handler object this._mouse = null; // Mouse input handler object this._disconnTimer = null; // disconnection timer @@ -189,7 +190,8 @@ // NB: nothing that needs explicit teardown should be done // before this point, since this can throw an exception try { - this._display = new Display({target: this._target}); + this._display = new Display({target: this._target, + onFlush: this._onFlush.bind(this)}); } catch (exc) { Util.Error("Display exception: " + exc); throw exc; @@ -567,7 +569,17 @@ Util.Error("Got data while disconnected"); break; case 'connected': - while (this._normal_msg() && this._sock.rQlen() > 0); + while (true) { + if (this._flushing) { + break; + } + if (!this._normal_msg()) { + break; + } + if (this._sock.rQlen() === 0) { + break; + } + } break; default: this._init_msg(); @@ -1232,6 +1244,14 @@ } }, + _onFlush: function() { + this._flushing = false; + // Resume processing + if (this._sock.rQlen() > 0) { + this._handle_message(); + } + }, + _framebufferUpdate: function () { var ret = true; var now; @@ -1246,6 +1266,14 @@ now = (new Date()).getTime(); Util.Info("First FBU latency: " + (now - this._timing.fbu_rt_start)); } + + // Make sure the previous frame is fully rendered first + // to avoid building up an excessive queue + if (this._display.pending()) { + this._flushing = true; + this._display.flush(); + return false; + } } while (this._FBU.rects > 0) {