search

mopidy / mopidy-mpd

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

[WIP] Album Art and Binary Responses #49

Closed CMurtagh-LGTM closed 2 years ago

CMurtagh-LGTM commented 2 years ago

resolves #17.

Adds the albumart, readpicture and binartlimit commands, and the ability to respond with binary data.

I need help working out how to get the location of local images with the information given to a command.

Also needs tests.

kingosticks commented 2 years ago

I've not looked at this PR properly yet but have you seen https://docs.mopidy.com/en/latest/api/ext/#mopidy.ext.Extension.get_cache_dir ?

CMurtagh-LGTM commented 2 years ago

I've not looked at this PR properly yet but have you seen https://docs.mopidy.com/en/latest/api/ext/#mopidy.ext.Extension.get_cache_dir ?

For this I would need to explicitly import from mopidy-local and somehow gain access to the config object.

kingosticks commented 2 years ago

Before you go any further with the cache, what's the aim? To avoid redownloading the image for each of the MPD client's chunked requests? If so, does it need to be on disk or is it better in memory and better as part of the connection context? I'm wary of caching anything in a frontend. Especially for long periods of time.

kingosticks commented 2 years ago

And there should be nothing backed specific in a frontend e.g. the mopidy-local stuff doesn't belong in here. But I'll have a proper look later.

kingosticks commented 2 years ago

Hm re the import, ok definitely not. I must have misunderstood what you were doing here.

CMurtagh-LGTM commented 2 years ago

Before you go any further with the cache, what's the aim? To avoid redownloading the image for each of the MPD client's chunked requests? If so, does it need to be on disk or is it better in memory and better as part of the connection context? I'm wary of caching anything in a frontend. Especially for long periods of time.

Currently it only saves the latest image in memory, it replaces this each time a new image is requested.

And there should be nothing backed specific in a frontend e.g. the mopidy-local stuff doesn't belong in here. But I'll have a proper look later.

It would be nice if mopidy.core had a function that asked the backend for the actual image. I can't think of any other way that would not be backend specific.

kingosticks commented 2 years ago

Ok so yes, I misunderstood why you were trying to find the cache directory, I thought you were saving stuff there rather than trying to work out where the local backend was keeping things. I blame trying to read it on a mobile.

Doesn't Mopidy-Local's get_images() provide an HTTP URI to the image which can be downloaded, just like any other backend? Why do we need to treat it differently? Am I remembering this wrong?! (I don't have it installed anywhere right now).

It would be nice if mopidy.core had a function that asked the backend for the actual image. I can't think of any other way that would not be backend specific.

For nearly all backends it would have to download the image, just as you are doing here. Except core doesn't have any MPD context and so it'd be harder to work out what was actually in use (when caching, which it would still need to do). And this core method seems specific to Mopidy-MPD, I don't think this would be generally useful.

Currently it only saves the latest image in memory, it replaces this each time a new image is requested.

This should be per-context. Imagine two different clients (or one client with two active connections) requesting different images at the same time .

girst commented 2 years ago

two thoughts:

from urllib.request import Request, urlopen
with urlopen(Request(image_uri, headers={'range': f'bytes={offset}-{offset+context.binary_limit-1}'})) as r:
    bytes = r.read(context.binary_limit)
    # length = r.headers.get('content-length')
kingosticks commented 2 years ago
kingosticks commented 2 years ago

Doesn't Mopidy-Local's get_images() provide an HTTP URI to the image which can be downloaded, just like any other backend? Why do we need to treat it differently? Am I remembering this wrong?! (I don't have it installed anywhere right now).

Yes, this is how it works. It provides relative URIs. i.e. /local/0c24d58571fdbe999ad1809ed65d8bc7-75x65.jpeg and we can GET these. However, unless I am missing something, Mopidy-MPD doesn't know the server's root URI so it is stuck. We could fix Mopidy-Local to output absolute image URIs.

But then none of this works if Mopidy-HTTP is not enabled. Which is ugly. And solved by loading the images directly from the disk, like you were doing. Although we an't assume we know where those files are. Or that Mopidy-Local hasnt changed and moved them.

girst commented 2 years ago

On Fri, Jan 14, 2022 at 10:30:14AM -0800, Nick Steel wrote:

However, unless I am missing something, Mopidy-MPD doesn't know the server's root URI so it is stuck.

a simple hack would be to just connect to http://localhost:. this should sort out ipv4/ipv6 automatically, as well. the only i can see this fail was if the user bound mopidy-http to a specific (non-loopback) interface, which i'd assume is very unlikely.

I think the only sensible thing to do there is to fix Mopidy-Local to use absolute image URIs. Thoughts?

does mopidy-local have a way to get the bind address? i assume not, otherwise mopidy-mpd could use the same technique to get it itself.

i don't think we can extract a useful hostname+port combination from tornado either; when the user binds to 0.0.0.0, we can't connect to that. so we're back to the hack above.

99% of the time the client is going to want all parts. I would imagine multiple small requests are going to be slower overall but removing caching sounds nice.

i'd assume that most album cover images are way less than the 1MB default binarylimit anyways, so the overhead from multiple requests hopefully shouldn't matter 99% of the time.

kingosticks commented 2 years ago

I was hoping to avoid guessing what the hostname was.

does mopidy-local have a way to get the bind address?

I think I have a way but it's too hacky to even consider.

I'm almost coming round to the idea of core.library.get_image_raw()...

djmattyg007 commented 2 years ago

do we need to bring in a new and large dependency (requests)? IMO, urllib.request from the stdlib would do plenty for the use case here.

What's wrong with a dependency on requests? The Mopidy core already has a hard dependency on it. Everything else already has a transitive dependency on it.