refactor(video): internalize frame reference counting

This commit takes the frame tracking code and internalizes it into the
VideoFrame class itself for better reusability. The code in
CameraSource has since been removed.

src/video/camerasource.cpp

@@ -74,9 +74,6 @@ open but the device closed if there are zero subscribers.
 @var QMutex CameraSource::biglock
 @brief True when locked. Faster than mutexes for video decoding.
 
-@var QMutex CameraSource::freelistLock
-@brief True when locked. Faster than mutexes for video decoding.
-
 @var std::atomic_bool CameraSource::streamBlocker
 @brief Holds the streaming thread still when true
 
@@ -186,14 +183,7 @@ CameraSource::~CameraSource()
         return;
 
     // Free all remaining VideoFrame
-    // Locking must be done precisely this way to avoid races
+    VideoFrame::untrackFrames(id, true);
-    for (int i = 0; i < freelist.size(); i++)
-    {
-        std::shared_ptr<VideoFrame> vframe = freelist[i].lock();
-        if (!vframe)
-            continue;
-        vframe->releaseFrame();
-    }
 
     if (cctx)
         avcodec_free_context(&cctx);
@@ -375,17 +365,7 @@ void CameraSource::closeDevice()
     qDebug() << "Closing device "<<deviceName;
 
     // Free all remaining VideoFrame
-    // Locking must be done precisely this way to avoid races
+    VideoFrame::untrackFrames(id, true);
-    for (int i = 0; i < freelist.size(); i++)
-    {
-        std::shared_ptr<VideoFrame> vframe = freelist[i].lock();
-        if (!vframe)
-            continue;
-
-        vframe->releaseFrame();
-    }
-    freelist.clear();
-    freelist.squeeze();
 
     // Free our resources and close the device
     videoStreamIndex = -1;
@@ -410,8 +390,6 @@ void CameraSource::stream()
         if (!frame)
             return;
 
-        frame->opaque = nullptr;
-
         AVPacket packet;
         if (av_read_frame(device->context, &packet) < 0)
             return;
@@ -425,14 +403,8 @@ void CameraSource::stream()
             if (!frameFinished)
                 return;
 
-            freelistLock.lock();
+            VideoFrame* vframe = new VideoFrame(id, frame);
-
+            emit frameAvailable(vframe->trackFrame());
-            int freeFreelistSlot = getFreelistSlotLockless();
-            auto frameFreeCb = std::bind(&CameraSource::freelistCallback, this, freeFreelistSlot);
-            std::shared_ptr<VideoFrame> vframe = std::make_shared<VideoFrame>(frame, frameFreeCb);
-            freelist.append(vframe);
-            freelistLock.unlock();
-            emit frameAvailable(vframe);
         }
 
       // Free the packet that was allocated by av_read_frame
@@ -461,36 +433,3 @@ void CameraSource::stream()
         QThread::yieldCurrentThread();
     }
 }
-
-/**
-@brief CameraSource::freelistCallback
-@param freelistIndex
-
-All VideoFrames must be deleted or released before we can close the device
-or the device will forcibly free them, and then ~VideoFrame() will double free.
-In theory very careful coding from our users could ensure all VideoFrames
-die before unsubscribing, even the ones currently in flight in the metatype system.
-But that's just asking for trouble and mysterious crashes, so we'll just
-maintain a freelist and have all VideoFrames tell us when they die so we can forget them.
-*/
-void CameraSource::freelistCallback(int freelistIndex)
-{
-    QMutexLocker l{&freelistLock};
-    freelist[freelistIndex].reset();
-}
-
-/**
-@brief Get the index of a free slot in the freelist.
-@note Callers must hold the freelistLock.
-@return Index of a free slot.
-*/
-int CameraSource::getFreelistSlotLockless()
-{
-    int size = freelist.size();
-    for (int i = 0; i < size; ++i)
-        if (freelist[i].expired())
-            return i;
-
-    freelist.resize(size + (size>>1) + 4); // Arbitrary growth strategy, should work well
-    return size;
-}

