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

leso-kn · 2022-08-12T16:50:36Z

Included in this PR:

Added support for Binary Responses
Added an implementation of the albumart command, making it possible to fetch album artwork for any media source that provides an artwork url
Support Binary Responses in network unit-tests

Example of album art displayed in home assistant

mopidy_mpd/dispatcher.py

mopidy_mpd/network.py

… command

leso-kn · 2022-08-19T16:08:06Z

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

leso-kn · 2022-09-24T20:47:12Z

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

gvc0 · 2022-12-20T00:47:04Z

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

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))
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)
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)
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
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 · 2022-12-21T17:27:33Z

As a follow up on yesterdays comments I tried to improve the album_art.py code, see commit gvc0@59bb77c . 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 · 2022-12-23T15:32:17Z

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 · 2023-02-15T00:03:36Z

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

Note: ⚠️ 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 · 2023-02-16T00:16:23Z

Rebase on master for fixed tests. Sorry about that.

leso-kn · 2023-02-16T09:20:47Z

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 · 2023-02-16T09:59:48Z

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

kingosticks · 2023-02-16T10:05:09Z

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 · 2023-02-16T11:51:40Z

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.

mopidy_mpd/protocol/album_art.py

leso-kn · 2023-02-16T13:23:46Z

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

leso-kn · 2023-02-16T15:16:25Z

Update

Removed mopidy-local-integration for the moment as suggested in discussion 57#110856.
Coverage raised to 100% (patch coverage).

mopidy_mpd/protocol/album_art.py

kingosticks · 2023-02-17T13:15:11Z

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

kingosticks · 2023-02-17T13:15:52Z

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

mopidy_mpd/protocol/album_art.py

kingosticks · 2023-02-17T13:21:23Z

mopidy_mpd/protocol/album_art.py

+        if art_url is None:
+            return b"binary: 0\n"
+
+        cover_cache[uri] = urlopen(art_url).read()


Needs error handling. And does it need to support a timeout, retries, proxy config. Lots to handle here.

A core API makes more and more sense here. This is a whole world of pain to add to Mopidy-MPD and it's really the job of each backend to provide data. Would a get_images_data() API be useful to other backends, I don't know (but it's not a requirement, we can do it similarly to get_distinct). Can mpris provide image data (https://gitlab.freedesktop.org/mpris/mpris-spec/-/issues/17)? It might also be useful in the case where the backend can't provide publicly accessible images.

@jodal @adamcik do you have any thoughts about what to here?

New core api get_image_data(uris) with an implementation for downloading over HTTP.

As above but as the default implementation for a matching backend extension API, allows backends to provide custom implementation I.e. Mopidy-Local would provide images directly

above functionality part of Mopidy-MPD, anything weird or more complicated is not supported.

One additional remark about my part of the code that was not accepted. I had foreseen a maximum (preferred) image size for images retrieved from -Local. The current code uses the largest images available (line 8), which is very inefficient (slow) for MPD clients that prefetch the images for all local albums. Ideally max preferred image size would be a config setting, in -MPD and/or -Local.

@kingosticks I see. When I took recourse to urlopen in the initial implementation I indeed didn't quite consider that mopidy-mpd in general utilitizes calls to the core API for fetching data.

I believe your suggestion 2 sounds like the most clean way forward (new core api get_image_data(uris) / HTTP download per default with an option for custom implementations by backends which require so).

I am currently implementing your other feedback – thanks for the through code-review :) – I'll leave this part of the code as is for the moment, as it's likely that things will move to the core (I did however implement ACK_ERROR_NO_EXIST as suggested by your other above review).

A quick note aswell: I foreseeably won't be available during the upcoming week. I'll try to upload adjustments in response to your feedback before leaving as far as possible, leaving this very part out for now as described above.

mopidy_mpd/protocol/album_art.py

kingosticks · 2023-02-18T00:15:41Z

tests/network/test_lineprotocol.py

+        result = network.LineProtocol.join_lines(
+            self.mock, ["1".encode(), "2".encode()]


Suggested change

result = network.LineProtocol.join_lines(

self.mock, ["1".encode(), "2".encode()]

assert b"1\n2\n" == network.LineProtocol.join_lines(

self.mock, [b"1", b"2"]

kingosticks · 2023-02-18T00:16:07Z

tests/network/test_lineprotocol.py

        )
+        assert "1\n2\n".encode() == result


Suggested change

assert "1\n2\n".encode() == result

kingosticks · 2023-02-18T00:18:14Z

tests/protocol/test_albumart.py

+            album=Album(uri="something:àlbum:12345"),
+        )
+        self.backend.library.dummy_library = [track]
+        self.core.tracklist.add(uris=[track.uri]).get()


Why do we need to add to the tracklist and play the track in these tests?

kingosticks · 2023-02-18T00:25:18Z

tests/protocol/test_albumart.py

+    return result
+
+
+class AlbumArtTest(protocol.BaseTestCase):


Needs tests for the image width sorting, caching, and offset. And all the many ways urlopen can fail.

kingosticks · 2023-02-18T00:42:21Z

tests/protocol/test_albumart.py

+        ):
+            self.send_request("albumart file:///home/test/music.flac 0")
+
+        self.assertInResponse("binary: " + str(len(expected)))


Suggested change

