Repository navigation
Support "readpicture" MPD command #121
Description
This is a followup to #102, the issue requesting albumart command support. Also, see PR #119, which adds albumart command support.
readpicture is an MPD command, similar to albumart, which extracts the embedded picture from a song. Like albumart, it has a binary response format. It is not yet released in MPD, but it is planned to be included in 0.22, and is implemented on MPD master branch. The Music Database documentation page has information on it.
From what I can tell, important differences from albumart are:
readpicturemay return atypeheader which has the MIME type of the picture. It may be useful to expose this via PyMPD2.- "If the song file was recognized, but there is no picture, the response is successful, but is otherwise empty." In other words, there are a few classes of responses: file does not exist (error response), file exists without picture (responds with "OK" but no headers or anything), (presumably) file exists with empty picture (I assume this would respond with a binary blob of size 0), and file exists with properly formed picture.
My primary question for discussion is: how should type be handled in terms of the API surface? Should readpicture simply return a tuple (bytes, str) where either might be None?
Once the API is settled I'm happy to write a PR for this after #119 is merged.
Activity
Mic92 commentedon Jun 18, 2020
So
(None, None)means no picture?Mic92 commentedon Jun 18, 2020
Do you think mpd treats an empty file a valid picture? I don't think it would return in that case.
glen3b commentedon Jun 18, 2020
Yeah, my proposal would be that
(None, None)means that the song exists but has no picture. MPD literally only returns the string "OK" in that case, not an error.For
albumartMPD returns an empty binary blob if cover.jpg exists as an empty file. (I.e. yes, it does return, at least foralbumart). It's not a particularly important edge case, but I think it's logically consistent to return(b'', None)in that case.chrysn commentedon Nov 23, 2020
To keep alignment with other APIs, binary commands could use object parsing like the other commands -- just adjusted for the binary content.
As long as the MPD API follows what seems to be laid out, it could look like this:
The albumart command that is already implemented would need a special parser that returns only the binary here, although it may be a good idea to use this opportunity to revisit and make it return
{'binary': b'...'}again, given that the command's output could still be extended on the MPD side to give a media type there as well.[edit: syntax highlighting]
Mic92 commentedon Nov 23, 2020
I agree with that.
albumarthas not been out for very long so breaking compatibility now is better than later.[do not merge] albumart: Return an object rather than plain binary data
chrysn commentedon Nov 23, 2020
Just to illustrate what this might mean for albumart, here is a commit that does this only for asyncio.
[do not merge] albumart: Return an object rather than plain binary data
albumart / binary: Switch to returning full dictionary
albumart / binary: Switch to returning full dictionary