mopidy / mopidy-mpd

Mopidy extension for controlling playback from MPD clients
https://mopidy.com/ext/mpd/
Apache License 2.0
98 stars 20 forks source link

Support album artwork ('albumart' command) and binary responses #57

Open leso-kn opened 2 years ago

leso-kn commented 2 years ago

Included in this PR:

Example Screenshot

Example of album art displayed in home assistant

leso-kn commented 2 years ago

Updated Fixed linter warnings and added unit tests to cover protocol.album_art.

leso-kn commented 1 year ago

@djmattyg007 Test coverage is finalized, should now be ready to be merged :)

gvc0 commented 1 year ago

I tested this code an ran into a lot of issues. Problems are in the script album_art.py:

  1. Line 11: largest_image.uri returns a URI without a protocol prefix. Missing 'file://' I guess'. As such line 55 fails to open the URI (ValueError("unknown url type))
  2. Line 11: largest_image.uri returns a URI with a relative path instead of an absolute path. As such line 55 fails to open the URI (No such file or directory)
  3. Line 11: largest_image.uri returns a wrong URI. It returns /local/<_art filename_> while these files are stored under /local/images. As such line 55 fails to open the URI (No such file or directory). Note that /local/<_art filename_> is how the images are referred to in the SQLite database images field, but I guess you need to use a specific function to translates these database values into the appropriate absolute path (retrieved from the config)
  4. Line 49 context.core.playback.get_current_tl_track().get() will throw an error when the albumart function is called when the play queue is empty
  5. Line 49 context.core.playback.get_current_tl_track().get() always returns the image URI of the track that's currently playing, independent from the track requested by the client. When an MPD client tries to download all the album images in its library overview all albums in the library will get the image of the current track.

The good news is that when issues 1-3 are corrected the images are successfully downloaded via the MPD protocol. However, as explained in issue 5 the images are wrong when downloaded from a library list.

gvc0 commented 1 year ago

As a follow up on yesterdays comments I tried to improve the album_art.py code, see commit https://github.com/gvc0/mopidy-mpd/commit/59bb77c6d57c90f661c944e17bc43e996d8cfea3 . It's working fine in Home Assistant (web GUI as well as mobile app). It now also works fine in MAFA, a solid and well maintained Android MPD client which offers more advanced MPD functionality than Home Assistant.

This is my first attempt at developing code for Mopidy so all feedback is welcome. I also would like to have some advice please on how to translate an image uri retrieved from the database to a uri type and absolute file path. For now I hard coded it, see line 69, as I couldn't figure out how to deal with this.

What I need is: /local/e8fb302a1e79c5b6edf72d06ccbd1941-500x500.jpeg >>> file:///media/Data1/Mopidy/Data/local/images/e8fb302a1e79c5b6edf72d06ccbd1941-500x500.jpeg

Note: I also tested some other MPD clients and noticed most use uri's in their albumart requests in a different format than Mopidy uses. If we want to support all these variations we'll need to implement a request uri normalization function. I didn't get to it yet because I didn't find another MPD client yet worth the effort.

gvc0 commented 1 year ago

I finished my work on album_art.py and I think it's ready for a first release. Made a lot of improvements and tested with different clients, all looks fine. I created a pull requests (on branch @leso-kn). As I don't have any experience with Mopidy development and it's release process, please let me know if anything more is required from me.

leso-kn commented 1 year ago

Update: Added @gvc0's changes for file://-support and improved exception handling.

Note: :warning: There currently seems to be an issue with the Github Actions configuration of mopidy-mpd. The code introduced by this PR (test_albumart.py respectively) is not affected by the issue and passes in all configured jobs (accessible when reviewing the pipeline logs). The remaining unit-tests succeed as well, when run in a clean (e.g. container) environment as described in !17.

kingosticks commented 1 year ago

Rebase on master for fixed tests. Sorry about that.

leso-kn commented 1 year ago

No worries, thanks for the quick fix :) Rebase done, all tests pass through Github Actions on my side now! Should run through here now once triggered by a maintainer.

kingosticks commented 1 year ago

I'll try and carve out some time to go through this in the next few days.

kingosticks commented 1 year ago

I also would like to have some advice please on how to translate an image uri retrieved from the database to a uri type and absolute file path. For now I hard coded it, see line 69, as I couldn't figure out how to deal with this.

You want a reverse lookup? This isn't possible. This is/was the blocking problem IIRC. I'll have to look closer.

gvc0 commented 1 year ago

I also would like to have some advice please on how to translate an image uri retrieved from the database to a uri type and absolute file path. For now I hard coded it, see line 69, as I couldn't figure out how to deal with this.

You want a reverse lookup? This isn't possible. This is/was the blocking problem IIRC. I'll have to look closer.

Don't bother, I solved that already months ago (before I commented my code was finished). Nothing hard coded anymore, altough I think you might be able to improve my way of accessing Mopidy config.

leso-kn commented 1 year ago

Update: Coverage raised to 94% (patch coverage). Should pass codecov now.

leso-kn commented 1 year ago

Update

kingosticks commented 1 year ago

Do you want to open a new pull request for leso-kn:feature/album-art ?

kingosticks commented 1 year ago

Oh, you force pushed it here, that's fine. ignore me