Reimplement the deferred update handling, this time in a more robust and

well-behaved manner.


git-svn-id: svn://svn.code.sf.net/p/tigervnc/code/trunk@4784 3789f03b-4d11-0410-bbf8-ca57d06f2519

common/rfb/VNCSConnectionST.cxx

@@ -668,7 +668,11 @@ void VNCSConnectionST::writeFramebufferUpdate()
 
   updates.enable_copyrect(cp.useCopyRect);
 
-  server->checkUpdate();
+  // 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).
+  if (!server->checkUpdate())
+    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

common/rfb/VNCServer.h

@@ -62,10 +62,6 @@ namespace rfb {
     //   their close() method with the supplied reason.
     virtual void closeClients(const char* reason) = 0;
 
-    // tryUpdate() causes the server to attempt to send updates to any waiting
-    // clients.
-    virtual void tryUpdate() = 0;
-
     // setCursor() tells the server that the cursor has changed.  The
     // cursorData argument contains width*height pixel values in the pixel
     // buffer's format.  The mask argument is a bitmask with a 1-bit meaning

common/rfb/VNCServerST.cxx

@@ -64,6 +64,12 @@ using namespace rfb;
 static LogWriter slog("VNCServerST");
 LogWriter VNCServerST::connectionsLog("Connections");
 
+rfb::IntParameter deferUpdateTime("DeferUpdate",
+                                  "Time in milliseconds to defer updates",10);
+
+rfb::BoolParameter alwaysSetDeferUpdateTimer("AlwaysSetDeferUpdateTimer",
+                  "Always reset the defer update timer on every change",false);
+
 //
 // -=- VNCServerST Implementation
 //
@@ -76,7 +82,8 @@ VNCServerST::VNCServerST(const char* name_, SDesktop* desktop_)
     renderedCursorInvalid(false),
     queryConnectionHandler(0), keyRemapper(&KeyRemapper::defInstance),
     useEconomicTranslate(false),
-    lastConnectionTime(0), disableclients(false)
+    lastConnectionTime(0), disableclients(false),
+    deferTimer(this), deferPending(false)
 {
   lastUserInputTime = lastDisconnectTime = time(0);
   slog.debug("creating single-threaded server %s", name.buf);
@@ -373,25 +380,22 @@ void VNCServerST::setName(const char* name_)
 
 void VNCServerST::add_changed(const Region& region)
 {
-  if (comparer != 0) {
-    comparer->add_changed(region);
-  }
+  if (comparer == NULL)
+    return;
+
+  comparer->add_changed(region);
+  startDefer();
+  tryUpdate();
 }
 
 void VNCServerST::add_copied(const Region& dest, const Point& delta)
 {
-  if (comparer != 0) {
-    comparer->add_copied(dest, delta);
-  }
-}
+  if (comparer == NULL)
+    return;
 
-void VNCServerST::tryUpdate()
-{
-  std::list<VNCSConnectionST*>::iterator ci, ci_next;
-  for (ci = clients.begin(); ci != clients.end(); ci = ci_next) {
-    ci_next = ci; ci_next++;
-    (*ci)->writeFramebufferUpdateOrClose();
-  }
+  comparer->add_copied(dest, delta);
+  startDefer();
+  tryUpdate();
 }
 
 void VNCServerST::setCursor(int width, int height, const Point& newHotspot,
@@ -471,6 +475,15 @@ SConnection* VNCServerST::getSConnection(network::Socket* sock) {
   return 0;
 }
 
+bool VNCServerST::handleTimeout(Timer* t)
+{
+  if (t != &deferTimer)
+    return false;
+
+  tryUpdate();
+
+  return false;
+}
 
 // -=- Internal methods
 
@@ -503,6 +516,44 @@ inline bool VNCServerST::needRenderedCursor()
   return false;
 }
 
+inline void VNCServerST::startDefer()
+{
+  if (deferUpdateTime == 0)
+    return;
+
+  if (deferPending && !alwaysSetDeferUpdateTimer)
+    return;
+
+  gettimeofday(&deferStart, NULL);
+  deferTimer.start(deferUpdateTime);
+
+  deferPending = true;
+}
+
+inline bool VNCServerST::checkDefer()
+{
+  if (!deferPending)
+    return true;
+
+  if (msSince(&deferStart) >= deferUpdateTime)
+    return true;
+
+  return false;
+}
+
+void VNCServerST::tryUpdate()
+{
+  std::list<VNCSConnectionST*>::iterator ci, ci_next;
+
+  if (!checkDefer())
+    return;
+
+  for (ci = clients.begin(); ci != clients.end(); ci = ci_next) {
+    ci_next = ci; ci_next++;
+    (*ci)->writeFramebufferUpdateOrClose();
+  }
+}
+
 // 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
@@ -510,7 +561,7 @@ inline bool VNCServerST::needRenderedCursor()
 // state of the (server-side) rendered cursor, if necessary rendering it again
 // with the correct background.
 
-void VNCServerST::checkUpdate()
+bool VNCServerST::checkUpdate()
 {
   UpdateInfo ui;
   comparer->getUpdateInfo(&ui, pb->getRect());
@@ -518,7 +569,13 @@ void VNCServerST::checkUpdate()
   bool renderCursor = needRenderedCursor();
 
   if (ui.is_empty() && !(renderCursor && renderedCursorInvalid))
-    return;
+    return true;
+
+  // Block client from updating if we are currently deferring updates
+  if (!checkDefer())
+    return false;
+
+  deferPending = false;
 
   Region toCheck = ui.changed.union_(ui.copied);
 
@@ -561,6 +618,8 @@ void VNCServerST::checkUpdate()
   }
 
   comparer->clear();
+
+  return true;
 }
 
 void VNCServerST::getConnInfo(ListConnInfo * listConn)

common/rfb/VNCServerST.h

@@ -23,6 +23,8 @@
 #ifndef __RFB_VNCSERVERST_H__
 #define __RFB_VNCSERVERST_H__
 
+#include <sys/time.h>
+
 #include <list>
 
 #include <rfb/SDesktop.h>
@@ -31,6 +33,7 @@
 #include <rfb/LogWriter.h>
 #include <rfb/Blacklist.h>
 #include <rfb/Cursor.h>
+#include <rfb/Timer.h>
 #include <network/Socket.h>
 #include <rfb/ListConnInfo.h>
 #include <rfb/ScreenSet.h>
@@ -42,7 +45,9 @@ namespace rfb {
   class PixelBuffer;
   class KeyRemapper;
 
-  class VNCServerST : public VNCServer, public network::SocketServer {
+  class VNCServerST : public VNCServer,
+                      public Timer::Callback,
+                      public network::SocketServer {
   public:
     // -=- Constructors
 
@@ -85,7 +90,6 @@ namespace rfb {
     virtual void serverCutText(const char* str, int len);
     virtual void add_changed(const Region &region);
     virtual void add_copied(const Region &dest, const Point &delta);
-    virtual void tryUpdate();
     virtual void setCursor(int width, int height, const Point& hotspot,
                            void* cursorData, void* mask);
     virtual void setCursorPos(const Point& p);
@@ -192,6 +196,11 @@ namespace rfb {
 
     friend class VNCSConnectionST;
 
+    // Timer callbacks
+    virtual bool handleTimeout(Timer* t);
+
+    // - Internal methods
+
     void startDesktop();
 
     static LogWriter connectionsLog;
@@ -222,7 +231,10 @@ namespace rfb {
     int authClientCount();
 
     bool needRenderedCursor();
-    void checkUpdate();
+    void startDefer();
+    bool checkDefer();
+    void tryUpdate();
+    bool checkUpdate();
 
     void notifyScreenLayoutChange(VNCSConnectionST *requester);
 
@@ -235,6 +247,10 @@ namespace rfb {
     time_t lastConnectionTime;
 
     bool disableclients;
+
+    Timer deferTimer;
+    bool deferPending;
+    struct timeval deferStart;
   };
 
 };

unix/x0vncserver/PollingManager.cxx

@@ -123,9 +123,7 @@ void PollingManager::poll(VNCServer *server)
   debugBeforePoll();
 #endif
 
-  // Perform polling and try update clients if changes were detected.
-  if (pollScreen(server))
-    server->tryUpdate();
+  pollScreen(server);
 
 #ifdef DEBUG
   debugAfterPoll();

unix/xserver/hw/vnc/Input.cc

@@ -207,7 +207,6 @@ void InputDevice::PointerSync(void)
 
 	oldCursorPos = cursorPos;
 	server->setCursorPos(cursorPos);
-	server->tryUpdate();
 }
 
 static int pointerProc(DeviceIntPtr pDevice, int onoff)

unix/xserver/hw/vnc/XserverDesktop.cc

@@ -64,12 +64,6 @@ using namespace network;
 
 static LogWriter vlog("XserverDesktop");
 
-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);
-
 IntParameter queryConnectTimeout("QueryConnectTimeout",
                                  "Number of seconds to show the Accept Connection dialog before "
                                  "rejecting the connection",
@@ -145,7 +139,7 @@ XserverDesktop::XserverDesktop(ScreenPtr pScreen_,
                                network::TcpListener* httpListener_,
                                const char* name, const rfb::PixelFormat &pf,
                                void* fbptr, int stride)
-  : pScreen(pScreen_), deferredUpdateTimer(0),
+  : pScreen(pScreen_),
     server(0), httpServer(0),
     listener(listener_), httpListener(httpListener_),
     cmap(0), deferredUpdateTimerSet(false),
@@ -171,7 +165,6 @@ XserverDesktop::~XserverDesktop()
 {
   if (!directFbptr)
     delete [] data;
-  TimerFree(deferredUpdateTimer);
   delete inputDevice;
   delete httpServer;
   delete server;
@@ -441,7 +434,6 @@ void XserverDesktop::setCursor(CursorPtr cursor)
     server->setCursor(cursor->bits->width, cursor->bits->height,
                       Point(cursor->bits->xhot, cursor->bits->yhot),
                       cursorData, cursorMask);
-    server->tryUpdate();
     delete [] cursorData;
     delete [] cursorMask;
   } catch (rdr::Exception& e) {
@@ -449,33 +441,6 @@ void XserverDesktop::setCursor(CursorPtr cursor)
   }
 }
 
-CARD32 XserverDesktop::deferredUpdateTimerCallback(OsTimerPtr timer,
-                                                   CARD32 now, pointer arg)
-{
-  XserverDesktop* desktop = (XserverDesktop*)arg;
-  desktop->deferredUpdateTimerSet = false;
-  try {
-    desktop->server->tryUpdate();
-  } catch (rdr::Exception& e) {
-    vlog.error("XserverDesktop::deferredUpdateTimerCallback: %s",e.str());
-  }
-  return 0;
-}
-
-void XserverDesktop::deferUpdate()
-{
-  if (deferUpdateTime != 0) {
-    if (!deferredUpdateTimerSet || alwaysSetDeferUpdateTimer) {
-      deferredUpdateTimerSet = true;
-      deferredUpdateTimer = TimerSet(deferredUpdateTimer, 0,
-                                     deferUpdateTime,
-                                     deferredUpdateTimerCallback, this);
-    }
-  } else {
-    server->tryUpdate();
-  }
-}
-
 void XserverDesktop::add_changed(RegionPtr reg)
 {
   if (ignoreHooks_) return;
@@ -486,7 +451,6 @@ void XserverDesktop::add_changed(RegionPtr reg)
                                      REGION_NUM_RECTS(reg),
                                      (ShortRect*)REGION_RECTS(reg));
     server->add_changed(rfbReg);
-    deferUpdate();
   } catch (rdr::Exception& e) {
     vlog.error("XserverDesktop::add_changed: %s",e.str());
   }
@@ -502,7 +466,6 @@ void XserverDesktop::add_copied(RegionPtr dst, int dx, int dy)
                                      REGION_NUM_RECTS(dst),
                                      (ShortRect*)REGION_RECTS(dst));
     server->add_copied(rfbReg, rfb::Point(dx, dy));
-    deferUpdate();
   } catch (rdr::Exception& e) {
     vlog.error("XserverDesktop::add_copied: %s",e.str());
   }

unix/xserver/hw/vnc/XserverDesktop.h

@@ -121,12 +121,8 @@ public:
 
 private:
   void setColourMapEntries(int firstColour, int nColours);
-  static CARD32 deferredUpdateTimerCallback(OsTimerPtr timer, CARD32 now,
-                                            pointer arg);
-  void deferUpdate();
   ScreenPtr pScreen;
   InputDevice *inputDevice;
-  OsTimerPtr deferredUpdateTimer;
   rfb::VNCServerST* server;
   rfb::HTTPServer* httpServer;
   network::TcpListener* listener;

win/rfb_win32/SDisplay.cxx

@@ -378,8 +378,6 @@ SDisplay::processEvent(HANDLE event) {
 
     // - Only process updates if the server is ready
     if (server) {
-      bool try_update = false;
-
       // - Check that the SDesktop doesn't need restarting
       if (isRestartRequired()) {
         restartCore();
@@ -411,16 +409,12 @@ SDisplay::processEvent(HANDLE event) {
         // NB: First translate from Screen coordinates to Desktop
         Point desktopPos = info.position.translate(screenRect.tl.negate());
         server->setCursorPos(desktopPos);
-        try_update = true;
 
         old_cursor = info;
       }
 
       // Flush any changes to the server
-      try_update = flushChangeTracker() || try_update;
-      if (try_update) {
-        server->tryUpdate();
-      }
+      flushChangeTracker();
     }
     return;
   }