cleanup: Ensure VideoFrame memory is never leaked.

By never having non-shared_ptr versions of it in any code other than private factories.
TokTok · Dec 31, 2024 · d8f28a5 · d8f28a5
1 parent 2bd629f
commit d8f28a5
Show file tree

Hide file tree

Showing 9 changed files with 94 additions and 55 deletions.
diff --git a/.ci-scripts/build-macos-qt.sh b/.ci-scripts/build-macos-qt.sh
@@ -9,6 +9,4 @@ set -euo pipefail
 readonly SCRIPT_DIR="$(dirname "$(realpath "$0")")"
 . "$SCRIPT_DIR/macos_install_deps.sh"
 
-# Needs openssl from build-macos-deps.sh to already have been installed.
-install_deps \
-  qt
+install_deps qt
diff --git a/.ci-scripts/macos_install_deps.sh b/.ci-scripts/macos_install_deps.sh
@@ -4,13 +4,15 @@
 # Copyright © 2022 by The qTox Project Contributors
 # Copyright © 2024 The TokTok team
 
-set -euo pipefail
+set -euxo pipefail
 
 if [ ! -d "$SCRIPT_DIR/dockerfiles" ]; then
   git clone --depth=1 https://github.com/TokTok/dockerfiles "$SCRIPT_DIR/dockerfiles"
 fi
 
