Skip to content

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

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mopidy_mpd/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ def get_config_schema(self):
schema["zeroconf"] = config.String(optional=True)
schema["command_blacklist"] = config.List(optional=True)
schema["default_playlist_scheme"] = config.String()
schema["albumart_chunk_size"] = config.Integer(minimum=1)
return schema

def setup(self, registry):
Expand Down
6 changes: 5 additions & 1 deletion mopidy_mpd/dispatcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,11 @@ def _add_ok_filter(self, request, response, filter_chain):
return response

def _has_error(self, response):
return response and response[-1].startswith("ACK")
return (
response
and type(response[-1]) is str
and response[-1].startswith("ACK")
)

# Filter: call handler

Expand Down
1 change: 1 addition & 0 deletions mopidy_mpd/ext.conf
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ connection_timeout = 60
zeroconf = Mopidy MPD server on $hostname
command_blacklist = listall,listallinfo
default_playlist_scheme = m3u
albumart_chunk_size = 8192
14 changes: 9 additions & 5 deletions mopidy_mpd/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -509,9 +509,8 @@ def decode(self, line):

def join_lines(self, lines):
if not lines:
return ""
line_terminator = self.decode(self.terminator)
return line_terminator.join(lines) + line_terminator
return b""
return self.terminator.join(lines) + self.terminator

def send_lines(self, lines):
"""
Expand All @@ -524,7 +523,12 @@ def send_lines(self, lines):
return

# Remove all control characters (first 32 ASCII characters)
lines = [line.translate(CONTROL_CHARS) for line in lines]
lines = [
self.encode(line.translate(CONTROL_CHARS))
if type(line) is str
else line
for line in lines
]

data = self.join_lines(lines)
self.connection.queue_send(self.encode(data))
self.connection.queue_send(data)
1 change: 1 addition & 0 deletions mopidy_mpd/protocol/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def load_protocol_modules():
:attr:`commands`.
"""
from . import ( # noqa
album_art,
audio_output,
channels,
command_list,
Expand Down
110 changes: 110 additions & 0 deletions mopidy_mpd/protocol/album_art.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
from mopidy_mpd import protocol
from urllib.request import urlopen
from mopidy_mpd.exceptions import MpdNoExistError, MpdArgError


def _get_art_url(self, uri):
images = self.core.library.get_images([uri]).get()
if images[uri]:
largest_image = sorted(
images[uri], key=lambda i: i.width or 0, reverse=True
)[0]
return largest_image.uri


cover_cache = {}


@protocol.commands.add("albumart", offset=protocol.UINT)
def albumart(context, uri, offset):
Copy link
Member

Choose a reason for hiding this comment

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

So how will this differ from what's eventually implemented for readpicture? This seems like it is readpicture. I don't actually understand why MPD has two different commands for this, why would an MPD client care if it was embedded art or from elsewhere? The problem being for us that a sensible "elsewhere" doesn't exist for many of our backends. It looks like readpicture was added later, why readpicture doesn't fallback to use albumart seems odd to me.

However, if the goal is to literally provide album art, then maybe we should be taking the song uri, get the album uri for it and then find artwork for that album instead? That sounds a bit closer to what the MPD docs describe.

Copy link

Choose a reason for hiding this comment

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

IMHO, if there exists embedded audio art, this should be prioritized over url.

"""
*musicpd.org, the music database section:*

``albumart {URI} {OFFSET}``

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

This is currently implemented by searching the directory
the file resides in for a file called cover.png, cover.jpg,
cover.tiff or cover.bmp.

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

.. versionadded:: 0.21
New in MPD protocol version 0.21
"""
global cover_cache

if uri not in cover_cache:
art_url = _get_art_url(context, uri)

if art_url is None:
raise MpdNoExistError("No art file exists")

cover_cache[uri] = urlopen(art_url).read()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@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).

Copy link
Author

@leso-kn leso-kn Feb 18, 2023

Choose a reason for hiding this comment

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

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.


data = cover_cache[uri]

total_size = len(data)
chunk_size = context.dispatcher.config["mpd"]["albumart_chunk_size"]

if offset > total_size:
return MpdArgError("Bad file offset")

size = min(chunk_size, total_size - offset)

if offset + size >= total_size:
cover_cache.pop(uri)
Copy link
Member

Choose a reason for hiding this comment

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

This is OK for now but a possible enhancement would be to have a smarter FIFO style cache as there's a chance the client will ask for this image again.


return b"size: %d\nbinary: %d\n%b" % (
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the response logging messed up for this? Can we return a dict or list here instead, with only the data itself as bytes? Does that simplify stuff elsewhere?

total_size,
size,
data[offset : offset + size],
)


# @protocol.commands.add("readpicture", offset=protocol.UINT) # not yet implemented
def readpicture(context, uri, offset):
"""
*musicpd.org, the music database section:*

``readpicture {URI} {OFFSET}``

Locate a picture for the given song and return a chunk
of the image file at offset OFFSET. This is usually
implemented by reading embedded pictures from
binary tags (e.g. ID3v2's APIC tag).

Returns the following values:

* size: the total file size
* type: the file's MIME type (optional)
* binary: see Binary Responses

If the song file was recognized, but there is no picture,
the response is successful, but is otherwise empty.

Example::

readpicture foo/bar.ogg 0
size: 1024768
type: image/jpeg
binary: 8192
<8192 bytes>
OK

.. versionadded:: 0.21
New in MPD protocol version 0.21
"""
# raise exceptions.MpdNotImplemented # TODO
6 changes: 5 additions & 1 deletion mopidy_mpd/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ def on_line_received(self, line):
logger.debug(
"Response to %s: %s",
self.connection,
formatting.indent(self.decode(self.terminator).join(response)),
formatting.indent(
Copy link
Member

Choose a reason for hiding this comment

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

As per my other comment, the formatting of binary responses seems like it'd be messed up. Is there a test that shows otherwise?

self.decode(self.terminator).join(
[str(line) for line in response]
)
),
)

