From 5f4275336ccac4dc7f14ae6bb07976f00f18a4d0 Mon Sep 17 00:00:00 2001 From: mnaumov Date: Tue, 20 Apr 2021 20:05:32 -0700 Subject: [PATCH] Fixing deadlock related to thumbnails Adding smoothing to thumbnails in AssetBrowser --- .../AssetBrowser/Views/EntryDelegate.cpp | 2 +- .../AzToolsFramework/Thumbnails/Thumbnail.cpp | 8 +++++++- .../AzToolsFramework/Thumbnails/ThumbnailContext.cpp | 12 +++++++++++- .../AzToolsFramework/Thumbnails/ThumbnailContext.h | 10 +++++++++- .../AzToolsFramework/Thumbnails/ThumbnailWidget.cpp | 3 +++ .../AzToolsFramework/Thumbnails/ThumbnailerBus.h | 12 ++++++++++++ 6 files changed, 43 insertions(+), 4 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/AssetBrowser/Views/EntryDelegate.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/AssetBrowser/Views/EntryDelegate.cpp index 0a25d1cf25..abc290a406 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/AssetBrowser/Views/EntryDelegate.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/AssetBrowser/Views/EntryDelegate.cpp @@ -140,7 +140,7 @@ namespace AzToolsFramework else { QPixmap pixmap = thumbnail->GetPixmap(size); - painter->drawPixmap(point.x(), point.y(), size.width(), size.height(), pixmap); + painter->drawPixmap(point, pixmap.scaled(size, Qt::IgnoreAspectRatio, Qt::SmoothTransformation)); } return m_iconSize; } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/Thumbnail.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/Thumbnail.cpp index 939ca813b6..2892e4efce 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/Thumbnail.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/Thumbnail.cpp @@ -10,12 +10,14 @@ * */ +#include #include AZ_PUSH_DISABLE_WARNING(4127 4251 4800 4244, "-Wunknown-warning-option") // 4127: conditional expression is constant // 4251: 'QTextCodec::ConverterState::flags': class 'QFlags' needs to have dll-interface to be used by clients of struct 'QTextCodec::ConverterState' // 4800: 'QTextBoundaryFinderPrivate *const ': forcing value to bool 'true' or 'false' (performance warning) // 4244: conversion from 'int' to 'qint8', possible loss of data #include +#include AZ_POP_DISABLE_WARNING namespace AzToolsFramework @@ -80,7 +82,11 @@ namespace AzToolsFramework if (m_state == State::Unloaded) { m_state = State::Loading; - QFuture future = QtConcurrent::run([this](){ LoadThread(); }); + QThreadPool* threadPool; + ThumbnailContextRequestBus::BroadcastResult( + threadPool, + &ThumbnailContextRequestBus::Handler::GetThreadPool); + QFuture future = QtConcurrent::run(threadPool, [this](){ LoadThread(); }); m_watcher.setFuture(future); } } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailContext.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailContext.cpp index 8f6b9c63fd..ae6d7c1178 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailContext.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailContext.cpp @@ -28,10 +28,15 @@ namespace AzToolsFramework : m_missingThumbnail(new MissingThumbnail(thumbnailSize)) , m_loadingThumbnail(new LoadingThumbnail(thumbnailSize)) , m_thumbnailSize(thumbnailSize) + , m_threadPool(this) { + ThumbnailContextRequestBus::Handler::BusConnect(); } - ThumbnailContext::~ThumbnailContext() = default; + ThumbnailContext::~ThumbnailContext() + { + ThumbnailContextRequestBus::Handler::BusDisconnect(); + } bool ThumbnailContext::IsLoading(SharedThumbnailKey key) { @@ -53,6 +58,11 @@ namespace AzToolsFramework AzToolsFramework::AssetBrowser::AssetBrowserViewRequestBus::Broadcast(&AzToolsFramework::AssetBrowser::AssetBrowserViewRequests::Update); } + QThreadPool* ThumbnailContext::GetThreadPool() + { + return &m_threadPool; + } + SharedThumbnail ThumbnailContext::GetThumbnail(SharedThumbnailKey key) { SharedThumbnail thumbnail; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailContext.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailContext.h index 589abad0df..6ba3d637f6 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailContext.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailContext.h @@ -19,6 +19,7 @@ #include #include +#include #endif class QString; @@ -40,6 +41,7 @@ namespace AzToolsFramework */ class ThumbnailContext : public QObject + , public ThumbnailContextRequestBus::Handler { Q_OBJECT public: @@ -58,10 +60,13 @@ namespace AzToolsFramework void UnregisterThumbnailProvider(const char* providerName); void RedrawThumbnail(); - + //! Default context used for most thumbnails static constexpr const char* DefaultContext = "Default"; + // ThumbnailContextRequestBus::Handler interface overrides... + QThreadPool* GetThreadPool() override; + private: struct ProviderCompare { bool operator() (const SharedThumbnailProvider& lhs, const SharedThumbnailProvider& rhs) const @@ -79,6 +84,9 @@ namespace AzToolsFramework SharedThumbnail m_loadingThumbnail; //! Thumbnail size (width and height in pixels) int m_thumbnailSize; + //! There is only a limited number of threads on global threadPool, because there can be many thumbnails rendering at once + //! an individual threadPool is needed to avoid deadlocks + QThreadPool m_threadPool; }; } // namespace Thumbnailer } // namespace AzToolsFramework diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailWidget.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailWidget.cpp index 1aefa6f189..038bfd5da5 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailWidget.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailWidget.cpp @@ -81,6 +81,9 @@ namespace AzToolsFramework int x = (originalWidth - realWidth) / 2; // pixmap needs to be manually scaled to produce smoother result and avoid looking pixelated // using painter.setRenderHint(QPainter::SmoothPixmapTransform); does not seem to work + // Note: there is a potential issue with pixmap.scaled: + // it is multithreaded (using global threadPool) and blocking until finished. + // A deadlock will happen if global threadPool has no free threads available. painter.drawPixmap(QPoint(x, 0), pixmap.scaled(realWidth, realHeight, Qt::IgnoreAspectRatio, Qt::SmoothTransformation)); } QWidget::paintEvent(event); diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailerBus.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailerBus.h index ebc721bb4c..02264e61f8 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailerBus.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Thumbnails/ThumbnailerBus.h @@ -17,11 +17,23 @@ #include class QPixmap; +class QThreadPool; namespace AzToolsFramework { namespace Thumbnailer { + //! Interaction with thumbnail context + class ThumbnailContextRequests + : public AZ::EBusTraits + { + public: + //! Get thread pool for drawing thumbnails + virtual QThreadPool* GetThreadPool() = 0; + }; + + using ThumbnailContextRequestBus = AZ::EBus; + //! Interaction with thumbnailer class ThumbnailerRequests : public AZ::EBusTraits