Push encoder and decoder handling down into the connection objects

This keeps the reader and writer objects clean and simple protocol
decoders/encoders.

common/rfb/CMsgHandler.h

@@ -59,8 +59,7 @@ namespace rfb {
 
     virtual void framebufferUpdateStart() = 0;
     virtual void framebufferUpdateEnd() = 0;
-    virtual void beginRect(const Rect& r, int encoding) = 0;
-    virtual void endRect(const Rect& r, int encoding) = 0;
+    virtual void dataRect(const Rect& r, int encoding) = 0;
 
     virtual void setColourMapEntries(int firstColour, int nColours,
 				     rdr::U16* rgbs) = 0;

common/rfb/CMsgReader.cxx

@@ -31,16 +31,10 @@ CMsgReader::CMsgReader(CMsgHandler* handler_, rdr::InStream* is_)
   : imageBufIdealSize(0), handler(handler_), is(is_),
     imageBuf(0), imageBufSize(0), nUpdateRectsLeft(0)
 {
-  for (int i = 0; i <= encodingMax; i++) {
-    decoders[i] = 0;
-  }
 }
 
 CMsgReader::~CMsgReader()
 {
-  for (int i = 0; i <= encodingMax; i++) {
-    delete decoders[i];
-  }
   delete [] imageBuf;
 }
 
@@ -196,23 +190,7 @@ void CMsgReader::readRect(const Rect& r, int encoding)
   if (r.is_empty())
     fprintf(stderr, "Warning: zero size rect\n");
 
-  handler->beginRect(r, encoding);
-
-  if (!Decoder::supported(encoding)) {
-    fprintf(stderr, "Unknown rect encoding %d\n", encoding);
-    throw Exception("Unknown rect encoding");
-  }
-
-  if (!decoders[encoding]) {
-    decoders[encoding] = Decoder::createDecoder(encoding, this);
-    if (!decoders[encoding]) {
-      fprintf(stderr, "Unknown rect encoding %d\n", encoding);
-      throw Exception("Unknown rect encoding");
-    }
-  }
-  decoders[encoding]->readRect(r, handler);
-
-  handler->endRect(r, encoding);
+  handler->dataRect(r, encoding);
 }
 
 void CMsgReader::readSetCursor(int width, int height, const Point& hotspot)

common/rfb/CMsgReader.h

@@ -33,7 +33,6 @@ namespace rdr { class InStream; }
 
 namespace rfb {
   class CMsgHandler;
-  class Decoder;
   struct Rect;
 
   class CMsgReader {
@@ -69,7 +68,6 @@ namespace rfb {
 
     CMsgHandler* handler;
     rdr::InStream* is;
-    Decoder* decoders[encodingMax+1];
     rdr::U8* imageBuf;
     int imageBufSize;
     int nUpdateRectsLeft;

common/rfb/RREEncoder.cxx

@@ -33,7 +33,7 @@ using namespace rfb;
 #include <rfb/rreEncode.h>
 #undef BPP
 
-RREEncoder::RREEncoder(SMsgWriter* writer) : Encoder(writer)
+RREEncoder::RREEncoder(SMsgWriter* writer) : RawEncoder(writer)
 {
 }
 
@@ -58,7 +58,7 @@ void RREEncoder::writeRect(const Rect& r, TransImageGetter* ig)
   }
   
   if (nSubrects < 0) {
-    writer->writeRect(r, encodingRaw, ig);
+    RawEncoder::writeRect(r, ig);
     return;
   }
 

common/rfb/RREEncoder.h

@@ -19,11 +19,11 @@
 #define __RFB_RREENCODER_H__
 
 #include <rdr/MemOutStream.h>
-#include <rfb/Encoder.h>
+#include <rfb/RawEncoder.h>
 
 namespace rfb {
 
-  class RREEncoder : public Encoder {
+  class RREEncoder : public RawEncoder {
   public:
     RREEncoder(SMsgWriter* writer);
     virtual ~RREEncoder();

common/rfb/SMsgWriter.cxx

@@ -42,7 +42,6 @@ SMsgWriter::SMsgWriter(ConnParams* cp_, rdr::OutStream* os_)
     imageBuf(0), imageBufSize(0)
 {
   for (int i = 0; i <= encodingMax; i++) {
-    encoders[i] = 0;
     bytesSent[i] = 0;
     rectsSent[i] = 0;
   }
@@ -53,7 +52,6 @@ SMsgWriter::~SMsgWriter()
   vlog.info("framebuffer updates %d",updatesSent);
   int bytes = 0;
   for (int i = 0; i <= encodingMax; i++) {
-    delete encoders[i];
     if (i != encodingCopyRect)
       bytes += bytesSent[i];
     if (rectsSent[i])
@@ -135,32 +133,6 @@ void SMsgWriter::writeEndOfContinuousUpdates()
   endMsg();
 }
 
-void SMsgWriter::setupCurrentEncoder()
-{
-  int encoding = cp->currentEncoding();
-
-  // FIXME: Code duplication, see writeRect().
-  if (!encoders[encoding]) {
-    encoders[encoding] = Encoder::createEncoder(encoding, this);
-    assert(encoders[encoding]);
-  }
-
-  encoders[encoding]->setCompressLevel(cp->compressLevel);
-  encoders[encoding]->setQualityLevel(cp->qualityLevel);
-  encoders[encoding]->setFineQualityLevel(cp->fineQualityLevel,
-                                          cp->subsampling);
-}
-
-int SMsgWriter::getNumRects(const Rect &r)
-{
-  int encoding = cp->currentEncoding();
-
-  if (!encoders[encoding])
-    setupCurrentEncoder();
-
-  return encoders[encoding]->getNumRects(r);
-}
-
 bool SMsgWriter::writeSetDesktopSize() {
   if (!cp->supportsDesktopResize)
     return false;
@@ -290,20 +262,6 @@ void SMsgWriter::writeNoDataUpdate()
   writeFramebufferUpdateEnd();
 }
 
-void SMsgWriter::writeRects(const UpdateInfo& ui, TransImageGetter* ig)
-{
-  std::vector<Rect> rects;
-  std::vector<Rect>::const_iterator i;
-
-  ui.copied.get_rects(&rects, ui.copy_delta.x <= 0, ui.copy_delta.y <= 0);
-  for (i = rects.begin(); i != rects.end(); i++)
-    writeCopyRect(*i, i->tl.x - ui.copy_delta.x, i->tl.y - ui.copy_delta.y);
-
-  ui.changed.get_rects(&rects);
-  for (i = rects.begin(); i != rects.end(); i++)
-    writeRect(*i, ig);
-}
-
 void SMsgWriter::writeFramebufferUpdateStart(int nRects)
 {
   startMsg(msgTypeFramebufferUpdate);
@@ -346,20 +304,6 @@ void SMsgWriter::writeFramebufferUpdateEnd()
   endMsg();
 }
 
-void SMsgWriter::writeRect(const Rect& r, TransImageGetter* ig)
-{
-  writeRect(r, cp->currentEncoding(), ig);
-}
-
-void SMsgWriter::writeRect(const Rect& r, int encoding, TransImageGetter* ig)
-{
-  if (!encoders[encoding]) {
-    encoders[encoding] = Encoder::createEncoder(encoding, this);
-    assert(encoders[encoding]);
-  }
-  encoders[encoding]->writeRect(r, ig);
-}
-
 void SMsgWriter::writeCopyRect(const Rect& r, int srcX, int srcY)
 {
   startRect(r,encodingCopyRect);

common/rfb/SMsgWriter.h

@@ -32,10 +32,6 @@ namespace rdr { class OutStream; }
 namespace rfb {
 
   class ConnParams;
-  class TransImageGetter;
-  class Region;
-  class UpdateInfo;
-  class Encoder;
   class ScreenSet;
 
   class WriteSetCursorCallback {
@@ -72,14 +68,6 @@ namespace rfb {
     // updates mode.
     void writeEndOfContinuousUpdates();
 
-    // setupCurrentEncoder() should be called before each framebuffer update,
-    // prior to calling getNumRects() or writeFramebufferUpdateStart().
-    void setupCurrentEncoder();
-
-    // getNumRects() computes the number of sub-rectangles that will compose a
-    // given rectangle, for current encoder.
-    int getNumRects(const Rect &r);
-
     // writeSetDesktopSize() won't actually write immediately, but will
     // write the relevant pseudo-rectangle as part of the next update.
     bool writeSetDesktopSize();
@@ -118,26 +106,17 @@ namespace rfb {
     // pseudo-rectangles.
     void writeNoDataUpdate();
 
-    // writeRects() accepts an UpdateInfo (changed & copied regions) and an
-    // ImageGetter to fetch pixels from.  It then calls writeCopyRect() and
-    // writeRect() as appropriate.  writeFramebufferUpdateStart() must be used
-    // before the first writeRects() call and writeFrameBufferUpdateEnd() after
-    // the last one.
-    void writeRects(const UpdateInfo& update, TransImageGetter* ig);
-
-    // To construct a framebuffer update you can call
-    // writeFramebufferUpdateStart(), followed by a number of writeCopyRect()s
-    // and writeRect()s, finishing with writeFramebufferUpdateEnd().
+    // writeFramebufferUpdateStart() initiates an update which you can fill
+    // in using writeCopyRect() and encoders. Finishing the update by calling
+    // writeFramebufferUpdateEnd().
     void writeFramebufferUpdateStart(int nRects);
     void writeFramebufferUpdateEnd();
 
-    // writeRect() writers the given rectangle using either the preferred
-    // encoder, or the one explicitly given.
-    void writeRect(const Rect& r, TransImageGetter* ig);
-    void writeRect(const Rect& r, int encoding, TransImageGetter* ig);
-
+    // There is no explicit encoder for CopyRect rects.
     void writeCopyRect(const Rect& r, int srcX, int srcY);
 
+    // Encoders should call these to mark the start and stop of individual
+    // rects.
     void startRect(const Rect& r, int enc);
     void endRect();
 
@@ -169,7 +148,6 @@ namespace rfb {
     ConnParams* cp;
     rdr::OutStream* os;
 
-    Encoder* encoders[encodingMax+1];
     int currentEncoding;
 
     int nRectsInUpdate;

common/rfb/VNCSConnectionST.cxx

@@ -37,6 +37,7 @@
 #include <rfb/ServerCore.h>
 #include <rfb/ComparingUpdateTracker.h>
 #include <rfb/KeyRemapper.h>
+#include <rfb/Encoder.h>
 #define XK_MISCELLANY
 #define XK_XKB_KEYS
 #include <rfb/keysymdef.h>
@@ -81,6 +82,8 @@ VNCSConnectionST::VNCSConnectionST(VNCServerST* server_, network::Socket *s,
   peerEndpoint.buf = sock->getPeerEndpoint();
   VNCServerST::connectionsLog.write(1,"accepted: %s", peerEndpoint.buf);
 
+  memset(encoders, 0, sizeof(encoders));
+
   // Configure the socket
   setSocketTimeouts();
   lastEventTime = time(0);
@@ -106,6 +109,9 @@ VNCSConnectionST::~VNCSConnectionST()
   // Remove this client from the server
   server->clients.remove(this);
 
+  for (int i = 0; i <= encodingMax; i++)
+    delete encoders[i];
+
   delete [] fenceData;
 }
 
@@ -1060,18 +1066,30 @@ void VNCSConnectionST::writeFramebufferUpdate()
   }
 
   if (!ui.is_empty() || writer()->needFakeUpdate() || drawRenderedCursor) {
+    std::vector<Rect> rects;
+    std::vector<Rect>::const_iterator i;
+    int encoding;
+
+    // Make sure the encoder has the latest settings
+    encoding = cp.currentEncoding();
+
+    if (!encoders[encoding])
+      encoders[encoding] = Encoder::createEncoder(encoding, writer());
+
+    encoders[encoding]->setCompressLevel(cp.compressLevel);
+    encoders[encoding]->setQualityLevel(cp.qualityLevel);
+    encoders[encoding]->setFineQualityLevel(cp.fineQualityLevel,
+                                            cp.subsampling);
+
     // Compute the number of rectangles. Tight encoder makes the things more
     // complicated as compared to the original VNC4.
-    writer()->setupCurrentEncoder();
     int nRects = (ui.copied.numRects() +
                   (drawRenderedCursor ? 1 : 0));
 
-    std::vector<Rect> rects;
-    std::vector<Rect>::const_iterator i;
     ui.changed.get_rects(&rects);
     for (i = rects.begin(); i != rects.end(); i++) {
       if (i->width() && i->height()) {
-        int nUpdateRects = writer()->getNumRects(*i);
+        int nUpdateRects = encoders[encoding]->getNumRects(*i);
         if (nUpdateRects == 0 && cp.currentEncoding() == encodingTight) {
           // With Tight encoding and LastRect support, the client does not
           // care about the number of rectangles in the update - it will
@@ -1088,17 +1106,33 @@ void VNCSConnectionST::writeFramebufferUpdate()
     
     writer()->writeFramebufferUpdateStart(nRects);
 
-    writer()->writeRects(ui, &image_getter);
-    updates.clear();
+    ui.copied.get_rects(&rects);
+    for (i = rects.begin(); i != rects.end(); i++)
+      writer()->writeCopyRect(*i, i->tl.x - ui.copy_delta.x,
+                              i->tl.y - ui.copy_delta.y);
+
+    ui.changed.get_rects(&rects);
+    for (i = rects.begin(); i != rects.end(); i++)
+      encoders[encoding]->writeRect(*i, &image_getter);
 
-    if (drawRenderedCursor)
-      writeRenderedCursorRect();
+    if (drawRenderedCursor) {
+      image_getter.setPixelBuffer(&server->renderedCursor);
+      image_getter.setOffset(server->renderedCursorTL);
+
+      encoders[encoding]->writeRect(renderedCursorRect, &image_getter);
+
+      image_getter.setPixelBuffer(server->pb);
+      image_getter.setOffset(Point(0,0));
+
+      drawRenderedCursor = false;
+    }
 
     writer()->writeFramebufferUpdateEnd();
 
     writeRTTPing();
 
     requested.clear();
+    updates.clear();
   }
 
 out:
@@ -1106,22 +1140,6 @@ out:
 }
 
 
-// writeRenderedCursorRect() writes a single rectangle drawing the rendered
-// cursor on the client.
-
-void VNCSConnectionST::writeRenderedCursorRect()
-{
-  image_getter.setPixelBuffer(&server->renderedCursor);
-  image_getter.setOffset(server->renderedCursorTL);
-
-  writer()->writeRect(renderedCursorRect, &image_getter);
-
-  image_getter.setPixelBuffer(server->pb);
-  image_getter.setOffset(Point(0,0));
-
-  drawRenderedCursor = false;
-}
-
 void VNCSConnectionST::screenLayoutChange(rdr::U16 reason)
 {
   if (!authenticated())

common/rfb/VNCSConnectionST.h

@@ -37,6 +37,8 @@
 struct RTTInfo;
 
 namespace rfb {
+  class Encoder;
+
   class VNCSConnectionST : public SConnection,
                            public WriteSetCursorCallback,
                            public Timer::Callback {
@@ -169,7 +171,6 @@ namespace rfb {
 
     void writeFramebufferUpdate();
 
-    void writeRenderedCursorRect();
     void screenLayoutChange(rdr::U16 reason);
     void setCursor();
     void setDesktopName(const char *name);
@@ -202,6 +203,7 @@ namespace rfb {
     Rect renderedCursorRect;
     bool continuousUpdates;
     Region cuRegion;
+    Encoder* encoders[encodingMax+1];
 
     Timer updateTimer;
 

vncviewer/CConn.cxx

@@ -1,6 +1,6 @@
 /* Copyright (C) 2002-2005 RealVNC Ltd.  All Rights Reserved.
- * Copyright 2009-2011 Pierre Ossman <ossman@cendio.se> for Cendio AB
  * Copyright (C) 2011 D. R. Commander.  All Rights Reserved.
+ * Copyright 2009-2014 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
@@ -29,6 +29,7 @@
 
 #include <rfb/CMsgWriter.h>
 #include <rfb/encodings.h>
+#include <rfb/Decoder.h>
 #include <rfb/Hostname.h>
 #include <rfb/LogWriter.h>
 #include <rfb/util.h>
@@ -80,6 +81,8 @@ CConn::CConn(const char* vncServerName, network::Socket* socket=NULL)
   setShared(::shared);
   sock = socket;
 
+  memset(decoders, 0, sizeof(decoders));
+
   int encNum = encodingNum(preferredEncoding);
   if (encNum != -1)
     currentEncoding = encNum;
@@ -131,6 +134,9 @@ CConn::~CConn()
 {
   OptionsDialog::removeCallback(handleOptions);
 
+  for (int i = 0; i < sizeof(decoders)/sizeof(decoders[0]); i++)
+    delete decoders[i];
+
   if (desktop)
     delete desktop;
 
@@ -380,19 +386,27 @@ void CConn::serverCutText(const char* str, rdr::U32 len)
   delete [] buffer;
 }
 
-// We start timing on beginRect and stop timing on endRect, to
-// avoid skewing the bandwidth estimation as a result of the server
-// being slow or the network having high latency
-void CConn::beginRect(const Rect& r, int encoding)
+void CConn::dataRect(const Rect& r, int encoding)
 {
   sock->inStream().startTiming();
-  if (encoding != encodingCopyRect) {
+
+  if (encoding != encodingCopyRect)
     lastServerEncoding = encoding;
+
+  if (!Decoder::supported(encoding)) {
+    fprintf(stderr, "Unknown rect encoding %d\n", encoding);
+    throw Exception("Unknown rect encoding");
   }
-}
 
-void CConn::endRect(const Rect& r, int encoding)
-{
+  if (!decoders[encoding]) {
+    decoders[encoding] = Decoder::createDecoder(encoding, reader());
+    if (!decoders[encoding]) {
+      fprintf(stderr, "Unknown rect encoding %d\n", encoding);
+      throw Exception("Unknown rect encoding");
+    }
+  }
+  decoders[encoding]->readRect(r, this);
+
   sock->inStream().stopTiming();
 }
 

vncviewer/CConn.h

@@ -1,5 +1,5 @@
 /* Copyright (C) 2002-2005 RealVNC Ltd.  All Rights Reserved.
- * Copyright 2009-2011 Pierre Ossman <ossman@cendio.se> for Cendio AB
+ * Copyright 2009-2014 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
@@ -23,8 +23,11 @@
 #include <FL/Fl.H>
 
 #include <rfb/CConnection.h>
+#include <rfb/encodings.h>
 #include <network/Socket.h>
 
+namespace rfb { class Decoder; }
+
 class DesktopWindow;
 
 class CConn : public rfb::CConnection,
@@ -62,8 +65,7 @@ public:
   void framebufferUpdateStart();
   void framebufferUpdateEnd();
 
-  void beginRect(const rfb::Rect& r, int encoding);
-  void endRect(const rfb::Rect& r, int encoding);
+  void dataRect(const rfb::Rect& r, int encoding);
 
   void fillRect(const rfb::Rect& r, rfb::Pixel p);
   void imageRect(const rfb::Rect& r, void* p);
@@ -102,6 +104,8 @@ private:
   bool pendingPFChange;
   rfb::PixelFormat pendingPF;
 
+  rfb::Decoder *decoders[rfb::encodingMax+1];
+
   int currentEncoding, lastServerEncoding;
 
   bool formatChange;