search

mopidy / mopidy-mpd

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

Support MPD albumart and readpicture command #17

Open adnidor opened 6 years ago

adnidor commented 6 years ago

Add support to the MPD frontend for the albumart command allowing clients to receive album art directly from mopidy.

Defined here: https://www.musicpd.org/doc/protocol/database.html

jjok commented 4 years ago

Does anyone know how the versioning works with MPD? Currently we declare version 0.19.0. 

#: The MPD protocol version is 0.19.0.
VERSION = "0.19.0"

It looks like, from this client, albumart was added in 0.21.0. If we want to support album art, are we meant to, in theory, add everything else from that version? Will it cause problems is we just add one command?

Should clients use the protocol version to find out if a server supports a command, or should they be using the commands command?

jodal commented 4 years ago

If I wrote a client, I'd use commands for feature detection. After 10 years of Mopidy, I believe most MPD client developers are aware of both MPD and Mopidy and hopefully test with both.

That way, we can add new commands one by one, and they'll be useful at least in some clients. To bump the declared version up, I think that we at least should make "empty shells" of any new commands that we don't support, so that the commands are correctly parsed but calling them doesn't change anything.

kingosticks commented 4 years ago

By "empty shells" do you mean they would feature in the commands output so they appear supported, but then don't actually do anything useful? How would clients then correctly detect that we don't actually support something? e.g. https://github.com/mopidy/mopidy-mpd/issues/19

jjok commented 4 years ago

There are a few places where we currently a raise NotImplemented exception at the moment.

jjok commented 4 years ago

Here's the relevant documentation for albumart.

albumart {URI} {OFFSET}

Locate album art for the given song and return a chunk of an album art image file at offset OFFSET.

Returns the file size and actual number of bytes read at the requested offset, followed by the chunk requested as raw bytes (see Binary Responses), then a newline and the completion code.

Example:

albumart foo/bar.ogg 0
size: 1024768
binary: 8192
<8192 bytes>
OK
jodal commented 4 years ago

By "empty shells" do you mean they would feature in the commands output so they appear supported, but then don't actually do anything useful? How would clients then correctly detect that we don't actually support something? e.g. #19

We should probably not include those in commands. There has been a very long time since I did any new MPD protocol implementation, so there might be better strategies than what I pull out of my head right now. I guess the most important thing is how clients handle commands missing from commands and/or commands not being implemented.

BarrettLowe commented 4 years ago

I'd like to have this feature also. I would think a NotImplemented would be okay - not great obviously - but I would assume that not all clients would hit the errors. Guess we should figure out what changed from 0.19 and now...

jjok commented 4 years ago

I had a go at implementing this recently, but I couldn't get it to work. We need to be able to allow bytes() in the response message lines, as well as regular strings. I'll see if I can share some code.

leso-kn commented 2 years ago

Hello! I have implemented the albumart command in #57. This does not include the readpicture command and thus works on with media source providing an album art url (e.g. dlna, jellyfin, etc.).

gvc0 commented 1 year ago

I tested leso-kn's implementation and did some updates which are waiting for review and commit. I verified the new feature with:

Can't test IOS unfortunately.

LevitatingBusinessMan commented 1 year ago

@gvc0 Just want to say thanks for all the activity from you. Contributors like you keep Mopidy alive.

leso-kn commented 1 year ago

@gvc0 Merged your changes into into leso-kn/mopidy-mpd (see #57). Sorry for the delay, last few months have been quite fluctuative.

Note: :warning: There currently seems to be an issue with the Github Actions pipeline of mopidy-mpd. The pipeline does however pass test_albumart.py and albumart.py, therefor it appears to be an unrelated issue. I am not able to reproduce it locally (in a containerized (clean) local environment, all unit-tests pass and flake8 is satisfied, see steps to do that below).

 # Steps to clean-run unit-tests and generate coverage.xml  (Python 3.9.16)
 > docker run --rm -ti -v $PWD:/pro --workdir /pro python:3.9-alpine ash -c "set -e
 apk add gcc gstreamer gst-plugins-bad gobject-introspection-dev cairo-dev musl-dev
 pip install tox PyGObject
 python -m tox -e py39 -- --cov-report=xml"

I would like to repeat that this feature is ready for merging! Thanks to @gvc0 it now additionally supports artwork of local tracks through mopidy-local (optional dependency and their changes are covered by the test_albumart.py unit-test).

tr1xem commented 4 days ago

Wrote Something if this helps

https://github.com/tr1xem/ncmpcpp-with-cover-art

Can add file thumnails if someone still uses that

Open for suggestions