Use shared pointers for moodbar pipelines

Possible fix for #1633

src/moodbar/moodbarcontroller.cpp

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2012, David Sansome <me@davidsansome.com>
- * Copyright 2019-2024, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2019-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -23,6 +23,7 @@
 #include <QByteArray>
 #include <QUrl>
 
+#include "includes/shared_ptr.h"
 #include "core/song.h"
 #include "core/settings.h"
 #include "core/player.h"
@@ -33,6 +34,8 @@
 #include "moodbarloader.h"
 #include "moodbarpipeline.h"
 
+using std::make_shared;
+
 MoodbarController::MoodbarController(const SharedPtr<Player> player, const SharedPtr<MoodbarLoader> moodbar_loader, QObject *parent)
     : QObject(parent),
       player_(player),
@@ -56,25 +59,27 @@ void MoodbarController::CurrentSongChanged(const Song &song) {
 
   if (!enabled_) return;
 
-  QByteArray data;
+  const MoodbarLoader::LoadResult load_result = moodbar_loader_->Load(song.url(), song.has_cue());
-  MoodbarPipeline *pipeline = nullptr;
+  switch (load_result.status) {
-  const MoodbarLoader::Result result = moodbar_loader_->Load(song.url(), song.has_cue(), &data, &pipeline);
+    case MoodbarLoader::LoadStatus::CannotLoad:
-
-  switch (result) {
-    case MoodbarLoader::Result::CannotLoad:
       Q_EMIT CurrentMoodbarDataChanged(QByteArray());
       break;
 
-    case MoodbarLoader::Result::Loaded:
+    case MoodbarLoader::LoadStatus::Loaded:
-      Q_EMIT CurrentMoodbarDataChanged(data);
+      Q_EMIT CurrentMoodbarDataChanged(load_result.data);
       break;
 
-    case MoodbarLoader::Result::WillLoadAsync:
+    case MoodbarLoader::LoadStatus::WillLoadAsync:
-      // Emit an empty array for now so the GUI reverts to a normal progress
+      // Emit an empty array for now so the GUI reverts to a normal progressbar.  Our slot will be called when the data is actually loaded.
-      // bar.  Our slot will be called when the data is actually loaded.
       Q_EMIT CurrentMoodbarDataChanged(QByteArray());
 
-      QObject::connect(pipeline, &MoodbarPipeline::Finished, this, [this, pipeline, song]() { AsyncLoadComplete(pipeline, song.url()); });
+      MoodbarPipelinePtr pipeline = load_result.pipeline;
+      Q_ASSERT(pipeline);
+      SharedPtr<QMetaObject::Connection> connection = make_shared<QMetaObject::Connection>();
+      *connection = QObject::connect(&*pipeline, &MoodbarPipeline::Finished, this, [this, connection, pipeline, song]() {
+        AsyncLoadComplete(pipeline, song.url());
+        QObject::disconnect(*connection);
+      });
       break;
   }
 
@@ -88,7 +93,7 @@ void MoodbarController::PlaybackStopped() {
 
 }
 
-void MoodbarController::AsyncLoadComplete(MoodbarPipeline *pipeline, const QUrl &url) {
+void MoodbarController::AsyncLoadComplete(MoodbarPipelinePtr pipeline, const QUrl &url) {
 
   // Is this song still playing?
   PlaylistItemPtr current_item = player_->GetCurrentItem();

src/moodbar/moodbarcontroller.h

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2012, David Sansome <me@davidsansome.com>
- * Copyright 2019-2024, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2019-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -28,9 +28,9 @@
 #include <QUrl>
 
 #include "includes/shared_ptr.h"
+#include "moodbarpipeline.h"
 
 class MoodbarLoader;
-class MoodbarPipeline;
 class Song;
 class Player;
 
@@ -51,7 +51,7 @@ class MoodbarController : public QObject {
   void PlaybackStopped();
 
  private Q_SLOTS:
-  void AsyncLoadComplete(MoodbarPipeline *pipeline, const QUrl &url);
+  void AsyncLoadComplete(MoodbarPipelinePtr pipeline, const QUrl &url);
 
  private:
   const SharedPtr<Player> player_;

src/moodbar/moodbaritemdelegate.cpp

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2012, David Sansome <me@davidsansome.com>
- * Copyright 2019-2024, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2019-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -36,6 +36,7 @@
 #include <QPainter>
 #include <QRect>
 
+#include "includes/shared_ptr.h"
 #include "core/logging.h"
 #include "core/settings.h"
 #include "playlist/playlist.h"
@@ -49,6 +50,8 @@
 
 #include "constants/moodbarsettings.h"
 
+using std::make_shared;
+
 MoodbarItemDelegate::Data::Data() : state_(State::None) {}
 
 MoodbarItemDelegate::MoodbarItemDelegate(const SharedPtr<MoodbarLoader> moodbar_loader, PlaylistView *playlist_view, QObject *parent)
@@ -155,21 +158,24 @@ void MoodbarItemDelegate::StartLoadingData(const QUrl &url, const bool has_cue,
   data->state_ = Data::State::LoadingData;
 
   // Load a mood file for this song and generate some colors from it
-  QByteArray bytes;
+  const MoodbarLoader::LoadResult load_result = moodbar_loader_->Load(url, has_cue);
-  MoodbarPipeline *pipeline = nullptr;
+  switch (load_result.status) {
-  switch (moodbar_loader_->Load(url, has_cue, &bytes, &pipeline)) {
+    case MoodbarLoader::LoadStatus::CannotLoad:
-    case MoodbarLoader::Result::CannotLoad:
       data->state_ = Data::State::CannotLoad;
       break;
 
-    case MoodbarLoader::Result::Loaded:
+    case MoodbarLoader::LoadStatus::Loaded:
-      // We got the data immediately.
+      StartLoadingColors(url, load_result.data, data);
-      StartLoadingColors(url, bytes, data);
       break;
 
-    case MoodbarLoader::Result::WillLoadAsync:
+    case MoodbarLoader::LoadStatus::WillLoadAsync:
-      // Maybe in a little while.
+      MoodbarPipelinePtr pipeline = load_result.pipeline;
-      QObject::connect(pipeline, &MoodbarPipeline::Finished, this, [this, url, pipeline]() { DataLoaded(url, pipeline); });
+      Q_ASSERT(pipeline);
+      SharedPtr<QMetaObject::Connection> connection = make_shared<QMetaObject::Connection>();
+      *connection = QObject::connect(&*pipeline, &MoodbarPipeline::Finished, this, [this, connection, url, pipeline]() {
+        DataLoaded(url, pipeline);
+        QObject::disconnect(*connection);
+      });
       break;
   }
 
@@ -199,7 +205,7 @@ void MoodbarItemDelegate::ReloadAllColors() {
 
 }
 
-void MoodbarItemDelegate::DataLoaded(const QUrl &url, MoodbarPipeline *pipeline) {
+void MoodbarItemDelegate::DataLoaded(const QUrl &url, MoodbarPipelinePtr pipeline) {
 
   if (!data_.contains(url)) return;
 

src/moodbar/moodbaritemdelegate.h

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2012, David Sansome <me@davidsansome.com>
- * Copyright 2019-2024, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2019-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -38,10 +38,10 @@
 
 #include "includes/shared_ptr.h"
 #include "constants/moodbarsettings.h"
+#include "moodbarpipeline.h"
 
 class QPainter;
 class MoodbarLoader;
-class MoodbarPipeline;
 class PlaylistView;
 
 class MoodbarItemDelegate : public QItemDelegate {
@@ -55,7 +55,7 @@ class MoodbarItemDelegate : public QItemDelegate {
  private Q_SLOTS:
   void ReloadSettings();
 
-  void DataLoaded(const QUrl &url, MoodbarPipeline *pipeline);
+  void DataLoaded(const QUrl &url, MoodbarPipelinePtr pipeline);
   void ColorsLoaded(const QUrl &url, const ColorVector &colors);
   void ImageLoaded(const QUrl &url, const QImage &image);
 

src/moodbar/moodbarloader.cpp

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2012, David Sansome <me@davidsansome.com>
- * Copyright 2019-2024, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2019-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -35,13 +35,13 @@
 #include <QFileInfo>
 #include <QAbstractNetworkCache>
 #include <QNetworkDiskCache>
-#include <QTimer>
 #include <QByteArray>
 #include <QString>
 #include <QUrl>
 #include <QSettings>
 
 #include "includes/scoped_ptr.h"
+#include "includes/shared_ptr.h"
 #include "core/logging.h"
 #include "core/settings.h"
 
@@ -51,6 +51,7 @@
 
 using namespace std::chrono_literals;
 using namespace Qt::Literals::StringLiterals;
+using std::make_shared;
 
 #ifdef Q_OS_WIN32
 #  include <windows.h>
@@ -104,16 +105,15 @@ QUrl MoodbarLoader::CacheUrlEntry(const QString &filename) {
 
 }
 
-MoodbarLoader::Result MoodbarLoader::Load(const QUrl &url, const bool has_cue, QByteArray *data, MoodbarPipeline **async_pipeline) {
+MoodbarLoader::LoadResult MoodbarLoader::Load(const QUrl &url, const bool has_cue) {
 
   if (!url.isLocalFile() || has_cue) {
-    return Result::CannotLoad;
+    return LoadStatus::CannotLoad;
   }
 
   // Are we in the middle of loading this moodbar already?
   if (requests_.contains(url)) {
-    *async_pipeline = requests_.value(url);
+    return LoadResult(LoadStatus::WillLoadAsync, requests_.value(url));
-    return Result::WillLoadAsync;
   }
 
   // Check if a mood file exists for this file already
@@ -121,16 +121,18 @@ MoodbarLoader::Result MoodbarLoader::Load(const QUrl &url, const bool has_cue, Q
 
   const QStringList possible_mood_files = MoodFilenames(filename);
   for (const QString &possible_mood_file : possible_mood_files) {
-    QFile f(possible_mood_file);
+    QFile file(possible_mood_file);
-    if (f.exists()) {
+    if (file.exists()) {
-      if (f.open(QIODevice::ReadOnly)) {
+      if (file.open(QIODevice::ReadOnly)) {
         qLog(Info) << "Loading moodbar data from" << possible_mood_file;
-        *data = f.readAll();
+        const QByteArray data = file.readAll();
-        f.close();
+        file.close();
-        return Result::Loaded;
+        if (!data.isEmpty()) {
+          return LoadResult(LoadStatus::Loaded, data);
+        }
       }
       else {
-        qLog(Error) << "Failed to load moodbar data from" << possible_mood_file << f.errorString();
+        qLog(Error) << "Failed to load moodbar data from" << possible_mood_file << file.errorString();
       }
     }
   }
@@ -142,9 +144,9 @@ MoodbarLoader::Result MoodbarLoader::Load(const QUrl &url, const bool has_cue, Q
     ScopedPtr<QIODevice> device_cache_file(cache_->data(disk_cache_metadata.url()));
     if (device_cache_file) {
       qLog(Info) << "Loading cached moodbar data for" << filename;
-      *data = device_cache_file->readAll();
+      const QByteArray data = device_cache_file->readAll();
-      if (!data->isEmpty()) {
+      if (!data.isEmpty()) {
-        return Result::Loaded;
+        return LoadResult(LoadStatus::Loaded, data);
       }
     }
   }
@@ -152,17 +154,20 @@ MoodbarLoader::Result MoodbarLoader::Load(const QUrl &url, const bool has_cue, Q
   if (!thread_->isRunning()) thread_->start(QThread::IdlePriority);
 
   // There was no existing file, analyze the audio file and create one.
-  MoodbarPipeline *pipeline = new MoodbarPipeline(url);
+  MoodbarPipelinePtr pipeline = MoodbarPipelinePtr(new MoodbarPipeline(url));
   pipeline->moveToThread(thread_);
-  QObject::connect(pipeline, &MoodbarPipeline::Finished, this, [this, pipeline, url]() { RequestFinished(pipeline, url); });
+  SharedPtr<QMetaObject::Connection> connection = make_shared<QMetaObject::Connection>();
+  *connection = QObject::connect(&*pipeline, &MoodbarPipeline::Finished, this, [this, connection, pipeline, url]() {
+    RequestFinished(pipeline, url);
+    QObject::disconnect(*connection);
+  });
 
   requests_[url] = pipeline;
   queued_requests_ << url;
 
   MaybeTakeNextRequest();
 
-  *async_pipeline = pipeline;
+  return LoadResult(LoadStatus::WillLoadAsync, pipeline);
-  return Result::WillLoadAsync;
 
 }
 
@@ -178,15 +183,17 @@ void MoodbarLoader::MaybeTakeNextRequest() {
   active_requests_ << url;
 
   qLog(Info) << "Creating moodbar data for" << url.toLocalFile();
-  QMetaObject::invokeMethod(requests_.value(url), &MoodbarPipeline::Start, Qt::QueuedConnection);
+
+  MoodbarPipelinePtr pipeline = requests_.value(url);
+  QMetaObject::invokeMethod(&*pipeline, &MoodbarPipeline::Start, Qt::QueuedConnection);
 
 }
 
-void MoodbarLoader::RequestFinished(MoodbarPipeline *request, const QUrl &url) {
+void MoodbarLoader::RequestFinished(MoodbarPipelinePtr pipeline, const QUrl &url) {
 
   Q_ASSERT(QThread::currentThread() == qApp->thread());
 
-  if (request->success()) {
+  if (pipeline->success()) {
 
     const QString filename = url.toLocalFile();
 
@@ -201,7 +208,7 @@ void MoodbarLoader::RequestFinished(MoodbarPipeline *request, const QUrl &url) {
 
     QIODevice *device_cache_file = cache_->prepare(disk_cache_metadata);
     if (device_cache_file) {
-      const qint64 data_written = device_cache_file->write(request->data());
+      const qint64 data_written = device_cache_file->write(pipeline->data());
       if (data_written > 0) {
         cache_->insert(device_cache_file);
       }
@@ -213,7 +220,7 @@ void MoodbarLoader::RequestFinished(MoodbarPipeline *request, const QUrl &url) {
       const QString mood_filename(mood_filenames[0]);
       QFile mood_file(mood_filename);
       if (mood_file.open(QIODevice::WriteOnly)) {
-        if (mood_file.write(request->data()) <= 0) {
+        if (mood_file.write(pipeline->data()) <= 0) {
           qLog(Error) << "Error writing to mood file" << mood_filename << mood_file.errorString();
         }
         mood_file.close();
@@ -233,8 +240,6 @@ void MoodbarLoader::RequestFinished(MoodbarPipeline *request, const QUrl &url) {
   requests_.remove(url);
   active_requests_.remove(url);
 
-  QTimer::singleShot(1s, request, &MoodbarLoader::deleteLater);
-
   MaybeTakeNextRequest();
 
 }

src/moodbar/moodbarloader.h

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2012, David Sansome <me@davidsansome.com>
- * Copyright 2019-2024, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2019-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -30,10 +30,11 @@
 #include <QStringList>
 #include <QUrl>
 
+#include "moodbarpipeline.h"
+
 class QThread;
 class QByteArray;
 class QNetworkDiskCache;
-class MoodbarPipeline;
 
 class MoodbarLoader : public QObject {
   Q_OBJECT
@@ -42,7 +43,7 @@ class MoodbarLoader : public QObject {
   explicit MoodbarLoader(QObject *parent = nullptr);
   ~MoodbarLoader() override;
 
-  enum class Result {
+  enum class LoadStatus {
     // The URL isn't a local file or the moodbar plugin was not available -
     // moodbar data can never be loaded.
     CannotLoad,
@@ -55,12 +56,22 @@ class MoodbarLoader : public QObject {
     WillLoadAsync
   };
 
+  class LoadResult {
+   public:
+    LoadResult(const LoadStatus _status) : status(_status) {}
+    LoadResult(const LoadStatus _status, MoodbarPipelinePtr _pipeline) : status(_status), pipeline(_pipeline) {}
+    LoadResult(const LoadStatus _status, const QByteArray &_data) : status(_status), data(_data) {}
+    LoadStatus status;
+    MoodbarPipelinePtr pipeline;
+    QByteArray data;
+  };
+
   void ReloadSettings();
 
-  Result Load(const QUrl &url, const bool has_cue, QByteArray *data, MoodbarPipeline **async_pipeline);
+  LoadResult Load(const QUrl &url, const bool has_cue);
 
  private Q_SLOTS:
-  void RequestFinished(MoodbarPipeline *request, const QUrl &url);
+  void RequestFinished(MoodbarPipelinePtr pipeline, const QUrl &url);
   void MaybeTakeNextRequest();
 
  private:
@@ -78,7 +89,7 @@ class MoodbarLoader : public QObject {
 
   const int kMaxActiveRequests;
 
-  QMap<QUrl, MoodbarPipeline*> requests_;
+  QMap<QUrl, MoodbarPipelinePtr> requests_;
   QList<QUrl> queued_requests_;
   QSet<QUrl> active_requests_;
 

src/moodbar/moodbarpipeline.cpp

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2012, David Sansome <me@davidsansome.com>
- * Copyright 2019-2024, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2019-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -56,7 +56,11 @@ MoodbarPipeline::MoodbarPipeline(const QUrl &url, QObject *parent)
       success_(false),
       running_(false) {}
 
-MoodbarPipeline::~MoodbarPipeline() { Cleanup(); }
+MoodbarPipeline::~MoodbarPipeline() {
+
+  Cleanup();
+
+}
 
 GstElement *MoodbarPipeline::CreateElement(const QString &factory_name) {
 

src/moodbar/moodbarpipeline.h

@@ -2,7 +2,7 @@
  * Strawberry Music Player
  * This file was part of Clementine.
  * Copyright 2012, David Sansome <me@davidsansome.com>
- * Copyright 2019-2024, Jonas Kvinge <jonas@jkvinge.net>
+ * Copyright 2019-2025, Jonas Kvinge <jonas@jkvinge.net>
  *
  * Strawberry is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -26,6 +26,7 @@
 #include <QByteArray>
 #include <QString>
 #include <QUrl>
+#include <QSharedPointer>
 
 #include <glib.h>
 #include <glib-object.h>
@@ -75,4 +76,6 @@ class MoodbarPipeline : public QObject {
   QByteArray data_;
 };
 
+using MoodbarPipelinePtr = QSharedPointer<MoodbarPipeline>;
+
 #endif  // MOODBARPIPELINE_H