From cf8769dcf4733e7d3b58f8f1d3a97dbfd32a5929 Mon Sep 17 00:00:00 2001
From: Guillaume Roguez <guillaume.roguez@savoirfairelinux.com>
Date: Thu, 9 Apr 2015 12:42:29 -0400
Subject: [PATCH] video: shared memory renderer refactoring

Direct rendering done by client

Refs #70057
---
 CMakeLists.txt                |   1 +
 src/private/shmrenderer.cpp   | 359 ++++++++++++++++------------------
 src/private/shmrenderer.h     |   8 +-
 src/private/videorenderer_p.h |   7 +-
 src/video/renderer.cpp        |  27 +--
 src/video/renderer.h          |   6 +-
 6 files changed, 187 insertions(+), 221 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 0a6cb115..3b073fbb 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -28,6 +28,7 @@ ADD_DEFINITIONS("-std=c++1y")
 ADD_DEFINITIONS(
    ${QT_DEFINITIONS}
    -fexceptions
+   -O2
 )
 
 PROJECT(ringclient)
diff --git a/src/private/shmrenderer.cpp b/src/private/shmrenderer.cpp
index 622f2133..d386fbd3 100644
--- a/src/private/shmrenderer.cpp
+++ b/src/private/shmrenderer.cpp
@@ -21,7 +21,6 @@
 #include <QtCore/QDebug>
 #include <QtCore/QMutex>
 #include <QtCore/QThread>
-#include <QtCore/QTime>
 
 #include <sys/ipc.h>
 #include <sys/sem.h>
@@ -37,33 +36,35 @@
 #endif
 
 #include <QtCore/QTimer>
+#include <chrono>
+
 #include "private/videorenderermanager.h"
 #include "video/resolution.h"
 #include "private/videorenderer_p.h"
 
+// Uncomment following line to output in console the FPS value
 //#define DEBUG_FPS
-#ifdef DEBUG_FPS
-#include <chrono>
-#endif
 
-///Shared memory object
-// Implementation note: double-buffering
-// Shared memory is divided in two regions, each representing one frame.
-// First byte of each frame is warranted to by aligned on 16 bytes.
-// One region is marked readable: this region can be safely read.
-// The other region is writeable: only the producer can use it.
+/* Shared memory object
+ * Implementation note: double-buffering
+ * Shared memory is divided in two regions, each representing one frame.
+ * First byte of each frame is warranted to by aligned on 16 bytes.
+ * One region is marked readable: this region can be safely read.
+ * The other region is writeable: only the producer can use it.
+ */
 
 struct SHMHeader {
-    sem_t mutex; // Lock it before any operations on following fields.
-    sem_t frameGenMutex; // unlocked by producer when frameGen modified
-    unsigned frameGen; // monotonically incremented when a producer changes readOffset
-    unsigned frameSize; // size in bytes of 1 frame
-    unsigned readOffset; // offset of readable frame in data
-    unsigned writeOffset; // offset of writable frame in data
+    sem_t mutex;                // Lock it before any operations on following fields.
+    sem_t frameGenMutex;        // unlocked by producer when frameGen modified
+    unsigned frameGen;          // monotonically incremented when a producer changes readOffset
+    unsigned frameSize;         // size in bytes of 1 frame
+    unsigned mapSize;           // size to map if you need to see all data
+    unsigned readOffset;        // offset of readable frame in data
+    unsigned writeOffset;       // offset of writable frame in data
 
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-pedantic"
-    char data[]; // the whole shared memory
+    char data[];                // the whole shared memory
 #pragma GCC diagnostic pop
 };
 
