Skip to content
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

Support new “albumart” command #102

Open
coderkun opened this issue Nov 20, 2018 · 7 comments
Open

Support new “albumart” command #102

coderkun opened this issue Nov 20, 2018 · 7 comments

Comments

@coderkun
Copy link

Is the new “albumart” command (see The music database) already supported or are there any plans to do so?

@Mic92
Copy link
Owner

Mic92 commented Nov 21, 2018

You can make a pull request.

@auchter
Copy link
Contributor

auchter commented Apr 18, 2020

See #111 for a possible implementation

@glen3b
Copy link
Contributor

glen3b commented Jun 10, 2020

On a related note, are there plans to support the readpicture command, another get-picture command with a binary response format? It seems like it would be a fairly straightforward addition to #111.

@Mic92
Copy link
Owner

Mic92 commented Jun 12, 2020

If someone makes a PR with tests.

@thouters
Copy link

thouters commented Oct 5, 2020

Hi, I see the albumart command is included, since this ticket is still open I guess something must be missing. How do I use this command? Are there already any real-world usecases?
I tried calling it like
File "examples/asyncio_example.py", line 32, in main print(client.albumart("/ji")) File "/usr/local/lib/python3.7/dist-packages/python_mpd2-1.1.0-py3.7.egg/mpd/base.py", line 361, in albumart AttributeError: 'MPDClient' object has no attribute '_execute_binary'

@thouters
Copy link

Hi, I'm working on a PR implementing both albumart and readpicture by adding binary payload support on a more fundamental level than is currently done with _execute_binary(). Since I don't see a way to integrate _execute_binary() with the asyncio classes.
The unittests for this are thorough, kudos! (it will take me some time to get my (albeit working in blue-sky) code to pass them).

Even though I'm not finished, I'd like to open the discussion on the data returned by the existing non-asyncio albumart call.
The albumart command now returns the binary data straight away. Since the readpicture command also has a mime-type keyvalue pair, which should be exposed, I think it makes sense to change the return value of albumart to a dict as well.
This is what my work-in-progress is returning:
albumart: [{'size': '1925', 'binary': bytearray(b'\xff....')}]
readpicture [{'size': '1925'}, {'type': 'image/jpeg', 'binary': bytearray(b'\xff\xd8.....')}]
Are there any objections to this?

@glen3b
Copy link
Contributor

glen3b commented Oct 16, 2020

Hi, I'm working on a PR implementing both albumart and readpicture by adding binary payload support on a more fundamental level than is currently done with _execute_binary(). Since I don't see a way to integrate _execute_binary() with the asyncio classes.
The unittests for this are thorough, kudos! (it will take me some time to get my (albeit working in blue-sky) code to pass them).

Even though I'm not finished, I'd like to open the discussion on the data returned by the existing non-asyncio albumart call.
The albumart command now returns the binary data straight away. Since the readpicture command also has a mime-type keyvalue pair, which should be exposed, I think it makes sense to change the return value of albumart to a dict as well.
This is what my work-in-progress is returning:
albumart: [{'size': '1925', 'binary': bytearray(b'\xff....')}]
readpicture [{'size': '1925'}, {'type': 'image/jpeg', 'binary': bytearray(b'\xff\xd8.....')}]
Are there any objections to this?

Isn't size redundant with the bytestream, since Python strings track length? That also seems like a rather large structural overhead (IMO) for the amount of data which is actually being returned, and many of the other calls in this library have a much more direct return.

That said I do agree we need a better way to handle binary returns with arbitrary header sequences. As far as readpicture is concerned I had some thoughts a while back in #121 about how to handle the API signature; I don't know what @Mic92's current thoughts are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants