diff --git a/picard/coverart/__init__.py b/picard/coverart/__init__.py index 88f3eea1f0..1141389047 100644 --- a/picard/coverart/__init__.py +++ b/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,6 +120,9 @@ 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 @@ -146,6 +130,7 @@ def next_in_queue(self): 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 diff --git a/picard/coverart/processing/__init__.py b/picard/coverart/processing/__init__.py index c663dcc249..05a0132ef3 100644 --- a/picard/coverart/processing/__init__.py +++ b/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() diff --git a/picard/extension_points/cover_art_processors.py b/picard/extension_points/cover_art_processors.py index 8989a0f024..d929300b28 100644 --- a/picard/extension_points/cover_art_processors.py +++ b/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): diff --git a/test/test_coverart_processing.py b/test/test_coverart_processing.py index 36c0c5d786..46358d1fa1 100644 --- a/test/test_coverart_processing.py +++ b/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)