@@ -71,85 +72,83 @@ namespace Video {
 
 class ShmRendererPrivate : public QObject
 {
-   Q_OBJECT
-public:
-   ShmRendererPrivate(Video::ShmRenderer* parent);
-
-   //Attributes
-   QString           m_ShmPath    ;
-   int               m_fd         ;
-   SHMHeader*        m_pShmArea   ;
-   unsigned          m_ShmAreaLen ;
-   uint              m_BufferGen  ;
-   QTimer*           m_pTimer     ;
-   int               m_fpsC       ;
-   int               m_Fps        ;
-   QTime             m_CurrentTime;
-
-#ifdef DEBUG_FPS
-   unsigned          m_frameCount;
-   std::chrono::time_point<std::chrono::system_clock> m_lastFrameDebug;
-#endif
-
-   //Constants
-   static const int TIMEOUT_SEC = 1; // 1 second
-
-   //Helpers
-   timespec createTimeout();
-   bool     shmLock      ();
-   void     shmUnlock    ();
-   bool     renderToBitmap();
-   bool     resizeShm();
-
-private:
-   Video::ShmRenderer* q_ptr;
-
-private Q_SLOTS:
-   void timedEvents();
+        Q_OBJECT
+
+    public:
+        ShmRendererPrivate(ShmRenderer* parent);
+
+        // Attributes
+        QString           m_ShmPath    ;
+        int               m_fd         ;
+        SHMHeader*        m_pShmArea   ;
+        unsigned          m_ShmAreaLen ;
+        uint              m_FrameGen  ;
+        QTimer*           m_pTimer     ;
+        int               m_fpsC       ;
+        int               m_Fps        ;
+        std::chrono::time_point<std::chrono::system_clock> m_lastFrameDebug;
+
+        // Constants
+        static const int FPS_RATE_SEC = 1;
+        static const int FRAME_CHECK_RATE_HZ = 120;
+
+        // Helpers
+        timespec createTimeout();
+        bool     shmLock();
+        void     shmUnlock();
+        bool     getNewFrame(bool wait);
+        bool     remapShm();
+
+    private:
+        Video::ShmRenderer* q_ptr;
 };
 
-}
-
-Video::ShmRendererPrivate::ShmRendererPrivate(Video::ShmRenderer* parent) : QObject(parent), q_ptr(parent),
-   m_fd(-1),m_fpsC(0),m_Fps(0),
-   m_pShmArea((SHMHeader*)MAP_FAILED), m_ShmAreaLen(0), m_BufferGen(0),
-   m_pTimer(nullptr)
+ShmRendererPrivate::ShmRendererPrivate(ShmRenderer* parent)
+    : QObject(parent)
+    , q_ptr(parent)
+    , m_fd(-1)
+    , m_fpsC(0)
+    , m_Fps(0)
+    , m_pShmArea((SHMHeader*)MAP_FAILED)
+    , m_ShmAreaLen(0)
+    , m_FrameGen(0)
+    , m_pTimer(nullptr)
 #ifdef DEBUG_FPS
-   , m_frameCount(0)
-   , m_lastFrameDebug(std::chrono::system_clock::now())
+    , m_frameCount(0)
+    , m_lastFrameDebug(std::chrono::system_clock::now())
 #endif
 {
 }
 
-///Constructor
-Video::ShmRenderer::ShmRenderer(const QByteArray& id, const QString& shmPath, const QSize& res): Renderer(id, res), d_ptr(new ShmRendererPrivate(this))
+/// Constructor
+ShmRenderer::ShmRenderer(const QByteArray& id, const QString& shmPath,
+                         const QSize& res)
+    : Renderer(id, res)
+    , d_ptr(new ShmRendererPrivate(this))
 {
    d_ptr->m_ShmPath = shmPath;
    setObjectName("Video::Renderer:"+id);
 }
 
-///Destructor
-Video::ShmRenderer::~ShmRenderer()
+/// Destructor
+ShmRenderer::~ShmRenderer()
 {
    stopShm();
 }
 
-///Get the data from shared memory and transform it into a QByteArray
+/// Wait new frame data from shared memory and save pointer
 bool