-ARCH="$1"
+ARCH="${1:-$(uname -m)}"
+BUILD_TYPE="${2:-release}"
+SANITIZE="${3:-}"
 
 install_deps() {
   for dep in "$@"; do
@@ -21,7 +23,7 @@ install_deps() {
     else
       SCRIPT="$SCRIPT_DIR/dockerfiles/qtox/build_$dep.sh"
     fi
-    "$SCRIPT" --arch "macos-$ARCH" --libtype "static"
+    "$SCRIPT" --arch "macos-$ARCH" --libtype "static" --buildtype "$BUILD_TYPE" --sanitize "$SANITIZE"
     popd
     rm -rf "external/$dep"
   done

diff --git a/.gitignore b/.gitignore
@@ -54,3 +54,6 @@ target/
 
 # Editor temp files
 *.swp
+
+# IDE support
+/compile_commands.json
diff --git a/CMakePresets.json b/CMakePresets.json
@@ -3,43 +3,60 @@
   "configurePresets": [
     {
       "name": "static-release",
-      "binaryDir": "${sourceDir}/_build-static",
+      "binaryDir": "${sourceDir}/_build-release",
       "generator": "Ninja",
       "environment": {
         "PKG_CONFIG_PATH": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/lib/pkgconfig"
       },
       "cacheVariables": {
         "UBSAN": true,
-        "SPELL_CHECK": true,
         "STRICT_OPTIONS": true,
         "UPDATE_CHECK": true,
         "CMAKE_BUILD_TYPE": "RelWithDebInfo",
         "CMAKE_COMPILE_WARNING_AS_ERROR": true,
         "CMAKE_EXPORT_COMPILE_COMMANDS": true,
         "CMAKE_PREFIX_PATH": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/lib/cmake",
-        "CMAKE_OSX_DEPLOYMENT_TARGET": "14",
+        "CMAKE_OSX_DEPLOYMENT_TARGET": "12",
         "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/qt/lib/cmake/Qt6/qt.toolchain.cmake"
       }
     },
     {
       "name": "static-asan",
-      "binaryDir": "${sourceDir}/_build-static",
+      "binaryDir": "${sourceDir}/_build-asan",
       "generator": "Ninja",
       "environment": {
         "PKG_CONFIG_PATH": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/lib/pkgconfig"
       },
       "cacheVariables": {
         "ASAN": true,
         "UBSAN": true,
-        "SPELL_CHECK": true,
         "STRICT_OPTIONS": true,
         "UPDATE_CHECK": true,
         "CMAKE_BUILD_TYPE": "Debug",
         "CMAKE_COMPILE_WARNING_AS_ERROR": true,
         "CMAKE_EXPORT_COMPILE_COMMANDS": true,
         "CMAKE_PREFIX_PATH": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/lib/cmake",
-        "CMAKE_OSX_DEPLOYMENT_TARGET": "14",
-        "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/qt/lib/cmake/Qt6/qt.toolchain.cmake"
+        "CMAKE_OSX_DEPLOYMENT_TARGET": "12",
+        "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/qt-asan/lib/cmake/Qt6/qt.toolchain.cmake"
+      }
+    },
+    {
+      "name": "static-tsan",
+      "binaryDir": "${sourceDir}/_build-tsan",
+      "generator": "Ninja",
+      "environment": {
+        "PKG_CONFIG_PATH": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/lib/pkgconfig"
+      },
+      "cacheVariables": {
+        "TSAN": true,
+        "STRICT_OPTIONS": true,
+        "UPDATE_CHECK": true,
+        "CMAKE_BUILD_TYPE": "Debug",
+        "CMAKE_COMPILE_WARNING_AS_ERROR": true,
+        "CMAKE_EXPORT_COMPILE_COMMANDS": true,
+        "CMAKE_PREFIX_PATH": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/lib/cmake",
+        "CMAKE_OSX_DEPLOYMENT_TARGET": "12",
+        "CMAKE_TOOLCHAIN_FILE": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/qt-tsan/lib/cmake/Qt6/qt.toolchain.cmake"
       }
     },
     {
@@ -52,14 +69,13 @@
       "cacheVariables": {
         "ASAN": true,
         "UBSAN": true,
-        "SPELL_CHECK": true,
         "STRICT_OPTIONS": true,
         "UPDATE_CHECK": true,
         "CMAKE_BUILD_TYPE": "Debug",
         "CMAKE_COMPILE_WARNING_AS_ERROR": true,
         "CMAKE_EXPORT_COMPILE_COMMANDS": true,
         "CMAKE_PREFIX_PATH": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/lib/cmake",
-        "CMAKE_OSX_DEPLOYMENT_TARGET": "14"
+        "CMAKE_OSX_DEPLOYMENT_TARGET": "12"
       }
     },
     {
@@ -71,14 +87,13 @@
       },
       "cacheVariables": {
         "TSAN": true,
-        "SPELL_CHECK": true,
         "STRICT_OPTIONS": true,
         "UPDATE_CHECK": true,
         "CMAKE_BUILD_TYPE": "Debug",
         "CMAKE_COMPILE_WARNING_AS_ERROR": true,
         "CMAKE_EXPORT_COMPILE_COMMANDS": true,
         "CMAKE_PREFIX_PATH": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/lib/cmake",
-        "CMAKE_OSX_DEPLOYMENT_TARGET": "14"
+        "CMAKE_OSX_DEPLOYMENT_TARGET": "12"
       }
     },
     {
@@ -89,14 +104,13 @@
         "PKG_CONFIG_PATH": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/lib/pkgconfig"
       },
       "cacheVariables": {
-        "SPELL_CHECK": true,
         "STRICT_OPTIONS": true,
         "UPDATE_CHECK": true,
         "CMAKE_BUILD_TYPE": "RelWithDebInfo",
         "CMAKE_COMPILE_WARNING_AS_ERROR": true,
         "CMAKE_EXPORT_COMPILE_COMMANDS": true,
         "CMAKE_PREFIX_PATH": "${sourceDir}/.ci-scripts/dockerfiles/local-deps/lib/cmake",
-        "CMAKE_OSX_DEPLOYMENT_TARGET": "14"
+        "CMAKE_OSX_DEPLOYMENT_TARGET": "12"
       }
     }
   ]

