bpd: support MPD 0.16 protocol and more clients - #3214
Conversation
|
I've been using this branch myself for some time now and it's mostly working OK. The main issues are because of parts of the MPD protocol we're missing, but I'd prefer to handle those additions separately. The other issue I've seen is that after running fine for some time, clients connected to bpd will sometimes crash. I caught mpDris2 and ncmpcpp crashing at the same time with log output that suggested a malformed response from bpd. This is rare enough that I haven't been able to correlate it with the specific output in bpd's protocol traffic dump. Whenever this has happened I've only needed to restart the clients to get things back to normal, without having to restart bpd. In the course of implementing this PR I encountered issues that went away when I implemented support for apparently unrelated protocol features. I guess the clients aren't robust to unexpected server behaviour since there's normally only one server implementation to test against. I'm hopeful that the issue I mentioned above will solve itself in the same way. So I think I'd like to merge this now and keep working on the unchecked todo items during the release cycle. This should hopefully encourage some other people to test the changes too :) |
When ncmpcpp quits after an error it causes a "connection reset by peer" exception, also known as ECONNRESET (104) in errno terms. In Python 2 this is mapped to a `socket.error` and in Python 3 this is `ConnectionResetError` which is thankfully a subclass of the `socket.error` exception class.
The playlistid command is supposed to list the whole playlist if no argument is provided, but we were accidentally trying to look up an impossible negative id in that case causing an error to always be returned.
The documented type is float, not integer, and clients like mpDris2 send fractional seconds, causing them to crash if these values ar enot accepted.
The real MPD ignores `noidle` when the client is not idle. It doesn't even send a successful response, just ignores the command. Although I don't understand why a client would fail to keep track of its own state, it seems that this is necessary to get ncmpcpp working.
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Great! Everything looks good to me, so let's go for it. I agree that getting more "eyes" on your new features will be the right way to shake out any bugs.
|
Alright, I've opened a bunch of issues to keep track of missing features, and collected them in a new milestone. Let's get this in 🎉 |
Original PR #3214 by arcresu Original: beetbox/beets#3214
Merged from original PR #3214 Original: beetbox/beets#3214
3 participants
Now that
idlehas been implemented, we're almost all the way to MPD 0.16 support. There are a couple of small commands that need adding, but most of what we need is already in place. More importantly, claiming to support the newer protocol version means that more clients will talk to us; fixes #800.nextsongandnextsongidfields instatusoutput.playlistinfo.move.stickers (which are basically flexible attrs).Moved to bpd: support MPD stickers #3290I've expanded my local tests to include ncmpcpp and two Android clients (MPDroid and MALP). So far bpd breaks all three of these clients. Known compatibility issues are listed below.
ncmpcpp (Linux, libmpdclient)
noidlewhen bpd doesn't expect it to be in the idle state.volumecommand to change the volume and we ignore it.ECONNRESETwhen quitting.decoders(fixed in bpd: support decoders command #3222).It notices when playback is paused, but doesn't notice when it's resumed again, even if it was resumed using ncmpcpp itself! Issuing a different command (e.g. changing the volume) will prompt it to refresh the status and update the playback state.This doesn't seem to happen any more. I'm not sure what fixed it.ncmpcpp expects stored playlists to be supported and when bpd sends an error it gets stuck in an infinite loop and the UI freezes.Moved to BPD: Persistent playlist #176.playlistevent and tries to update its playlist usingplchangesit doesn't like the response returned by bpd and ncmpcpp's playlist becomes blank.M.A.L.P. (Android)
MPDroid (Android)
idle "database" "mixer" "options" "output" "player" "playlist" "sticker" "update").mpDris2 (Linux)
statefield in a response to thestatuscommand. This doesn't seem to be possible though. Its backtrace starts withKeyError: 'state'mpdscribble (Linux, libmpdclient)
mpdscribble tries to register a client channel. It doesn't mind that it fails, but it would be nice to support it.Moved to bpd: support client channels #3291mpd error (7): expected more list_OK's. I didn't manage to correlate it to the protocol log in bpd though so I'm not sure what happened.Other clients
beets' own mpdupdate plugin doesn't work with bpd since it expects output from theMoved to bpd: database update could be async #3292updatecommand, but we never send any.