-Video::ShmRendererPrivate::renderToBitmap()
+ShmRendererPrivate::getNewFrame(bool wait)
 {
-    QMutexLocker locker {q_ptr->mutex()};
-
-#ifdef Q_OS_LINUX
-    if (!q_ptr->isRendering())
-        return false;
-
     if (!shmLock())
         return false;
 
-    if (m_BufferGen == m_pShmArea->frameGen) {
+    if (m_FrameGen == m_pShmArea->frameGen) {
         shmUnlock();
 
+        if (not wait)
+            return false;
+
         // wait for a new frame, max 33ms
         static const struct timespec timeout = {0, 33000000};
         if (::sem_timedwait(&m_pShmArea->frameGenMutex, &timeout) < 0)
@@ -159,69 +158,76 @@ Video::ShmRendererPrivate::renderToBitmap()
             return false;
     }
 
-    // valid frame to render?
-    if (not m_pShmArea->frameSize)
+    // valid frame to render (daemon may have stopped)?
+    if (not m_pShmArea->frameSize) {
+        shmUnlock();
         return false;
+    }
 
-    if (!resizeShm()) {
+    // map frame data
+    if (!remapShm()) {
         qDebug() << "Could not resize shared memory";
         return false;
     }
 
-    auto& renderer = static_cast<Video::Renderer*>(q_ptr)->d_ptr;
-    auto& frame = renderer->otherFrame();
-    if ((unsigned)frame.size() != m_pShmArea->frameSize)
-        frame.resize(m_pShmArea->frameSize);
-    std::copy_n(m_pShmArea->data + m_pShmArea->readOffset, m_pShmArea->frameSize, frame.data());
-    m_BufferGen = m_pShmArea->frameGen;
-    renderer->updateFrameIndex();
+    q_ptr->setFramePtr(m_pShmArea->data + m_pShmArea->readOffset);
+    m_FrameGen = m_pShmArea->frameGen;
+
     shmUnlock();
 
-#ifdef DEBUG_FPS
+    ++m_fpsC;
+
+    // Compute the FPS shown to the client
     auto currentTime = std::chrono::system_clock::now();
     const std::chrono::duration<double> seconds = currentTime - m_lastFrameDebug;
-    ++m_frameCount;
-    if (seconds.count() > 1) {
-        qDebug() << this << ": FPS " << (m_frameCount / seconds.count());
-        m_frameCount = 0;
+    if (seconds.count() >= FPS_RATE_SEC) {
+        m_Fps = m_fpsC / seconds.count();
+        m_fpsC = 0;
         m_lastFrameDebug = currentTime;
-    }
+#ifdef DEBUG_FPS
+        qDebug() << this << ": FPS " << m_fps;
 #endif
+    }
 
+    emit q_ptr->frameUpdated();
     return true;
-#else
-    return false;
-#endif
 }
 
-///Resize the shared memory
+/// Remap the shared memory
+/// Shared memory in unlocked state if returns false (resize failed).
 bool
-Video::ShmRendererPrivate::resizeShm()
+ShmRendererPrivate::remapShm()
 {
-    const auto areaSize = sizeof(SHMHeader) + 2 * m_pShmArea->frameSize + 15;
-    if (m_ShmAreaLen == areaSize)
-        return true;
+    // This loop handles case where deamon resize shared memory
+    // during time we unlock it for remapping.
+    while (m_ShmAreaLen != m_pShmArea->mapSize) {
+        auto mapSize = m_pShmArea->mapSize;
+        shmUnlock();
 
-    shmUnlock();
-    if (::munmap(m_pShmArea, m_ShmAreaLen)) {
-        qDebug() << "Could not unmap shared area:" << strerror(errno);
-        return false;
-    }
+        if (::munmap(m_pShmArea, m_ShmAreaLen)) {
+            qDebug() << "Could not unmap shared area: " << strerror(errno);
+            return false;
+        }
 
-    m_pShmArea = (SHMHeader*) ::mmap(nullptr, areaSize,
-                                     PROT_READ | PROT_WRITE,
-                                     MAP_SHARED, m_fd, 0);
-    if (m_pShmArea == MAP_FAILED) {
-        qDebug() << "Could not remap shared area";
-        return false;
+        m_pShmArea = (SHMHeader*) ::mmap(nullptr, mapSize, PROT_READ | PROT_WRITE,
+                                         MAP_SHARED, m_fd, 0);
+        if (m_pShmArea == MAP_FAILED) {
+            qDebug() << "Could not remap shared area: " << strerror(errno);
+            return false;
+        }
+
+        if (!shmLock())
+            return false;
+
+        m_ShmAreaLen = mapSize;
     }
 
-    m_ShmAreaLen = areaSize;
-    return shmLock();
+    return true;
 }
 
-///Connect to the shared memory
-bool Video::ShmRenderer::startShm()
+/// Connect to the shared memory
+bool
+ShmRenderer::startShm()
 {
    if (d_ptr->m_fd != -1) {
       qDebug() << "fd must be -1";
@@ -235,8 +241,9 @@ bool Video::ShmRenderer::startShm()
       return false;
    }
 
-   const auto areaSize = sizeof(SHMHeader);
-   d_ptr->m_pShmArea = (SHMHeader*) ::mmap(nullptr, areaSize,
+   // Map only header data
+   const auto mapSize = sizeof(SHMHeader);
+   d_ptr->m_pShmArea = (SHMHeader*) ::mmap(nullptr, mapSize,
                                            PROT_READ | PROT_WRITE,
                                            MAP_SHARED, d_ptr->m_fd, 0);
    if (d_ptr->m_pShmArea == MAP_FAILED) {
@@ -244,14 +251,13 @@ bool Video::ShmRenderer::startShm()
        return false;
    }
 
-   d_ptr->m_ShmAreaLen = areaSize;
-
-   emit started();
+   d_ptr->m_ShmAreaLen = mapSize;
    return true;
 }
 
-///Disconnect from the shared memory
-void Video::ShmRenderer::stopShm()
+/// Disconnect from the shared memory
+void
+ShmRenderer::stopShm()
 {
    if (d_ptr->m_fd < 0)
        return;
@@ -267,103 +273,73 @@ void Video::ShmRenderer::stopShm()
    d_ptr->m_pShmArea = (SHMHeader*) MAP_FAILED;
 }
 
-///Lock the memory while the copy is being made
-bool Video::ShmRendererPrivate::shmLock()
+/// Lock the memory while the copy is being made
+bool
+ShmRendererPrivate::shmLock()
 {
-#ifdef Q_OS_LINUX
-   return sem_wait(&m_pShmArea->mutex) >= 0;
-#else
-   return false;
-#endif
+    return ::sem_wait(&m_pShmArea->mutex) >= 0;
 }
 
-///Remove the lock, allow a new frame to be drawn
-void Video::ShmRendererPrivate::shmUnlock()
+/// Remove the lock, allow a new frame to be drawn
+void
+ShmRendererPrivate::shmUnlock()
 {
-#ifdef Q_OS_LINUX
-   sem_post(&m_pShmArea->mutex);
-#endif
+    ::sem_post(&m_pShmArea->mutex);
 }
 
-
 /*****************************************************************************
  *                                                                           *
  *                                   Slots                                   *
  *                                                                           *
  ****************************************************************************/
 
-///Update the buffer
-void Video::ShmRendererPrivate::timedEvents()
+/// Start the rendering loop
+void
+ShmRenderer::startRendering()
 {
-   if (renderToBitmap()) {
-      //Compute the FPS shown to the client
-      if (m_CurrentTime.second() != QTime::currentTime().second()) {
-         m_Fps = m_fpsC;
-         m_fpsC=0;
-         m_CurrentTime = QTime::currentTime();
-      }
-      m_fpsC++;
-
-      emit q_ptr->frameUpdated();
-   }
-   /*else {
-      qDebug() << "Frame dropped";
-      usleep(rand()%1000); //Be sure it can come back in sync
-   }*/
-}
+    QMutexLocker locker {mutex()};
 
-///Start the rendering loop
-void Video::ShmRenderer::startRendering()
-{
-   QMutexLocker locker {mutex()};
-
-   if (!startShm()) {
-      qDebug() << "Cannot start rendering on " << d_ptr->m_ShmPath;
-      return;
-   }
-
-   if (!d_ptr->m_pTimer) {
-      d_ptr->m_pTimer = new QTimer(nullptr);
-
-//       m_pTimer->moveToThread(thread());
-      connect(d_ptr->m_pTimer,SIGNAL(timeout()),d_ptr.data(),SLOT(timedEvents()));
-      d_ptr->m_pTimer->setInterval(30);
-   }
-
-   if (!d_ptr->m_pTimer->isActive()) {
-      qDebug() << "Is running" << thread()->isRunning();
-      d_ptr->m_pTimer->start();
-   }
-   else
-      qDebug() << "Timer already started!";
+    if (!startShm())
+        return;
 
-   setRendering(true);
+    setRendering(true);
+    emit started();
 }
 
-///Stop the rendering loop
-void Video::ShmRenderer::stopRendering()
+/// Stop the rendering loop
+void
+ShmRenderer::stopRendering()
 {
-   setRendering(false);
+    QMutexLocker locker {mutex()};
+    setRendering(false);
 
-   QMutexLocker locker {mutex()};
-   qDebug() << "Stopping rendering on" << this;
-   if (d_ptr->m_pTimer)
-      d_ptr->m_pTimer->stop();
-   emit stopped();
-   stopShm();
+    emit stopped();
+    stopShm();
 }
 
-
 /*****************************************************************************
  *                                                                           *
  *                                 Getters                                   *
  *                                                                           *
  ****************************************************************************/
 
-///Get the current frame rate of this renderer
-int Video::ShmRenderer::fps() const
+/// Get the current frame rate of this renderer
+int
+ShmRenderer::fps() const
 {
-   return d_ptr->m_Fps;
+    return d_ptr->m_Fps;
+}
+
+/// Get frame data pointer from shared memory
+void*
+ShmRenderer::getFramePtr() const
+{
+    if (!isRendering())
+        return nullptr;
+
+    QMutexLocker lk {mutex()};
+    d_ptr->getNewFrame(false);
+    return Renderer::getFramePtr();
 }
 
 /*****************************************************************************
@@ -372,9 +348,12 @@ int Video::ShmRenderer::fps() const
  *                                                                           *
  ****************************************************************************/
 
-void Video::ShmRenderer::setShmPath(const QString& path)
+void
+ShmRenderer::setShmPath(const QString& path)
 {
-   d_ptr->m_ShmPath = path;
+    d_ptr->m_ShmPath = path;
 }
 
+} // namespace Video
+
 #include <shmrenderer.moc>
diff --git a/src/private/shmrenderer.h b/src/private/shmrenderer.h
index 138134fd..e6074d27 100644
--- a/src/private/shmrenderer.h
+++ b/src/private/shmrenderer.h
@@ -52,10 +52,11 @@ class LIB_EXPORT ShmRenderer : public Renderer {
       bool startShm ();
 
       //Getters
-      virtual int fps             () const         ;
+      virtual int   fps() const;
+      virtual void* getFramePtr() const;
 
       //Setters
-      void setShmPath   (const QString& path);
+      void setShmPath(const QString& path);
 
    private:
       QScopedPointer<ShmRendererPrivate> d_ptr;
@@ -64,9 +65,6 @@ class LIB_EXPORT ShmRenderer : public Renderer {
    public Q_SLOTS:
       void startRendering();
       void stopRendering ();
-
-
-
 };
 
 }
diff --git a/src/private/videorenderer_p.h b/src/private/videorenderer_p.h
index 6770d5e5..dc88330b 100644
--- a/src/private/videorenderer_p.h
+++ b/src/private/videorenderer_p.h
@@ -38,15 +38,10 @@ public:
 
    //Attributes
    std::atomic_bool  m_isRendering;
-   QByteArray        m_Frame[2]   ;
-   bool              m_FrameIdx   ;
    QMutex*           m_pMutex     ;
    QString           m_Id         ;
    QSize             m_pSize      ;
-
-   //Helpers
-   QByteArray& otherFrame       ()      ;
-   void        updateFrameIndex ()      ;
+   void*             m_framePtr   ;
 
 private:
    Video::Renderer* q_ptr;
diff --git a/src/video/renderer.cpp b/src/video/renderer.cpp
index d6ae913a..575caa87 100644
--- a/src/video/renderer.cpp
+++ b/src/video/renderer.cpp
@@ -23,8 +23,9 @@
 //Qt
 #include <QtCore/QMutex>
 
-Video::RendererPrivate::RendererPrivate(Video::Renderer* parent) : QObject(parent), q_ptr(parent),
-m_pMutex(new QMutex()), m_FrameIdx(false)
+Video::RendererPrivate::RendererPrivate(Video::Renderer* parent)
+    : QObject(parent), q_ptr(parent)
+    , m_pMutex(new QMutex())
 {
 }
 
@@ -49,19 +50,6 @@ bool Video::Renderer::isRendering() const
   return d_ptr->m_isRendering;
 }
 
-QByteArray& Video::RendererPrivate::otherFrame()
-{
-  static QByteArray empty;
-  return q_ptr->isRendering()?m_Frame[!m_FrameIdx]:empty;
-}
-
-///Return the current framerate
-const QByteArray& Video::Renderer::currentFrame() const
-{
-  static QByteArray empty;
-  return isRendering()?d_ptr->m_Frame[d_ptr->m_FrameIdx]:empty;
-}
-
 ///Get mutex, in case renderer and views are not in the same thread
 QMutex* Video::Renderer::mutex() const
 {
@@ -74,6 +62,11 @@ QSize Video::Renderer::size() const
   return d_ptr->m_pSize;
 }
 
+void* Video::Renderer::getFramePtr() const
+{
+    return d_ptr->m_framePtr;
+}
+
 /*****************************************************************************
  *                                                                           *
  *                                 Setters                                   *
@@ -91,9 +84,9 @@ void Video::Renderer::setSize(const QSize& size) const
   d_ptr->m_pSize = size;
 }
 
-void Video::RendererPrivate::updateFrameIndex()
+void Video::Renderer::setFramePtr(void* ptr) const
 {
-   m_FrameIdx = !m_FrameIdx;
+  d_ptr->m_framePtr = ptr;
 }
 
 #include <renderer.moc>
diff --git a/src/video/renderer.h b/src/video/renderer.h
index aa5f6548..cd38e0ab 100644
--- a/src/video/renderer.h
+++ b/src/video/renderer.h
@@ -60,17 +60,17 @@ public:
 
    //Getters
    virtual bool              isRendering     () const;
-   virtual const QByteArray& currentFrame    () const;
    virtual QSize             size            () const;
    virtual QMutex*           mutex           () const;
+   virtual void*             getFramePtr     () const;
 
    //Setters
    void setRendering(bool rendering)            const;
    void setSize(const QSize& size)              const;
+   void setFramePtr(void* ptr)                  const;
 
 Q_SIGNALS:
-   ///Emitted when a new frame is ready
-   void frameUpdated();
+   void frameUpdated(); // Emitted when a new frame is ready
    void stopped     ();
    void started     ();
 
-- 
GitLab