video: shm: replace fixed timer with waiting on producer

Removes the fixed rate QTimer that was used to query shm frames, and waits on the producer in a thread loop.

Also factors FPS value tracking into the Renderer base class.

Gitlab: #938
Change-Id: Icf44c8399d70c4127c512802b6cf6c6dccdccfd6

src/libclient/avmodel.cpp

@@ -20,7 +20,6 @@
 #include "api/avmodel.h"
 
 #include "api/video.h"
-#include "api/call.h"
 #include "api/lrc.h"
 #ifdef ENABLE_LIBWRAP
 #include "directrenderer.h"
@@ -943,44 +942,48 @@ createRenderer(const QString& id, const QSize& res, const QString& shmPath = {})
 void
 AVModelPimpl::addRenderer(const QString& id, const QSize& res, const QString& shmPath)
 {
-    auto connectRenderer = [this](Renderer* renderer, const QString& id) {
+    // First remove the existing renderer.
+    renderers_.erase(id);
+
+    // Create a new one and add it.
+    auto renderer = createRenderer(id, res, shmPath);
+    std::lock_guard<std::mutex> lk(renderers_mtx_);
+    auto& r = renderers_[id];
+    r = std::move(renderer);
+    renderers_mtx_.unlock();
+
+    // Listen and forward id-bound signals upwards.
     connect(
-            renderer,
+        r.get(),
         &Renderer::fpsChanged,
         this,
-            [this, id](void) { Q_EMIT linked_.updateRenderersFPSInfo(id); },
+        [this, id](void) { linked_.updateRenderersFPSInfo(id); },
         Qt::QueuedConnection);
     connect(
-            renderer,
+        r.get(),
         &Renderer::started,
         this,
         [this, id](const QSize& size) { Q_EMIT linked_.rendererStarted(id, size); },
         Qt::DirectConnection);
     connect(
-            renderer,
+        r.get(),
         &Renderer::frameBufferRequested,
         this,
         [this, id](AVFrame* frame) { Q_EMIT linked_.frameBufferRequested(id, frame); },
         Qt::DirectConnection);
     connect(
-            renderer,
+        r.get(),
         &Renderer::frameUpdated,
         this,
         [this, id] { Q_EMIT linked_.frameUpdated(id); },
         Qt::DirectConnection);
     connect(
-            renderer,
+        r.get(),
         &Renderer::stopped,
         this,
         [this, id] { Q_EMIT linked_.rendererStopped(id); },
         Qt::DirectConnection);
-    };
+
-    std::lock_guard<std::mutex> lk(renderers_mtx_);
-    renderers_.erase(id); // Because it should be done before creating the renderer
-    auto renderer = createRenderer(id, res, shmPath);
-    auto& r = renderers_[id];
-    r = std::move(renderer);
-    connectRenderer(r.get(), id);
     r->startRendering();
 }
 

src/libclient/directrenderer.cpp

@@ -33,19 +33,10 @@ using namespace lrc::api::video;
 struct DirectRenderer::Impl : public QObject
 {
     Q_OBJECT
-private:
-    int fpsC;
-    int fps;
-
 public:
-    std::chrono::time_point<std::chrono::system_clock> lastFrameDebug;
-
     Impl(DirectRenderer* parent)
         : QObject(nullptr)
         , parent_(parent)
-        , fpsC(0)
-        , fps(0)
-        , lastFrameDebug(std::chrono::system_clock::now())
     {
         configureTarget();
         if (!VideoManager::instance().registerSinkTarget(parent_->id(), target))
@@ -90,17 +81,8 @@ public:
             QMutexLocker lk(&mutex);
             frameBufferPtr = std::move(buf);
         }
-        // compute FPS
-        ++fpsC;
-        auto currentTime = std::chrono::system_clock::now();
-        const std::chrono::duration<double> seconds = currentTime - lastFrameDebug;
-        if (seconds.count() >= FPS_RATE_SEC) {
-            fps = static_cast<int>(fpsC / seconds.count());
-            fpsC = 0;
-            lastFrameDebug = currentTime;
-            parent_->setFPS(fps);
-        }
 
+        parent_->updateFpsTracker();
         Q_EMIT parent_->frameUpdated();
     };
 
@@ -109,6 +91,7 @@ private:
 
 public:
     libjami::SinkTarget target;
+    FpsTracker fpsTracker;
     QMutex mutex;
     libjami::FrameBuffer frameBufferPtr;
 };

src/libclient/renderer.cpp

@@ -21,20 +21,34 @@
 #include <QSize>
 #include <QMutex>
 
