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

fetchart: Option to download all defined sources and choose the biggest one #3262

Open
msdos opened this issue May 9, 2019 · 7 comments
Open
Assignees
Labels
feature features we would like to implement

Comments

@msdos
Copy link

msdos commented May 9, 2019

Use case

I have a minwidth of 1024 for fetchart plugin. source priority is filesystem.

My cover.jpg image in filesystem is 500x500. Because of that, it tries to get a better picture using the fetchart plugin.

Problem is: the fetched image is 300x300. In this situation, since the fetched image is worse than my filesystem image, I would prefer to use it instead of the downloaded one.

Solution

I would like to have a better approach, but don't know exactly what to do:

  • Create a new config, download_all that tries to get the image from all sources, and chooses the best one? This is a completely new development of the fetchart feature;
  • Create a new config, fallback_worse_fetched_ratio, that uses the filesystem cover.jpg instead of the fetched image if the downloaded resource has a worse ratio.

Alternatives

Today is a manual approach.

@sampsyo
Copy link
Member

sampsyo commented May 9, 2019

Interesting idea! Seems like a reasonable thing to add.

@sampsyo sampsyo added needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." feature features we would like to implement and removed needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." labels May 9, 2019
@sampsyo sampsyo changed the title Use best width image as fallback in fetchart plugin fetchart: Allow low-resolution local images when they are higher-resolution than downloaded images May 9, 2019
@wisp3rwind
Copy link
Member

With respect to not respecting minwidth, that would be an actual bug. However, looking at the code, the only way this could happen appears to missing dependencies: fetchart requires either of Pillow or ImageMagick to be installed (this is rather hidden in the docs). If that is indeed the issue at hand, when running beets in verbose mode, you should see a warning "Could not get size of image...". Could you confirm whether or not these are installed? If so, and if the issue persists, would you mind posting a verbose log of beets replacing such an image?

About the download_all feature request: The infrastructure for such an addition would actually be mostly in place because I considered implementing a related feature in #1917 (which I didn't continue to work on because after all, beets was usually finding sufficiently good artwork without manual intervention).

@wisp3rwind wisp3rwind self-assigned this May 10, 2019
@wisp3rwind wisp3rwind added the needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." label May 10, 2019
@sampsyo
Copy link
Member

sampsyo commented May 10, 2019

Hmm, that’s true. Upon more careful reflection, I’m not sure what the desired behavior would be here—if not minwidth, how should the system decide whether a local image is high enough resolution? Reductively: should a 2x2 image be applied, if it’s the highest-resolution image available?

@sampsyo sampsyo removed the feature features we would like to implement label May 10, 2019
@msdos
Copy link
Author

msdos commented May 10, 2019

If so, and if the issue persists, would you mind posting a verbose log of beets replacing such an image?

@wisp3rwind I think I mixed concepts here since I was importing like 12 albums and refining my yaml configuration at the same time and I'm sorry about that.

The situation that I wrote in the Use Case is actually impossible: if you set minwidth to 1024, the filesystem is 500 and the downloaded is 300, it will simply not download the cover since it doesn't fit the requirements.

This is what happens: it downloads from every source and since the image it's too small, it's discarded:

fetchart: image size: (500, 446)
fetchart: image too small (500 < 1600)
fetchart: image size: (599, 535)
fetchart: image too small (599 < 1600)

What actually happened that made me create this confusion: in fetchart sources, coverart was first. My cover.jpg was 500x500. But since coverart was first source, it downloaded a 300x300 and used it instead of using my cover.jpg. But this makes sense, since I put coverart as first source. No bug: intended behavior.

So my use case was wrong, but the motivation is the same: a download_all option, but now that I think that the filesystem source would be used as well a better name would be use_higher_image_size_from_sources or something like that.

how should the system decide whether a local image is high enough resolution? Reductively: should a 2x2 image be applied, if it’s the highest-resolution image available?

The system just downloads everything and chooses the image with the higher size, using the same logic that was used to discard the small ones like I put here in the log: if someone sets the minwidth option, and even downloading every source it doesn't fit the desired size, it's normally discarded.

Problem: the order in sources is ignored, since the desired behavior now is just do download from all sources and keep the best one; and lots more of bandwidth are used. This should be added to docs as well.

@msdos
Copy link
Author

msdos commented May 11, 2019

@sampsyo would it be the case to change the name of this issue from

fetchart: Allow low-resolution local images when they are higher-resolution than downloaded images

to

fetchart: Option to download all defined sources and choose the biggest one

?

@sampsyo sampsyo changed the title fetchart: Allow low-resolution local images when they are higher-resolution than downloaded images fetchart: Option to download all defined sources and choose the biggest one May 12, 2019
@sampsyo sampsyo added feature features we would like to implement and removed needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." labels May 12, 2019
@sampsyo
Copy link
Member

sampsyo commented May 12, 2019

Yep sounds good!

@wisp3rwind
Copy link
Member

I'm still not sure whether I understand the intended behaviour fully: Are you referring to fetchart's action during and import process, or when run as beet fetchart?

I'd like to propose a somewhat different remedy, maybe you could comment on whether that would fix your usecase? Then I might actually go about implementing this.

Consider the following amended fetchart config:

fetchart:
  minwidth: 1024
  enforce_ratio: yes
  fallback:
    minwidth: 500
    enforce_ratio: 5%

fetchart could interpret this in the following way:

  • In a first run, proceed as before: Query one source after the other, and if a candidate passes all the criteria, apply it directly and do not search further. In addition, store all candidates that didn't make it.
  • If no candidate was good enough, repeat the process (with the cached artwork) using the fallback criteria. One thing to ponder is whether to sort the candidates now (According to what metric?) which would however disregard the sources priority (which IMO matters, since services like coverart are likely to have higher quality artwork than say the google cse). As long as filesystem is in your sources array, this step would automatically pick up the existing image, i.e. implement fallback_worse_fetched_ratio.

An alternative config syntax could be

fetchart:
  minwidth: [1024, 500]
  enforce_ratio: [yes, 5%]

The reasoning behind this (more complicated?) approach is that I'm not convinced that always downloading all of the art is worth the bandwidth (and is also not very nice towards the backend providers I guess). I absolutely do see the need to have more graceful fallbacks though.
In fact, you could "abuse" this to mimic download_all by doing e.g.

fetchart:
  minwidth: 100000
  fallback:
    minwidth: 1024
    enforce_ratio: yes

Opinions? Is this overkill?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature features we would like to implement
Projects
None yet
Development

No branches or pull requests

3 participants