PICARD-1001 Rework local cover art handling

[kepstin]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences: Rework local cover art handling from a cover art provider to a property of files.

Problem

Local cover art as a provider doesn't make sense - local cover art is a property of a file, same as tag cover art. Problems caused by local cover art being treated as a provider include incorrectly indicating that files have been modified, or if an album was loaded without any files associated (for example via a tagger link on musicbrainz.org), no cover art at all.

• JIRA ticket (optional): PICARD-1001

Solution

Move the handling of local cover art to the File class and delete the Local File cover art provider

Action

I don't know how to write Qt UI, so someone familiar with that will have to update the UI for this change.

In the current state, the local_cover_regex setting is re-used, and a new load_local_cover_art boolean is used to enable the feature. The new setting should default to the same as the enabled status of the Local FIle cover art provider used to be.

Some decision has to be made on how to result ordering of loading cover art from tags vs. local files. Currently local file art loading is skipped if cover art was loaded from tags.

[kepstin]

(Pushed a minor updates, this adds the settings back that had disappeared when I deleted the provider ui code, and fixed initialization of the local file cover art object from url)

[kepstin]

Ah, looking into it, this doesn't actually solve the problem.

The issue I am having now is that when a release has a local file loaded, and also gets a cover image from CAA, it shows up as "changed" even if both images are the same image. I'm not sure what the solution to this is.

(in this example, top cover is from CAA provider, bottom one is loaded from local file)

How is this resolved for the case of images saved in tags? As far as I can tell, i'm using the same codepaths here - does that also fail to match?

[kepstin]

Looking at the contents of orig_metadata and metadata in the File.update method, I'm seeing this:

