Merge pull request #3185

initramfs (26):
      feat(video): redesign and improve VideoFrame class
      fix(video): fix CoreAV and VideoSurface to conform to new VideoFrame
      refactor(video): rename and make the frame alignment propety public
      fix(video): fix memory leak caused by unfreed buffers in CoreVideoSource
      fix(video): fix slanted video when video size is not divisible by 8
      refactor(video): use a new ToxAVFrame structure instead of vpx_image
      refactor(video): static cast video dimensions to suppress warnings
      feat(video): adds an ID parameter to the VideoSource class
      refactor(video): internalize frame reference counting
      feat(video): add accessor functions for sourceID and frameID
      refactor(video): make type aliases public
      refactor(video): use generics to simply VideoFrame conversion functions
      refactor(video): rename ToxAVFrame to ToxYUVFrame and add documentation
      refactor(video): update documentation to match new format (issue #3559)
      refactor(videoframe): correct mistakes in commit documentation format
      fix(video): fix a use-after-free with VideoFrame
      fix(video): added declaration for missing biglock in CameraSource
      docs(video): remove old unnecessary comment pertaining to removed code
      fix(video): fix invalid VideoSource ID allocation
      fix(video): specify color ranges for pixel formats that are not YUV
      fix(video): use a QReadWriteLock to manage camera access
      fix(video): force the use of non-deprecated pixel formats for YUV
      refactor(video): update code and documentation to honour QSize validity
      refactor(videoframe): move all inline/template functions into source
      fix(video): guard storeVideoFrame() against freeing in-use memory
      feat(video): add a isValid() function to ToxTUVFrame

qtox.pro

@@ -415,6 +415,7 @@ SOURCES += \
     src/persistence/db/rawdatabase.cpp \
     src/persistence/history.cpp \
     src/video/videoframe.cpp \
+    src/video/videosource.cpp \
     src/video/cameradevice.cpp \
     src/video/camerasource.cpp \
     src/video/corevideosource.cpp \

src/core/coreav.cpp

@@ -360,12 +360,10 @@ void CoreAV::sendCallVideo(uint32_t callId, std::shared_ptr<VideoFrame> vframe)
         call.nullVideoBitrate = false;
     }
 
-    // This frame shares vframe's buffers, we don't call vpx_img_free but just delete it
+    ToxYUVFrame frame = vframe->toToxYUVFrame();
-    vpx_image* frame = vframe->toVpxImage();
+
-    if (frame->fmt == VPX_IMG_FMT_NONE)
+    if(!frame)
     {
-        qWarning() << "Invalid frame";
-        vpx_img_free(frame);
         return;
     }
 
