[KPhotoAlbum] image decoding priorities & multithreading
Henner Zeller
h.zeller at acm.org
Fri Jun 27 13:53:37 BST 2008
Hi,
(sorry if this is late, I haven't had really internet the last couple of days)
I welcome having a priority queue! I just only had a quick look at the
change (just being at a conference), so just some quick remarks here:
Re: container
Instead of having a new stl container (std::priority_queue) here
(which Jesper probably would like to avoid), can't we just use the
QSet (as before) that is just sorted by priority ? I just had just a
quick look right now, so I might be missing something.
Re: addRequest
Does it suffice to change the priority and priority_queue does take
care of the proper sorting ? Then using std::priority_queue seems to
be actually the way to go ;-)
Re: typedef
I'd name the tupe QueueType or something not _queueType. From my
perspective, a type should be an uppercase name. Underscores should
only be used in member variables (however, this is only my quick take
not knowing the details yet of coding conventions in KPA)
On Mon, Jun 23, 2008 at 7:51 PM, Jan Kundrát <jkt at gentoo.org> wrote:
> now really attached :p
>
> --
> cd /local/pub && more beer > /dev/mouth
>adding a
> Index: Exif/InfoDialog.cpp
> ===================================================================
> --- Exif/InfoDialog.cpp (revision 823475)
> +++ Exif/InfoDialog.cpp (working copy)
> @@ -56,7 +56,9 @@
>
> _pix = new QLabel( top );
> hlay->addWidget( _pix );
> - ImageManager::Manager::instance()->load( new
> ImageManager::ImageRequest( fileName, QSize( 128, 128 ),
> DB::ImageDB::instance()->info(fileName)->angle(), this ) );
> + ImageManager::ImageRequest* request = new ImageManager::ImageRequest(
> fileName, QSize( 128, 128 ),
> DB::ImageDB::instance()->info(fileName)->angle(), this );
> + request->setPriority( ImageManager::Viewer );
> + ImageManager::Manager::instance()->load( request );
>
> // -------------------------------------------------- Exif Grid
> Exif::Grid* grid = new Exif::Grid( fileName, top );
> Index: HTMLGenerator/Generator.cpp
> ===================================================================
> --- HTMLGenerator/Generator.cpp (revision 823475)
> +++ HTMLGenerator/Generator.cpp (working copy)
> @@ -398,7 +398,7 @@
> else {
> ImageManager::ImageRequest* request =
> new ImageManager::ImageRequest( fileName, QSize( size, size ),
> DB::ImageDB::instance()->info(fileName)->angle(), this );
> - request->setPriority();
> + request->setPriority( ImageManager::BatchTask );
> ImageManager::Manager::instance()->load( request );
> _generatedFiles.insert( qMakePair( fileName, size ) );
> }
> Index: Utilities/Set.cpp
> ===================================================================
> --- Utilities/Set.cpp (revision 823475)
> +++ Utilities/Set.cpp (working copy)
> @@ -86,8 +86,5 @@
> #include "CategoryListView/DragItemInfo.h"
> INSTANTIATE_SET_CLASS_WITH_OPERATORS(CategoryListView::DragItemInfo);
>
> -#include "ImageManager/RequestQueue.h"
> -INSTANTIATE_SET_CLASS(ImageManager::RequestQueue::ImageRequestReference);
> -
> #include "ImageManager/ImageRequest.h"
> INSTANTIATE_SET_CLASS(ImageManager::ImageRequest*);
> Index: ImageManager/ImageRequest.h
> ===================================================================
> --- ImageManager/ImageRequest.h (revision 823475)
> +++ ImageManager/ImageRequest.h (working copy)
> @@ -34,6 +34,22 @@
> namespace ImageManager
> {
>
> +/** @short Priority of an image request
> + *
> + * The higher the priority, the sooner the image is expected to be decoded
> + * */
> +enum Priority {
> + BuildThumbnails, /**< @short Requests generated through the "Rebuild
> Thumbnails" command */
> + ThumbnailInvisible, /**< @short Thumbnails in current search scope, but
> invisible */
> + ViewerPreload, /**< @short Image that will be displayed later */
> + BatchTask, /**< @short Requests like resizing images for HTML pages
> + *
> + * As they are requested by user, they are expected to
> finish
> + * sooner than invisible thumbnails */
> + ThumbnailVisible, /**< @short Thumbnail visible on screen right now
> (might get invalidated later) */
> + Viewer /**< @short Image is visible in the viewer right now */
> +};
> +
> class ImageRequest {
> public:
> virtual ~ImageRequest() {}
> @@ -54,12 +70,14 @@
> void setLoadedOK( bool ok );
> bool loadedOK() const;
>
> - void setPriority( bool b = true );
> - bool priority() const;
> + void setPriority( const Priority prio );
> + Priority priority() const;
>
> bool operator<( const ImageRequest& other ) const;
> bool operator==( const ImageRequest& other ) const;
>
> + bool equalNoPriority( const ImageRequest& other ) const;
> +
> virtual bool stillNeeded() const;
>
> bool doUpScale() const;
> @@ -76,7 +94,7 @@
> ImageClient* _client;
> int _angle;
> QSize _fullSize;
> - bool _priority;
> + Priority _priority;
> bool _loadedOK;
> bool _dontUpScale;
> };
> @@ -88,6 +106,4 @@
>
> }
>
> -
> #endif /* IMAGEREQUEST_H */
> -
> Index: ImageManager/RequestQueue.h
> ===================================================================
> --- ImageManager/RequestQueue.h (revision 823475)
> +++ ImageManager/RequestQueue.h (working copy)
> @@ -18,11 +18,10 @@
> #ifndef REQUESTQUEUE_H
> #define REQUESTQUEUE_H
>
> +#include <queue>
> #include "StopAction.h"
> -#include <q3valuelist.h>
> #include "Utilities/Set.h"
> #include "ImageManager/ImageRequest.h"
> -#include <qobject.h>
>
> namespace ImageManager
> {
> @@ -59,40 +58,35 @@
> void print();
>
> private:
> - // A Reference to a ImageRequest with value semantic.
> - // This only stores the pointer to an ImageRequest object but behaves
> - // regarding the less-than and equals-operator like the object.
> - // This allows to store ImageRequests with value-semantic in a Set.
> - class ImageRequestReference {
> + /** @short std::priority_queue helper */
> + class ImageRequestPriorityCmp {
> public:
> - ImageRequestReference() : _ptr(0) {}
> - ImageRequestReference(const ImageRequestReference& other)
> - : _ptr(other._ptr) {}
> - ImageRequestReference(ImageRequest* ptr) : _ptr(ptr) {}
> -
> - bool operator<(const ImageRequestReference &other) const {
> - return *_ptr < *other._ptr;
> + bool operator()( const ImageRequest* const left, const
> ImageRequest* const right ) const {
> + return left->priority() < right->priority();
> }
> - bool operator==(const ImageRequestReference &other) const {
> - return *_ptr == *other._ptr;
> - }
> + };
>
> - operator const ImageRequest&() const
> - {
> - return *_ptr;
> + /** @short Helper for queue filtering. */
> + class ImageRequestEq {
> + const ImageRequest* reference;
> + public:
> + ImageRequestEq( const ImageRequest* const reference ):
> reference(reference) {};
> + bool operator()( const ImageRequest* const other ) const {
> + return other->equalNoPriority( *reference );
> }
> -
> - private:
> - ImageRequest * _ptr;
> };
>
> - // Input queue of pending input requests.
> - Q3ValueList<ImageRequest*> _pendingRequests;
> + /** @short A convenience typedef used in cancelRequests() */
> + typedef std::priority_queue<ImageRequest*, std::vector<ImageRequest*>,
> ImageRequestPriorityCmp> _queueType;
>
> - // Set of unique requests currently pending; used to discard the exact
> - // same requests.
> - Set<ImageRequestReference> _uniquePending;
> + /** @short Ordered priority queue of image requests that are waiting
> for processing */
> + _queueType _pendingRequests;
>
> + /** @short Set of unique requests currently pending.
> + *
> + * Used to discard multiple equaling requests (module the priority). */
> + Set<ImageRequest*> _uniquePending;
> +
> // All active requests that have a client
> Set<ImageRequest*> _activeRequests;
> };
> Index: ImageManager/Manager.cpp
> ===================================================================
> --- ImageManager/Manager.cpp (revision 823475)
> +++ ImageManager/Manager.cpp (working copy)
> @@ -50,7 +50,7 @@
> 3) Most important, it did not allow loading only thumbnails when the
> image themself weren't available.
> */
> -ImageManager::Manager::Manager() :_currentLoading(0)
> +ImageManager::Manager::Manager()
> {
> }
>
> @@ -83,10 +83,11 @@
> void ImageManager::Manager::loadImage( ImageRequest* request )
> {
> QMutexLocker dummy( &_lock );
> - if ( _currentLoading && *_currentLoading == *request &&
> _loadList.isRequestStillValid( _currentLoading )) {
> + QSet<ImageRequest*>::const_iterator req = _currentLoading.find( request
> );
> + if ( req != _currentLoading.end() && _loadList.isRequestStillValid(
> request ) ) {
> // The last part of the test above is needed to not fail on a race
> condition from AnnotationDialog::ImagePreview, where the preview
> // at startup request the same image numerous time (likely from
> resize event).
> - Q_ASSERT (_currentLoading != request);
> + Q_ASSERT ( *req != request);
> delete request;
>
> return; // We are currently loading it, calm down and wait please
> ;-)
> @@ -109,10 +110,12 @@
> ImageManager::ImageRequest* ImageManager::Manager::next()
> {
> QMutexLocker dummy(&_lock );
> - while ( !(_currentLoading = _loadList.popNext() ) )
> + ImageRequest* request = 0;
> + while ( !( request = _loadList.popNext() ) )
> _sleepers.wait( &_lock );
> + _currentLoading.insert( request );
>
> - return _currentLoading;
> + return request;
> }
>
> void ImageManager::Manager::customEvent( QEvent* ev )
> @@ -146,8 +149,7 @@
> }
>
> _loadList.removeRequest(request);
> - if ( _currentLoading == request )
> - _currentLoading = 0;
> + _currentLoading.remove( request );
> delete request;
>
> _lock.unlock();
> Index: ImageManager/Manager.h
> ===================================================================
> --- ImageManager/Manager.h (revision 823475)
> +++ ImageManager/Manager.h (working copy)
> @@ -80,7 +80,7 @@
> RequestQueue _loadList;
> QWaitCondition _sleepers;
> QMutex _lock;
> - ImageRequest* _currentLoading;
> + QSet<ImageRequest*> _currentLoading;
> };
>
> }
> Index: ImageManager/ImageRequest.cpp
> ===================================================================
> --- ImageManager/ImageRequest.cpp (revision 823475)
> +++ ImageManager/ImageRequest.cpp (working copy)
> @@ -19,7 +19,7 @@
>
> ImageManager::ImageRequest::ImageRequest( const QString& fileName, const
> QSize& size, int angle, ImageManager::ImageClient* client )
> : _null( false ), _fileName( fileName ), _width( size.width() ),
> _height( size.height() ),
> - _cache( false ), _client( client ), _angle( angle ), _priority(
> false ), _loadedOK( false ), _dontUpScale( false )
> + _cache( false ), _client( client ), _angle( angle ), _priority(
> ThumbnailVisible ), _loadedOK( false ), _dontUpScale( false )
> {
> }
>
> @@ -73,6 +73,13 @@
> _angle == other._angle && _client == other._client &&
> _priority == other._priority );
> }
> +bool ImageManager::ImageRequest::equalNoPriority( const ImageRequest& other
> ) const
> +{
> + // compare everything but priority
> + return ( _null == other._null && fileName() == other.fileName() &&
> + _width == other._width && _height == other._height &&
> + _angle == other._angle && _client == other._client );
> +}
>
>
> void ImageManager::ImageRequest::setCache( bool b )
> @@ -110,14 +117,14 @@
> _loadedOK = ok;
> }
>
> -bool ImageManager::ImageRequest::priority() const
> +ImageManager::Priority ImageManager::ImageRequest::priority() const
> {
> return _priority;
> }
>
> -void ImageManager::ImageRequest::setPriority( bool b )
> +void ImageManager::ImageRequest::setPriority( const Priority prio )
> {
> - _priority = b;
> + _priority = prio;
> }
>
> bool ImageManager::ImageRequest::stillNeeded() const
> Index: ImageManager/RequestQueue.cpp
> ===================================================================
> --- ImageManager/RequestQueue.cpp (revision 823475)
> +++ ImageManager/RequestQueue.cpp (working copy)
> @@ -15,6 +15,7 @@
> the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA 02110-1301, USA.
> */
> +#include <algorithm>
> #include "RequestQueue.h"
> #include "ImageRequest.h"
> #include <qtimer.h>
> @@ -25,16 +26,17 @@
>
> void ImageManager::RequestQueue::addRequest( ImageRequest* request )
> {
> - if ( _uniquePending.contains( request ) && ! request->priority() ) {
> + Set<ImageRequest*>::const_iterator it = std::find_if(
> _uniquePending.begin(), _uniquePending.end(),
> + ImageRequestEq( request ) );
> + if ( it != _uniquePending.end() ) {
> + if ( (*it)->priority() < request->priority() )
> + (*it)->setPriority( request->priority() );
> delete request;
> return;
> }
> -
> +
> _uniquePending.insert( request );
> - if ( request->priority() )
> - _pendingRequests.prepend( request );
> - else
> - _pendingRequests.append( request );
> + _pendingRequests.push( request );
>
> if ( request->client() )
> _activeRequests.insert( request );
> @@ -42,9 +44,9 @@
>
> ImageManager::ImageRequest* ImageManager::RequestQueue::popNext()
> {
> - while ( _pendingRequests.count() != 0 ) {
> - ImageRequest* request = _pendingRequests.first();
> - _pendingRequests.pop_front();
> + while ( ! _pendingRequests.empty() ) {
> + ImageRequest* request = _pendingRequests.top();
> + _pendingRequests.pop();
> _uniquePending.erase( request );
>
> if ( !request->stillNeeded() ) {
> @@ -72,15 +74,16 @@
> }
> }
>
> - for( Q3ValueList<ImageRequest*>::Iterator it =
> _pendingRequests.begin(); it != _pendingRequests.end(); ) {
> + for (Set<ImageRequest*>::iterator it = _uniquePending.begin(); it !=
> _uniquePending.end(); ) {
> ImageRequest* request = *it;
> - ++it;
> - if ( request->client() == client && ( action == StopAll ||
> !request->priority() ) ) {
> - _pendingRequests.remove( request );
> - _uniquePending.erase( request );
> + if ( request->client() == client && ( action == StopAll ||
> request->priority() < ThumbnailVisible ) ) {
> + it = _uniquePending.erase( it );
> delete request;
> + } else {
> + ++it;
> }
> }
> + _pendingRequests = _queueType( _uniquePending.begin(),
> _uniquePending.end() );
> }
>
> bool ImageManager::RequestQueue::isRequestStillValid( ImageRequest* request
> )
> @@ -96,15 +99,15 @@
> void ImageManager::RequestQueue::print()
> {
> kDebug() << "**************************************";
> - kDebug() << "Active: " << _activeRequests.size() << ", pending: " <<
> _pendingRequests.count();
> + kDebug() << "Active: " << _activeRequests.size() << ", pending: " <<
> _pendingRequests.size();
> kDebug() << "Active:";
> for (Set<ImageRequest*>::const_iterator it = _activeRequests.begin(); it
> != _activeRequests.end(); ++it ) {
> kDebug() << (*it)->fileName() << " " << (*it)->width() << "x" <<
> (*it)->height();
> }
> - kDebug() << "pending:";
> + /*kDebug() << "pending:";
> for (Q3ValueList<ImageRequest*>::const_iterator it =
> _pendingRequests.begin(); it != _pendingRequests.end(); ++it ) {
> kDebug() << (*it)->fileName() << " " << (*it)->width() << "x" <<
> (*it)->height();
> - }
> + }*/
> }
>
> ImageManager::RequestQueue::RequestQueue()
> Index: ThumbnailView/ThumbnailToolTip.cpp
> ===================================================================
> --- ThumbnailView/ThumbnailToolTip.cpp (revision 823475)
> +++ ThumbnailView/ThumbnailToolTip.cpp (working copy)
> @@ -161,7 +161,7 @@
> if ( fileName != _currentFileName ) {
> ImageManager::ImageRequest* request = new
> ImageManager::ImageRequest( fileName, QSize( size, size ), info->angle(),
> this );
> request->setCache();
> - request->setPriority();
> + request->setPriority( ImageManager::Viewer );
> ImageManager::Manager::instance()->load( request );
> return false;
> }
> Index: ThumbnailView/ThumbnailWidget.cpp
> ===================================================================
> --- ThumbnailView/ThumbnailWidget.cpp (revision 823475)
> +++ ThumbnailView/ThumbnailWidget.cpp (working copy)
> @@ -134,6 +134,7 @@
> dimensions.height() - 2 *
> Settings::SettingsData::instance()->thumbnailSpace() ),
> angle, this );
> request->setCache();
> + request->setPriority( ImageManager::ThumbnailVisible );
> ImageManager::Manager::instance()->load( request );
> }
> }
> Index: ThumbnailView/ThumbnailBuilder.cpp
> ===================================================================
> --- ThumbnailView/ThumbnailBuilder.cpp (revision 823475)
> +++ ThumbnailView/ThumbnailBuilder.cpp (working copy)
> @@ -23,6 +23,8 @@
> #include "DB/ImageInfo.h"
> #include <stdexcept>
>
> +// FIXME: needs a rewrite to make use of SMP & priorities
> +
> ThumbnailView::ThumbnailBuilder::ThumbnailBuilder( QWidget* parent, const
> char* name )
> :Q3ProgressDialog( parent, name )
> {
> @@ -47,7 +49,7 @@
> _infoMap.insert( info->fileName(), info );
> ImageManager::ImageRequest* request = new ImageManager::ImageRequest(
> info->fileName(), QSize(256,256), info->angle(), this );
> request->setCache();
> - request->setPriority();
> + request->setPriority( ImageManager::BuildThumbnails );
> ImageManager::Manager::instance()->load( request );
> }
>
> Index: ChangeLog
> ===================================================================
> --- ChangeLog (revision 823475)
> +++ ChangeLog (working copy)
> @@ -1,5 +1,7 @@
> 2008-06-23 Jan Kundrat <jkt at gentoo.org>
>
> + * Improve the concept of "priorities" for image loading
> +
> * Use all CPU cores for parallelization of image loading
>
> 2008-06-22 Jesper K. Pedersen <blackie at klaralvdalens-datakonsult.se>
> Index: AnnotationDialog/ImagePreview.cpp
> ===================================================================
> --- AnnotationDialog/ImagePreview.cpp (revision 823475)
> +++ AnnotationDialog/ImagePreview.cpp (working copy)
> @@ -89,7 +89,7 @@
> setPixmap(QPixmap()); //erase old image
> ImageManager::Manager::instance()->stop(this);
> ImageManager::ImageRequest* request = new
> ImageManager::ImageRequest( _info.fileName(), QSize( width(), height() ),
> _info.angle(), this );
> - request->setPriority();
> + request->setPriority( ImageManager::Viewer );
> ImageManager::Manager::instance()->load( request );
> }
> }
> @@ -191,7 +191,7 @@
> reset();
> ImageManager::Manager::instance()->stop(this);
> ImageManager::ImageRequest* request = new ImageManager::ImageRequest(
> fileName, QSize( width, height ), angle, this );
> - request->setPriority();
> + request->setPriority( ImageManager::ViewerPreload );
> ImageManager::Manager::instance()->load( request );
> }
>
> Index: Viewer/ImageDisplay.cpp
> ===================================================================
> --- Viewer/ImageDisplay.cpp (revision 823475)
> +++ Viewer/ImageDisplay.cpp (working copy)
> @@ -531,7 +531,7 @@
> {
> if ( _info->size() != _loadedImage.size() ) {
> ImageManager::ImageRequest* request = new
> ImageManager::ImageRequest( _info->fileName(), QSize(-1,-1), _info->angle(),
> this );
> - request->setPriority();
> + request->setPriority( ImageManager::Viewer );
> ImageManager::Manager::instance()->load( request );
> busy();
> _reloadImageInProgress = true;
> @@ -560,8 +560,7 @@
>
> ImageManager::ImageRequest* request = new ImageManager::ImageRequest(
> info->fileName(), s, info->angle(), this );
> request->setUpScale( viewSize == Settings::FullSize );
> - if ( priority )
> - request->setPriority();
> + request->setPriority( priority ? ImageManager::Viewer :
> ImageManager::ViewerPreload );
> ImageManager::Manager::instance()->load( request );
> }
>
> Index: ImportExport/Export.cpp
> ===================================================================
> --- ImportExport/Export.cpp (revision 823475)
> +++ ImportExport/Export.cpp (working copy)
> @@ -233,7 +233,7 @@
> _filesRemaining = list.count(); // Used to break the event loop.
> for( QStringList::ConstIterator it = list.begin(); it != list.end();
> ++it ) {
> ImageManager::ImageRequest* request = new
> ImageManager::ImageRequest( *it, QSize( 128, 128 ),
> DB::ImageDB::instance()->info(*it)->angle(), this );
> - request->setPriority();
> + request->setPriority( ImageManager::BatchTask );
> ImageManager::Manager::instance()->load( request );
> }
> if ( _filesRemaining > 0 ) {
> @@ -274,7 +274,7 @@
> _filesRemaining++;
> ImageManager::ImageRequest* request =
> new ImageManager::ImageRequest( *it, QSize( _maxSize,
> _maxSize ), DB::ImageDB::instance()->info(*it)->angle(), this );
> - request->setPriority();
> + request->setPriority( ImageManager::BatchTask );
> ImageManager::Manager::instance()->load( request );
> }
>
> Index: MainWindow/Window.cpp
> ===================================================================
> --- MainWindow/Window.cpp (revision 823475)
> +++ MainWindow/Window.cpp (working copy)
> @@ -1616,6 +1616,7 @@
> int size = Settings::SettingsData::instance()->thumbSize();
> DB::ImageInfoPtr info = DB::ImageDB::instance()->info( *imageIt );
> ImageManager::ImageRequest* request = new
> ImageManager::ImageRequest( *imageIt, QSize(size,size), info->angle(),
> _thumbnailView );
> + request->setPriority( ImageManager::BatchTask );
> ImageManager::Manager::instance()->load( request );
> }
>
>
> _______________________________________________
> KPhotoAlbum mailing list
> KPhotoAlbum at kdab.net
> http://mail.kdab.net/mailman/listinfo/kphotoalbum
>
>
--
Henner Zeller | h.zeller at acm.org
Bücher kaufen und freie Software fördern | http://bookzilla.de
More information about the Kphotoalbum
mailing list