Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CoverArt Post Processing #347

Open
wants to merge 4 commits into
base: 2.0
Choose a base branch
from
Open
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions plugins/coverart_processing/coverart_processing.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
PLUGIN_NAME = "CoverArt Processing"
PLUGIN_AUTHOR = "Pranay"
PLUGIN_DESCRIPTION = """
Post Processing Features for the CoverArt Images.
Ignore images smaller than specified width and height.
Resize the image if larger than specified dimensions.
"""
PLUGIN_VERSION = '0.1'
PLUGIN_API_VERSIONS = ['2.2']
PLUGIN_LICENSE = "GPL-2.0"
PLUGIN_LICENSE_URL = "https://www.gnu.org/licenses/gpl-2.0.html"

from PIL import Image
Copy link
Contributor

@rdswift rdswift Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@Pranay-Pandey Pranay-Pandey Apr 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I package the PIL library along with the plugin?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Picard executable downloading the plugin - so you need either to include PIL or add code to download it.

  2. Picard running from source - PIL not already installed - as 1.

  3. Picard running from source + Python install already incudes PIL - you will need to decide which PIL to call.

from io import BytesIO
from picard.util import imageinfo
from picard import log
from picard.metadata import register_track_metadata_processor
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those should be made user-configurable as plugin config options.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a window open every time the user opens the Picard application?

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


class CoverArtProcessor:
def __init__(self):
self.max_dimension = MAX_DIMENSION
self.min_dimension = MIN_DIMENSION

self.cache = {}
log.debug(f"{PLUGIN_NAME}: Initialized with max_dimension={self.max_dimension} and min_dimension={self.min_dimension}.")

def ignore_image(self, img_data):
"""Ignore The image file if the dimensions are smaller than predefined"""
(width, height) = imageinfo.identify(img_data)[:2]
return width < self.min_dimension or height < self.max_dimension

def resize_image(self, image_data):
"""Resize the image to max_size if larger than max_size."""
with Image.open(BytesIO(image_data)) as img:
width, height = img.size

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if width > self.max_dimension or height > self.max_dimension:
# Calculate the new size while maintaining aspect ratio
aspect_ratio = width / height
if width > height:
new_width = self.max_dimension
new_height = int(new_width / aspect_ratio)
else:
new_height = self.max_dimension
new_width = int(new_height * aspect_ratio)

img = img.resize((new_width, new_height), Image.LANCZOS)

with BytesIO() as output:
img.save(output, format="JPEG", quality=85)
return output.getvalue()

# If image dimensions are already within the max limit, original image is to be used.
return image_data

def process_images(self, album, metadata, track, release):
"""Process the Cover Art Images"""
for image_index, image in enumerate(metadata.images):

image_hash = image.url.toString()
if image_hash in self.cache:
if self.cache[image_hash] is None:
metadata.images.pop(image_index)
else:
metadata.images[image_index].set_data(self.cache[image_hash])

else:
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.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering whether this should be a log.warning rather than a log,.debug?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
new_image_data = self.resize_image(image.data)
metadata.images[image_index].set_data(new_image_data)
self.cache[image_hash] = new_image_data
log.debug(f"{PLUGIN_NAME}: Resizing Image {image_index+1}.")


coverart_processing = CoverArtProcessor()
register_track_metadata_processor(coverart_processing.process_images, priority=PluginPriority.HIGH)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.