From 2eebecfb179c5033188672347f6177da7b36d12c Mon Sep 17 00:00:00 2001 From: Megamouse Date: Sun, 6 Jun 2021 10:43:39 +0200 Subject: [PATCH] Qt: Fix concurrency bug in trophy manager The missing mutex frequently caused a crash after I improved the individual trophy folder parsing by deferring icon loading to when it is actually needed. --- rpcs3/rpcs3qt/trophy_manager_dialog.cpp | 77 ++++++++++++++----------- rpcs3/rpcs3qt/trophy_manager_dialog.h | 8 +-- 2 files changed, 46 insertions(+), 39 deletions(-) diff --git a/rpcs3/rpcs3qt/trophy_manager_dialog.cpp b/rpcs3/rpcs3qt/trophy_manager_dialog.cpp index 8617ae9f0b..457a7b42bf 100644 --- a/rpcs3/rpcs3qt/trophy_manager_dialog.cpp +++ b/rpcs3/rpcs3qt/trophy_manager_dialog.cpp @@ -210,7 +210,7 @@ trophy_manager_dialog::trophy_manager_dialog(std::shared_ptr gui_s setLayout(all_layout); // Make connects - connect(m_icon_slider, &QSlider::valueChanged, this, [=, this](int val) + connect(m_icon_slider, &QSlider::valueChanged, this, [this, trophy_slider_label](int val) { m_icon_height = val; if (trophy_slider_label) @@ -225,12 +225,12 @@ trophy_manager_dialog::trophy_manager_dialog(std::shared_ptr gui_s } }); - connect(m_icon_slider, &QSlider::sliderReleased, this, [&]() + connect(m_icon_slider, &QSlider::sliderReleased, this, [this]() { m_gui_settings->SetValue(gui::tr_icon_height, m_icon_slider->value()); }); - connect(m_icon_slider, &QSlider::actionTriggered, [&](int action) + connect(m_icon_slider, &QSlider::actionTriggered, this, [this](int action) { if (action != QAbstractSlider::SliderNoAction && action != QAbstractSlider::SliderMove) { // we only want to save on mouseclicks or slider release (the other connect handles this) @@ -238,7 +238,7 @@ trophy_manager_dialog::trophy_manager_dialog(std::shared_ptr gui_s } }); - connect(m_game_icon_slider, &QSlider::valueChanged, this, [=, this](int val) + connect(m_game_icon_slider, &QSlider::valueChanged, this, [this, game_slider_label](int val) { m_game_icon_size_index = val; m_game_icon_size = gui_settings::SizeFromSlider(val); @@ -254,12 +254,12 @@ trophy_manager_dialog::trophy_manager_dialog(std::shared_ptr gui_s } }); - connect(m_game_icon_slider, &QSlider::sliderReleased, this, [&]() + connect(m_game_icon_slider, &QSlider::sliderReleased, this, [this]() { m_gui_settings->SetValue(gui::tr_game_iconSize, m_game_icon_slider->value()); }); - connect(m_game_icon_slider, &QSlider::actionTriggered, [&](int action) + connect(m_game_icon_slider, &QSlider::actionTriggered, this, [this](int action) { if (action != QAbstractSlider::SliderNoAction && action != QAbstractSlider::SliderMove) { // we only want to save on mouseclicks or slider release (the other connect handles this) @@ -267,49 +267,49 @@ trophy_manager_dialog::trophy_manager_dialog(std::shared_ptr gui_s } }); - connect(check_lock_trophy, &QCheckBox::clicked, [this](bool checked) + connect(check_lock_trophy, &QCheckBox::clicked, this, [this](bool checked) { m_show_locked_trophies = checked; ApplyFilter(); m_gui_settings->SetValue(gui::tr_show_locked, checked); }); - connect(check_unlock_trophy, &QCheckBox::clicked, [this](bool checked) + connect(check_unlock_trophy, &QCheckBox::clicked, this, [this](bool checked) { m_show_unlocked_trophies = checked; ApplyFilter(); m_gui_settings->SetValue(gui::tr_show_unlocked, checked); }); - connect(check_hidden_trophy, &QCheckBox::clicked, [this](bool checked) + connect(check_hidden_trophy, &QCheckBox::clicked, this, [this](bool checked) { m_show_hidden_trophies = checked; ApplyFilter(); m_gui_settings->SetValue(gui::tr_show_hidden, checked); }); - connect(check_bronze_trophy, &QCheckBox::clicked, [this](bool checked) + connect(check_bronze_trophy, &QCheckBox::clicked, this, [this](bool checked) { m_show_bronze_trophies = checked; ApplyFilter(); m_gui_settings->SetValue(gui::tr_show_bronze, checked); }); - connect(check_silver_trophy, &QCheckBox::clicked, [this](bool checked) + connect(check_silver_trophy, &QCheckBox::clicked, this, [this](bool checked) { m_show_silver_trophies = checked; ApplyFilter(); m_gui_settings->SetValue(gui::tr_show_silver, checked); }); - connect(check_gold_trophy, &QCheckBox::clicked, [this](bool checked) + connect(check_gold_trophy, &QCheckBox::clicked, this, [this](bool checked) { m_show_gold_trophies = checked; ApplyFilter(); m_gui_settings->SetValue(gui::tr_show_gold, checked); }); - connect(check_platinum_trophy, &QCheckBox::clicked, [this](bool checked) + connect(check_platinum_trophy, &QCheckBox::clicked, this, [this](bool checked) { m_show_platinum_trophies = checked; ApplyFilter(); @@ -318,13 +318,13 @@ trophy_manager_dialog::trophy_manager_dialog(std::shared_ptr gui_s connect(m_trophy_table, &QTableWidget::customContextMenuRequested, this, &trophy_manager_dialog::ShowContextMenu); - connect(m_game_combo, &QComboBox::currentTextChanged, [this] + connect(m_game_combo, &QComboBox::currentTextChanged, this, [this] { PopulateTrophyTable(); ApplyFilter(); }); - connect(m_game_table, &QTableWidget::itemSelectionChanged, [this] + connect(m_game_table, &QTableWidget::itemSelectionChanged, this, [this] { if (m_game_table->selectedItems().isEmpty()) { @@ -356,11 +356,11 @@ bool trophy_manager_dialog::LoadTrophyFolderToDB(const std::string& trop_name) game_trophy_data->path = vfs::get(trophyPath + "/"); game_trophy_data->trop_usr.reset(new TROPUSRLoader()); - const std::string trophyUsrPath = trophyPath + "/TROPUSR.DAT"; - const std::string trophyConfPath = trophyPath + "/TROPCONF.SFM"; - const bool success = game_trophy_data->trop_usr->Load(trophyUsrPath, trophyConfPath).success; + const std::string tropusr_path = trophyPath + "/TROPUSR.DAT"; + const std::string tropconf_path = trophyPath + "/TROPCONF.SFM"; + const bool success = game_trophy_data->trop_usr->Load(tropusr_path, tropconf_path).success; - fs::file config(vfs::get(trophyConfPath)); + fs::file config(vfs::get(tropconf_path)); if (!success || !config) { @@ -379,7 +379,7 @@ bool trophy_manager_dialog::LoadTrophyFolderToDB(const std::string& trop_name) for (u32 trophy_id = 0; trophy_id < trophy_count; ++trophy_id) { // A trophy icon has 3 digits from 000 to 999, for example TROP001.PNG - game_trophy_data->trophy_image_paths << qstr(fmt::format("%sTROP%03d.PNG", game_trophy_data->path, trophy_id)); + game_trophy_data->trophy_image_paths[trophy_id] = qstr(fmt::format("%sTROP%03d.PNG", game_trophy_data->path, trophy_id)); } // Get game name @@ -398,7 +398,10 @@ bool trophy_manager_dialog::LoadTrophyFolderToDB(const std::string& trop_name) } } - m_trophies_db.push_back(std::move(game_trophy_data)); + { + std::scoped_lock lock(m_trophies_db_mtx); + m_trophies_db.push_back(std::move(game_trophy_data)); + } config.release(); return true; @@ -531,23 +534,20 @@ void trophy_manager_dialog::ResizeTrophyIcons() const qreal dpr = devicePixelRatioF(); const int new_height = m_icon_height * dpr; - QList indices; + QList trophy_ids; for (int i = 0; i < m_trophy_table->rowCount(); ++i) - indices.append(i); - - const std::function get_scaled = [this, data = m_trophies_db.at(db_pos).get(), dpr, new_height](const int& i) { - QTableWidgetItem* item = m_trophy_table->item(i, TrophyColumns::Id); - QTableWidgetItem* icon_item = m_trophy_table->item(i, TrophyColumns::Icon); - if (!item || !icon_item) + if (QTableWidgetItem* item = m_trophy_table->item(i, TrophyColumns::Id)) { - return QPixmap(); + trophy_ids.append(item->text().toInt()); } + } - const int trophy_id = item->text().toInt(); + const std::function get_scaled = [this, data = m_trophies_db.at(db_pos).get(), dpr, new_height](const int& trophy_id) + { QPixmap icon; { - std::scoped_lock lock(data->mtx); + std::scoped_lock lock(m_trophies_db_mtx); if (data->trophy_images.contains(trophy_id)) { icon = data->trophy_images[trophy_id]; @@ -577,13 +577,18 @@ void trophy_manager_dialog::ResizeTrophyIcons() return new_icon.scaledToHeight(new_height, Qt::SmoothTransformation); }; - QList scaled = QtConcurrent::blockingMapped>(indices, get_scaled); - for (int i = 0; i < m_trophy_table->rowCount() && i < scaled.count(); ++i) + // NOTE: Due to a Qt bug, QtConcurrent::blockingMapped has a high risk of deadlocking + // during the QPixmap operations in get_scaled. So let's just use QtConcurrent::mapped. + QFutureWatcher watcher; + watcher.setFuture(QtConcurrent::mapped(trophy_ids, get_scaled)); + watcher.waitForFinished(); + + for (int i = 0; i < m_trophy_table->rowCount() && i < watcher.future().resultCount(); ++i) { QTableWidgetItem* icon_item = m_trophy_table->item(i, TrophyColumns::Icon); if (icon_item) - icon_item->setData(Qt::DecorationRole, scaled[i]); + icon_item->setData(Qt::DecorationRole, watcher.future().resultAt(i)); } ReadjustTrophyTable(); @@ -657,7 +662,7 @@ void trophy_manager_dialog::ShowContextMenu(const QPoint& pos) const int db_ind = m_game_combo->currentData().toInt(); - connect(show_trophy_dir, &QAction::triggered, [=, this]() + connect(show_trophy_dir, &QAction::triggered, this, [this, db_ind]() { const QString path = qstr(m_trophies_db[db_ind]->path); QDesktopServices::openUrl(QUrl::fromLocalFile(path)); @@ -763,6 +768,8 @@ void trophy_manager_dialog::PopulateGameTable() m_game_combo->addItem(name, i); } + m_game_combo->model()->sort(0, Qt::AscendingOrder); + ResizeGameIcons(); m_game_table->setSortingEnabled(true); // Enable sorting only after using setItem calls diff --git a/rpcs3/rpcs3qt/trophy_manager_dialog.h b/rpcs3/rpcs3qt/trophy_manager_dialog.h index 7c96a39c27..4fc0189656 100644 --- a/rpcs3/rpcs3qt/trophy_manager_dialog.h +++ b/rpcs3/rpcs3qt/trophy_manager_dialog.h @@ -11,8 +11,8 @@ #include #include -#include #include +#include class game_list; class gui_settings; @@ -22,11 +22,10 @@ struct GameTrophiesData { std::unique_ptr trop_usr; rXmlDocument trop_config; // I'd like to use unique but the protocol inside of the function passes around shared pointers.. - std::map trophy_images; // Cache trophy images to avoid loading from disk as much as possible. - QStringList trophy_image_paths; + std::unordered_map trophy_images; // Cache trophy images to avoid loading from disk as much as possible. + std::unordered_map trophy_image_paths; std::string game_name; std::string path; - std::mutex mtx; }; enum TrophyColumns @@ -98,6 +97,7 @@ private: std::shared_ptr m_gui_settings; std::vector> m_trophies_db; //! Holds all the trophy information. + std::mutex m_trophies_db_mtx; QComboBox* m_game_combo; //! Lets you choose a game QLabel* m_game_progress; //! Shows you the current game's progress QSplitter* m_splitter; //! Contains the game and trophy tables