diff --git a/macos/run-debug b/macos/run-debug
@@ -0,0 +1,22 @@
+#!/usr/bin/env bash
+
+set -euxo pipefail
+
+readonly SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+cd "$SCRIPT_DIR/../"
+
+PRESET=${1:-release}
+DEBUGGER=${2:-none}
+
+if [ "$DEBUGGER" = "none" ]; then
+  DEBUGGER=()
+elif [ "$DEBUGGER" = "lldb" ]; then
+  DEBUGGER=(lldb -o run)
+else
+  DEBUGGER=("$DEBUGGER")
+fi
+
+test -d "_build-$PRESET" || cmake --preset "static-$PRESET"
+ln -sf "_build-$PRESET/compile_commands.json" compile_commands.json
+cmake --build "_build-$PRESET" --target qtox
+"${DEBUGGER[@]}" "_build-$PRESET/qtox.app/Contents/MacOS/qtox"
diff --git a/src/video/camerasource.cpp b/src/video/camerasource.cpp
@@ -22,8 +22,6 @@ extern "C"
 #include <QReadLocker>
 #include <QWriteLocker>
 #include <QtConcurrent/QtConcurrentRun>
-#include <functional>
-#include <memory>
 
 /**
  * @class CameraSource
@@ -449,10 +447,11 @@ void CameraSource::openDevice()
         return;
     }
 
-    if (streamFuture.isRunning())
+    if (streamFuture.isRunning()) {
         qDebug() << "The stream thread is already running! Keeping the current one open.";
-    else
-        streamFuture = QtConcurrent::run(std::bind(&CameraSource::stream, this));
+    } else {
+        streamFuture = QtConcurrent::run([this] { stream(); });
+    }
 
     // Synchronize with our stream thread
     while (!streamFuture.isRunning())
@@ -522,8 +521,7 @@ void CameraSource::stream()
                 return;
             }
 
-            VideoFrame* vframe = new VideoFrame(id, frame);
-            emit frameAvailable(vframe->trackFrame());
+            emit frameAvailable(VideoFrame::fromAVFrame(id, frame));
         }
 #else
 
@@ -534,8 +532,7 @@ void CameraSource::stream()
         if (readyToReceive) {
             AVFrame* frame = av_frame_alloc();
             if (frame && !avcodec_receive_frame(cctx, frame)) {
-                VideoFrame* vframe = new VideoFrame(id, frame);
-                emit frameAvailable(vframe->trackFrame());
+                emit frameAvailable(VideoFrame::fromAVFrame(id, frame));
             } else {
                 av_frame_free(&frame);
             }

diff --git a/src/video/corevideosource.cpp b/src/video/corevideosource.cpp
@@ -88,8 +88,7 @@ void CoreVideoSource::pushFrame(const vpx_image_t* vpxFrame)
         }
     }
 
-    vframe = std::make_shared<VideoFrame>(id, avFrame, true);
-    emit frameAvailable(vframe);
+    emit frameAvailable(VideoFrame::fromAVFrameUntracked(id, avFrame, true));
 }
 
 void CoreVideoSource::subscribe()

diff --git a/src/video/videoframe.cpp b/src/video/videoframe.cpp
@@ -78,19 +78,18 @@ QReadWriteLock VideoFrame::refsLock{};
  *
  * @param sourceID the VideoSource's ID to track the frame under.
  * @param sourceFrame the source AVFrame pointer to use, must be valid.
+ * @param freeSourceFrame whether to free the source frame buffers or not.
  * @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 freeSourceFrame whether to free the source frame buffers or not.
  */