self.assertInResponse("binary: " + str(len(expected)))

self.assertInResponse("size: " + str(len(expected)))

self.assertInResponse("binary: " + str(len(expected)))

self.assertInResponse("OK")

Unfortunately, I think self.assertInResponse("result") is also going to be true. Which is wrong.... Maybe we need to fix the MockConnection so we work entirely in bytes when testing commands that provide binary responses?

leso-kn mentioned this pull request Aug 12, 2022

Support MPD albumart and readpicture command #17

Open

djmattyg007 reviewed Aug 14, 2022

View reviewed changes

mopidy_mpd/dispatcher.py Outdated Show resolved Hide resolved

mopidy_mpd/network.py Outdated Show resolved Hide resolved

leso-kn force-pushed the feature/album-art branch from 20c0ee0 to 8d64840 Compare August 17, 2022 20:04

Add support for binary responses and album artwork via the 'albumart'…

cf238f0

… command

leso-kn force-pushed the feature/album-art branch from 8d64840 to 10a74a1 Compare August 18, 2022 19:05

Adjust test_lineprotocol() for binary responses

97e0ff2

leso-kn force-pushed the feature/album-art branch from 50c5edc to 42bd5ac Compare August 19, 2022 14:36

Add unit tests to cover protocol.album_art

7d7292f

leso-kn force-pushed the feature/album-art branch from 42bd5ac to 21ce51f Compare August 19, 2022 16:05

leso-kn force-pushed the feature/album-art branch from 21ce51f to be4776c Compare September 24, 2022 20:43

kingosticks reviewed Feb 16, 2023

View reviewed changes

mopidy_mpd/protocol/album_art.py Outdated Show resolved Hide resolved

leso-kn force-pushed the feature/album-art branch 3 times, most recently from c85fa74 to a0e10f9 Compare February 16, 2023 15:01

kingosticks requested changes Feb 16, 2023

View reviewed changes

mopidy_mpd/protocol/album_art.py Outdated Show resolved Hide resolved

mopidy_mpd/protocol/album_art.py Outdated Show resolved Hide resolved

leso-kn force-pushed the feature/album-art branch 2 times, most recently from daad059 to 27e73d8 Compare February 16, 2023 20:36

kingosticks reviewed Feb 17, 2023

View reviewed changes

mopidy_mpd/protocol/album_art.py Outdated Show resolved Hide resolved

kingosticks requested changes Feb 18, 2023

View reviewed changes

leso-kn added 2 commits February 18, 2023 21:39

Add config setting 'albumart_chunk_size'

12a7572

Merge branch 'master' into feature/album-art

12ca0fa

leso-kn force-pushed the feature/album-art branch from 27e73d8 to 12ca0fa Compare February 18, 2023 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

leso-kn commented Aug 12, 2022 •

edited

leso-kn commented Aug 19, 2022

leso-kn commented Sep 24, 2022

gvc0 commented Dec 20, 2022 •

edited

gvc0 commented Dec 21, 2022 •

edited

gvc0 commented Dec 23, 2022 •

edited

leso-kn commented Feb 15, 2023 •

edited

kingosticks commented Feb 16, 2023 •

edited

leso-kn commented Feb 16, 2023

kingosticks commented Feb 16, 2023

kingosticks commented Feb 16, 2023

gvc0 commented Feb 16, 2023 •

edited

leso-kn commented Feb 16, 2023

leso-kn commented Feb 16, 2023

kingosticks commented Feb 17, 2023

kingosticks commented Feb 17, 2023

kingosticks Feb 17, 2023

kingosticks Feb 18, 2023

kingosticks Feb 18, 2023

gvc0 Feb 18, 2023

leso-kn Feb 18, 2023

leso-kn Feb 18, 2023 •

edited

kingosticks Feb 18, 2023

kingosticks Feb 18, 2023

kingosticks Feb 18, 2023

kingosticks Feb 18, 2023

kingosticks Feb 18, 2023

		result = network.LineProtocol.join_lines(
		self.mock, ["1".encode(), "2".encode()]

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

Are you sure you want to change the base?

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

Conversation

leso-kn commented Aug 12, 2022 • edited

leso-kn commented Aug 19, 2022

leso-kn commented Sep 24, 2022

gvc0 commented Dec 20, 2022 • edited

gvc0 commented Dec 21, 2022 • edited

gvc0 commented Dec 23, 2022 • edited

leso-kn commented Feb 15, 2023 • edited

kingosticks commented Feb 16, 2023 • edited

leso-kn commented Feb 16, 2023

kingosticks commented Feb 16, 2023

kingosticks commented Feb 16, 2023

gvc0 commented Feb 16, 2023 • edited

leso-kn commented Feb 16, 2023

leso-kn commented Feb 16, 2023

kingosticks commented Feb 17, 2023

kingosticks commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leso-kn Feb 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leso-kn commented Aug 12, 2022 •

edited

gvc0 commented Dec 20, 2022 •

edited

gvc0 commented Dec 21, 2022 •

edited

gvc0 commented Dec 23, 2022 •

edited

leso-kn commented Feb 15, 2023 •

edited

kingosticks commented Feb 16, 2023 •

edited

gvc0 commented Feb 16, 2023 •

edited

leso-kn Feb 18, 2023 •

edited