@@ -374,8 +372,8 @@ void CoreAV::sendCallVideo(uint32_t callId, std::shared_ptr<VideoFrame> vframe)
     TOXAV_ERR_SEND_FRAME err;
     int retries = 0;
     do {
-        if (!toxav_video_send_frame(toxav, callId, frame->d_w, frame->d_h,
+        if (!toxav_video_send_frame(toxav, callId, frame.width, frame.height,
-                                    frame->planes[0], frame->planes[1], frame->planes[2], &err))
+                                    frame.y, frame.u, frame.v, &err))
         {
             if (err == TOXAV_ERR_SEND_FRAME_SYNC)
             {
@@ -390,8 +388,6 @@ void CoreAV::sendCallVideo(uint32_t callId, std::shared_ptr<VideoFrame> vframe)
     } while (err == TOXAV_ERR_SEND_FRAME_SYNC && retries < 5);
     if (err == TOXAV_ERR_SEND_FRAME_SYNC)
         qDebug() << "toxav_video_send_frame error: Lock busy, dropping frame";
-
-    vpx_img_free(frame);
 }
 
 void CoreAV::micMuteToggle(uint32_t callId)

src/video/camerasource.cpp

@@ -23,7 +23,8 @@ extern "C" {
 #include <libavformat/avformat.h>
 #include <libswscale/swscale.h>
 }
-#include <QMutexLocker>
+#include <QWriteLocker>
+#include <QReadLocker>
 #include <QDebug>
 #include <QtConcurrent/QtConcurrentRun>
 #include <memory>
@@ -74,9 +75,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
 
@@ -141,12 +139,10 @@ void CameraSource::open(const QString& deviceName)
 
 void CameraSource::open(const QString& DeviceName, VideoMode Mode)
 {
-    streamBlocker = true;
+    QWriteLocker locker{&streamMutex};
-    QMutexLocker l{&biglock};
 
     if (DeviceName == deviceName && Mode == mode)
     {
-        streamBlocker = false;
         return;
     }
 
@@ -159,8 +155,6 @@ void CameraSource::open(const QString& DeviceName, VideoMode Mode)
 
     if (subscriptions && _isOpen)
         openDevice();
-
-    streamBlocker = false;
 }
 
 /**
@@ -180,20 +174,15 @@ bool CameraSource::isOpen()
 
 CameraSource::~CameraSource()
 {
-    QMutexLocker l{&biglock};
+    QWriteLocker locker{&streamMutex};
 
     if (!_isOpen)
+    {
         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);
@@ -208,9 +197,7 @@ CameraSource::~CameraSource()
         device = nullptr;
     }
 
-    // Memfence so the stream thread sees a nullptr device
+    locker.unlock();
-    std::atomic_thread_fence(std::memory_order_release);
-    l.unlock();
 
     // Synchronize with our stream thread
     while (streamFuture.isRunning())
@@ -219,7 +206,7 @@ CameraSource::~CameraSource()
 
 bool CameraSource::subscribe()
 {
-    QMutexLocker l{&biglock};
+    QWriteLocker locker{&streamMutex};
 
     if (!_isOpen)
     {
@@ -238,18 +225,13 @@ bool CameraSource::subscribe()
         device = nullptr;
         cctx = cctxOrig = nullptr;
         videoStreamIndex = -1;
-        // Memfence so the stream thread sees a nullptr device
-        std::atomic_thread_fence(std::memory_order_release);
         return false;
     }
-
 }
 
 void CameraSource::unsubscribe()
 {
-    streamBlocker = true;
+    QWriteLocker locker{&streamMutex};
-    QMutexLocker l{&biglock};
-    streamBlocker = false;
 
     if (!_isOpen)
     {
@@ -266,11 +248,6 @@ void CameraSource::unsubscribe()
     if (subscriptions - 1 == 0)
     {
         closeDevice();
-        l.unlock();
-
-        // Synchronize with our stream thread
-        while (streamFuture.isRunning())
-            QThread::yieldCurrentThread();
     }
     else
     {
@@ -375,17 +352,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;
@@ -394,8 +361,6 @@ void CameraSource::closeDevice()
     cctxOrig = nullptr;
     while (device && !device->close()) {}
     device = nullptr;
-    // Memfence so the stream thread sees a nullptr device
-    std::atomic_thread_fence(std::memory_order_release);
 }
 
 /**
@@ -410,8 +375,6 @@ void CameraSource::stream()
         if (!frame)
             return;
 
-        frame->opaque = nullptr;
-
         AVPacket packet;
         if (av_read_frame(device->context, &packet) < 0)
             return;
@@ -425,14 +388,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
@@ -441,56 +398,14 @@ void CameraSource::stream()
 
     forever
     {
-        biglock.lock();
+        QReadLocker locker{&streamMutex};
 
-        // When a thread makes device null, it releases it, so we acquire here
+        // Exit if device is no longer valid
-        std::atomic_thread_fence(std::memory_order_acquire);
         if(!device)
         {
-            biglock.unlock();
+            break;
-            return;
         }
 
         streamLoop();
-
-        // Give a chance to other functions to pick up the lock if needed
-        biglock.unlock();
-        while (streamBlocker)
-            QThread::yieldCurrentThread();
-
-        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

@@ -24,6 +24,7 @@
 #include <QString>
 #include <QFuture>
 #include <QVector>
+#include <QReadWriteLock>
 #include <atomic>
 #include "src/video/videosource.h"
 #include "src/video/videomode.h"
@@ -55,20 +56,17 @@ 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;
+    QReadWriteLock streamMutex;
     std::atomic_bool _isOpen;
     std::atomic_bool streamBlocker;
     std::atomic_int subscriptions;

src/video/corevideosource.cpp

@@ -70,22 +70,19 @@ void CoreVideoSource::pushFrame(const vpx_image_t* vpxframe)
     AVFrame* avframe = av_frame_alloc();
     if (!avframe)
         return;
+
     avframe->width = width;
     avframe->height = height;
     avframe->format = AV_PIX_FMT_YUV420P;
 
-    int imgBufferSize = av_image_get_buffer_size(AV_PIX_FMT_YUV420P, width, height, 1);
+    int bufSize = av_image_alloc(avframe->data, avframe->linesize,
-    uint8_t* buf = (uint8_t*)av_malloc(imgBufferSize);
+                                 width, height,
-    if (!buf)
+                                 static_cast<AVPixelFormat>(AV_PIX_FMT_YUV420P), VideoFrame::dataAlignment);
-    {
+
+    if(bufSize < 0){
         av_frame_free(&avframe);
         return;
     }
-    avframe->opaque = buf;
-
-    uint8_t** data = avframe->data;
-    int* linesize = avframe->linesize;
-    av_image_fill_arrays(data, linesize, buf, AV_PIX_FMT_YUV420P, width, height, 1);
 
     for (int i = 0; i < 3; i++)
     {
@@ -102,8 +99,7 @@ void CoreVideoSource::pushFrame(const vpx_image_t* vpxframe)
         }
     }
 
-    vframe = std::make_shared<VideoFrame>(avframe);
+    vframe = std::make_shared<VideoFrame>(id, avframe, true);
-
     emit frameAvailable(vframe);
 }
 

src/video/videoframe.h

@@ -20,44 +20,140 @@
 #ifndef VIDEOFRAME_H
 #define VIDEOFRAME_H
 
-#include <QMutex>
 #include <QImage>
-#include <functional>
+#include <QMutex>
+#include <QReadWriteLock>
+#include <QRect>
+#include <QSize>
 
-struct AVFrame;
+extern "C"{
-struct AVCodecContext;
+#include <libavcodec/avcodec.h>
-struct vpx_image;
+}
+
+#include <atomic>
+#include <cstdint>
+#include <functional>
+#include <memory>
+#include <unordered_map>
+
+struct ToxYUVFrame
+{
+public:
+    bool isValid() const;
+    explicit operator bool() const;
+
+    const std::uint16_t width;
+    const std::uint16_t height;
+
+    const uint8_t* y;
+    const uint8_t* u;
+    const uint8_t* v;
+};
 
 class VideoFrame
 {
 public:
-    explicit VideoFrame(AVFrame* frame);
+    // Declare type aliases
-    VideoFrame(AVFrame* frame, std::function<void()> freelistCallback);
+    using IDType = std::uint_fast64_t;
-    VideoFrame(AVFrame* frame, int w, int h, int fmt, std::function<void()> freelistCallback);
+    using AtomicIDType = std::atomic_uint_fast64_t;
+
+public:
+    VideoFrame(IDType sourceID, AVFrame* sourceFrame, QRect dimensions, int pixFmt, bool freeSourceFrame = false);
+    VideoFrame(IDType sourceID, AVFrame* sourceFrame, bool freeSourceFrame = false);
+
     ~VideoFrame();
 
-    QSize getSize();
+    // Copy/Move operations are disabled for the VideoFrame, encapsulate with a std::shared_ptr to manage.
+
+    VideoFrame(const VideoFrame& other) = delete;
+    VideoFrame(VideoFrame&& other) = delete;
+
+    const VideoFrame& operator=(const VideoFrame& other) = delete;
+    const VideoFrame& operator=(VideoFrame&& other) = delete;
+
+    bool isValid();
+
+    std::shared_ptr<VideoFrame> trackFrame();
+    static void untrackFrames(const IDType& sourceID, bool releaseFrames = false);
 
     void releaseFrame();
 
-    QImage toQImage(QSize size = QSize());
+    const AVFrame* getAVFrame(QSize frameSize, const int pixelFormat, const bool requireAligned);
-    vpx_image* toVpxImage();
+    QImage toQImage(QSize frameSize = {});
+    ToxYUVFrame toToxYUVFrame(QSize frameSize = {});
 
-protected:
+    IDType getFrameID() const;
-    bool convertToRGB24(QSize size = QSize());
+    IDType getSourceID() const;
-    bool convertToYUV420();
+    QRect getSourceDimensions() const;
-    void releaseFrameLockless();
+    int getSourcePixelFormat() const;
+
+    static constexpr int dataAlignment = 32;
 
 private:
-    VideoFrame(const VideoFrame& other)=delete;
+    class FrameBufferKey{
-    VideoFrame& operator=(const VideoFrame& other)=delete;
+    public:
+        FrameBufferKey(const int width, const int height, const int pixFmt, const bool lineAligned);
+
+        // Explictly state default constructor/destructor
+
+        FrameBufferKey(const FrameBufferKey&) = default;
+        FrameBufferKey(FrameBufferKey&&) = default;
+        ~FrameBufferKey() = default;
+
+        // Assignment operators are disabled for the FrameBufferKey
+
+        const FrameBufferKey& operator=(const FrameBufferKey&) = delete;
+        const FrameBufferKey& operator=(FrameBufferKey&&) = delete;
+
+        bool operator==(const FrameBufferKey& other) const;
+        bool operator!=(const FrameBufferKey& other) const;
+
+        static size_t hash(const FrameBufferKey& key);
+
+    public:
+        const int frameWidth;
+        const int frameHeight;
+        const int pixelFormat;
+        const bool linesizeAligned;
+    };
 
 private:
-    std::function<void()> freelistCallback;
+    static FrameBufferKey getFrameKey(const QSize& frameSize, const int pixFmt, const int linesize);
-    QMutex biglock;
+    static FrameBufferKey getFrameKey(const QSize& frameSize, const int pixFmt, const bool frameAligned);
-    AVFrame* frameOther, *frameYUV420, *frameRGB24;
+
-    int width, height;
+    AVFrame* retrieveAVFrame(const QSize& dimensions, const int pixelFormat, const bool requireAligned);
-    int pixFmt;
+    AVFrame* generateAVFrame(const QSize& dimensions, const int pixelFormat, const bool requireAligned);
+    AVFrame* storeAVFrame(AVFrame* frame, const QSize& dimensions, const int pixelFormat);
+
+    void deleteFrameBuffer();
+
+    template <typename T>
+    T toGenericObject(const QSize& dimensions, const int pixelFormat, const bool requireAligned,
+                      const std::function<T(AVFrame* const)> objectConstructor, const T& nullObject);
+
+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};
+
+    // Source frame
+    const QRect sourceDimensions;
+    int sourcePixelFormat;
+    const FrameBufferKey sourceFrameKey;
+    const bool freeSourceFrame;
+
+    // Reference store
+    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

src/video/videosource.cpp

@@ -0,0 +1,31 @@
+/*
+    Copyright © 2016 by The qTox Project
+
+    This file is part of qTox, a Qt-based graphical interface for Tox.
+
+    qTox is libre software: you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation, either version 3 of the License, or
+    (at your option) any later version.
+
+    qTox is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with qTox.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include "videosource.h"
+
+/**
+ * @class VideoSource
+ * @brief An abstract source of video frames
+ *
+ * When it has at least one subscriber the source will emit new video frames.
+ * Subscribing is recursive, multiple users can subscribe to the same VideoSource.
+ */
+
+// Initialize sourceIDs to 0
+VideoSource::AtomicIDType VideoSource::sourceIDs {0};

src/video/videosource.h

@@ -21,42 +21,51 @@
 #define VIDEOSOURCE_H
 
 #include <QObject>
+
+#include <atomic>
 #include <memory>
 
 class VideoFrame;
 
-/**
-@brief An abstract source of video frames
 
-When it has at least one subscriber the source will emit new video frames
-Subscribing is recursive, multiple users can subscribe to the same VideoSource
-*/
 class VideoSource : public QObject
 {
     Q_OBJECT
 
 public:
+    // Declare type aliases
+    using IDType = std::uint_fast64_t;
+    using AtomicIDType = std::atomic_uint_fast64_t;
+
+public:
+    VideoSource() : id(sourceIDs++){}
+
     virtual ~VideoSource() = default;
     /**
-    If subscribe sucessfully opens the source, it will start emitting frameAvailable signals.
+     * @brief If subscribe sucessfully opens the source, it will start emitting frameAvailable signals.
      */
     virtual bool subscribe() = 0;
     /**
-    Stop emitting frameAvailable signals, and free associated resources if necessary.
+     * @brief Stop emitting frameAvailable signals, and free associated resources if necessary.
      */
     virtual void unsubscribe() = 0;
 
+    /// ID of this VideoSource
+    const IDType id;
 signals:
     /**
-    Emitted when new frame available to use.
+     * @brief Emitted when new frame available to use.
-    @param frame New frame.
+     * @param frame New frame.
      */
     void frameAvailable(std::shared_ptr<VideoFrame> frame);
     /**
-    Emitted when the source is stopped for an indefinite amount of time,
+     * @brief Emitted when the source is stopped for an indefinite amount of time, but might restart
-    but might restart sending frames again later
+     * sending frames again later
      */
     void sourceStopped();
+private:
+    /// Used to manage a global ID for all VideoSources
+    static AtomicIDType sourceIDs;
 };
 
 #endif // VIDEOSOURCE_H

src/video/videosurface.cpp

@@ -146,7 +146,7 @@ void VideoSurface::onNewFrameAvailable(std::shared_ptr<VideoFrame> newFrame)
 
     lock();
     lastFrame = newFrame;
-    newSize = lastFrame->getSize();
+    newSize = lastFrame->getSourceDimensions().size();
     unlock();
 
     float newRatio = getSizeRatio(newSize);