+// Uncomment following line to output in console the FPS value for the
+// current renderer type (DirectRenderer, ShmRenderer, etc.).
+// #define DEBUG_FPS
+
 namespace lrc {
 namespace video {
 
 using namespace lrc::api::video;
 
 Renderer::Renderer(const QString& id, const QSize& res)
-    : id_(id)
+    : QObject(nullptr)
+    , id_(id)
     , size_(res)
-    , QObject(nullptr)
+    , fps_(0.0)
-{}
+    , fpsTracker_(new FpsTracker(this))
+{
+    // Subscribe to frame rate updates.
+    connect(fpsTracker_, &FpsTracker::fpsUpdated, this, [this](double fps) {
+        setFPS(fps);
+#ifdef DEBUG_FPS
+        qDebug() << this << ": FPS " << fps;
+#endif
+    });
+}
 
 Renderer::~Renderer() {}
 
-int
+double
 Renderer::fps() const
 {
     return fps_;
@@ -52,12 +66,18 @@ Renderer::size() const
     return size_;
 }
 void
-Renderer::setFPS(int fps)
+Renderer::setFPS(double fps)
 {
     fps_ = fps;
     Q_EMIT fpsChanged();
 }
 
+void
+Renderer::updateFpsTracker()
+{
+    fpsTracker_->update();
+}
+
 MapStringString
 Renderer::getInfos() const
 {
@@ -68,5 +88,24 @@ Renderer::getInfos() const
     return map;
 }
 
+FpsTracker::FpsTracker(QObject* parent)
+    : QObject(parent)
+    , lastTime_(clock_type::now())
+{}
+
+void
+FpsTracker::update()
+{
+    frameCount_++;
+    auto now = clock_type::now();
+    const std::chrono::duration<double> elapsed = now - lastTime_;
+    if (elapsed.count() >= checkInterval_) {
+        double fps = static_cast<double>(frameCount_) / elapsed.count();
+        Q_EMIT fpsUpdated(fps);
+        frameCount_ = 0;
+        lastTime_ = now;
+    }
+}
+
 } // namespace video
 } // namespace lrc

src/libclient/renderer.h

@@ -32,6 +32,8 @@
 namespace lrc {
 namespace video {
 
+class FpsTracker;
+
 class Renderer : public QObject
 {
     Q_OBJECT
@@ -39,7 +41,6 @@ public:
     constexpr static const char RENDERER_ID[] = "RENDERER_ID";
     constexpr static const char FPS[] = "FPS";
     constexpr static const char RES[] = "RES";
-    constexpr static const int FPS_RATE_SEC = 1;
 
     Renderer(const QString& id, const QSize& res);
     virtual ~Renderer();
@@ -47,7 +48,7 @@ public:
     /**
      * @return renderer's fps
      */
-    int fps() const;
+    double fps() const;
 
     /**
      * @return renderer's id
@@ -67,7 +68,12 @@ public:
     /**
      * set fps
      */
-    void setFPS(int fps);
+    void setFPS(double fps);
+
+    /**
+     * Update the FPS tracker.
+     */
+    void updateFpsTracker();
 
     MapStringString getInfos() const;
 
@@ -85,7 +91,30 @@ Q_SIGNALS:
 private:
     QString id_;
     QSize size_;
-    int fps_;
+    double fps_;
+
+    FpsTracker* fpsTracker_;
+};
+
+// Helper that counts ticks, and notifies of FPS changes.
+class FpsTracker : public QObject
+{
+    Q_OBJECT
+public:
+    FpsTracker(QObject* parent = nullptr);
+    ~FpsTracker() = default;
+
+    // Call this function every frame.
+    void update();
+
+    // Emitted after every checkInterval_ when update() is called.
+    Q_SIGNAL void fpsUpdated(double fps);
+
+private:
+    using clock_type = std::chrono::high_resolution_clock;
+    const double checkInterval_ {1.0};
+    unsigned frameCount_ {0};
+    std::chrono::time_point<clock_type> lastTime_;
 };
 
 } // namespace video

src/libclient/shmrenderer.cpp

@@ -20,7 +20,6 @@
 #include "shmrenderer.h"
 
 #include "dbus/videomanager.h"
-#include "videomanager_interface.h"
 
 #include <QDebug>
 #include <QMutex>
@@ -39,19 +38,12 @@
 #define CLOCK_REALTIME 0
 #endif
 
-#include <QTimer>
-
-#include <chrono>
-
 namespace lrc {
 
 using namespace api::video;
 
 namespace video {
 
-// Uncomment following line to output in console the FPS value
-//#define DEBUG_FPS
-
 /* Shared memory object
  * Implementation note: double-buffering
  * Shared memory is divided in two regions, each representing one frame.
@@ -87,27 +79,46 @@ public:
         , shmArea((SHMHeader*) MAP_FAILED)
         , shmAreaLen(0)
         , frameGen(0)
-        , fpsC(0)
-        , fps(0)
-        , timer(new QTimer(this))
-        , lastFrameDebug(std::chrono::system_clock::now())
     {
-        timer->setInterval(33);
-        connect(timer, &QTimer::timeout, [this]() { Q_EMIT parent_->frameUpdated(); });
         VideoManager::instance().startShmSink(parent_->id(), true);
 
-        parent_->moveToThread(&thread);
+        // Continuously check for new frames on a separate thread.
-        connect(&thread, &QThread::finished, [this] { parent_->stopRendering(); });
+        // This is necessary because the frame rate is not constant.
-        thread.start();
+        // The function getNewFrame() will return false if no new frame is available.
-    };
+        thread = QThread::create([this] {
-    ~Impl()
+            forever {
-    {
+                if (QThread::currentThread()->isInterruptionRequested()) {
-        thread.quit();
+                    return;
-        thread.wait();
                 }
 
-    // Constants
+                if (!waitForNewFrame()) {
-    constexpr static const int FRAME_CHECK_RATE_HZ = 120;
+                    continue;
+                }
+
+                parent_->updateFpsTracker();
+                Q_EMIT parent_->frameUpdated();
+            }
+        });
+    };
+    ~Impl() {} // Thread is stopped by parent in ShmRenderer::stopShm.
+
+    void stopThread()
+    {
+        // Request thread loop interruption and then unblock the sem_wait.
+        thread->requestInterruption();
+
+        // Set the isDestroying flag to true so that the thread loop can exit
+        // without emitting the frameUpdated signal for an invalid resolution
+        // (e.g. smartphone rotation).
+        // This works as ShmHolder::renderFrame should reset frameSize appropriately.
+        shmLock();
+        shmArea->frameSize = 0;
+        shmUnlock();
+
+        ::sem_post(&shmArea->frameGenMutex);
+
+        thread->wait();
+    }
 
     // Lock the memory while the copy is being made
     bool shmLock()
@@ -122,7 +133,7 @@ public:
     };
 
     // Wait for new frame data from shared memory and save pointer.
-    bool getNewFrame(bool wait)
+    bool waitForNewFrame()
     {
         if (!shmLock())
             return false;
@@ -130,12 +141,7 @@ public:
         if (frameGen == shmArea->frameGen) {
             shmUnlock();
 
-            if (not wait)
+            if (::sem_wait(&shmArea->frameGenMutex) < 0)
-                return false;
-
-            // wait for a new frame, max 33ms
-            static const struct timespec timeout = {0, 33000000};
-            if (::sem_timedwait(&shmArea->frameGenMutex, &timeout) < 0)
                 return false;
 
             if (!shmLock())
@@ -161,22 +167,6 @@ public:
         frameGen = shmArea->frameGen;
 
         shmUnlock();
-
-        ++fpsC;
-
-        // Compute the FPS shown to the client
-        auto currentTime = std::chrono::system_clock::now();
-        const std::chrono::duration<double> seconds = currentTime - lastFrameDebug;
-        if (seconds.count() >= FPS_RATE_SEC) {
-            fps = static_cast<int>(fpsC / seconds.count());
-            fpsC = 0;
-            lastFrameDebug = currentTime;
-            parent_->setFPS(fps);
-#ifdef DEBUG_FPS
-            qDebug() << this << ": FPS " << fps;
-#endif
-        }
-
         return true;
     };
 
@@ -222,13 +212,8 @@ public:
     unsigned shmAreaLen;
     uint frameGen;
 
-    int fpsC;
-    int fps;
-    std::chrono::time_point<std::chrono::system_clock> lastFrameDebug;
-
-    QTimer* timer;
     QMutex mutex;
-    QThread thread;
+    QThread* thread;
     std::shared_ptr<lrc::api::video::Frame> frame;
 };
 
@@ -249,10 +234,8 @@ Frame
 ShmRenderer::currentFrame() const
 {
     QMutexLocker lk {&pimpl_->mutex};
-    if (pimpl_->getNewFrame(false)) {
     if (auto frame_ptr = pimpl_->frame)
         return std::move(*frame_ptr);
-    }
     return {};
 }
 
@@ -283,6 +266,7 @@ ShmRenderer::startShm()
     }
 
     pimpl_->shmAreaLen = mapSize;
+    pimpl_->thread->start();
     return true;
 }
 
@@ -292,12 +276,12 @@ ShmRenderer::stopShm()
     if (pimpl_->fd < 0)
         return;
 
-    pimpl_->timer->stop();
-
     // Emit the signal before closing the file, this lower the risk of invalid
     // memory access
     Q_EMIT stopped();
 
+    pimpl_->stopThread();
+
     {
         QMutexLocker lk(&pimpl_->mutex);
         // reset the frame so it doesn't point to an old value
@@ -323,8 +307,6 @@ ShmRenderer::startRendering()
     if (!startShm())
         return;
 
-    pimpl_->timer->start();
-
     Q_EMIT started(size());
 }