self.send_lines(response)
Expand Down
22 changes: 13 additions & 9 deletions tests/network/test_lineprotocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,32 +211,36 @@ def test_send_lines_calls_join_lines(self):
self.mock.connection = Mock(spec=network.Connection)
self.mock.join_lines.return_value = "lines"
Copy link
Member

Choose a reason for hiding this comment

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

It'd be more accurate if this now used b"lines"


network.LineProtocol.send_lines(self.mock, ["line 1", "line 2"])
self.mock.join_lines.assert_called_once_with(["line 1", "line 2"])
network.LineProtocol.send_lines(
self.mock, ["line 1".encode(), "line 2".encode()]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.mock, ["line 1".encode(), "line 2".encode()]
self.mock, ["line 1", b"line 2"]

)
self.mock.join_lines.assert_called_once_with(
["line 1".encode(), "line 2".encode()]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
["line 1".encode(), "line 2".encode()]
[b"line 1", b"line 2"]

)

def test_send_line_encodes_joined_lines_with_final_terminator(self):
Copy link
Member

Choose a reason for hiding this comment

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

Should prob rename this to reflect what's actually being tested here.

self.mock.connection = Mock(spec=network.Connection)
self.mock.join_lines.return_value = "lines\n"

network.LineProtocol.send_lines(self.mock, ["line 1", "line 2"])
self.mock.encode.assert_called_once_with("lines\n")
network.LineProtocol.send_lines(self.mock, ["line 1"])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
network.LineProtocol.send_lines(self.mock, ["line 1"])
network.LineProtocol.send_lines(self.mock, ["line 1", b"line 2"])

self.mock.encode.assert_called_once_with("line 1")

def test_send_lines_sends_encoded_string(self):
self.mock.connection = Mock(spec=network.Connection)
self.mock.join_lines.return_value = "lines"
self.mock.join_lines.return_value = sentinel.data
self.mock.encode.return_value = sentinel.data

network.LineProtocol.send_lines(self.mock, ["line 1", "line 2"])
self.mock.connection.queue_send.assert_called_once_with(sentinel.data)

def test_join_lines_returns_empty_string_for_no_lines(self):
assert "" == network.LineProtocol.join_lines(self.mock, [])
assert b"" == network.LineProtocol.join_lines(self.mock, [])

def test_join_lines_returns_joined_lines(self):
self.mock.decode.return_value = "\n"
Copy link
Member

Choose a reason for hiding this comment

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

Remove this line

assert "1\n2\n" == network.LineProtocol.join_lines(
self.mock, ["1", "2"]
result = network.LineProtocol.join_lines(
self.mock, ["1".encode(), "2".encode()]
Comment on lines +240 to +241
Copy link
Member

Choose a reason for hiding this comment

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

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"]

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

Choose a reason for hiding this comment

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

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


def test_decode_calls_decode_on_string(self):
string = Mock()
Expand Down
6 changes: 5 additions & 1 deletion tests/protocol/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@ class BaseTestCase(unittest.TestCase):
def get_config(self):
return {
"core": {"max_tracklist_length": 10000},
"mpd": {"password": None, "default_playlist_scheme": "dummy"},
"mpd": {
"password": None,
"default_playlist_scheme": "dummy",
"albumart_chunk_size": 8192,
},
}

def setUp(self): # noqa: N802
Expand Down
53 changes: 53 additions & 0 deletions tests/protocol/test_albumart.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
from io import BytesIO
from mopidy_mpd.protocol import album_art
from unittest import mock
from mopidy.models import Album, Track, Image

from tests import protocol


def mock_get_images(self, uris):
result = {}
for uri in uris:
result[uri] = [Image(uri="dummy:/albumart.jpg", width=128, height=128)]
return result


class AlbumArtTest(protocol.BaseTestCase):
Copy link
Member

Choose a reason for hiding this comment

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

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

def test_albumart_for_track_without_art(self):
track = Track(
uri="dummy:/à",
name="a nàme",
album=Album(uri="something:àlbum:12345"),
)
self.backend.library.dummy_library = [track]
self.core.tracklist.add(uris=[track.uri]).get()
Copy link
Member

Choose a reason for hiding this comment

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

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


self.core.playback.play().get()

self.send_request("albumart file:///home/test/music.flac 0")
self.assertInResponse("ACK [50@0] {albumart} No art file exists")

@mock.patch.object(
protocol.core.library.LibraryController, "get_images", mock_get_images
)
def test_albumart(self):
track = Track(
uri="dummy:/à",
name="a nàme",
album=Album(uri="something:àlbum:12345"),
)
self.backend.library.dummy_library = [track]
self.core.tracklist.add(uris=[track.uri]).get()

self.core.playback.play().get()

##
expected = b"result"

with mock.patch.object(
album_art, "urlopen", return_value=BytesIO(expected)
):
self.send_request("albumart file:///home/test/music.flac 0")

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

Choose a reason for hiding this comment

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

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?

1 change: 1 addition & 0 deletions tests/test_extension.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,4 @@ def test_get_config_schema():
assert "zeroconf" in schema
assert "command_blacklist" in schema
assert "default_playlist_scheme" in schema
assert "albumart_chunk_size" in schema