Merge pull request #2525 from twodoorcoupe/cover_processing_multithre…

…ading

Make image processing multithreaded

picard/coverart/__init__.py

@@ -32,21 +32,15 @@
 
 from picard import log
 from picard.config import get_config
-from picard.coverart.image import (
-    CoverArtImageIdentificationError,
-    CoverArtImageIOError,
-)
+from picard.coverart.image import CoverArtImageIOError
 from picard.coverart.processing import (
+    CoverArtImageProcessing,
     run_image_filters,
-    run_image_processors,
 )
 from picard.coverart.providers import (
     CoverArtProvider,
     cover_art_providers,
 )
-from picard.extension_points.cover_art_processors import (
-    CoverArtProcessingError,
-)
 from picard.extension_points.metadata import register_album_metadata_processor
 from picard.i18n import N_
 from picard.util import imageinfo
@@ -60,6 +54,7 @@ def __init__(self, album, metadata, release):
         self.metadata = metadata
         self.release = release  # not used in this class, but used by providers
         self.front_image_found = False
+        self.image_processing = CoverArtImageProcessing(album)
 
     def __repr__(self):
         return "%s for %r" % (self.__class__.__name__, self.album)
@@ -74,31 +69,19 @@ def retrieve(self):
             log.debug("Cover art disabled by user options.")
 
     def _set_metadata(self, coverartimage, data, image_info):
-        try:
-            if coverartimage.can_be_processed:
-                run_image_processors(coverartimage, data, image_info)
-            else:
-                coverartimage.set_tags_data(data)
-            if coverartimage.can_be_saved_to_metadata:
-                log.debug("Storing to metadata: %r [%s]",
-                    coverartimage, coverartimage.imageinfo_as_string())
-                self.metadata.images.append(coverartimage)
-                for track in self.album._new_tracks:
-                    track.metadata.images.append(coverartimage)
-                # If the image already was a front image,
-                # there might still be some other non-CAA front
-                # images in the queue - ignore them.
-                if not self.front_image_found:
-                    self.front_image_found = coverartimage.is_front_image()
-            else:
-                log.debug("Not storing to metadata: %r [%s]",
-                    coverartimage, coverartimage.imageinfo_as_string())
-        except CoverArtImageIOError as e:
-            self.album.error_append(e)
-            self.album._finalize_loading(error=True)
-            raise e
-        except (CoverArtImageIdentificationError, CoverArtProcessingError) as e:
-            self.album.error_append(e)
+        self.image_processing.run_image_processors(coverartimage, data, image_info)
+        if coverartimage.can_be_saved_to_metadata:
+            log.debug("Storing to metadata: %r", coverartimage)
+            self.metadata.images.append(coverartimage)
+            for track in self.album._new_tracks:
+                track.metadata.images.append(coverartimage)
+            # If the image already was a front image,
+            # there might still be some other non-CAA front
+            # images in the queue - ignore them.
+            if not self.front_image_found:
+                self.front_image_found = coverartimage.is_front_image()
+        else:
+            log.debug("Not storing to metadata: %r", coverartimage)
 
     def _coverart_downloaded(self, coverartimage, data, http, error):
         """Handle finished download, save it to metadata"""
@@ -125,9 +108,7 @@ def _coverart_downloaded(self, coverartimage, data, http, error):
                     filters_result = run_image_filters(data, image_info, self.album, coverartimage)
                 if filters_result:
                     self._set_metadata(coverartimage, data, image_info)
-            except (CoverArtImageIOError, imageinfo.IdentificationError):
-                # It doesn't make sense to store/download more images if we can't
-                # save them in the temporary folder, abort.
+            except imageinfo.IdentificationError:
                 return
 
         self.next_in_queue()
@@ -139,13 +120,17 @@ def next_in_queue(self):
         if self.album.id not in self.album.tagger.albums:
             # album removed
             return
+        if self.album.loaded:
+            # album has finished loading due to errors
+            return
 
         config = get_config()
         if (self.front_image_found
             and config.setting['save_images_to_tags']
             and not config.setting['save_images_to_files']
             and config.setting['embed_only_one_front_image']):
             # no need to continue
+            self.image_processing.wait_for_processing()
             self.album._finalize_loading(None)
             return
 
@@ -170,6 +155,7 @@ def next_in_queue(self):
                 return
             except StopIteration:
                 # nothing more to do
+                self.image_processing.wait_for_processing()
                 self.album._finalize_loading(None)
                 return
 

picard/coverart/processing/__init__.py

@@ -18,10 +18,17 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
 
+from functools import partial
 import time
 
+from PyQt6.QtCore import QThreadPool
+
 from picard import log
 from picard.config import get_config
+from picard.coverart.image import (
+    CoverArtImageIdentificationError,
+    CoverArtImageIOError,
+)
 from picard.coverart.processing import (  # noqa: F401 # pylint: disable=unused-import
     filters,
     processors,
@@ -36,6 +43,7 @@
     ProcessingTarget,
     get_cover_art_processors,
 )
+from picard.util import thread
 from picard.util.imageinfo import IdentificationError
 
 
@@ -53,37 +61,82 @@ def run_image_metadata_filters(metadata):
     return True
 
 
-def run_image_processors(coverartimage, data, image_info):
-    config = get_config()
-    tags_data = data
-    file_data = data
-    try:
-        start_time = time.time()
-        image = ProcessingImage(data, image_info)
-        both_queue, tags_queue, file_queue = get_cover_art_processors()
-        for processor in both_queue:
-            processor.run(image, ProcessingTarget.BOTH)
-        if config.setting['save_images_to_tags']:
-            tags_image = image.copy()
-            for processor in tags_queue:
-                processor.run(tags_image, ProcessingTarget.TAGS)
-            tags_data = tags_image.get_result()
-        coverartimage.set_tags_data(tags_data)
-        if config.setting['save_images_to_files']:
-            file_image = image.copy()
-            for processor in file_queue:
-                processor.run(file_image, ProcessingTarget.FILE)
-            file_data = file_image.get_result()
-            coverartimage.set_external_file_data(file_data)
-        log.debug(
-            "Image processing for %s finished in %d ms",
-            coverartimage,
-            1000 * (time.time() - start_time)
-        )
-    except IdentificationError as e:
-        raise CoverArtProcessingError(e)
-    except CoverArtProcessingError as e:
-        coverartimage.set_tags_data(tags_data)
-        if config.setting['save_images_to_files']:
-            coverartimage.set_external_file_data(file_data)
-        raise e
+def handle_processing_exceptions(func):
+    def wrapper(self, *args, **kwargs):
+        try:
+            func(self, *args, **kwargs)
+        except CoverArtImageIOError as e:
+            self.album.error_append(e)
+            self.threadpool.clear()
+            if self.album.loaded:
+                self.album._finalize_loading(error=True)
+        except (CoverArtImageIdentificationError, CoverArtProcessingError) as e:
+            self.album.error_append(e)
+    return wrapper
+
+
+class CoverArtImageProcessing:
+
+    def __init__(self, album):
+        self.album = album
+        self.queues = get_cover_art_processors()
+        self.threadpool = QThreadPool()
+        self.threadpool.setMaxThreadCount(1)
+
+    @handle_processing_exceptions
+    def _run_processors_queue(self, coverartimage, initial_data, start_time, image, target):
+        data = initial_data
+        try:
+            queue = self.queues[target]
+            for processor in queue:
+                processor.run(image, target)
+            data = image.get_result()
+        except CoverArtProcessingError as e:
+            raise e
+        finally:
+            if target == ProcessingTarget.TAGS:
+                coverartimage.set_tags_data(data)
+            else:
+                coverartimage.set_external_file_data(data)
+            log.debug(
+                "Image processing for %s cover art image %s finished in %d ms",
+                target.name,
+                coverartimage,
+                1000 * (time.time() - start_time)
+            )
+
+    @handle_processing_exceptions
+    def _run_image_processors(self, coverartimage, initial_data, image_info):
+        config = get_config()
+        try:
+            start_time = time.time()
+            image = ProcessingImage(initial_data, image_info)
+            for processor in self.queues[ProcessingTarget.BOTH]:
+                processor.run(image, ProcessingTarget.BOTH)
+            run_queue_common = partial(self._run_processors_queue, coverartimage, initial_data, start_time)
+            if config.setting['save_images_to_files']:
+                run_queue = partial(run_queue_common, image.copy(), ProcessingTarget.FILE)
+                thread.run_task(run_queue, thread_pool=self.threadpool)
+            if config.setting['save_images_to_tags']:
+                run_queue = partial(run_queue_common, image.copy(), ProcessingTarget.TAGS)
+                thread.run_task(run_queue, thread_pool=self.threadpool)
+            else:
+                coverartimage.set_tags_data(initial_data)
+        except IdentificationError as e:
+            raise CoverArtProcessingError(e)
+        except CoverArtProcessingError as e:
+            coverartimage.set_tags_data(initial_data)
+            if config.setting['save_images_to_files']:
+                coverartimage.set_external_file_data(initial_data)
+            raise e
+
+    def run_image_processors(self, coverartimage, initial_data, image_info):
+        if coverartimage.can_be_processed:
+            run_processors = partial(self._run_image_processors, coverartimage, initial_data, image_info)
+            thread.run_task(run_processors, thread_pool=self.threadpool)
+        else:
+            set_data = partial(handle_processing_exceptions, coverartimage.set_tags_data, initial_data)
+            thread.run_task(set_data, thread_pool=self.threadpool)
+
+    def wait_for_processing(self):
+        self.threadpool.waitForDone()

picard/extension_points/cover_art_processors.py

@@ -92,16 +92,16 @@ def run(self, image, target):
 
 
 def get_cover_art_processors():
-    queue_both, queue_tags, queue_file = [], [], []
+    queues = dict.fromkeys(list(ProcessingTarget), [])
     for processor in ext_point_cover_art_processors:
         if processor.same_processing():
-            queue_both.append(processor)
+            queues[ProcessingTarget.BOTH].append(processor)
         else:
             if processor.save_to_tags():
-                queue_tags.append(processor)
+                queues[ProcessingTarget.TAGS].append(processor)
             if processor.save_to_file():
-                queue_file.append(processor)
-    return queue_both, queue_tags, queue_file
+                queues[ProcessingTarget.FILE].append(processor)
+    return queues
 
 
 def register_cover_art_processor(cover_art_processor):

test/test_coverart_processing.py

@@ -29,7 +29,7 @@
 from picard.album import Album
 from picard.const.cover_processing import ResizeModes
 from picard.coverart.image import CoverArtImage
-from picard.coverart.processing import run_image_processors
+from picard.coverart.processing import CoverArtImageProcessing
 from picard.coverart.processing.filters import (
     bigger_previous_image_filter,
     image_types_filter,
@@ -152,7 +152,10 @@ def setUp(self):
     def _check_image_processors(self, size, expected_tags_size, expected_file_size=None):
         coverartimage = CoverArtImage()
         image, info = create_fake_image(size[0], size[1], 'jpg')
-        run_image_processors(coverartimage, image, info)
+        album = Album(None)
+        image_processing = CoverArtImageProcessing(album)
+        image_processing.run_image_processors(coverartimage, image, info)
+        image_processing.threadpool.waitForDone()
         tags_size = (coverartimage.width, coverartimage.height)
         if config.setting['save_images_to_tags']:
             self.assertEqual(tags_size, expected_tags_size)
@@ -300,8 +303,22 @@ def test_format_conversion(self):
             self._check_convert_image('jpeg', format)
         self.set_config_values(self.settings)
 
-    def test_identification_error(self):
-        image, info = create_fake_image(0, 0, 'jpg')
+    def _check_processing_error(self, image, info):
+        self.set_config_values(self.settings)
         coverartimage = CoverArtImage()
-        with self.assertRaises(CoverArtProcessingError):
-            run_image_processors(coverartimage, image, info)
+        album = Album(None)
+        image_processing = CoverArtImageProcessing(album)
+        image_processing.run_image_processors(coverartimage, image, info)
+        image_processing.threadpool.waitForDone()
+        self.assertNotEqual(album.errors, [])
+        for error in album.errors:
+            self.assertIsInstance(error, CoverArtProcessingError)
+
+    def test_identification_error(self):
+        image, info = create_fake_image(0, 0, "jpg")
+        self._check_processing_error(image, info)
+
+    def test_encoding_error(self):
+        image, info = create_fake_image(500, 500, "jpg")
+        info.extension = ".test"
+        self._check_processing_error(image, info)