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

bpd: extend coverage of MPD protocol #3200

Merged
merged 23 commits into from Apr 2, 2019

Conversation

Projects
None yet
2 participants
@arcresu
Copy link
Member

commented Mar 31, 2019

Adds support for some of the "easy" MPD protocol features bpd is missing, as part of #800. I'll do more complicated features like the idle command separately, and unfortunately we need idle to be able to increment the protocol version string.

The MPD protocol specification document is not very precise and is intended to be read together with the MPD source code and an actual MPD server. Some of the clients I've seen tend to ignore the MPD version in the client hello message (or maybe at most issue a warning if it looks too old) and attempt to use the commands they want to use anyway, whereas others abort on an old version. For those reasons I think incremental updating of the protocol support should be OK, as long as we don't bump the protocol version string past a version of the protocol that we can actually support.

This adds/changes:

  • new consume mode, which erases songs from the playlist as they are played,
  • new single mode, which stops playback when the current song finishes,
  • repeat mode now repeats the entire playlist rather than a single track unless used in conjunction with single mode,
  • issuing previous when on track 0 now restarts track 0 instead of stopping bpd,
  • status gained some new fields, like the higher-precision times elapsed: and duration:,
  • improved reliability by sending protocol errors instead of crashing as before,
  • dummy support (i.e. the server accepts the commands but doesn't actually do anything) for crossfade, mixramp*, replay_gain_*, volume, and stored playlist commands (listplaylists, save, etc.).

@arcresu arcresu force-pushed the arcresu:bpd2 branch from e8f030a to cb17deb Apr 1, 2019

arcresu added some commits Mar 30, 2019

bpd: fix crossfade command
Although crossfade is not implemented in bpd, we can store the setting
and repeat is back to clients. Also log a warning that the operation is
not implemented.

The real MPD doesn't show the crossfade in status if it's zero since
that means no crossfade, so now we don't either.
bpd: error instead of crashing on extra argument
If an MPC client is expecting a command to take an argument that bpd
isn't expecting (e.g. because of a difference in protocol versions) then
bpd currently crashes completely. Instead, do what the real MPD does and
return an error message over the protocol.
bpd: add mixramp commands
These are a more sophisticated version of crossfade so we're free to
ignore them, at least for now. We now track the values of the two
settings, and show them in the status output. Like MPD, we suppress the
mixrampdb value if it's set to nan, which is how it signals that the
feature should be turned off.
bpd: add dummy command for volume
MPD supports a deprecated command 'volume' which was used to change the
volume by a relative amount unlike its replacement 'setvol' which uses
an absolute amount. As far as I can tell 'volume' always responds with a system
error message "No mixer".
bpd: add replay_gain_* commands
There's a special status command for checking the replay gain mode,
which can be set to one of a short list of possible values. For now at
least we can ignore this feature, but track the setting anyway.

@arcresu arcresu force-pushed the arcresu:bpd2 branch from cb17deb to 2b4f9de Apr 1, 2019

arcresu added some commits Mar 30, 2019

bpd: no-op support for persistent playlists
The real MPD offers persistent playlist manipulation, storing the
playlists in a directory set in the config file. If that directory is
not available then the feature is disabled and the relevant commands all
respond with errors. Based on this, the initial support in bpd just
returns errors matching the MPD server in the disabled mode.

For playlistadd, extend the _bpd_add helper to work with playlists other
than the queue in order to support testing the real implementations of
these commands in the future.
bpd: support the single command
This command instructs bpd to stop playing when the current song
finishes. In the MPD 0.20 protocol this flag gains a value 'oneshot' but
for now we just support its older version with a boolean value.

@arcresu arcresu force-pushed the arcresu:bpd2 branch from 2b4f9de to 45935ba Apr 1, 2019

@arcresu arcresu force-pushed the arcresu:bpd2 branch from 45935ba to b245c0e Apr 1, 2019

arcresu added some commits Apr 1, 2019

bpd: fix repeat mode behaviour
The repeat flag indicates that the entire playlist should be repeated.
If both the repeat and single flags are set then this triggers the old
behaviour of looping over a single track.
bpd: fix bug in bounds check of current song index
The songs are indexed starting from zero for the play command, however
the bound check was off by one. An index matching the length of the
playlist would crash the server instead of responding with an error
message over the protocol.
bpd: skipping backwards through zero keeps playing
Previously issuing the 'previous' command when at position 0 on the
playlist would cause bpd to stop playing. MPD instead just restarts the
currently playing song instead, so we now match this behaviour.
bpd: fix repeat, consume and single in reverse
These flags are all relevant to the 'previous' command as well as the
'next' command.

@arcresu arcresu marked this pull request as ready for review Apr 1, 2019

@arcresu

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

Ok I think this might be enough for the first batch of bpd changes before this PR gets too long. The main feature in this PR that benefits current users right now is that bpd will crash less often.

After merging this bpd will be in a sort of hybrid state in terms of protocol support since since e.g. repeat is now using a more modern interpretation than our protocol version string indicates, which could potentially confuse some clients. However even then I don't think it would make bpd unusable. We really need idle to be able to start incrementing the protocol version, but that's a more complicated command since it requires implementing an event system and pushing messages to the client. I'll do that in another MR but just wanted to start with some easy ones.

The stored playlist features could be implemented for real easily enough, but since they're optional in MPD anyway I'm starting off with placeholders. Advanced playback features like crossfade and replaygain could potentially have real implementations instead of the current placeholders if we pick up an optional dependency on a more sophisticated audio player backend library. We can think about that after catching up with MPD though, if there's even a demand for it.

I've been testing locally with mpc and comparing behaviour with a real MPD version 0.21.4 server. Other clients like ncmpcpp refuse to talk to bpd since the protocol version is too old, so it's hard to do more robust real world testing until this effort is a little further along.

@sampsyo
Copy link
Member

left a comment

Looks awesome! I agree that it is totally fine to start with partial support for the new features—this seems really unlikely to confuse any clients. We can increment the version number, as you say, once we cover all the major features for that version.

I have just a few minor comments inline.

Show resolved Hide resolved beetsplug/bpd/__init__.py Outdated
Show resolved Hide resolved beetsplug/bpd/__init__.py Outdated

arcresu added some commits Apr 1, 2019

bpd: improve exception handling
Check function signature instead of using TypeError to crudely guess
that the wrong number of arguments were provided.

Prevent bpd from crashing when trying to log a traceback. The
`traceback.format_exc` function takes an optional argument which is
supposed to be an integer restricting the length of the backtrace to
show. Instead we were passing the exception object to this function and
causing a new exception to be raised.
@arcresu

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

I've run into an issue with beets.util.inspect in that the Python 3 compatibility shim isn't actually compatible with the deprecated Python 2 function in the case of bound methods.

In Python 2 the wrapper is using:

Python 2.7.16 (default, Mar  4 2019, 15:29:09) 
[GCC 8.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import inspect
>>> class A:
...   def cmd_a(self, a, b, c): pass
... 
>>> an_a = A()
>>> inspect.getargspec(an_a.cmd_a)
ArgSpec(args=['self', 'a', 'b', 'c'], varargs=None, keywords=None, defaults=None)

Whereas in Python 3 it's using:

Python 3.7.3 (default, Mar 26 2019, 07:25:18) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import inspect
>>> class A:
...   def cmd_a(self, a, b, c): pass
... 
>>> an_a = A(); inspect.signature(an_a.cmd_a)
<Signature (a, b, c)>
>>> sig = inspect.signature(an_a.cmd_a)
>>> sig.parameters
mappingproxy(OrderedDict([('a', <Parameter "a">), ('b', <Parameter "b">), ('c', <Parameter "c">)]))
>>> inspect.getfullargspec(an_a.cmd_a)
FullArgSpec(args=['self', 'a', 'b', 'c'], varargs=None, varkw=None, defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={})

That is, in the shim the self parameter is missing from ArgSpec.args unlike the original, meaning we get different argument counts depending on the Python version. The docs tell us that inspect.getfullargspec is mostly a drop-in replacement for the deprecated function, and indeed replacing the shim with that seems not to cause any issues.

Edit: inspect.getfullargspec is un-deprecated now according to the docs, so I think it's safe to use. With that function the beets.util.inspect wrapper becomes a two-liner and since it's only used in two places I think we can safely drop it.

Fix beets.util.inspect for Python 3
Under the original compatibility shim we weren't correctly inclusing
`self` in the argument list for bound methods.

@arcresu arcresu force-pushed the arcresu:bpd2 branch from d60110e to 36c85a8 Apr 1, 2019

arcresu added some commits Apr 1, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Thank you for looking into the inspect deprecation! I'm really happy the stdlib authors un-deprecated that call. I like this so much better without the wrapper than with it. 🎉

@sampsyo

sampsyo approved these changes Apr 2, 2019

Copy link
Member

left a comment

This looks fantastic! Would you mind adding a short changelog about the new protocol features that BPD supports so far?

Show resolved Hide resolved beetsplug/bpd/__init__.py
Show resolved Hide resolved beetsplug/bpd/__init__.py
@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Wonderful! Thanks again for your attentiveness here!

@sampsyo sampsyo merged commit a1e26bd into beetbox:master Apr 2, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@arcresu arcresu deleted the arcresu:bpd2 branch Apr 6, 2019

@arcresu arcresu added this to the Modern MPD support milestone Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.