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 for playerctl module #179

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Apeiros-46B
Copy link

Summary

This PR

  1. Replaces the usage of os.tmpname() with a URL-based path in the playerctl module and prevents album art from being downloaded multiple times, and
    • The path is determined based on the following logic: all occurences of https:// and http:// are first removed from the URL, then /tmp/bling_album_art .. trimmed_url is used as the path
  2. Adds new params for save_image_async_curl function in helpers/filesystem.lua (required for 1.):
    • boolean redownload: if this is false and the file already exists, curl isn't called
    • boolean create_dirs: if this is true the --create-dirs flag is added to the curl call

This means that

  1. Disk space is saved by not unnecessarily re-downloading album art
  2. Already downloaded album art will work when you don't have an internet connection
  3. For people with slow internet speeds, getting the album art should be faster if it isn't the first time

Notes

Haven't tested if this works when passing some other values for redownload and create_dirs to the save image function, but it theoretically should still work just fine

@Apeiros-46B
Copy link
Author

Another thing I wanted to point out: on some systems the /tmp dir is cleared on boot; maybe it would be better to store album art in ~/.cache instead? Or make the dir configurable

@nawuko
Copy link

nawuko commented Jul 19, 2022

Consider that some clients cache there own artwork and generate a temporary filename, with this implementation every time you play a youtube video (even if its the same video!) for example from firefox a new file is created, which, with this solution, will never be deleted. I think a task to clean old files is outside of the scope, but this new behavior should be considered with this change

Edit: Since those files are local (they begin with a file:// uri) maybe they could be filtered and set directly cause there is no need to run curl for them anyways. (And according to freedesktop spec this should be always the case - "URIs should be sent as (UTF-8) strings. Local files should use the "file://" schema.")

Example metadata from firefox:

{
  ["mpris:artUrl"] = "file:///home/nawuko/.mozilla/firefox/firefox-mpris/4688_6.png",
  ["xesam:album"] = "",
  ["xesam:artist"] = { "Practical Engineering" },
  ["xesam:title"] = "What Happens When a Reservoir Goes Dry?"
}

@Nooo37
Copy link
Member

Nooo37 commented Jul 26, 2022

Don't know much about the playerctl module. @Kasper24 is the contact person for that. Nawuko seems to have valid concerns too

@Nooo37 Nooo37 removed their request for review July 26, 2022 10:05
@Apeiros-46B
Copy link
Author

Apeiros-46B commented Jul 27, 2022

Would checking if the art url starts with file:// and then use that path instead of downloading it work for this? Or are there also other clients that don't report the file path correctly

@javacafe01
Copy link
Member

@Kasper24

@Kasper24
Copy link
Contributor

Kasper24 commented Jan 29, 2023

@javacafe01 The issue with storing it in cache is it won't get deleted, that's why I saved it to tmp instead. It also uses a blocking function to query if the file exists or not, so that would need to be converted to async.
Another issue I saw mentioned in the discord is that when no network connection is available, trying to download the artwork will cause the metadata signal to not be emitted, so it might be worth to split them into 2 signals ("metadata" and "artwork") while we're at it

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.

None yet

5 participants