src/video/camerasource.h

@@ -55,20 +55,16 @@ private:
     CameraSource();
     ~CameraSource();
     void stream();
-    void freelistCallback(int freelistIndex);
-    int getFreelistSlotLockless();
     bool openDevice();
     void closeDevice();
 
 private:
-    QVector<std::weak_ptr<VideoFrame>> freelist;
     QFuture<void> streamFuture;
     QString deviceName;
     CameraDevice* device;
     VideoMode mode;
     AVCodecContext* cctx, *cctxOrig;
     int videoStreamIndex;
-    QMutex biglock, freelistLock;
     std::atomic_bool _isOpen;
     std::atomic_bool streamBlocker;
     std::atomic_int subscriptions;

src/video/corevideosource.cpp

@@ -99,7 +99,7 @@ void CoreVideoSource::pushFrame(const vpx_image_t* vpxframe)
         }
     }
 
-    vframe = std::make_shared<VideoFrame>(avframe, true);
+    vframe = std::make_shared<VideoFrame>(id, avframe, true);
     emit frameAvailable(vframe);
 }
 

src/video/videoframe.cpp

@@ -24,34 +24,50 @@ extern "C"{
 #include <libswscale/swscale.h>
 }
 
-VideoFrame::VideoFrame(AVFrame* sourceFrame, QRect dimensions, int pixFmt, std::function<void ()> destructCallback, bool freeSourceFrame)
+// Initialize static fields
-    : sourceDimensions(dimensions),
+VideoFrame::AtomicIDType VideoFrame::frameIDs {0};
+
+std::unordered_map<VideoFrame::IDType, QMutex> VideoFrame::mutexMap {};
+std::unordered_map<VideoFrame::IDType, std::unordered_map<VideoFrame::IDType, std::weak_ptr<VideoFrame>>> VideoFrame::refsMap {};
+
+QReadWriteLock VideoFrame::refsLock {};
+
+VideoFrame::VideoFrame(IDType sourceID, AVFrame* sourceFrame, QRect dimensions, int pixFmt, bool freeSourceFrame)
+    : frameID(frameIDs.fetch_add(std::memory_order_relaxed)),
+      sourceID(sourceID),
+      sourceDimensions(dimensions),
       sourcePixelFormat(pixFmt),
       sourceFrameKey(getFrameKey(dimensions.size(), pixFmt, sourceFrame->linesize[0])),
-      freeSourceFrame(freeSourceFrame),
+      freeSourceFrame(freeSourceFrame)
-      destructCallback(destructCallback)
 {
     frameBuffer[sourceFrameKey] = sourceFrame;
 }
 
-VideoFrame::VideoFrame(AVFrame* sourceFrame, std::function<void ()> destructCallback, bool freeSourceFrame)
+VideoFrame::VideoFrame(IDType sourceID, AVFrame* sourceFrame, bool freeSourceFrame)
-    : VideoFrame(sourceFrame, QRect {0, 0, sourceFrame->width, sourceFrame->height}, sourceFrame->format, destructCallback, freeSourceFrame){}
+    : VideoFrame(sourceID, sourceFrame, QRect {0, 0, sourceFrame->width, sourceFrame->height}, sourceFrame->format, freeSourceFrame){}
-
-VideoFrame::VideoFrame(AVFrame* sourceFrame, bool freeSourceFrame)
-    : VideoFrame(sourceFrame, QRect {0, 0, sourceFrame->width, sourceFrame->height}, sourceFrame->format, nullptr, freeSourceFrame){}
 
 VideoFrame::~VideoFrame()
 {
+    // Release frame
     frameLock.lockForWrite();
 
-    if(destructCallback && frameBuffer.size() > 0)
-    {
-        destructCallback();
-    }
-
     deleteFrameBuffer();
 
     frameLock.unlock();
+
+    // Delete tracked reference
+    refsLock.lockForRead();
+
+    if(refsMap.count(sourceID) > 0)
+    {
+        QMutex& sourceMutex = mutexMap[sourceID];
+
+        sourceMutex.lock();
+        refsMap[sourceID].erase(frameID);
+        sourceMutex.unlock();
+    }
+
+    refsLock.unlock();
 }
 
 bool VideoFrame::isValid()
@@ -63,6 +79,80 @@ bool VideoFrame::isValid()
     return retValue;
 }
 
+std::shared_ptr<VideoFrame> VideoFrame::trackFrame()
+{
+    // Add frame to tracked reference list
+    refsLock.lockForRead();
+
+    if(refsMap.count(sourceID) == 0)
+    {
+        // We need to add a new source to our reference map, obtain write lock
+        refsLock.unlock();
+        refsLock.lockForWrite();
+    }
+
+    QMutex& sourceMutex = mutexMap[sourceID];
+
+    sourceMutex.lock();
+
+    std::shared_ptr<VideoFrame> ret {this};
+
+    refsMap[sourceID][frameID] = ret;
+
+    sourceMutex.unlock();
+    refsLock.unlock();
+
+    return ret;
+}
+
+void VideoFrame::untrackFrames(const VideoFrame::IDType& sourceID, bool releaseFrames)
+{
+    refsLock.lockForWrite();
+
+    if(refsMap.count(sourceID) == 0)
+    {
+        // No tracking reference exists for source, simply return
+        refsLock.unlock();
+
+        return;
+    }
+
+    if(releaseFrames)
+    {
+        QMutex& sourceMutex = mutexMap[sourceID];
+
+        sourceMutex.lock();
+
+        for(auto& frameIterator : refsMap[sourceID])
+        {
+            std::shared_ptr<VideoFrame> frame = frameIterator.second.lock();
+
+            if(frame)
+            {
+                frame->releaseFrame();
+            }
+        }
+
+        sourceMutex.unlock();
+    }
+
+    refsMap[sourceID].clear();
+
+    mutexMap.erase(sourceID);
+    refsMap.erase(sourceID);
+
+    refsLock.unlock();
+}
+
+void VideoFrame::releaseFrame()
+{
+    frameLock.lockForWrite();
+
+    deleteFrameBuffer();
+
+    frameLock.unlock();
+}
+
 const AVFrame* VideoFrame::getAVFrame(QSize frameSize, const int pixelFormat, const bool requireAligned)
 {
     frameLock.lockForRead();
@@ -106,15 +196,6 @@ const AVFrame* VideoFrame::getAVFrame(QSize frameSize, const int pixelFormat, co
     return ret;
 }
 
-void VideoFrame::releaseFrame()
-{
-    frameLock.lockForWrite();
-
-    deleteFrameBuffer();
-
-    frameLock.unlock();
-}
-
 QImage VideoFrame::toQImage(QSize frameSize)
 {
     frameLock.lockForRead();

src/video/videoframe.h

@@ -21,6 +21,7 @@
 #define VIDEOFRAME_H
 
 #include <QImage>
+#include <QMutex>
 #include <QReadWriteLock>
 #include <QRect>
 #include <QSize>
@@ -29,6 +30,8 @@ extern "C"{
 #include <libavcodec/avcodec.h>
 }
 
+#include <atomic>
+#include <cstdint>
 #include <functional>
 #include <memory>
 #include <unordered_map>
@@ -68,20 +71,20 @@ public:
  */
 class VideoFrame
 {
+    using IDType = std::uint_fast64_t;
+    using AtomicIDType = std::atomic_uint_fast64_t;
+
 public:
     /**
      * @brief Constructs a new instance of a VideoFrame, sourced by a given AVFrame pointer.
+     * @param sourceID the VideoSource's ID to track the frame under.
      * @param sourceFrame the source AVFrame pointer to use, must be valid.
      * @param dimensions the dimensions of the AVFrame, obtained from the AVFrame if not given.
      * @param pixFmt the pixel format of the AVFrame, obtained from the AVFrame if not given.
-     * @param destructCallback callback function to run upon destruction of the VideoFrame
-     * this callback is only run when destroying a valid VideoFrame (e.g. a VideoFrame instance in
-     * which releaseFrame() was called upon it will not call the callback).
      * @param freeSourceFrame whether to free the source frame buffers or not.
      */
-    VideoFrame(AVFrame* sourceFrame, QRect dimensions, int pixFmt, std::function<void()> destructCallback, bool freeSourceFrame = false);
+    VideoFrame(IDType sourceID, AVFrame* sourceFrame, QRect dimensions, int pixFmt, bool freeSourceFrame = false);
-    VideoFrame(AVFrame* sourceFrame, std::function<void()> destructCallback, bool freeSourceFrame = false);
+    VideoFrame(IDType sourceID, AVFrame* sourceFrame, bool freeSourceFrame = false);
-    VideoFrame(AVFrame* sourceFrame, bool freeSourceFrame = false);
 
     /**
      * Destructor for VideoFrame.
@@ -106,6 +109,34 @@ public:
      */
     bool isValid();
 
+    /**
+     * @brief Causes the VideoFrame class to maintain an internal reference for the frame.
+     *
+     * The internal reference is managed via a std::weak_ptr such that it doesn't inhibit
+     * destruction of the object once all external references are no longer reachable.
+     *
+     * @return a std::shared_ptr holding a reference to this frame.
+     */
+    std::shared_ptr<VideoFrame> trackFrame();
+
+    /**
+     * @brief Untracks all the frames for the given VideoSource, releasing them if specified.
+     *
+     * This function causes all internally tracked frames for the given VideoSource to be dropped.
+     * If the releaseFrames option is set to true, the frames are sequentially released on the
+     * caller's thread in an unspecified order.
+     *
+     * @param sourceID the ID of the VideoSource to untrack frames from.
+     * @param releaseFrames true to release the frames as necessary, false otherwise. Defaults to
+     * false.
+     */
+    static void untrackFrames(const IDType& sourceID, bool releaseFrames = false);
+
+    /**
+     * @brief Releases all frames managed by this VideoFrame and invalidates it.
+     */
+    void releaseFrame();
+
     /**
      * @brief Retrieves an AVFrame derived from the source based on the given parameters.
      *
@@ -121,11 +152,6 @@ public:
      */
     const AVFrame* getAVFrame(QSize frameSize, const int pixelFormat, const bool requireAligned);
 
-    /**
-     * @brief Releases all frames managed by this VideoFrame and invalidates it.
-     */
-    void releaseFrame();
-
     /**
      * @brief Retrieves a copy of the source VideoFrame's dimensions.
      * @return QRect copy representing the source VideoFrame's dimensions.
@@ -339,6 +365,10 @@ private:
     void deleteFrameBuffer();
 
 private:
+    // ID
+    const IDType frameID;
+    const IDType sourceID;
+
     // Main framebuffer store
     std::unordered_map<FrameBufferKey, AVFrame*, std::function<decltype(FrameBufferKey::hash)>> frameBuffer {3, FrameBufferKey::hash};
 
@@ -348,11 +378,15 @@ private:
     const FrameBufferKey sourceFrameKey;
     const bool freeSourceFrame;
 
-    // Destructor callback
+    // Reference store
-    const std::function<void ()> destructCallback;
+    static AtomicIDType frameIDs;
+
+    static std::unordered_map<IDType, QMutex> mutexMap;
+    static std::unordered_map<IDType, std::unordered_map<IDType, std::weak_ptr<VideoFrame>>> refsMap;
 
     // Concurrency
     QReadWriteLock frameLock {};
+    static QReadWriteLock refsLock;
 };
 
 #endif // VIDEOFRAME_H