orig_metadata = ImageList([LocalFileCoverArtImage(url='file:///mnt/makoto/home/cwalton/Music/Flac/brainz-flac/ALI PROJECT/わが﨟たし悪の華/cover.jpg', types=['front'], support_types=True, support_multi_types=True)])
metadata = ImageList([CaaCoverArtImage(url='http://coverartarchive.org/release/81b330ce-62ee-4d9a-a2a7-6594204d3131/23095838320-500.jpg', types=['front', 'booklet'], support_types=True, support_multi_types=True, is_front=True)]

So I think what's needed here is a better equality or comparison method on the picard.util.ImageList or maybe picard.coverart.image.CoverArtImage implementation which can figure out that the two images are the same despite being different classes, so a save is not required.

[kepstin]

Interesting… took a look at the __eq__ method on CoverArtImage, and I see that the reason these two images aren't considered to match is that both are flagged as supports_multi_types = True, but the list of types is disjoint.

Either we need to have some special handling for the case of the frontiest image that ignores multiple types, or I can set supports_multi_types = False on the local files loader. (I'm not sure if that even works, anyways? Would require a custom regex with a capture that could match multiple times, and I don't think i saw code to handle that…)

I'll push the supports_multi_types = False change for the time being since it fixes the single cover.jpg case for now.

[phw]

First off, your observations are correct. For the reasons you stated "local cover art" never worked as it should and moving this away from being a cover art provider is definitely right.

But we must be aware that there are differences in behavior with this change. One is that Picard will overwrite the cover art with cover loaded from providers. Before that you could put local cover art provider to be the first one, and it would mean local cover art ist used and other providers not even called. I think we should do the following:

• Have an option to "Do not overwrite existing images", which will keep the images loaded for a file even if different cover art has been loaded.
• Keep the "Local Files" provider, but rename it to "Local Files (deprecated)". That way people relying on this can continue using this for now, and we get some time to see what users need. Alternative to this would be to put it in a plugin instead. But my preference is to keep it in the code for the next release.
• Maybe we should have a settings kigration that nebales load_local_cover_art if the provider is active? But not sure about this.

Regarding the UI I can help out and do the necessary changes this evening. I can also do an actual code review then.

[zas]

This approach doesn't let the user set the order of preference for cover art sources.
I'm not sure the actual problem is the fact it's a "cover art provider".
We are well aware of limitations of cover art stuff (which was, at start, a hack), and I tried to work on this more than once, but the whole thing is very complex, probably much more you think it is.

Before any review, PR needs to pass tests.

Also I disagree with statement "local cover art" never worked as it should, because it was never defined how it should work to start with (or I'm not aware of such document).

That said, it's clear cover art code needs improvements and any patch is welcome.
Can we first define how the local cover art should work and what's broken?

[kepstin]

So, the way I think about local cover art files is that they should be treated equivalently to cover art embedded into tags in a file. Aligning the picard code to match that was the goal of this PR.

I agree that an option to preserve existing cover art would be desirable. Such an option would be applicable both to local cover art files and embedded cover art in tags.

The particular goals of this PR were to:

• Have Picard update the local cover art files using images from configured cover art providers, in the same way as it does for images embedded in tags.
• Have Picard indicate when it is updating local cover art files with images from cover art providers, with working image differences in the cover art panel.
• Have Picard show the album as having no changes to save if the local cover art files match the cover art from the cover art provider.

[phw]

Also I disagree with statement "local cover art" never worked as it should, because it was never defined how it should work to start with (or I'm not aware of such document).

Agreed. The local cover art provider definitely did work for specific use cases. I think the main problem with it is that by design it can only run if files are attached to the release already when it gets loaded. That means it does not work for quite a few cases. But one use case worked quite well: I have the local cover art provider on top of my provider list. That way when I retag files the existing images are used. That means it remembers my choice of cover art, even if when tagging I chose a different cover art than what my primary provider would have given.

So, the way I think about local cover art files is that they should be treated equivalently to cover art embedded into tags in a file. Aligning the picard code to match that was the goal of this PR.

I fully agree with that. I often had the opposite desire, though: Have embedded images work the same as the local cover art provider.

I think we must provide some use cases which users might have with local cover art. Some that come to my mind:

1. Load local cover art for files the same as embedded images, so the user can compare them to the new images which have been loaded by providers.
2. If there is already cover art for the files (local or embedded) keep this and do not use cover art from the providers.
3. Use cover art from one preferred provider (e.g. CAA), but fall back to local files and only if there are no local files load from another fallback provider (e.g. Amazon).
4. Load local cover art files in order to embed them into the files.

I would claim that Picard currently does not support any of the above use cases well, and both this implementation and the current local cover art provider solve different use cases differently well.

Local cover art provider does not solve 1. It can solve 2, 3 and 4, but only under certain circumstances (files are available when a release gets loaded). It cannot solve 2, 3 and 4 for embedded cover art.

This PR does solve 1. It does not solve 2 or 3. It can be used for 4 given that there is no cover art loaded from providers.

If we add an option to prefer local or embedded files and not replace it with provider files (that means "Use original cover art" is the default behavior), and maybe also an option to toggle loading embedded images on and off then it would also solve 2 and 4. Not sure how it could solve 3 (which is based on provider order).

What did I forget?

[phw]

This could be written directly as {t['name'] for t in CAA_TYPES}

[phw]

Can also be done with a set comprehension {x.lower() for x in self._types_split_re.split(string) if x}

[phw]

I think loading the data here makes sense, but this needs fixing a couple of tests that currently fail due to this. The LocalFileCoverArtImageTest currently works without files, should be updated to work on temporary files.

[zas]

You summed it up pretty well, I agree on all 4 cases.

First, we need to define what are local cover art files, they may have a way to determine types or not. Also they may not be at same directory level, I can imagine someone having following structure:

dir/
  file1.flac
  file2.flac
  coverart/
    front.jpg
    back.jpg

Also source image files can be named in infinite number of ways.

dir/
  file1.flac (with embedded track front cover)
  file2.flac (with embedded track front cover)
  track1.jpg
  track2.jpg
  folder.png
  booklet.pdf

We have a regex to match those, but I think it's insufficient. MB doesn't have track cover art support yet.
Local files type identification may be complex (or even impossible), or the type can be in the filename, but with a different language.
All this makes "merging" local cover art with remote cover art quite messy.

So, I think we need a way to "map" local cover art, to "remote" cover art, and decide how to handle multiple images of the same type.

[kepstin]

If we add an option to prefer local or embedded files and not replace it with provider files (that means "Use original cover art" is the default behavior), and maybe also an option to toggle loading embedded images on and off then it would also solve 2 and 4. Not sure how it could solve 3 (which is based on provider order).

What did I forget?

I think I pretty much agree with this - an option to preserve existing cover art would be a necessity before any change like this gets merged. The use case for 3 seems to be fairly niche? And yeah, that would be hard to do.

The particular concern I had was that there's no ability to set the relative priority of cover art from local file vs. embedded cover art right now. Adding a whole second priority list to the UI for this doesn't seem like a good idea, so I'm kind of stumped on that front.

… another possible option might be to have "cover art providers" actually have multiple possible hook points? So when a cover art provider registers itself, it could register itself to run at either the album_metadata_processor stage or the file_post_load_processors stage? (or both, i suppose). This would require some extra work to resolve (probably in the file metadata update function) the relative priorities of cover art from the file vs from album metadata, but it could work.

Any opinions on that?

If this is done, it might make sense to have tag cover art be in the providers list as well to allow setting its relative priority (even tho it's a special case, since I think the implementation for that might need to stay in the specific file format implementations)