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

Cache album art #495

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Cache album art #495

merged 2 commits into from
Jul 11, 2024

Conversation

jacksongoode
Copy link
Collaborator

Currently, this stores the album art work by hashed URI.

Ideally, we would be storing and retrieving the album art by the album ID. However, that would mean modifying the function in a way that I think would warrant its own PR. With that extension, we would only need to download album artwork for each album rather than album art for each track, which is happening now.

I think the increase in cache size is negligible here, given that each album cover as a small thumbnail is about 2-6kb and has a large thumbnail about ~40kb.

This will reduce the number of requests, improve responsiveness and CPU likely.

@Insprill
Copy link
Collaborator

Insprill commented Jul 9, 2024

I think this cache could add up quickly for people with large libraries. It might be worth bringing in the image crate to store the images in a more efficient format. It might be a little complex, but having a limit on size would probably be good too.

@jacksongoode
Copy link
Collaborator Author

jacksongoode commented Jul 9, 2024

@Insprill Hmm, right now having loaded 2,200 small thumbnails, it only totals around 10mb but I would definitely be curious about how we could compress them further. Do you have any ideas? I suppose we could group them into blocks and then compress the blocks and save the index? There's not much compression gain on such small images individually.

I agree about the cache management limit. So that seems like it would be a separate PR and would go about managing the audio cache differently than the image cache. There's also work I'd like to do in caching the actual data structures returned by fetching users, playlists, users albums, users liked songs. All of that seems like it should be done first before we do the cache management.

@jacksongoode
Copy link
Collaborator Author

I think additional improvements can be made in a subsequent PR? What do you think @Insprill? I'd rather merge this, see if it actually does pile up in some noticeable way and then make changes after.

Copy link
Collaborator

@Insprill Insprill left a comment

Choose a reason for hiding this comment

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

LGTM. 10mb isn't bad, I agree we can tweak later.

@Insprill Insprill merged commit 23bd55e into master Jul 11, 2024
8 checks passed
@Insprill Insprill deleted the jackson/cache-album-art branch July 11, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants