From d00cadf4eb0bd6f9221226013fe802d37ba88d00 Mon Sep 17 00:00:00 2001
From: nerix <nerixdev@outlook.de>
Date: Sun, 2 Jun 2024 16:31:17 +0200
Subject: [PATCH] refactor: load images in workers and push immediately (#5431)

---
 .clang-tidy               |   3 +-
 CHANGELOG.md              |   1 +
 src/messages/Image.cpp    | 400 +++++++++++++++++---------------------
 src/messages/Image.hpp    |  89 +++++----
 src/util/PostToThread.hpp |   9 +
 5 files changed, 239 insertions(+), 263 deletions(-)

diff --git a/.clang-tidy b/.clang-tidy
index ae07280120b..42cca83f6b7 100644
--- a/.clang-tidy
+++ b/.clang-tidy
@@ -30,7 +30,8 @@ Checks: "-*,
   -readability-function-cognitive-complexity,
   -bugprone-easily-swappable-parameters,
   -cert-err58-cpp,
-  -modernize-avoid-c-arrays
+  -modernize-avoid-c-arrays,
+  -misc-include-cleaner
   "
 CheckOptions:
   - key: readability-identifier-naming.ClassCase
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1ac43b47a39..3a14c431c84 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -23,6 +23,7 @@
 - Dev: Reduced the amount of scale events. (#5404, #5406)
 - Dev: Removed unused timegate settings. (#5361)
 - Dev: All Lua globals now show in the `c2` global in the LuaLS metadata. (#5385)
+- Dev: Images are now loaded in worker threads. (#5431)
 
 ## 2.5.1
 
diff --git a/src/messages/Image.cpp b/src/messages/Image.cpp
index f39485d5fd8..30706969105 100644
--- a/src/messages/Image.cpp
+++ b/src/messages/Image.cpp
@@ -21,276 +21,238 @@
 #include <QNetworkRequest>
 #include <QTimer>
 
-#include <functional>
-#include <queue>
-#include <thread>
+#include <atomic>
 
 // Duration between each check of every Image instance
 const auto IMAGE_POOL_CLEANUP_INTERVAL = std::chrono::minutes(1);
 // Duration since last usage of Image pixmap before expiration of frames
 const auto IMAGE_POOL_IMAGE_LIFETIME = std::chrono::minutes(10);
 
-namespace chatterino {
-namespace detail {
-    // Frames
-    Frames::Frames()
+namespace chatterino::detail {
+
+Frames::Frames()
+{
+    DebugCount::increase("images");
+}
+
+Frames::Frames(QList<Frame> &&frames)
+    : items_(std::move(frames))
+{
+    assertInGuiThread();
+    DebugCount::increase("images");
+    if (!this->empty())
     {
-        DebugCount::increase("images");
+        DebugCount::increase("loaded images");
     }
 
-    Frames::Frames(QVector<Frame<QPixmap>> &&frames)
-        : items_(std::move(frames))
+    if (this->animated())
     {
-        assertInGuiThread();
-        DebugCount::increase("images");
-        if (!this->empty())
-        {
-            DebugCount::increase("loaded images");
-        }
-
-        if (this->animated())
-        {
-            DebugCount::increase("animated images");
+        DebugCount::increase("animated images");
 
-            this->gifTimerConnection_ =
-                getIApp()->getEmotes()->getGIFTimer().signal.connect([this] {
-                    this->advance();
-                });
-        }
+        this->gifTimerConnection_ =
+            getIApp()->getEmotes()->getGIFTimer().signal.connect([this] {
+                this->advance();
+            });
+    }
 
-        auto totalLength =
-            std::accumulate(this->items_.begin(), this->items_.end(), 0UL,
-                            [](auto init, auto &&frame) {
-                                return init + frame.duration;
-                            });
+    auto totalLength = std::accumulate(this->items_.begin(), this->items_.end(),
+                                       0UL, [](auto init, auto &&frame) {
+                                           return init + frame.duration;
+                                       });
 
-        if (totalLength == 0)
-        {
-            this->durationOffset_ = 0;
-        }
-        else
-        {
-            this->durationOffset_ = std::min<int>(
-                int(getIApp()->getEmotes()->getGIFTimer().position() %
-                    totalLength),
-                60000);
-        }
-        this->processOffset();
-        DebugCount::increase("image bytes", this->memoryUsage());
-        DebugCount::increase("image bytes (ever loaded)", this->memoryUsage());
+    if (totalLength == 0)
+    {
+        this->durationOffset_ = 0;
     }
-
-    Frames::~Frames()
+    else
     {
-        assertInGuiThread();
-        DebugCount::decrease("images");
-        if (!this->empty())
-        {
-            DebugCount::decrease("loaded images");
-        }
+        this->durationOffset_ = std::min<int>(
+            int(getIApp()->getEmotes()->getGIFTimer().position() % totalLength),
+            60000);
+    }
+    this->processOffset();
+    DebugCount::increase("image bytes", this->memoryUsage());
+    DebugCount::increase("image bytes (ever loaded)", this->memoryUsage());
+}
 
-        if (this->animated())
-        {
-            DebugCount::decrease("animated images");
-        }
-        DebugCount::decrease("image bytes", this->memoryUsage());
-        DebugCount::increase("image bytes (ever unloaded)",
-                             this->memoryUsage());
+Frames::~Frames()
+{
+    assertInGuiThread();
+    DebugCount::decrease("images");
+    if (!this->empty())
+    {
+        DebugCount::decrease("loaded images");
+    }
 
-        this->gifTimerConnection_.disconnect();
+    if (this->animated())
+    {
+        DebugCount::decrease("animated images");
     }
+    DebugCount::decrease("image bytes", this->memoryUsage());
+    DebugCount::increase("image bytes (ever unloaded)", this->memoryUsage());
+
+    this->gifTimerConnection_.disconnect();
+}
 
-    int64_t Frames::memoryUsage() const
+int64_t Frames::memoryUsage() const
+{
+    int64_t usage = 0;
+    for (const auto &frame : this->items_)
     {
-        int64_t usage = 0;
-        for (const auto &frame : this->items_)
-        {
-            auto sz = frame.image.size();
-            auto area = sz.width() * sz.height();
-            auto memory = area * frame.image.depth() / 8;
+        auto sz = frame.image.size();
+        auto area = sz.width() * sz.height();
+        auto memory = area * frame.image.depth() / 8;
 
-            usage += memory;
-        }
-        return usage;
+        usage += memory;
     }
+    return usage;
+}
+
+void Frames::advance()
+{
+    this->durationOffset_ += GIF_FRAME_LENGTH;
+    this->processOffset();
+}
 
-    void Frames::advance()
+void Frames::processOffset()
+{
+    if (this->items_.isEmpty())
     {
-        this->durationOffset_ += GIF_FRAME_LENGTH;
-        this->processOffset();
+        return;
     }
 
-    void Frames::processOffset()
+    while (true)
     {
-        if (this->items_.isEmpty())
-        {
-            return;
-        }
+        this->index_ %= this->items_.size();
 
-        while (true)
+        if (this->durationOffset_ > this->items_[this->index_].duration)
         {
-            this->index_ %= this->items_.size();
-
-            if (this->durationOffset_ > this->items_[this->index_].duration)
-            {
-                this->durationOffset_ -= this->items_[this->index_].duration;
-                this->index_ = (this->index_ + 1) % this->items_.size();
-            }
-            else
-            {
-                break;
-            }
+            this->durationOffset_ -= this->items_[this->index_].duration;
+            this->index_ = (this->index_ + 1) % this->items_.size();
         }
-    }
-
-    void Frames::clear()
-    {
-        assertInGuiThread();
-        if (!this->empty())
+        else
         {
-            DebugCount::decrease("loaded images");
+            break;
         }
-        DebugCount::decrease("image bytes", this->memoryUsage());
-        DebugCount::increase("image bytes (ever unloaded)",
-                             this->memoryUsage());
-
-        this->items_.clear();
-        this->index_ = 0;
-        this->durationOffset_ = 0;
-        this->gifTimerConnection_.disconnect();
     }
+}
 
-    bool Frames::empty() const
+void Frames::clear()
+{
+    assertInGuiThread();
+    if (!this->empty())
     {
-        return this->items_.empty();
+        DebugCount::decrease("loaded images");
     }
+    DebugCount::decrease("image bytes", this->memoryUsage());
+    DebugCount::increase("image bytes (ever unloaded)", this->memoryUsage());
+
+    this->items_.clear();
+    this->index_ = 0;
+    this->durationOffset_ = 0;
+    this->gifTimerConnection_.disconnect();
+}
+
+bool Frames::empty() const
+{
+    return this->items_.empty();
+}
+
+bool Frames::animated() const
+{
+    return this->items_.size() > 1;
+}
 
-    bool Frames::animated() const
+std::optional<QPixmap> Frames::current() const
+{
+    if (this->items_.empty())
     {
-        return this->items_.size() > 1;
+        return std::nullopt;
     }
 
-    std::optional<QPixmap> Frames::current() const
-    {
-        if (this->items_.empty())
-        {
-            return std::nullopt;
-        }
+    return this->items_[this->index_].image;
+}
 
-        return this->items_[this->index_].image;
+std::optional<QPixmap> Frames::first() const
+{
+    if (this->items_.empty())
+    {
+        return std::nullopt;
     }
 
-    std::optional<QPixmap> Frames::first() const
-    {
-        if (this->items_.empty())
-        {
-            return std::nullopt;
-        }
+    return this->items_.front().image;
+}
 
-        return this->items_.front().image;
-    }
+QList<Frame> readFrames(QImageReader &reader, const Url &url)
+{
+    QList<Frame> frames;
+    frames.reserve(reader.imageCount());
 
-    // functions
-    QVector<Frame<QImage>> readFrames(QImageReader &reader, const Url &url)
+    for (int index = 0; index < reader.imageCount(); ++index)
     {
-        QVector<Frame<QImage>> frames;
-        frames.reserve(reader.imageCount());
-
-        QImage image;
-        for (int index = 0; index < reader.imageCount(); ++index)
+        auto pixmap = QPixmap::fromImageReader(&reader);
+        if (!pixmap.isNull())
         {
-            if (reader.read(&image))
+            // It seems that browsers have special logic for fast animations.
+            // This implements Chrome and Firefox's behavior which uses
+            // a duration of 100 ms for any frames that specify a duration of <= 10 ms.
+            // See http://webkit.org/b/36082 for more information.
+            // https://github.com/SevenTV/chatterino7/issues/46#issuecomment-1010595231
+            int duration = reader.nextImageDelay();
+            if (duration <= 10)
             {
-                // It seems that browsers have special logic for fast animations.
-                // This implements Chrome and Firefox's behavior which uses
-                // a duration of 100 ms for any frames that specify a duration of <= 10 ms.
-                // See http://webkit.org/b/36082 for more information.
-                // https://github.com/SevenTV/chatterino7/issues/46#issuecomment-1010595231
-                int duration = reader.nextImageDelay();
-                if (duration <= 10)
-                {
-                    duration = 100;
-                }
-                duration = std::max(20, duration);
-                frames.push_back(Frame<QImage>{std::move(image), duration});
+                duration = 100;
             }
+            duration = std::max(20, duration);
+            frames.append(Frame{
+                .image = std::move(pixmap),
+                .duration = duration,
+            });
         }
-
-        if (frames.empty())
-        {
-            qCDebug(chatterinoImage)
-                << "Error while reading image" << url.string << ": '"
-                << reader.errorString() << "'";
-        }
-
-        return frames;
     }
 
-    // parsed
-    template <typename Assign>
-    void assignDelayed(
-        std::queue<std::pair<Assign, QVector<Frame<QPixmap>>>> &queued,
-        std::mutex &mutex, std::atomic_bool &loadedEventQueued)
+    if (frames.empty())
     {
-        std::lock_guard<std::mutex> lock(mutex);
-        int i = 0;
+        qCDebug(chatterinoImage) << "Error while reading image" << url.string
+                                 << ": '" << reader.errorString() << "'";
+    }
 
-        while (!queued.empty())
-        {
-            auto front = std::move(queued.front());
-            queued.pop();
+    return frames;
+}
 
-            // Call Assign with the vector of frames
-            front.first(std::move(front.second));
+void assignFrames(std::weak_ptr<Image> weak, QList<Frame> parsed)
+{
+    static bool isPushQueued;
 
-            if (++i > 50)
-            {
-                QTimer::singleShot(3, [&] {
-                    assignDelayed(queued, mutex, loadedEventQueued);
-                });
-                return;
-            }
+    auto cb = [parsed = std::move(parsed), weak = std::move(weak)]() mutable {
+        auto shared = weak.lock();
+        if (!shared)
+        {
+            return;
         }
+        shared->frames_ = std::make_unique<detail::Frames>(std::move(parsed));
+
+        // Avoid too many layouts in one event-loop iteration
+        //
+        // This callback is called for every image, so there might be multiple
+        // callbacks queued on the event-loop in this iteration, but we only
+        // want to generate one invalidation.
+        if (!isPushQueued)
+        {
+            isPushQueued = true;
+            postToThread([] {
+                isPushQueued = false;
+                getIApp()->getWindows()->forceLayoutChannelViews();
+            });
+        }
+    };
 
-        getIApp()->getWindows()->forceLayoutChannelViews();
+    postToGuiThread(cb);
+}
 
-        loadedEventQueued = false;
-    }
+}  // namespace chatterino::detail
 
-    template <typename Assign>
-    auto makeConvertCallback(const QVector<Frame<QImage>> &parsed,
-                             Assign assign)
-    {
-        static std::queue<std::pair<Assign, QVector<Frame<QPixmap>>>> queued;
-        static std::mutex mutex;
-        static std::atomic_bool loadedEventQueued{false};
-
-        return [parsed, assign] {
-            // convert to pixmap
-            QVector<Frame<QPixmap>> frames;
-            frames.reserve(parsed.size());
-            std::transform(parsed.begin(), parsed.end(),
-                           std::back_inserter(frames), [](auto &frame) {
-                               return Frame<QPixmap>{
-                                   QPixmap::fromImage(frame.image),
-                                   frame.duration};
-                           });
-
-            // put into stack
-            std::lock_guard<std::mutex> lock(mutex);
-            queued.emplace(assign, frames);
-
-            if (!loadedEventQueued)
-            {
-                loadedEventQueued = true;
-
-                QTimer::singleShot(100, [=] {
-                    assignDelayed(queued, mutex, loadedEventQueued);
-                });
-            }
-        };
-    }
-}  // namespace detail
+namespace chatterino {
 
 // IMAGE2
 Image::~Image()
@@ -402,7 +364,7 @@ void Image::setPixmap(const QPixmap &pixmap)
 {
     auto setFrames = [shared = this->shared_from_this(), pixmap]() {
         shared->frames_ = std::make_unique<detail::Frames>(
-            QVector<detail::Frame<QPixmap>>{detail::Frame<QPixmap>{pixmap, 1}});
+            QList<detail::Frame>{detail::Frame{pixmap, 1}});
     };
 
     if (isGuiThread())
@@ -512,11 +474,8 @@ void Image::actuallyLoad()
                 return;
             }
 
-            auto data = result.getData();
-
-            // const cast since we are only reading from it
-            QBuffer buffer(const_cast<QByteArray *>(&data));
-            buffer.open(QIODevice::ReadOnly);
+            QBuffer buffer;
+            buffer.setData(result.getData());
             QImageReader reader(&buffer);
 
             if (!reader.canRead())
@@ -557,14 +516,7 @@ void Image::actuallyLoad()
 
             auto parsed = detail::readFrames(reader, shared->url());
 
-            postToThread(makeConvertCallback(
-                parsed, [weak = std::weak_ptr<Image>(shared)](auto &&frames) {
-                    if (auto shared = weak.lock())
-                    {
-                        shared->frames_ = std::make_unique<detail::Frames>(
-                            std::forward<decltype(frames)>(frames));
-                    }
-                }));
+            assignFrames(shared, parsed);
         })
         .onError([weak](auto /*result*/) {
             auto shared = weak.lock();
diff --git a/src/messages/Image.hpp b/src/messages/Image.hpp
index 2eb0fcf04ca..31351ab78dc 100644
--- a/src/messages/Image.hpp
+++ b/src/messages/Image.hpp
@@ -1,15 +1,14 @@
 #pragma once
 
 #include "common/Aliases.hpp"
-#include "common/Common.hpp"
 
 #include <boost/variant.hpp>
 #include <pajlada/signals/signal.hpp>
+#include <QList>
 #include <QPixmap>
 #include <QString>
 #include <QThread>
 #include <QTimer>
-#include <QVector>
 
 #include <atomic>
 #include <chrono>
@@ -19,41 +18,53 @@
 #include <optional>
 
 namespace chatterino {
-namespace detail {
-    template <typename Image>
-    struct Frame {
-        Image image;
-        int duration;
-    };
-    class Frames
-    {
-    public:
-        Frames();
-        Frames(QVector<Frame<QPixmap>> &&frames);
-        ~Frames();
-
-        Frames(const Frames &) = delete;
-        Frames &operator=(const Frames &) = delete;
-
-        Frames(Frames &&) = delete;
-        Frames &operator=(Frames &&) = delete;
-
-        void clear();
-        bool empty() const;
-        bool animated() const;
-        void advance();
-        std::optional<QPixmap> current() const;
-        std::optional<QPixmap> first() const;
-
-    private:
-        int64_t memoryUsage() const;
-        void processOffset();
-        QVector<Frame<QPixmap>> items_;
-        int index_{0};
-        int durationOffset_{0};
-        pajlada::Signals::Connection gifTimerConnection_;
-    };
-}  // namespace detail
+
+class Image;
+
+}  // namespace chatterino
+
+namespace chatterino::detail {
+
+struct Frame {
+    QPixmap image;
+    int duration;
+};
+
+class Frames
+{
+public:
+    Frames();
+    Frames(QList<Frame> &&frames);
+    ~Frames();
+
+    Frames(const Frames &) = delete;
+    Frames &operator=(const Frames &) = delete;
+
+    Frames(Frames &&) = delete;
+    Frames &operator=(Frames &&) = delete;
+
+    void clear();
+    bool empty() const;
+    bool animated() const;
+    void advance();
+    std::optional<QPixmap> current() const;
+    std::optional<QPixmap> first() const;
+
+private:
+    int64_t memoryUsage() const;
+    void processOffset();
+    QList<Frame> items_;
+    QList<Frame>::size_type index_{0};
+    int durationOffset_{0};
+    pajlada::Signals::Connection gifTimerConnection_;
+};
+
+QList<Frame> readFrames(QImageReader &reader, const Url &url);
+void assignFrames(std::weak_ptr<Image> weak, QList<Frame> parsed);
+
+}  // namespace chatterino::detail
+
+namespace chatterino {
 
 class Image;
 using ImagePtr = std::shared_ptr<Image>;
@@ -116,9 +127,11 @@ class Image : public std::enable_shared_from_this<Image>
     mutable std::chrono::time_point<std::chrono::steady_clock> lastUsed_;
 
     // gui thread only
-    std::unique_ptr<detail::Frames> frames_{};
+    std::unique_ptr<detail::Frames> frames_;
 
     friend class ImageExpirationPool;
+    friend void detail::assignFrames(std::weak_ptr<Image>,
+                                     QList<detail::Frame>);
 };
 
 // forward-declarable function that calls Image::getEmpty() under the hood.
diff --git a/src/util/PostToThread.hpp b/src/util/PostToThread.hpp
index afeb34d06b0..e0db1fdd075 100644
--- a/src/util/PostToThread.hpp
+++ b/src/util/PostToThread.hpp
@@ -70,4 +70,13 @@ static void runInGuiThread(F &&fun)
     }
 }
 
+template <typename F>
+inline void postToGuiThread(F &&fun)
+{
+    assert(!isGuiThread() &&
+           "postToGuiThread must be called from a non-GUI thread");
+
+    postToThread(std::forward<F>(fun));
+}
+
 }  // namespace chatterino