From 8302a95bc15cc8fa06068744152120921bff4487 Mon Sep 17 00:00:00 2001 From: Jonas Kvinge Date: Sun, 5 Jan 2025 18:28:41 +0100 Subject: [PATCH] Use shared pointers for moodbar pipelines Possible fix for #1633 --- src/moodbar/moodbarcontroller.cpp | 33 +++++++++------- src/moodbar/moodbarcontroller.h | 6 +-- src/moodbar/moodbaritemdelegate.cpp | 30 +++++++++------ src/moodbar/moodbaritemdelegate.h | 6 +-- src/moodbar/moodbarloader.cpp | 59 ++++++++++++++++------------- src/moodbar/moodbarloader.h | 23 ++++++++--- src/moodbar/moodbarpipeline.cpp | 8 +++- src/moodbar/moodbarpipeline.h | 5 ++- 8 files changed, 102 insertions(+), 68 deletions(-) diff --git a/src/moodbar/moodbarcontroller.cpp b/src/moodbar/moodbarcontroller.cpp index 0aba7d958..a66323644 100644 --- a/src/moodbar/moodbarcontroller.cpp +++ b/src/moodbar/moodbarcontroller.cpp @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2012, David Sansome - * Copyright 2019-2024, Jonas Kvinge + * Copyright 2019-2025, Jonas Kvinge * * 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 #include +#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, const SharedPtr moodbar_loader, QObject *parent) : QObject(parent), player_(player), @@ -56,25 +59,27 @@ void MoodbarController::CurrentSongChanged(const Song &song) { if (!enabled_) return; - QByteArray data; - MoodbarPipeline *pipeline = nullptr; - const MoodbarLoader::Result result = moodbar_loader_->Load(song.url(), song.has_cue(), &data, &pipeline); - - switch (result) { - case MoodbarLoader::Result::CannotLoad: + const MoodbarLoader::LoadResult load_result = moodbar_loader_->Load(song.url(), song.has_cue()); + switch (load_result.status) { + case MoodbarLoader::LoadStatus::CannotLoad: Q_EMIT CurrentMoodbarDataChanged(QByteArray()); break; - case MoodbarLoader::Result::Loaded: - Q_EMIT CurrentMoodbarDataChanged(data); + case MoodbarLoader::LoadStatus::Loaded: + Q_EMIT CurrentMoodbarDataChanged(load_result.data); break; - case MoodbarLoader::Result::WillLoadAsync: - // Emit an empty array for now so the GUI reverts to a normal progress - // bar. Our slot will be called when the data is actually loaded. + case MoodbarLoader::LoadStatus::WillLoadAsync: + // 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. 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 connection = make_shared(); + *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(); diff --git a/src/moodbar/moodbarcontroller.h b/src/moodbar/moodbarcontroller.h index 7dc6fae03..94028fa9b 100644 --- a/src/moodbar/moodbarcontroller.h +++ b/src/moodbar/moodbarcontroller.h @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2012, David Sansome - * Copyright 2019-2024, Jonas Kvinge + * Copyright 2019-2025, Jonas Kvinge * * 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 #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_; diff --git a/src/moodbar/moodbaritemdelegate.cpp b/src/moodbar/moodbaritemdelegate.cpp index 821837053..f718aa665 100644 --- a/src/moodbar/moodbaritemdelegate.cpp +++ b/src/moodbar/moodbaritemdelegate.cpp @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2012, David Sansome - * Copyright 2019-2024, Jonas Kvinge + * Copyright 2019-2025, Jonas Kvinge * * 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 #include +#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 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; - MoodbarPipeline *pipeline = nullptr; - switch (moodbar_loader_->Load(url, has_cue, &bytes, &pipeline)) { - case MoodbarLoader::Result::CannotLoad: + const MoodbarLoader::LoadResult load_result = moodbar_loader_->Load(url, has_cue); + switch (load_result.status) { + case MoodbarLoader::LoadStatus::CannotLoad: data->state_ = Data::State::CannotLoad; break; - case MoodbarLoader::Result::Loaded: - // We got the data immediately. - StartLoadingColors(url, bytes, data); + case MoodbarLoader::LoadStatus::Loaded: + StartLoadingColors(url, load_result.data, data); break; - case MoodbarLoader::Result::WillLoadAsync: - // Maybe in a little while. - QObject::connect(pipeline, &MoodbarPipeline::Finished, this, [this, url, pipeline]() { DataLoaded(url, pipeline); }); + case MoodbarLoader::LoadStatus::WillLoadAsync: + MoodbarPipelinePtr pipeline = load_result.pipeline; + Q_ASSERT(pipeline); + SharedPtr connection = make_shared(); + *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; diff --git a/src/moodbar/moodbaritemdelegate.h b/src/moodbar/moodbaritemdelegate.h index 82d9e3f3e..94b1237c8 100644 --- a/src/moodbar/moodbaritemdelegate.h +++ b/src/moodbar/moodbaritemdelegate.h @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2012, David Sansome - * Copyright 2019-2024, Jonas Kvinge + * Copyright 2019-2025, Jonas Kvinge * * 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); diff --git a/src/moodbar/moodbarloader.cpp b/src/moodbar/moodbarloader.cpp index cd05b7e13..bcc16b4bb 100644 --- a/src/moodbar/moodbarloader.cpp +++ b/src/moodbar/moodbarloader.cpp @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2012, David Sansome - * Copyright 2019-2024, Jonas Kvinge + * Copyright 2019-2025, Jonas Kvinge * * 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 #include #include -#include #include #include #include #include #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 @@ -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 Result::WillLoadAsync; + return LoadResult(LoadStatus::WillLoadAsync, requests_.value(url)); } // 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); - if (f.exists()) { - if (f.open(QIODevice::ReadOnly)) { + QFile file(possible_mood_file); + if (file.exists()) { + if (file.open(QIODevice::ReadOnly)) { qLog(Info) << "Loading moodbar data from" << possible_mood_file; - *data = f.readAll(); - f.close(); - return Result::Loaded; + const QByteArray data = file.readAll(); + file.close(); + 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 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(); - if (!data->isEmpty()) { - return Result::Loaded; + const QByteArray data = device_cache_file->readAll(); + if (!data.isEmpty()) { + 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 connection = make_shared(); + *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 Result::WillLoadAsync; + return LoadResult(LoadStatus::WillLoadAsync, pipeline); } @@ -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(); } diff --git a/src/moodbar/moodbarloader.h b/src/moodbar/moodbarloader.h index a07ff6b0d..b5d07d3de 100644 --- a/src/moodbar/moodbarloader.h +++ b/src/moodbar/moodbarloader.h @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2012, David Sansome - * Copyright 2019-2024, Jonas Kvinge + * Copyright 2019-2025, Jonas Kvinge * * 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 #include +#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 requests_; + QMap requests_; QList queued_requests_; QSet active_requests_; diff --git a/src/moodbar/moodbarpipeline.cpp b/src/moodbar/moodbarpipeline.cpp index cfe80c5ea..f11751d82 100644 --- a/src/moodbar/moodbarpipeline.cpp +++ b/src/moodbar/moodbarpipeline.cpp @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2012, David Sansome - * Copyright 2019-2024, Jonas Kvinge + * Copyright 2019-2025, Jonas Kvinge * * 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) { diff --git a/src/moodbar/moodbarpipeline.h b/src/moodbar/moodbarpipeline.h index 604bc5db2..d24367f02 100644 --- a/src/moodbar/moodbarpipeline.h +++ b/src/moodbar/moodbarpipeline.h @@ -2,7 +2,7 @@ * Strawberry Music Player * This file was part of Clementine. * Copyright 2012, David Sansome - * Copyright 2019-2024, Jonas Kvinge + * Copyright 2019-2025, Jonas Kvinge * * 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 #include #include +#include #include #include @@ -75,4 +76,6 @@ class MoodbarPipeline : public QObject { QByteArray data_; }; +using MoodbarPipelinePtr = QSharedPointer; + #endif // MOODBARPIPELINE_H