From 19c473f792d5a5cdb83d30e67a2a57842ed15df6 Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Sat, 9 May 2020 20:03:06 +0200 Subject: [PATCH] Properly limit mouse moves to once every 17 ms Previous attempt in c958269 had a number of issues, this is a full rewrite, complete with improved unit tests. Fixes github issue #1402 --- kasmweb/core/input/mouse.js | 16 +---- kasmweb/core/rfb.js | 69 +++++++++++++----- kasmweb/tests/test.mouse.js | 68 ------------------ kasmweb/tests/test.rfb.js | 137 +++++++++++++++++++++++++++++------- 4 files changed, 165 insertions(+), 125 deletions(-) diff --git a/kasmweb/core/input/mouse.js b/kasmweb/core/input/mouse.js index d479d5c..5f1b6b4 100644 --- a/kasmweb/core/input/mouse.js +++ b/kasmweb/core/input/mouse.js @@ -11,7 +11,6 @@ import { setCapture, stopEvent, getPointerEvent } from '../util/events.js'; const WHEEL_STEP = 10; // Delta threshold for a mouse wheel step const WHEEL_STEP_TIMEOUT = 50; // ms const WHEEL_LINE_HEIGHT = 19; -const MOUSE_MOVE_DELAY = 17; // Minimum wait (ms) between two mouse moves export default class Mouse { constructor(target) { @@ -23,7 +22,6 @@ export default class Mouse { this._pos = null; this._wheelStepXTimer = null; this._wheelStepYTimer = null; - this._oldMouseMoveTime = 0; this._accumulatedWheelDeltaX = 0; this._accumulatedWheelDeltaY = 0; @@ -200,19 +198,7 @@ export default class Mouse { _handleMouseMove(e) { this._updateMousePosition(e); - - // Limit mouse move events to one every MOUSE_MOVE_DELAY ms - clearTimeout(this.mouseMoveTimer); - const newMouseMoveTime = Date.now(); - if (newMouseMoveTime < this._oldMouseMoveTime + MOUSE_MOVE_DELAY) { - this.mouseMoveTimer = setTimeout(this.onmousemove.bind(this), - MOUSE_MOVE_DELAY, - this._pos.x, this._pos.y); - } else { - this.onmousemove(this._pos.x, this._pos.y); - } - this._oldMouseMoveTime = newMouseMoveTime; - + this.onmousemove(this._pos.x, this._pos.y); stopEvent(e); } diff --git a/kasmweb/core/rfb.js b/kasmweb/core/rfb.js index f8e0992..29c1f00 100644 --- a/kasmweb/core/rfb.js +++ b/kasmweb/core/rfb.js @@ -40,6 +40,9 @@ const DEFAULT_BACKGROUND = 'rgb(40, 40, 40)'; var _videoQuality = 2; var _enableWebP = false; +// Minimum wait (ms) between two mouse moves +const MOUSE_MOVE_DELAY = 17; + // Extended clipboard pseudo-encoding formats const extendedClipboardFormatText = 1; /*eslint-disable no-unused-vars */ @@ -138,6 +141,7 @@ export default class RFB extends EventTargetMixin { // Timers this._disconnTimer = null; // disconnection timer this._resizeTimeout = null; // resize rate limiting + this._mouseMoveTimer = null; // Decoder states this._decoders = {}; @@ -152,7 +156,9 @@ export default class RFB extends EventTargetMixin { }; // Mouse state + this._mousePos = {}; this._mouseButtonMask = 0; + this._mouseLastMoveTime = 0; this._viewportDragging = false; this._viewportDragPos = {}; this._viewportHasMoved = false; @@ -564,6 +570,7 @@ export default class RFB extends EventTargetMixin { } } clearTimeout(this._resizeTimeout); + clearTimeout(this._mouseMoveTimer); Log.Debug("<< RFB.disconnect"); } @@ -880,12 +887,6 @@ export default class RFB extends EventTargetMixin { } _handleMouseButton(x, y, down, bmask) { - if (down) { - this._mouseButtonMask |= bmask; - } else { - this._mouseButtonMask &= ~bmask; - } - if (this.dragViewport) { if (down && !this._viewportDragging) { this._viewportDragging = true; @@ -903,24 +904,29 @@ export default class RFB extends EventTargetMixin { return; } - if (this._viewOnly) { return; } - // Otherwise we treat this as a mouse click event. // Send the button down event here, as the button up // event is sent at the end of this function. this.sentEventsCounter+=1; - RFB.messages.pointerEvent(this._sock, - this._display.absX(x), - this._display.absY(y), - bmask); + this._sendMouse(x, y, bmask); } } - if (this._viewOnly) { return; } // View only, skip mouse events - - if (this._rfbConnectionState !== 'connected') { return; } this.sentEventsCounter+=1; - RFB.messages.pointerEvent(this._sock, this._display.absX(x), this._display.absY(y), this._mouseButtonMask); + // Flush waiting move event first + if (this._mouseMoveTimer !== null) { + clearTimeout(this._mouseMoveTimer); + this._mouseMoveTimer = null; + this._sendMouse(x, y, this._mouseButtonMask); + } + + if (down) { + this._mouseButtonMask |= bmask; + } else { + this._mouseButtonMask &= ~bmask; + } + + this._sendMouse(x, y, this._mouseButtonMask); } _handleMouseMove(x, y) { @@ -940,10 +946,37 @@ export default class RFB extends EventTargetMixin { return; } + this._mousePos = { 'x': x, 'y': y }; + + // Limit many mouse move events to one every MOUSE_MOVE_DELAY ms + if (this._mouseMoveTimer == null) { + + const timeSinceLastMove = Date.now() - this._mouseLastMoveTime; + if (timeSinceLastMove > MOUSE_MOVE_DELAY) { + this._sendMouse(x, y, this._mouseButtonMask); + this._mouseLastMoveTime = Date.now(); + } else { + // Too soon since the latest move, wait the remaining time + this._mouseMoveTimer = setTimeout(() => { + this._handleDelayedMouseMove(); + }, MOUSE_MOVE_DELAY - timeSinceLastMove); + } + } + } + + _handleDelayedMouseMove() { + this._mouseMoveTimer = null; + this._sendMouse(this._mousePos.x, this._mousePos.y, + this._mouseButtonMask); + this._mouseLastMoveTime = Date.now(); + } + + _sendMouse(x, y, mask) { + if (this._rfbConnectionState !== 'connected') { return; } if (this._viewOnly) { return; } // View only, skip mouse events - if (this._rfbConnectionState !== 'connected') { return; } - RFB.messages.pointerEvent(this._sock, this._display.absX(x), this._display.absY(y), this._mouseButtonMask); + RFB.messages.pointerEvent(this._sock, this._display.absX(x), + this._display.absY(y), mask); } // Message Handlers diff --git a/kasmweb/tests/test.mouse.js b/kasmweb/tests/test.mouse.js index 5636a71..8e066c1 100644 --- a/kasmweb/tests/test.mouse.js +++ b/kasmweb/tests/test.mouse.js @@ -300,72 +300,4 @@ describe('Mouse Event Handling', function () { expect(mouse.onmousebutton).to.have.callCount(4); // mouse down and up }); }); - - describe('Move events should be limited to one each 17 ms', function () { - - let mouse; - beforeEach(function () { - this.clock = sinon.useFakeTimers(Date.now()); - mouse = new Mouse(target); - mouse.onmousemove = sinon.spy(); - }); - afterEach(function () { - this.clock.restore(); - }); - - it('should send a single move instantly', function () { - mouse._handleMouseMove(mouseevent( - 'mousemove', { clientX: 1, clientY: 2 })); - - expect(mouse.onmousemove).to.have.callCount(1); - }); - - it('should delay one if two events are too close', function () { - mouse._handleMouseMove(mouseevent( - 'mousemove', { clientX: 18, clientY: 30 })); - mouse._handleMouseMove(mouseevent( - 'mousemove', { clientX: 20, clientY: 50 })); - - expect(mouse.onmousemove).to.have.callCount(1); - - this.clock.tick(100); - - expect(mouse.onmousemove).to.have.callCount(2); - }); - - it('should only send first and last of many close events', function () { - mouse._handleMouseMove(mouseevent( - 'mousemove', { clientX: 18, clientY: 30 })); - mouse._handleMouseMove(mouseevent( - 'mousemove', { clientX: 20, clientY: 50 })); - mouse._handleMouseMove(mouseevent( - 'mousemove', { clientX: 21, clientY: 55 })); - - // Check positions to verify that the correct calls got through. - // - // The test canvas starts 10px from top and 10 px from left, - // that means the relative coordinates are clientCoords - 10px - expect(mouse.onmousemove).to.have.been.calledWith(8, 20); - - this.clock.tick(60); - - expect(mouse.onmousemove).to.have.callCount(2); - expect(mouse.onmousemove).to.have.been.calledWith(11, 45); - }); - - it('should send events with enough time apart normally', function () { - mouse._handleMouseMove(mouseevent( - 'mousemove', { clientX: 58, clientY: 60 })); - - expect(mouse.onmousemove).to.have.callCount(1); - - this.clock.tick(20); - - mouse._handleMouseMove(mouseevent( - 'mousemove', { clientX: 25, clientY: 60 })); - - expect(mouse.onmousemove).to.have.callCount(2); - }); - }); - }); diff --git a/kasmweb/tests/test.rfb.js b/kasmweb/tests/test.rfb.js index b7b7e3d..fe3800a 100644 --- a/kasmweb/tests/test.rfb.js +++ b/kasmweb/tests/test.rfb.js @@ -2730,56 +2730,145 @@ describe('Remote Frame Buffer Protocol Client', function () { }); describe('Mouse event handlers', function () { + beforeEach(function () { + this.clock = sinon.useFakeTimers(Date.now()); + sinon.spy(RFB.messages, 'pointerEvent'); + }); + afterEach(function () { + this.clock.restore(); + RFB.messages.pointerEvent.restore(); + }); + it('should not send button messages in view-only mode', function () { client._viewOnly = true; - sinon.spy(client._sock, 'flush'); client._handleMouseButton(0, 0, 1, 0x001); - expect(client._sock.flush).to.not.have.been.called; + expect(RFB.messages.pointerEvent).to.not.have.been.called; }); it('should not send movement messages in view-only mode', function () { client._viewOnly = true; - sinon.spy(client._sock, 'flush'); client._handleMouseMove(0, 0); - expect(client._sock.flush).to.not.have.been.called; + expect(RFB.messages.pointerEvent).to.not.have.been.called; }); it('should send a pointer event on mouse button presses', function () { client._handleMouseButton(10, 12, 1, 0x001); - const pointer_msg = {_sQ: new Uint8Array(6), _sQlen: 0, flush: () => {}}; - RFB.messages.pointerEvent(pointer_msg, 10, 12, 0x001); - expect(client._sock).to.have.sent(pointer_msg._sQ); + expect(RFB.messages.pointerEvent).to.have.been.calledOnce; }); it('should send a mask of 1 on mousedown', function () { - client._handleMouseButton(10, 12, 1, 0x001); - const pointer_msg = {_sQ: new Uint8Array(6), _sQlen: 0, flush: () => {}}; - RFB.messages.pointerEvent(pointer_msg, 10, 12, 0x001); - expect(client._sock).to.have.sent(pointer_msg._sQ); + client._handleMouseButton(11, 13, 1, 0x001); + expect(RFB.messages.pointerEvent).to.have.been.calledWith( + client._sock, 11, 13, 0x001); }); it('should send a mask of 0 on mouseup', function () { client._mouse_buttonMask = 0x001; - client._handleMouseButton(10, 12, 0, 0x001); - const pointer_msg = {_sQ: new Uint8Array(6), _sQlen: 0, flush: () => {}}; - RFB.messages.pointerEvent(pointer_msg, 10, 12, 0x000); - expect(client._sock).to.have.sent(pointer_msg._sQ); + client._handleMouseButton(105, 120, 0, 0x001); + expect(RFB.messages.pointerEvent).to.have.been.calledWith( + client._sock, 105, 120, 0x000); }); - it('should send a pointer event on mouse movement', function () { - client._handleMouseMove(10, 12); - const pointer_msg = {_sQ: new Uint8Array(6), _sQlen: 0, flush: () => {}}; - RFB.messages.pointerEvent(pointer_msg, 10, 12, 0x000); - expect(client._sock).to.have.sent(pointer_msg._sQ); + it('should send a mask of 0 on mousemove', function () { + client._handleMouseMove(100, 200); + expect(RFB.messages.pointerEvent).to.have.been.calledWith( + client._sock, 100, 200, 0x000); }); it('should set the button mask so that future mouse movements use it', function () { client._handleMouseButton(10, 12, 1, 0x010); client._handleMouseMove(13, 9); - const pointer_msg = {_sQ: new Uint8Array(12), _sQlen: 0, flush: () => {}}; - RFB.messages.pointerEvent(pointer_msg, 10, 12, 0x010); - RFB.messages.pointerEvent(pointer_msg, 13, 9, 0x010); - expect(client._sock).to.have.sent(pointer_msg._sQ); + expect(RFB.messages.pointerEvent).to.have.been.calledTwice; + expect(RFB.messages.pointerEvent).to.have.been.calledWith( + client._sock, 13, 9, 0x010); + }); + + it('should send a single pointer event on mouse movement', function () { + client._handleMouseMove(100, 200); + this.clock.tick(100); + expect(RFB.messages.pointerEvent).to.have.been.calledOnce; + }); + + it('should delay one move if two events are too close', function () { + client._handleMouseMove(18, 30); + client._handleMouseMove(20, 50); + expect(RFB.messages.pointerEvent).to.have.been.calledOnce; + this.clock.tick(100); + expect(RFB.messages.pointerEvent).to.have.been.calledTwice; + }); + + it('should only send first and last move of many close events', function () { + client._handleMouseMove(18, 40); + client._handleMouseMove(20, 50); + client._handleMouseMove(21, 55); + + expect(RFB.messages.pointerEvent).to.have.been.calledOnce; + this.clock.tick(60); + + expect(RFB.messages.pointerEvent).to.have.been.calledTwice; + expect(RFB.messages.pointerEvent.firstCall).to.have.been.calledWith( + client._sock, 18, 40, 0x000); + expect(RFB.messages.pointerEvent.secondCall).to.have.been.calledWith( + client._sock, 21, 55, 0x000); + }); + + // We selected the 17ms since that is ~60 FPS + it('should send move events every 17 ms', function () { + client._handleMouseMove(1, 10); // instant send + this.clock.tick(10); + client._handleMouseMove(2, 20); // delayed + this.clock.tick(10); // timeout send + client._handleMouseMove(3, 30); // delayed + this.clock.tick(10); + client._handleMouseMove(4, 40); // delayed + this.clock.tick(10); // timeout send + client._handleMouseMove(5, 50); // delayed + + expect(RFB.messages.pointerEvent).to.have.callCount(3); + expect(RFB.messages.pointerEvent.firstCall).to.have.been.calledWith( + client._sock, 1, 10, 0x000); + expect(RFB.messages.pointerEvent.secondCall).to.have.been.calledWith( + client._sock, 2, 20, 0x000); + expect(RFB.messages.pointerEvent.thirdCall).to.have.been.calledWith( + client._sock, 4, 40, 0x000); + }); + + it('should send waiting move events before a button press', function () { + client._handleMouseMove(13, 9); + client._handleMouseMove(20, 70); + client._handleMouseButton(10, 12, 1, 0x100); + expect(RFB.messages.pointerEvent).to.have.been.calledThrice; + expect(RFB.messages.pointerEvent.firstCall).to.have.been.calledWith( + client._sock, 13, 9, 0x000); + expect(RFB.messages.pointerEvent.secondCall).to.have.been.calledWith( + client._sock, 10, 12, 0x000); + expect(RFB.messages.pointerEvent.thirdCall).to.have.been.calledWith( + client._sock, 10, 12, 0x100); + }); + + it('should not delay events when button mask changes', function () { + client._handleMouseMove(13, 9); // instant + client._handleMouseMove(11, 10); // delayed + client._handleMouseButton(10, 12, 1, 0x010); // flush delayed + expect(RFB.messages.pointerEvent).to.have.been.calledThrice; + }); + + it('should send move events with enough time apart normally', function () { + client._handleMouseMove(58, 60); + expect(RFB.messages.pointerEvent).to.have.been.calledOnce; + + this.clock.tick(20); + + client._handleMouseMove(25, 60); + expect(RFB.messages.pointerEvent).to.have.been.calledTwice; + }); + + it('should not send waiting move events if disconnected', function () { + client._handleMouseMove(88, 99); + client._handleMouseMove(66, 77); + client.disconnect(); + this.clock.tick(20); + expect(RFB.messages.pointerEvent).to.have.been.calledOnce; }); });