mpd: mimick httpd plugin as output #1779
Conversation
|
Thanks for this. The original maintainer of the mpd module is inactive, and I'm not so well into it, so please clarify a bit. Which mpd client(s) are you testing with? Where is the port 8000 standard coming from? |
|
Ah, so I've just been copying exactly what MPD does. The client that needs to see httpd plugin is Stylophone (iOS app). The port 8000 is a thing that comes from the default MPD config: https://github.com/MusicPlayerDaemon/MPD/blob/e380ae90ebb6325d1820b6f34e10bf3474710899/doc/mpdconf.example#L276 |
|
Ok thanks for clarifying. Do you know why the outputid is that particular value? Then please update the comment (ushort max - 1 doesn't explain why). |
|
Well, that value is point of discussion. All I need is a unique value, I suppose. But 0 is already taken, and other devices seem to use some random or hardware address in an unsigned short value (id). So I decided not to take the risk to conflict with any other speaker/output and use a value they can never emit – one that is beyond their datatype range. Looping over all speakers twice just to get the first free id seemed like a bit overkill, that said, if I can change the context of the callback to become a struct in which I can keep the first free ID, I might just emit the httpd plugin after all speakers have been produced as output. That said, what remained a mystery for now is where "Computer" comes from. It has ID 0 and perhaps we can inject the plugin thing just for that entry, instead of making a separate one just for "MP3 steam". What do you think? |
|
I have been away for a while due to vacation, but now I have come back to this. I'm not sure what the right approach regarding the ID would be, but setting it as USHRT_MAX + 1 would seem to conflict with the protocol. As you can see from this the internal ID in OwnTone (which I think is a UINT64) is casted to unsigned short. I expect the reason for that must be that it is required by the protocol? It looks to me like there is also a collision risk in the current implementation coming the shortening of the long ID's. Maybe the author just thought the risk was sufficiently low to not be worth handling. So we could decide to go with that and just set the ID to some number - maybe the port number? |
|
I think to solve the ID problem properly (and yes I think that's necessary) the MPD code just needs to keep its own ID that it increments every time a new output is added. Not sure about the threading here, but a static var with atomic increments could do that trick. |
|
Yes I kind of agree, but that would mean the id's wouldn't be persistent. Maybe that would be a problem for some clients/use cases? Or does mpd work like that? |
|
I'm not that deep into mpd, but I was assuming it's sort of session id's, the client connect and asks for outputs and gets a list identified by ids. So every time a client connects, it should re-ask for the list of outputs. That doesn't mean there aren't clients that cache this. However, it it isn't that outputs get added and removed all the time, is it? So as long as we process the outputs in serial order, their ids will be the same unless the list of outputs changes. Given that that should rarely be the case, and clients being able to deal with refreshing the output lists, it seems rather safe to assume we can generate the list of outputs and producing an ascending list of ids. In the case we have a client caching the ids, the current approach doesn't guarantee persistence, yet will probably work in case of additions better. I really wonder whether we should care about this at all. Unless there is a pressing case for considering frequent updates to outputs, it is really just an isolated problem to clients who cache the outputs lists (which from the protocol's point of view, seems like a really unfavourable thing to do). |
Switching speakers on and off is not uncommon. Also, when the server is restarted the speakers will be added in an unpredictable order. I think the important thing here would be to do whatever mpd does, since the clients are probably tested with that. |
|
fair, will see if I can toy with mpd and see what it does with ids |
after adding a shout stream: So we can safely assume id are session-specific, and not guaranteed to be constant or anything over server restarts |
Dont' think this matters. All outputs are returned with a |
|
Quick check, nice! Do you know how mpd then avoids clients being in an inconsistent state? Is there an event it sends to force clients to reload the output list? |
No, enabled is not the same as on or off. |
|
I did some brief experiments with other clients, but it seems either they think our protocol version is too low to cooperate, or they treat outputs as a very trivial thing being either enabled/disabled. Stylophone has an even odder behaviour where it ignores any output, except the httpd plugin, and considers all of the rest combined as "MPD server". |
|
I've checked the mpd docs, and they say that "ID of the output. May change between executions". I don't understand how that's supposed to work. If the client wants to change the volume of an output, but the ID can change at any time, then how is the client supposed to know which one to use. Seems strange. Maybe it's just the documentation. |
|
Yes, but it basically means that any client that wants to do something via ID, needs to retrieve it first (and then theoretically there's still a possibility of doing the action for an output that wasn't intended, lol) but that makes our lives just so much simpler. |
|
perhaps of interest to this discussion, the idle command can return output: an audio output has been added, removed or modified (e.g. renamed, enabled or disabled) |
|
please let me know if this kind of direction of coding is acceptable |
|
I've tried this with Rigelian (and Stylophone - but like you say it weirdly doesn't even list the speakers), and the app picked up adding/removing speakers just fine. So I think it listen to events via mpd idle. Strangely, Rigelian orders its speaker list by id, so the speakers may change position in the list. Annoying for the user, I think, but we can consider that a client issue. However, I notice that with this change, "MP3 stream" becomes a permanent new speaker in Rigelians list. I expect it might be the same with other clients. Users that don't use the feature won't appreciate that. The only solution for that I can think of is to make this a config option. |
|
Yeah, MPD would enable/disable the httpd plugin based on config. Where would an option to enable/disable this belong? Would switching it on the existence of the "streaming" section in owntone.conf be suitable? |
|
My preference would be the mpd section |
|
how about this? |
There was a problem hiding this comment.
I've also tested with "MaximumMPD". This client seems a bit more primitive, it does catch events with idle, but it does poll a whole lot, so it picked up changes anyway. Have you tested with other clients? I'm not sure what the major mpd clients actually are.
The change you made with the config file looks good to me.
I've gone through the rest and added a few change requests. I wil merge it when they are done.
grobian
force-pushed
the
stream-plugin
branch
2 times, most recently
from
faa41d8
to
9a1f815
Compare
|
|
||
| # Whether to emit an output with plugin type "httpd" to tell clients | ||
| # that a stream is available for local playback. | ||
| # enable_httpd_plugin = false |
There was a problem hiding this comment.
You also have to define the config value in src/conffile.c, otherwise it won't actually work. Make sure to test it also.
There was a problem hiding this comment.
I tested it disabled ... go figure :)
src/mpd.c
Outdated
| @@ -3657,6 +3668,7 @@ mpd_command_enableoutput(struct evbuffer *evbuf, int argc, char **argv, char **e | |||
| } | |||
|
|
|||
| memset(¶m, 0, sizeof(struct output_get_param)); | |||
| param.curid = 0; | |||
There was a problem hiding this comment.
there were 3, I thought I got them all, bleh
Simply casting the speaker ID from a 64-bits int to an unsigned short and hoping that there will be no clashes is just optimistic. Use an ascending number instead which is what MPD does too. The MPD server specifically documents no persistence in these IDs so we can simply enumerate the speakers to meet the requirements. Signed-off-by: Fabian Groffen <[email protected]>
The plugin key is used by some clients to determine whether local playback is possible via HTTP stream, so mimick it. Signed-off-by: Fabian Groffen <[email protected]>
Signed-off-by: Fabian Groffen <[email protected]>
Return MPD-compatible output of plugin type "httpd" when enable_http_plugin in mpd section of the config is set. Signed-off-by: Fabian Groffen <[email protected]>
|
Excellent, merged now. Thanks for the contribution. |
MPD produces the following entry on outputs command:
OK MPD 0.23.5
outputs
outputid: 0
outputname: MP3 stream
plugin: httpd
outputenabled: 1
OK
This indicates to some clients that a HTTP stream is available, and without it they will refuse to do any config/support for it (if at all). While this stream normally would be available on port 8000 that is something that can easily be directed to stream.mp3 using a proxy in front.
Faking this "plugin" simply allows some client to make use of owntone's http stream.