-VideoFrame::VideoFrame(IDType sourceID_, AVFrame* sourceFrame, QRect dimensions, int pixFmt,
-                       bool freeSourceFrame_)
+VideoFrame::VideoFrame(IDType sourceID_, AVFrame* sourceFrame, bool freeSourceFrame_,
+                       QRect dimensions, int pixFmt)
     : frameID(frameIDs++)
     , sourceID(sourceID_)
     , sourceDimensions(dimensions)
     , sourceFrameKey(getFrameKey(dimensions.size(), pixFmt, sourceFrame->linesize[0]))
     , freeSourceFrame(freeSourceFrame_)
 {
-
     // We override the pixel format in the case a deprecated one is used
     switch (pixFmt) {
     case AV_PIX_FMT_YUVJ420P: {
@@ -133,8 +132,8 @@ VideoFrame::VideoFrame(IDType sourceID_, AVFrame* sourceFrame, QRect dimensions,
 }
 
 VideoFrame::VideoFrame(IDType sourceID_, AVFrame* sourceFrame, bool freeSourceFrame_)
-    : VideoFrame(sourceID_, sourceFrame, QRect{0, 0, sourceFrame->width, sourceFrame->height},
-                 sourceFrame->format, freeSourceFrame_)
+    : VideoFrame(sourceID_, sourceFrame, freeSourceFrame_,
+                 QRect{0, 0, sourceFrame->width, sourceFrame->height}, sourceFrame->format)
 {
 }
 
@@ -176,15 +175,21 @@ bool VideoFrame::isValid()
     return frameBuffer.size() > 0;
 }
 
+std::shared_ptr<VideoFrame> VideoFrame::fromAVFrameUntracked(IDType sourceID, AVFrame* sourceFrame,
+                                                             bool freeSourceFrame)
+{
+    return std::shared_ptr<VideoFrame>{new VideoFrame(sourceID, sourceFrame, freeSourceFrame)};
+}
+
 /**
  * @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.
+ * @return a std::shared_ptr holding a reference to the new frame.
  */
-std::shared_ptr<VideoFrame> VideoFrame::trackFrame()
+std::shared_ptr<VideoFrame> VideoFrame::fromAVFrame(IDType sourceID, AVFrame* sourceFrame)
 {
     // Add frame to tracked reference list
     refsLock.lockForRead();
@@ -195,18 +200,16 @@ std::shared_ptr<VideoFrame> VideoFrame::trackFrame()
         refsLock.lockForWrite();
     }
 
-    QMutex& sourceMutex = mutexMap[sourceID];
-
-    sourceMutex.lock();
-
-    std::shared_ptr<VideoFrame> ret{this};
+    const auto frame = [sourceID, sourceFrame] {
+        QMutexLocker<QMutex> sourceMutexLock{&mutexMap[sourceID]};
+        auto ret = fromAVFrameUntracked(sourceID, sourceFrame, false);
+        refsMap[sourceID][ret->frameID] = ret;
+        return ret;
+    }();
 
-    refsMap[sourceID][frameID] = ret;
-
-    sourceMutex.unlock();
     refsLock.unlock();
 
-    return ret;
+    return frame;
 }
 
 /**

diff --git a/src/video/videoframe.h b/src/video/videoframe.h
@@ -21,7 +21,6 @@ extern "C"
 
 #include <atomic>
 #include <cstdint>
-#include <functional>
 #include <memory>
 #include <unordered_map>
 #include <vector>
@@ -48,12 +47,6 @@ class VideoFrame
     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();
-
     // Copy/Move operations are disabled for the VideoFrame, encapsulate with a std::shared_ptr to
     // manage.
 
@@ -63,9 +56,13 @@ class VideoFrame
     const VideoFrame& operator=(const VideoFrame& other) = delete;
     const VideoFrame& operator=(VideoFrame&& other) = delete;
 
+    ~VideoFrame();
+
     bool isValid();
 
-    std::shared_ptr<VideoFrame> trackFrame();
+    static std::shared_ptr<VideoFrame> fromAVFrameUntracked(IDType sourceID, AVFrame* sourceFrame,
+                                                            bool freeSourceFrame);
+    static std::shared_ptr<VideoFrame> fromAVFrame(IDType sourceID, AVFrame* sourceFrame);
     static void untrackFrames(const IDType& sourceID, bool releaseFrames = false);
 
     void releaseFrame();
@@ -82,6 +79,10 @@ class VideoFrame
     static constexpr int dataAlignment = 32;
 
 private:
+    VideoFrame(IDType sourceID_, AVFrame* sourceFrame, bool freeSourceFrame, QRect dimensions,
+               int pixFmt);
+    VideoFrame(IDType sourceID_, AVFrame* sourceFrame, bool freeSourceFrame_);
+
     class FrameBufferKey
     {
     public: