Properly limit mouse moves to once every 17 mslimitmouse

Previous attempt in c958269 had a number of issues, this is a full
rewrite, complete with improved unit tests.

Fixes github issue #1402

4 files changed, 164 insertions, 124 deletions

diff --git a/core/input/mouse.js b/core/input/mouse.js
index d479d5c..5f1b6b4 100644
--- a/core/input/mouse.js
+++ b/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/core/rfb.js b/core/rfb.js
index 351e4b3..07d5bc0 100644
--- a/core/rfb.js
+++ b/core/rfb.js
@@ -36,6 +36,9 @@ import TightPNGDecoder from "./decoders/tightpng.js";
 const DISCONNECT_TIMEOUT = 3;
 const DEFAULT_BACKGROUND = 'rgb(40, 40, 40)';
 
+// 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 */
@@ -119,6 +122,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 = {};
@@ -133,7 +137,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;
@@ -529,6 +535,7 @@ export default class RFB extends EventTargetMixin {
             }
         }
         clearTimeout(this._resizeTimeout);
+        clearTimeout(this._mouseMoveTimer);
         Log.Debug("<< RFB.disconnect");
     }
 
@@ -813,12 +820,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;
@@ -836,22 +837,27 @@ 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.
-                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
+        // Flush waiting move event first
+        if (this._mouseMoveTimer !== null) {
+            clearTimeout(this._mouseMoveTimer);
+            this._mouseMoveTimer = null;
+            this._sendMouse(x, y, this._mouseButtonMask);
+        }
 
-        if (this._rfbConnectionState !== 'connected') { return; }
-        RFB.messages.pointerEvent(this._sock, this._display.absX(x), this._display.absY(y), this._mouseButtonMask);
+        if (down) {
+            this._mouseButtonMask |= bmask;
+        } else {
+            this._mouseButtonMask &= ~bmask;
+        }
+
+        this._sendMouse(x, y, this._mouseButtonMask);
     }
 
     _handleMouseMove(x, y) {
@@ -871,10 +877,37 @@ export default class RFB extends EventTargetMixin {
             return;
         }
 
-        if (this._viewOnly) { return; } // View only, skip mouse events
+        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; }
-        RFB.messages.pointerEvent(this._sock, this._display.absX(x), this._display.absY(y), this._mouseButtonMask);
+        if (this._viewOnly) { return; } // View only, skip mouse events
+
+        RFB.messages.pointerEvent(this._sock, this._display.absX(x),
+                                  this._display.absY(y), mask);
     }
 
     // Message Handlers
diff --git a/tests/test.mouse.js b/tests/test.mouse.js
index 5636a71..8e066c1 100644
--- a/tests/test.mouse.js
+++ b/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/tests/test.rfb.js b/tests/test.rfb.js
index 7c6128e..029a103 100644
--- a/tests/test.rfb.js
+++ b/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;
             });
         });