refactor: load images in workers and push immediately (#5431)

.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

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
 

src/messages/Image.cpp

@@ -21,26 +21,23 @@
 #include <QNetworkRequest>
 #include <QTimer>
 
-#include <functional>
+#include <atomic>
-#include <queue>
-#include <thread>
 
 // 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 chatterino::detail {
-namespace detail {
-    // Frames
-    Frames::Frames()
-    {
-        DebugCount::increase("images");
-    }
 
-    Frames::Frames(QVector<Frame<QPixmap>> &&frames)
+Frames::Frames()
+{
+    DebugCount::increase("images");
+}
+
+Frames::Frames(QList<Frame> &&frames)
     : items_(std::move(frames))
-    {
+{
     assertInGuiThread();
     DebugCount::increase("images");
     if (!this->empty())
@@ -58,9 +55,8 @@ namespace detail {
             });
     }
 
-        auto totalLength =
+    auto totalLength = std::accumulate(this->items_.begin(), this->items_.end(),
-            std::accumulate(this->items_.begin(), this->items_.end(), 0UL,
+                                       0UL, [](auto init, auto &&frame) {
-                            [](auto init, auto &&frame) {
                                            return init + frame.duration;
                                        });
 
@@ -71,17 +67,16 @@ namespace detail {
     else
     {
         this->durationOffset_ = std::min<int>(
-                int(getIApp()->getEmotes()->getGIFTimer().position() %
+            int(getIApp()->getEmotes()->getGIFTimer().position() % totalLength),
-                    totalLength),
             60000);
     }
     this->processOffset();
     DebugCount::increase("image bytes", this->memoryUsage());
     DebugCount::increase("image bytes (ever loaded)", this->memoryUsage());
-    }
+}
 
-    Frames::~Frames()
+Frames::~Frames()
-    {
+{
     assertInGuiThread();
     DebugCount::decrease("images");
     if (!this->empty())
@@ -94,14 +89,13 @@ namespace detail {
         DebugCount::decrease("animated images");
     }
     DebugCount::decrease("image bytes", this->memoryUsage());
-        DebugCount::increase("image bytes (ever unloaded)",
+    DebugCount::increase("image bytes (ever unloaded)", this->memoryUsage());
-                             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_)
     {
@@ -112,16 +106,16 @@ namespace detail {
         usage += memory;
     }
     return usage;
-    }
+}
 
-    void Frames::advance()
+void Frames::advance()
-    {
+{
     this->durationOffset_ += GIF_FRAME_LENGTH;
     this->processOffset();
-    }
+}
 
-    void Frames::processOffset()
+void Frames::processOffset()
-    {
+{
     if (this->items_.isEmpty())
     {
         return;
@@ -141,65 +135,63 @@ namespace detail {
             break;
         }
     }
-    }
+}
 
-    void Frames::clear()
+void Frames::clear()
-    {
+{
     assertInGuiThread();
     if (!this->empty())
     {
         DebugCount::decrease("loaded images");
     }
     DebugCount::decrease("image bytes", this->memoryUsage());
-        DebugCount::increase("image bytes (ever unloaded)",
+    DebugCount::increase("image bytes (ever unloaded)", this->memoryUsage());
-                             this->memoryUsage());
 
     this->items_.clear();
     this->index_ = 0;
     this->durationOffset_ = 0;
     this->gifTimerConnection_.disconnect();
-    }
+}
 
-    bool Frames::empty() const
+bool Frames::empty() const
-    {
+{
     return this->items_.empty();
-    }
+}
 
-    bool Frames::animated() const
+bool Frames::animated() const
-    {
+{
     return this->items_.size() > 1;
-    }
+}
 
-    std::optional<QPixmap> Frames::current() const
+std::optional<QPixmap> Frames::current() const
-    {
+{
     if (this->items_.empty())
     {
         return std::nullopt;
     }
 
     return this->items_[this->index_].image;
-    }
+}
 
-    std::optional<QPixmap> Frames::first() const
+std::optional<QPixmap> Frames::first() const
-    {
+{
     if (this->items_.empty())
     {
         return std::nullopt;
     }
 
     return this->items_.front().image;
-    }
+}
 
-    // functions
+QList<Frame> readFrames(QImageReader &reader, const Url &url)
-    QVector<Frame<QImage>> readFrames(QImageReader &reader, const Url &url)
+{
-    {
+    QList<Frame> frames;
-        QVector<Frame<QImage>> frames;
     frames.reserve(reader.imageCount());
 
-        QImage image;
     for (int index = 0; index < reader.imageCount(); ++index)
     {
-            if (reader.read(&image))
+        auto pixmap = QPixmap::fromImageReader(&reader);
+        if (!pixmap.isNull())
         {
             // It seems that browsers have special logic for fast animations.
             // This implements Chrome and Firefox's behavior which uses
@@ -212,85 +204,55 @@ namespace detail {
                 duration = 100;
             }
             duration = std::max(20, duration);
-                frames.push_back(Frame<QImage>{std::move(image), duration});
+            frames.append(Frame{
+                .image = std::move(pixmap),
+                .duration = duration,
+            });
         }
     }
 
     if (frames.empty())
     {
-            qCDebug(chatterinoImage)
+        qCDebug(chatterinoImage) << "Error while reading image" << url.string
-                << "Error while reading image" << url.string << ": '"
+                                 << ": '" << reader.errorString() << "'";
-                << reader.errorString() << "'";
     }
 
     return frames;
-    }
+}
 
-    // parsed
+void assignFrames(std::weak_ptr<Image> weak, QList<Frame> parsed)
-    template <typename Assign>
+{
-    void assignDelayed(
+    static bool isPushQueued;
-        std::queue<std::pair<Assign, QVector<Frame<QPixmap>>>> &queued,
+
-        std::mutex &mutex, std::atomic_bool &loadedEventQueued)
+    auto cb = [parsed = std::move(parsed), weak = std::move(weak)]() mutable {
+        auto shared = weak.lock();
+        if (!shared)
         {
-        std::lock_guard<std::mutex> lock(mutex);
-        int i = 0;
-
-        while (!queued.empty())
-        {
-            auto front = std::move(queued.front());
-            queued.pop();
-
-            // Call Assign with the vector of frames
-            front.first(std::move(front.second));
-
-            if (++i > 50)
-            {
-                QTimer::singleShot(3, [&] {
-                    assignDelayed(queued, mutex, loadedEventQueued);
-                });
             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();
-
-        loadedEventQueued = false;
-    }
-
-    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
+    postToGuiThread(cb);
+}
+
+}  // namespace chatterino::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();
+            QBuffer buffer;
-
+            buffer.setData(result.getData());
-            // const cast since we are only reading from it
-            QBuffer buffer(const_cast<QByteArray *>(&data));
-            buffer.open(QIODevice::ReadOnly);
             QImageReader reader(&buffer);
 
             if (!reader.canRead())
@@ -557,14 +516,7 @@ void Image::actuallyLoad()
 
             auto parsed = detail::readFrames(reader, shared->url());
 
-            postToThread(makeConvertCallback(
+            assignFrames(shared, parsed);
-                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));
-                    }
-                }));
         })
         .onError([weak](auto /*result*/) {
             auto shared = weak.lock();

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,17 +18,23 @@
 #include <optional>
 
 namespace chatterino {
-namespace detail {
+
-    template <typename Image>
+class Image;
-    struct Frame {
+
-        Image image;
+}  // namespace chatterino
+
+namespace chatterino::detail {
+
+struct Frame {
+    QPixmap image;
     int duration;
-    };
+};
-    class Frames
+
-    {
+class Frames
-    public:
+{
+public:
     Frames();
-        Frames(QVector<Frame<QPixmap>> &&frames);
+    Frames(QList<Frame> &&frames);
     ~Frames();
 
     Frames(const Frames &) = delete;
@@ -45,15 +50,21 @@ namespace detail {
     std::optional<QPixmap> current() const;
     std::optional<QPixmap> first() const;
 
-    private:
+private:
     int64_t memoryUsage() const;
     void processOffset();
-        QVector<Frame<QPixmap>> items_;
+    QList<Frame> items_;
-        int index_{0};
+    QList<Frame>::size_type index_{0};
     int durationOffset_{0};
     pajlada::Signals::Connection gifTimerConnection_;
-    };
+};
-}  // namespace detail
+
+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 @@ private:
     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.

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