Merge branch 'fps' of https://github.com/CendioOssman/tigervnc

7 files changed, 175 insertions, 160 deletions

diff --git a/common/rfb/ServerCore.cxx b/common/rfb/ServerCore.cxx
index 6e221d53..59a7cff3 100644
--- a/common/rfb/ServerCore.cxx
+++ b/common/rfb/ServerCore.cxx
@@ -52,6 +52,10 @@ rfb::IntParameter rfb::Server::compareFB
  "Perform pixel comparison on framebuffer to reduce unnecessary updates "
  "(0: never, 1: always, 2: auto)",
  2);
+rfb::IntParameter rfb::Server::frameRate
+("FrameRate",
+ "The maximum number of updates per second sent to each client",
+ 60);
 rfb::BoolParameter rfb::Server::protocol3_3
 ("Protocol3.3",
  "Always use protocol version 3.3 for backwards compatibility with "
diff --git a/common/rfb/ServerCore.h b/common/rfb/ServerCore.h
index c4d7d537..37923cc1 100644
--- a/common/rfb/ServerCore.h
+++ b/common/rfb/ServerCore.h
@@ -38,6 +38,7 @@ namespace rfb {
     static IntParameter maxIdleTime;
     static IntParameter clientWaitTimeMillis;
     static IntParameter compareFB;
+    static IntParameter frameRate;
     static BoolParameter protocol3_3;
     static BoolParameter alwaysShared;
     static BoolParameter neverShared;
diff --git a/common/rfb/Timer.cxx b/common/rfb/Timer.cxx
index 676f24e1..efae36e2 100644
--- a/common/rfb/Timer.cxx
+++ b/common/rfb/Timer.cxx
@@ -1,4 +1,5 @@
 /* Copyright (C) 2002-2005 RealVNC Ltd.  All Rights Reserved.
+ * Copyright 2016 Pierre Ossman for Cendio AB
  * 
  * This is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -58,20 +59,35 @@ inline static int diffTimeMillis(timeval later, timeval earlier) {
 std::list<Timer*> Timer::pending;
 
 int Timer::checkTimeouts() {
+  timeval start;
+
   if (pending.empty())
     return 0;
-  timeval now;
-  gettimeofday(&now, 0);
-  while (pending.front()->isBefore(now)) {
-    Timer* timer = pending.front();
+
+  gettimeofday(&start, 0);
+  while (pending.front()->isBefore(start)) {
+    Timer* timer;
+    timeval before;
+
+    timer = pending.front();
     pending.pop_front();
+
+    gettimeofday(&before, 0);
     if (timer->cb->handleTimeout(timer)) {
+      timeval now;
+
+      gettimeofday(&now, 0);
+
       timer->dueTime = addMillis(timer->dueTime, timer->timeoutMs);
       if (timer->isBefore(now)) {
-        // Time has jumped forwards!
-	      vlog.info("time has moved forwards!");
-        timer->dueTime = addMillis(now, timer->timeoutMs);
+        // Time has jumped forwards, or we're not getting enough
+        // CPU time for the timers
+
+        timer->dueTime = addMillis(before, timer->timeoutMs);
+        if (timer->isBefore(now))
+          timer->dueTime = now;
       }
+
       insertTimer(timer);
     } else if (pending.empty()) {
       return 0;
diff --git a/common/rfb/VNCSConnectionST.cxx b/common/rfb/VNCSConnectionST.cxx
index 5ab1b4b2..0a2ca334 100644
--- a/common/rfb/VNCSConnectionST.cxx
+++ b/common/rfb/VNCSConnectionST.cxx
@@ -1,5 +1,5 @@
 /* Copyright (C) 2002-2005 RealVNC Ltd.  All Rights Reserved.
- * Copyright 2009-2015 Pierre Ossman for Cendio AB
+ * Copyright 2009-2016 Pierre Ossman for Cendio AB
  * 
  * This is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -377,9 +377,16 @@ void VNCSConnectionST::renderedCursorChange()
 
 bool VNCSConnectionST::needRenderedCursor()
 {
-  bool pointerpos = (!server->cursorPos.equals(pointerEventPos) && (time(0) - pointerEventTime) > 0);
-  return (state() == RFBSTATE_NORMAL
-          && ((!cp.supportsLocalCursor && !cp.supportsLocalXCursor) || pointerpos));
+  if (state() != RFBSTATE_NORMAL)
+    return false;
+
+  if (!cp.supportsLocalCursor && !cp.supportsLocalXCursor)
+    return true;
+  if (!server->cursorPos.equals(pointerEventPos) &&
+      (time(0) - pointerEventTime) > 0)
+    return true;
+
+  return false;
 }
 
 
@@ -602,7 +609,6 @@ void VNCSConnectionST::framebufferUpdateRequest(const Rect& r,bool incremental)
   if (!incremental) {
     // Non-incremental update - treat as if area requested has changed
     updates.add_changed(reqRgn);
-    server->comparer->add_changed(reqRgn);
 
     // And send the screen layout to the client (which, unlike the
     // framebuffer dimensions, the client doesn't get during init)
@@ -936,11 +942,6 @@ void VNCSConnectionST::updateCongestion()
 
 void VNCSConnectionST::writeFramebufferUpdate()
 {
-  Region req;
-  UpdateInfo ui;
-  bool needNewUpdateInfo;
-  bool drawRenderedCursor;
-
   // We're in the middle of processing a command that's supposed to be
   // synchronised. Allowing an update to slip out right now might violate
   // that synchronisation.
@@ -963,37 +964,59 @@ void VNCSConnectionST::writeFramebufferUpdate()
   if (isCongested())
     return;
 
-  // In continuous mode, we will be outputting at least three distinct
-  // messages. We need to aggregate these in order to not clog up TCP's
-  // congestion window.
+  // Updates often consists of many small writes, and in continuous
+  // mode, we will also have small fence messages around the update. We
+  // need to aggregate these in order to not clog up TCP's congestion
+  // window.
   network::TcpSocket::cork(sock->getFd(), true);
 
   // First take care of any updates that cannot contain framebuffer data
   // changes.
-  if (writer()->needNoDataUpdate()) {
-    writer()->writeNoDataUpdate();
-    requested.clear();
-    if (!continuousUpdates)
-      goto out;
-  }
+  writeNoDataUpdate();
+
+  // Then real data (if possible)
+  writeDataUpdate();
+
+  network::TcpSocket::cork(sock->getFd(), false);
+}
+
+void VNCSConnectionST::writeNoDataUpdate()
+{
+  if (!writer()->needNoDataUpdate())
+    return;
+
+  writer()->writeNoDataUpdate();
+
+  // Make sure no data update is sent until next request
+  requested.clear();
+}
+
+void VNCSConnectionST::writeDataUpdate()
+{
+  Region req;
+  UpdateInfo ui;
+  bool needNewUpdateInfo;
+  const RenderedCursor *cursor;
 
   updates.enable_copyrect(cp.useCopyRect);
 
-  // Fetch updates from server object, and see if we are allowed to send
-  // anything right now (the framebuffer might have changed in ways we
-  // haven't yet been informed of).
+  // See if we are allowed to send anything right now (the framebuffer
+  // might have changed in ways we haven't yet been informed of).
   if (!server->checkUpdate())
-    goto out;
-
-  // Get the lists of updates. Prior to exporting the data to the `ui' object,
-  // getUpdateInfo() will normalize the `updates' object such way that its
-  // `changed' and `copied' regions would not intersect.
+    return;
 
+  // See what the client has requested (if anything)
   if (continuousUpdates)
     req = cuRegion.union_(requested);
   else
     req = requested;
 
+  if (req.is_empty())
+    return;
+
+  // Get the lists of updates. Prior to exporting the data to the `ui' object,
+  // getUpdateInfo() will normalize the `updates' object such way that its
+  // `changed' and `copied' regions would not intersect.
   updates.getUpdateInfo(&ui, req);
   needNewUpdateInfo = false;
 
@@ -1026,7 +1049,7 @@ void VNCSConnectionST::writeFramebufferUpdate()
   // Return if there is nothing to send the client.
 
   if (updates.is_empty() && !writer()->needFakeUpdate() && !updateRenderedCursor)
-    goto out;
+    return;
 
   // The `updates' object could change, make sure we have valid update info.
 
@@ -1038,59 +1061,46 @@ void VNCSConnectionST::writeFramebufferUpdate()
   // with the update region, we need to draw the rendered cursor regardless of
   // whether it has changed.
 
-  drawRenderedCursor = false;
+  cursor = NULL;
   if (needRenderedCursor()) {
     Rect renderedCursorRect;
 
+    cursor = server->getRenderedCursor();
+
     renderedCursorRect
-      = server->renderedCursor.getEffectiveRect()
-         .intersect(req.get_bounding_rect());
+      = cursor->getEffectiveRect().intersect(req.get_bounding_rect());
 
     if (renderedCursorRect.is_empty()) {
-      drawRenderedCursor = false;
-    } else if (updateRenderedCursor) {
-      drawRenderedCursor = true;
-    } else if (!ui.changed.union_(ui.copied)
+      cursor = NULL;
+    } else if (!updateRenderedCursor &&
+               ui.changed.union_(ui.copied)
                .intersect(renderedCursorRect).is_empty()) {
-      drawRenderedCursor = true;
+      cursor = NULL;
     }
 
-    // We could remove the new cursor rect from updates here.  It's not clear
-    // whether this is worth it.  If we do remove it, then we won't draw over
-    // the same bit of screen twice, but we have the overhead of a more complex
-    // region.
-
-    //if (drawRenderedCursor) {
-    //  updates.subtract(renderedCursorRect);
-    //  updates.getUpdateInfo(&ui, req);
-    //}
+    if (cursor) {
+      updates.subtract(renderedCursorRect);
+      updates.getUpdateInfo(&ui, req);
+    }
 
     damagedCursorRegion.assign_union(renderedCursorRect);
     updateRenderedCursor = false;
   }
 
-  if (!ui.is_empty() || writer()->needFakeUpdate() || drawRenderedCursor) {
-    RenderedCursor *cursor;
-
-    cursor = NULL;
-    if (drawRenderedCursor)
-      cursor = &server->renderedCursor;
-
-    writeRTTPing();
+  if (ui.is_empty() && !writer()->needFakeUpdate() && !cursor)
+    return;
 
-    encodeManager.writeUpdate(ui, server->getPixelBuffer(), cursor);
+  writeRTTPing();
 
-    writeRTTPing();
+  encodeManager.writeUpdate(ui, server->getPixelBuffer(), cursor);
 
-    // The request might be for just part of the screen, so we cannot
-    // just clear the entire update tracker.
-    updates.subtract(req);
+  writeRTTPing();
 
-    requested.clear();
-  }
+  // The request might be for just part of the screen, so we cannot
+  // just clear the entire update tracker.
+  updates.subtract(req);
 
-out:
-  network::TcpSocket::cork(sock->getFd(), false);
+  requested.clear();
 }
 
 
diff --git a/common/rfb/VNCSConnectionST.h b/common/rfb/VNCSConnectionST.h
index 3f0163a3..74a6946d 100644
--- a/common/rfb/VNCSConnectionST.h
+++ b/common/rfb/VNCSConnectionST.h
@@ -1,5 +1,5 @@
 /* Copyright (C) 2002-2005 RealVNC Ltd.  All Rights Reserved.
- * Copyright 2009-2011 Pierre Ossman for Cendio AB
+ * Copyright 2009-2016 Pierre Ossman for Cendio AB
  * 
  * This is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -168,6 +168,8 @@ namespace rfb {
     // client.
 
     void writeFramebufferUpdate();
+    void writeNoDataUpdate();
+    void writeDataUpdate();
 
     void screenLayoutChange(rdr::U16 reason);
     void setCursor();
diff --git a/common/rfb/VNCServerST.cxx b/common/rfb/VNCServerST.cxx
index 81eed37a..ec5e962f 100644
--- a/common/rfb/VNCServerST.cxx
+++ b/common/rfb/VNCServerST.cxx
@@ -66,12 +66,6 @@ using namespace rfb;
 static LogWriter slog("VNCServerST");
 LogWriter VNCServerST::connectionsLog("Connections");
 
-rfb::IntParameter deferUpdateTime("DeferUpdate",
-                                  "Time in milliseconds to defer updates",1);
-
-rfb::BoolParameter alwaysSetDeferUpdateTimer("AlwaysSetDeferUpdateTimer",
-                  "Always reset the defer update timer on every change",false);
-
 //
 // -=- VNCServerST Implementation
 //
@@ -86,7 +80,7 @@ VNCServerST::VNCServerST(const char* name_, SDesktop* desktop_)
     renderedCursorInvalid(false),
     queryConnectionHandler(0), keyRemapper(&KeyRemapper::defInstance),
     lastConnectionTime(0), disableclients(false),
-    deferTimer(this), deferPending(false)
+    frameTimer(this)
 {
   lastUserInputTime = lastDisconnectTime = time(0);
   slog.debug("creating single-threaded server %s", name.buf);
@@ -99,6 +93,9 @@ VNCServerST::~VNCServerST()
   // Close any active clients, with appropriate logging & cleanup
   closeClients("Server shutdown");
 
+  // Stop trying to render things
+  stopFrameClock();
+
   // Delete all the clients, and their sockets, and any closing sockets
   //   NB: Deleting a client implicitly removes it from the clients list
   while (!clients.empty()) {
@@ -286,6 +283,8 @@ int VNCServerST::checkTimeouts()
 void VNCServerST::blockUpdates()
 {
   blockCounter++;
+
+  stopFrameClock();
 }
 
 void VNCServerST::unblockUpdates()
@@ -294,9 +293,11 @@ void VNCServerST::unblockUpdates()
 
   blockCounter--;
 
-  // Flush out any updates we might have blocked
-  if (blockCounter == 0)
-    tryUpdate();
+  // Restart the frame clock if we have updates
+  if (blockCounter == 0) {
+    if (!comparer->is_empty())
+      startFrameClock();
+  }
 }
 
 void VNCServerST::setPixelBuffer(PixelBuffer* pb_, const ScreenSet& layout)
@@ -417,8 +418,7 @@ void VNCServerST::add_changed(const Region& region)
     return;
 
   comparer->add_changed(region);
-  startDefer();
-  tryUpdate();
+  startFrameClock();
 }
 
 void VNCServerST::add_copied(const Region& dest, const Point& delta)
@@ -427,8 +427,7 @@ void VNCServerST::add_copied(const Region& dest, const Point& delta)
     return;
 
   comparer->add_copied(dest, delta);
-  startDefer();
-  tryUpdate();
+  startFrameClock();
 }
 
 void VNCServerST::setCursor(int width, int height, const Point& newHotspot,
@@ -507,10 +506,14 @@ SConnection* VNCServerST::getSConnection(network::Socket* sock) {
 
 bool VNCServerST::handleTimeout(Timer* t)
 {
-  if (t != &deferTimer)
-    return false;
+  if (t == &frameTimer) {
+    // We keep running until we go a full interval without any updates
+    if (comparer->is_empty())
+      return false;
 
-  tryUpdate();
+    writeUpdate();
+    return true;
+  }
 
   return false;
 }
@@ -546,87 +549,47 @@ inline bool VNCServerST::needRenderedCursor()
   return false;
 }
 
-inline void VNCServerST::startDefer()
+void VNCServerST::startFrameClock()
 {
-  if (deferUpdateTime == 0)
+  if (frameTimer.isStarted())
     return;
-
-  if (deferPending && !alwaysSetDeferUpdateTimer)
+  if (blockCounter > 0)
     return;
 
-  gettimeofday(&deferStart, NULL);
-  deferTimer.start(deferUpdateTime);
-
-  deferPending = true;
+  frameTimer.start(1000/rfb::Server::frameRate);
 }
 
-inline bool VNCServerST::checkDefer()
+void VNCServerST::stopFrameClock()
 {
-  if (!deferPending)
-    return true;
-
-  if (msSince(&deferStart) >= (unsigned)deferUpdateTime)
-    return true;
-
-  return false;
+  frameTimer.stop();
 }
 
-void VNCServerST::tryUpdate()
-{
-  std::list<VNCSConnectionST*>::iterator ci, ci_next;
-
-  if (blockCounter > 0)
-    return;
-
-  if (!checkDefer())
-    return;
-
-  for (ci = clients.begin(); ci != clients.end(); ci = ci_next) {
-    ci_next = ci; ci_next++;
-    (*ci)->writeFramebufferUpdateOrClose();
-  }
-}
+// writeUpdate() is called on a regular interval in order to see what
+// updates are pending and propagates them to the update tracker for
+// each client. It uses the ComparingUpdateTracker's compare() method
+// to filter out areas of the screen which haven't actually changed. It
+// also checks the state of the (server-side) rendered cursor, if
+// necessary rendering it again with the correct background.
 
-// checkUpdate() is called just before sending an update.  It checks to see
-// what updates are pending and propagates them to the update tracker for each
-// client.  It uses the ComparingUpdateTracker's compare() method to filter out
-// areas of the screen which haven't actually changed.  It also checks the
-// state of the (server-side) rendered cursor, if necessary rendering it again
-// with the correct background.
-
-bool VNCServerST::checkUpdate()
+void VNCServerST::writeUpdate()
 {
   UpdateInfo ui;
-  comparer->getUpdateInfo(&ui, pb->getRect());
-
-  bool renderCursor = needRenderedCursor();
+  Region toCheck;
 
-  if (ui.is_empty() && !(renderCursor && renderedCursorInvalid))
-    return true;
-
-  // Block clients as the frame buffer cannot be safely accessed
-  if (blockCounter > 0)
-    return false;
-
-  // Block client from updating if we are currently deferring updates
-  if (!checkDefer())
-    return false;
+  std::list<VNCSConnectionST*>::iterator ci, ci_next;
 
-  deferPending = false;
+  assert(blockCounter == 0);
 
-  Region toCheck = ui.changed.union_(ui.copied);
+  comparer->getUpdateInfo(&ui, pb->getRect());
+  toCheck = ui.changed.union_(ui.copied);
 
-  if (renderCursor) {
+  if (needRenderedCursor()) {
     Rect clippedCursorRect = Rect(0, 0, cursor->width(), cursor->height())
                              .translate(cursorPos.subtract(cursor->hotspot()))
                              .intersect(pb->getRect());
 
-    if (!renderedCursorInvalid && (toCheck.intersect(clippedCursorRect)
-                                   .is_empty())) {
-      renderCursor = false;
-    } else {
-      toCheck.assign_union(clippedCursorRect);
-    }
+    if (!toCheck.intersect(clippedCursorRect).is_empty())
+      renderedCursorInvalid = true;
   }
 
   pb->grabRegion(toCheck);
@@ -639,23 +602,42 @@ bool VNCServerST::checkUpdate()
   if (comparer->compare())
     comparer->getUpdateInfo(&ui, pb->getRect());
 
-  if (renderCursor) {
-    renderedCursor.update(pb, cursor, cursorPos);
-    renderedCursorInvalid = false;
-  }
+  comparer->clear();
 
-  std::list<VNCSConnectionST*>::iterator ci, ci_next;
   for (ci = clients.begin(); ci != clients.end(); ci = ci_next) {
     ci_next = ci; ci_next++;
     (*ci)->add_copied(ui.copied, ui.copy_delta);
     (*ci)->add_changed(ui.changed);
+    (*ci)->writeFramebufferUpdateOrClose();
   }
+}
 
-  comparer->clear();
+// checkUpdate() is called by clients to see if it is safe to read from
+// the framebuffer at this time.
+
+bool VNCServerST::checkUpdate()
+{
+  // Block clients as the frame buffer cannot be safely accessed
+  if (blockCounter > 0)
+    return false;
+
+  // Block client from updating if there are pending updates
+  if (!comparer->is_empty())
+    return false;
 
   return true;
 }
 
+const RenderedCursor* VNCServerST::getRenderedCursor()
+{
+  if (renderedCursorInvalid) {
+    renderedCursor.update(pb, cursor, cursorPos);
+    renderedCursorInvalid = false;
+  }
+
+  return &renderedCursor;
+}
+
 void VNCServerST::getConnInfo(ListConnInfo * listConn)
 {
   listConn->Clear();
diff --git a/common/rfb/VNCServerST.h b/common/rfb/VNCServerST.h
index 49757164..00f77c73 100644
--- a/common/rfb/VNCServerST.h
+++ b/common/rfb/VNCServerST.h
@@ -1,4 +1,5 @@
 /* Copyright (C) 2002-2005 RealVNC Ltd.  All Rights Reserved.
+ * Copyright 2009-2016 Pierre Ossman for Cendio AB
  * 
  * This is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -226,10 +227,11 @@ namespace rfb {
     int authClientCount();
 
     bool needRenderedCursor();
-    void startDefer();
-    bool checkDefer();
-    void tryUpdate();
+    void startFrameClock();
+    void stopFrameClock();
+    void writeUpdate();
     bool checkUpdate();
+    const RenderedCursor* getRenderedCursor();
 
     void notifyScreenLayoutChange(VNCSConnectionST *requester);
 
@@ -244,9 +246,7 @@ namespace rfb {
 
     bool disableclients;
 
-    Timer deferTimer;
-    bool deferPending;
-    struct timeval deferStart;
+    Timer frameTimer;
   };
 
 };