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

Move to MPD #45

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Move to MPD #45

wants to merge 13 commits into from

Conversation

dbrgn
Copy link
Owner

@dbrgn dbrgn commented Nov 11, 2014

I think it would be best if we replace Mplayer with MPD. One of the advantages is that it features a much better IPC API than the hack we're doing currently with the mplayer subprocess. Additionally the configurability is much better. A challenge will be to handle remote control by 3rd party applications, but I guess we can just ignore that feature and declare it as not supported.

I hope to find time to work on this in February.

@darnir
Copy link

darnir commented Jan 28, 2014

I completely back your move here. A switch to MPD would be an excellent choice and may remove a lot of the headaches caused by various Mplayer crashes. MPD is extremely stable and a great choice for this project.

Remote control may be a problem, but that can be fixed at a later date. I'd like to help if possible to this shift. Once we decide on the design specifics, I'll try contributing as much as possible.

@mindstorms6
Copy link
Contributor

Agreed. I am glad to help with a part of this.

@dbrgn
Copy link
Owner Author

dbrgn commented Jan 29, 2014

Great. I'll make some initial drafts in a separate branch in the next few days. Then we can start the discussion.

@dbrgn
Copy link
Owner Author

dbrgn commented Jan 31, 2014

Here's the first design decision: Should we communicate with the mpd directly via TCP, or should we use a client library like https://github.com/Mic92/python-mpd2? Any opinions? :)

Right now I tend to use the TCP based protocol directly, as we won't use most of the features anyways. All we need is to enqueue a song, and standard playback control like pausing, skipping and volume control.

On the other hand, a library would abstract away things like error handling...


Edit: What speaks for python-mpd2 is that it's well tested, actively developed and it does all the connection handling stuff like timeouts etc... Maybe it wouldn't be a bad idea to use it after all.

@dbrgn
Copy link
Owner Author

dbrgn commented Jan 31, 2014

Never mind, I took a quick look at the library and it looks really nice. It saves us all the connection checking, timeout handling etc. So I'll use it.

I'll push a few first drafts to the mpd branch soon.

What do you think about creating a generic interface with abstract methods (load, playpause, stop, volume and terminate) to couple the player more loosely with the rest of the client? On one hand it's a great way to allow future backends to be created, on the other hand it might be a case of YAGNI and overly complex design...

@dbrgn
Copy link
Owner Author

dbrgn commented Feb 1, 2014

Current design idea:

  • MPD client backend that implements the player interface (backends/interface.py).
  • Main connection instance that does playback control.
  • Separate background thread with its own MPD client connection that waits for changes in playback status using the IDLE command (see http://www.musicpd.org/doc/protocol/ch03.html#idp49751888). When a relevant event occurs (e.g. end of song) it sends a signal to the process.

Current problems:

  • The main connection instance looses connection after 1 minute without any action due to a server timeout (see Persistent MPDClient Mic92/python-mpd2#31). Possible workarounds: Wrapper or decorator to reconnect on connection errors. Or using the idle mode or pings to keep the connection alive. I don't know yet which one would work and which one would make more sense.
  • I don't know yet how I'm going to detect the "30 seconds played" event. Probably with a "dumb" timeout, like in the MPlayer implementation.
  • When trying to terminate the MPDPlayer while the status thread is still running, the entire process is blocked until an event happens.

@dbrgn
Copy link
Owner Author

dbrgn commented Feb 2, 2014

Two possibilities to handle the connection timeout:

It doesn't appear that connecting/disconnecting is a big overhead, so I'd suggest using the context manager in the link above. If someone thinks pings are the better solution, let me know.

dbrgn added a commit that referenced this pull request Feb 2, 2014
After this change, each command to MPD results in a connect() and
disconnect() call. This ensures that server connections never time out.

Refs #45.
dbrgn added a commit that referenced this pull request Feb 2, 2014
The ``StatusThread`` now uses ``select.select()`` calls with a timeout
to check whether the MPD ``idle`` command has new information already or
not. This prevents a blocked client if something goes wrong.

Refs #45.
@dbrgn dbrgn added this to the Release v0.2.0 milestone Jun 4, 2014
dbrgn added a commit that referenced this pull request Nov 11, 2014
After this change, each command to MPD results in a connect() and
disconnect() call. This ensures that server connections never time out.

Refs #45.
dbrgn added a commit that referenced this pull request Nov 11, 2014
The ``StatusThread`` now uses ``select.select()`` calls with a timeout
to check whether the MPD ``idle`` command has new information already or
not. This prevents a blocked client if something goes wrong.

Refs #45.
dbrgn added 9 commits May 24, 2015 09:47
- Logging in interesting places
- Status thread with second MPD client instance to check for song
  ending.

There is still a problem with server side timeouts. We'll have to use
pings or the idle command to overcome them. Or maybe create a wrapper
around the MPD client instance that automatically reconnects and
re-runs the command.
After this change, each command to MPD results in a connect() and
disconnect() call. This ensures that server connections never time out.
The ``StatusThread`` now uses ``select.select()`` calls with a timeout
to check whether the MPD ``idle`` command has new information already or
not. This prevents a blocked client if something goes wrong.
@dbrgn
Copy link
Owner Author

dbrgn commented May 24, 2015

I tried to continue with the development today. Unfortunately there's a problem with the signals.

The signal handlers execute code that can cause reentrancy problems. That means the following can happen:

  • "Next track" command is received
  • Connection to mpd is opened
  • In the connection context manager, while the connection is open, the "song ended" signal handler fires and interrupts the context manager
  • In the song end handler, the code to play the next song is called
  • That code is already in the middle of execution though. That means that the already open connection is re-opened (which fails). Also, it can mean that locks are in an undefined state.

The signal handlers should not call any non-reentrant code. For that, we probably need an event loop of some sort.

@dbrgn dbrgn mentioned this pull request Oct 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants