CoverArt Post Processing #347
Conversation
Check all the images in the metadata of track. Remove Image form the metadata if dimension less than minimum. Resize the image if larger than maximum dimension. Requires Pillow library to be installed.
|
|
||
| def ignore_image(img_data): | ||
| """Ignore The image file if the dimensions are smaller than a predefined""" | ||
| (width, height, _, _, _) = imageinfo.identify(img_data) |
There was a problem hiding this comment.
Picard is using _() for gettext translations, so it isn't recommended to use _ for unused variables,.
| from picard.plugin import PluginPriority | ||
|
|
||
| MAX_DIMENSION = 600 # Set maximum allowable dimensions of image in pixels | ||
| MIN_DIMENSION = 400 # Set minimum allowable dimensions of image in pixels |
There was a problem hiding this comment.
Those should be made user-configurable as plugin config options.
There was a problem hiding this comment.
Should a window open every time the user opens the Picard application?
There was a problem hiding this comment.
No. It should be added to the config settings pages. See the code in https://github.com/metabrainz/picard-plugins/tree/2.0/plugins/format_performer_tags for an example of how this can be done.
| ) | ||
| metadata.images.pop(ids) | ||
| else: | ||
| img_data_edited = resize_image(image_data) |
There was a problem hiding this comment.
I guess some levels of caching would avoid extra processing.
This approach is rather simplistic, do the plugin resize multiple times the same image?
|
|
||
| def resize_image(image_data, max_size=MAX_DIMENSION): | ||
| """Resize the image to max_size and center crop if larger than max_size.""" | ||
| with Image.open(BytesIO(image_data)) as img: |
There was a problem hiding this comment.
I would be extra careful about potential exceptions here, we do not want the plugin to crash the whole app in cased of malformed images.
Also I wonder about performance impact (and threading).
| PLUGIN_LICENSE = "GPL-2.0" | ||
| PLUGIN_LICENSE_URL = "https://www.gnu.org/licenses/gpl-2.0.html" | ||
|
|
||
| from PIL import Image |
There was a problem hiding this comment.
Does the PIL library need to be installed separately? If so, how will this be handled on systems (e.g. Windows) where Picard has been installed using a binary installer and the system does not have Python installed?
There was a problem hiding this comment.
Should I package the PIL library along with the plugin?
There was a problem hiding this comment.
You need to handle several use cases, where the location of the PIL library may vary and so the import statement may need to vary:
-
Picard executable downloading the plugin - so you need either to include PIL or add code to download it.
-
Picard running from source - PIL not already installed - as 1.
-
Picard running from source + Python install already incudes PIL - you will need to decide which PIL to call.
| return False | ||
|
|
||
| def resize_image(image_data, max_size=MAX_DIMENSION): | ||
| """Resize the image to max_size and center crop if larger than max_size.""" |
There was a problem hiding this comment.
Perhaps a user-configurable option could be to determine whether the image should be center cropped or filled (with a specific color) to provide a square image?
| left = (new_width - max_size) / 2 | ||
| top = (new_height - max_size) / 2 | ||
| right = left + max_size | ||
| bottom = top + max_size | ||
| img = img.crop((left, top, right, bottom)) |
There was a problem hiding this comment.
Perhaps first check if new_width or new_height is greater than max_size and skip this processing if not?
| output_buffer = BytesIO() | ||
| img.save(output_buffer, format='JPEG') | ||
| return output_buffer.getvalue() | ||
|
|
There was a problem hiding this comment.
Shouldn't the original image be returned if it is within the specified size parameters? Otherwise there could be a problem in Lines 64-65.
|
|
||
|
|
||
| coverart_processing = CoverArtProcessor() | ||
| register_track_metadata_processor(coverart_processing.process_images, priority=PluginPriority.HIGH) |
There was a problem hiding this comment.
I think you need to provide a comment again explaining why you run this at high priority. Also, the previous comment that was removed in the latest commit did not make much sense to me.
| if self.ignore_image(image.data): | ||
| metadata.images.pop(image_index) | ||
| self.cache[image_hash] = None | ||
| log.debug(f"{PLUGIN_NAME}: Ignoring Image {image_index+1} because it is smaller than the minimum allowed dimensions.") |
There was a problem hiding this comment.
I am wondering whether this should be a log.warning rather than a log,.debug?
There was a problem hiding this comment.
I used log.debug() for this message because I wanted to keep track of when images are being ignored due to their size. However, I agree that it might be better to use log.warning() instead to make it more noticeable in the logs.
|
|
||
|
|
||
| coverart_processing = CoverArtProcessor() | ||
| register_track_metadata_processor(coverart_processing.process_images, priority=PluginPriority.HIGH) |
There was a problem hiding this comment.
I am unclear why this is a track metadata processor rather than an album/release metadata processor. Cover art is AFAIK associated with an album in MB, so surely you only need to process it once per release? Or have I misunderstood something?
There was a problem hiding this comment.
No, this is correct.
But when I was trying it as an album processor, I saw the metadata.images list is empty. Is there some other attribute where the images list is stored?
There was a problem hiding this comment.
That sounds like it may be a bug in Picard - IMO the cover art images should be associated with the album not the tracks and so should be available to an album metadata processor.
There was a problem hiding this comment.
Upon further thought, I am not sure that my original comment is true. It is true to the extent that new cover art is downloaded by album, and that existing file cover art is also (generally or always? how is existing cover art handled when you have multiple media stored in subdirectories) common to the album, however cover art already embedded in the audio files can (obviously) be unique to a track.
So if this plugin is supposed to work on existing embedded cover art, then I guess it needs to be a track metadata processor after all.
There was a problem hiding this comment.
@Pranay-Pandey is correct here. But this also highlights one of the limitations of Picard's cover art processing and why in the end the GSoC proposal needs to be considered (where it is proposed to have explicit cover art processing plugin hooks).
When a release is loaded the cover art is loaded by the cover art processors. Internally loading cover art is invoked as an asynchroneous album metadata processor. As you cannot chain async album metadata processors this functionality currently cannot be implemented as an album metadata processor. Track metadata processors however run after all album metadata processors have finished. Hence this plugin can do its work.
|
Can I suggest that you add some unit tests to check each aspect of the plugin's intended functionality. (There is a GitHub workflow to run tests.) In particular, I think that having tests to download the plugin and then to check that PIL is downloaded and accessible would be beneficial. |
|
@Pranay-Pandey Sorry for the long delay from my side here, I had been mostly unavailable the last few weeks. Thanks a lot for this plugin. Overall this looks good to me except for the comments already done above. Given the current limitations of Picard's cover art handling this plugin works quite well. I just wonder if it is a good approach to merge this now. Maybe we should rather see this plugin as a testbed for actually implementing this functionality inside Picard properly Your GSoC proposal is much more detailed and would allow a better implementation anyway. Essentially we need to run the image processing directly after images have been actually loaded, and using a track metadata processor is kind of a workaround. Also it runs for each track, which is wasteful. |
|
If this is going ahead as a GSoC project in main Picard, then I would support Philipp's reasons that we avoid merging this. |
|
Thank you for your feedback and for considering my proposal. I completely understand your decision to not merge the pull request at this time, and I am looking forward to working on the project during GSoC. |
|
@Pranay-Pandey are you on #metabrainz? as who? |
As pranay_pandey. Here is my profile - https://community.metabrainz.org/u/pranay_pandey |
I think @zas was asking if you're connected to the #metabrainz channel on IRC, which is where much of the developer communications happens. |
|
Thanks for telling me about the #metabrainz channel on IRC. I joined using this name and am eager to contribute to the project and connect with the community. |
Under which nick are you on #metabrainz ? During the GSoC you have to be around so you can discuss with the Picard team. |
My nickname is pranay |
Check all the images in the metadata of track. Remove Image form the metadata if dimension less than minimum threshold. Resize the image if larger than maximum dimension. Requires Pillow library to be installed.