Song queue – patch

Submitted by mmilata on Mon, 2009-07-06 20:10

Forums:

Dear MOC users,

the feature I missed most in moc player was queueing, and I think I am not the only one who would like moc to have this feature. So I decided to try to implement it and here is the patch. Unfortunately, it can't be considered fully working because I ran into a few problems regarding the queue behaviour. So I post it here for you to try and tell me what do you think about it and how do you think it should work:)

Download:

Usage:
By default, use z to add file to queue, z again to remove the file from queue, Z to clear the queue. These keys can be changed in keymap under keywords enqueue_file, respectively clear_queue. You can enqueue any file, either from playlist or directory listing.

Behaviour:
The queue is local to current playlist. If you clear playlist (on server), the queue is also cleared; if you send different playlist from another client, it is sent with its own queue. This isn't as noticeable if you have SyncPlaylist option se to yes, which I guess is usually the case. If the local queue is non-empty, number of songs in it is displayed between status line and volume bar.

The queue doesn't work when you play a directory (from directory view), not playlist. I'm not sure how it should behave in this case, because when you start playing a directory it is sent as a playlist to the server, but not maintained in any way on the client so it's not possible to have a queue for it (without major modifications to source code). And even if it had it's own queue, if you switched between directory view and playlist and started playing playlist, the queue would be discarded.

Other approach might be to make the queue global -- that it wouldn't depend on the playlist and all clients would share the queue even with SyncPlaylist turned off. This would personally suit me best, but it goes against mocp's client-server conception a bit. What do you think?

Please note that it is not possible to have two instances of the same file in the queue (as in any playlist in mocp).

I didn't test the patch much with SyncPlaylist turned off so it's possible I overlooked some major illogicality when using queue in this mode.

In final version, I would also like to be able to display queue contents (in a way e.g. themes menu is displayed) and be able to reorder/delete the items. Maybe also display the order of the files in queue in "winamp style", i.e. show number next to playlist entry indicating on what position is that file in the queue.

Compilation & testing tips:
Make sure to kill your regular mocp server before starting the patched client. Running patched client against normal server will probably result in crash/freeze of one of them.

It might be a good idea to use different config directory than you normally use. The -C option will accomplish this. If you want to use your regular config directory, there may be tag database libdb version mismatch. In this case, just delete or rename the cache file.

You will want to install the patched mocp in /usr/local, or if your regular mocp is already there, in something like ~/.local/. Pass this directory to --prefix= parameter of ./configure (/usr/local is the default). To uninstall, cd to the directory with source code and do make uninstall.

If you hit some bug, please include logs and/or backtrace. See http://moc.daper.net/node/96

Thanks for any suggestions/bugreports/comments, with your help this may become usable & maybe even integrated into mainline mocp:)

Martin Milata

daper

Wed, 2009-07-22 19:20

Nice patch :) What do you think about not creating another playlist, but add a boolean property to plist_item called "queue" or "delete_after_play" so that the item is automatically deleted from the playlist at the time it's played when this flag is set to true? Wouldn't it simplify the code?

mmilata

Tue, 2009-08-11 11:14

Thanks:) It would certainly simplify the code, but I think it would be worse from the usability viewpoint. I would like this feature to be similar to winamp (or eg. audacious) queue as I find it quite intuitive to work with (though I'll try to be open to other suggestions). Or is the patch too complex to be eventually considered for inclusion into mocp?

mmilata

Mon, 2009-08-17 19:09

Hi again,

here is the second version of the patch:

Changes:

  • The queue is now global. All clients synchronize against the server.
  • Option QueueNextSongReturn determines what file to play after playing files from the queue has been finished. Value 1 means return to the song played before playing the queue and play the song after it. Value 0 means continue with song which is in the playlist after the last song in the queue (and if the last file is not in the playlist, return to the file as described with value 1). 0 is the default.
  • Numbers indicating the position of the playlist/dirlist entries in the queue are displayed (similarly to winamp/audacious).

Problems:

  • Updating the queue position numbers in playlist/dirlist may be too slow when there are a lot of files both in the queue and in the playlist.
  • When mocp is "Reading tags ..." (e.g. on exit) and you try to enqueue a file, mocp freezes (not sure).

Questions:

  • I don't have much experience with multithreaded programming, but some things in audio.c/server.c seem weird to me. For example function server.c:state_change() which is called from several places in audio.c, from play thread. This function calls add_event_all() which accesses clients[] array. This array is also accessed from several functions in server.c which are called from server thread. Even though the clients' event queues are protected by mutexes, the array itself is not. Isn't it possible for some race condition to happen this way?
  • Is command line switch for enqueuing file worth adding?

I consider this version to be much better than the previous and after solving the problems and a bit of testing ready to be considered for inclusion into svn (if there aren't other problems). Anyway, any feedback is welcome:)

Thanks,
MM

mmilata

Sat, 2009-08-22 14:46

Regarding problem #2:

This bug is not part of my patch:) and can be (with a bit of luck) reproduced by starting moc, loading lots of files in the playlist, pressing Q and before the client exists, starting a second client and adding some files to the playlist.

Because fill_tags() ignores the event queue and only reads the events from the socket and because calling server_event() may result to adding some incoming events to the event queue, the loop doesn't decrement files variable to 0 and the client is therefore stuck.

This patch should fix it (hopefully it doesn't introduce more bugs than it fixes).

<br /> Index: moc/interface.c<br /> ===================================================================<br /> --- moc.orig/interface.c 2009-08-22 15:38:28.000000000 +0200<br /> +++ moc/interface.c 2009-08-22 15:50:39.000000000 +0200<br /> @@ -1065,9 +1065,23 @@</p> <p> /* Process events until we have all tags. */<br /> while (files &amp;&amp; !user_wants_interrupt()) {<br /> - int type = get_int_from_srv ();<br /> - void *data = get_event_data (type);<br /> -<br /> + int type;<br /> + void *data;<br /> +<br /> + /* Event queue is not initialized if there is<br /> + * no interface */<br /> + if (!no_iface &amp;&amp; !event_queue_empty (&amp;events)) {<br /> + struct event e = *event_get_first (&amp;events);<br /> +<br /> + type = e.type;<br /> + data = e.data;<br /> + event_pop (&amp;events);<br /> + }<br /> + else {<br /> + type = get_int_from_srv ();<br /> + data = get_event_data (type);<br /> + }<br /> +<br /> if (type == EV_FILE_TAGS) {<br /> struct tag_ev_response *ev<br /> = (struct tag_ev_response *)data;<br />

I don't know if the patch is worth applying, because the problem is quite improbable to encounter, but since I spent last night figuring it out, at least I post it here ...

daper

Sun, 2009-08-23 13:50

I'm not sure if it completely fixes the issue, but patch applied, thanks.

mmilata

Sat, 2009-08-22 18:38

Here's the third version. Updating queue position numbers is faster in some cases and the fill_tags() fix is included.

daper

Sun, 2009-08-23 14:23

Thanks for you hard work! I'd like to include your patches but need your full name to put it in the changelog.

I must admit that the queue works great - the user interface is very nice, I really like that items on the playlist are marked with numbers meaning their positions and there is also number of files in the queue displayed at the bottom, great!

Although I still think it would simplify the code to have a "queued" flag for playlist items instead of another playlist but since I have no time to implement it myself your solution is acceptable. Most important: it works really nice :)

mmilata

Mon, 2009-08-24 13:05

I'm very glad you like it;) The name is Martin Milata. I don't think I'll be able to check this forum very often so if there is some problem with the code my email is xmilata at fi.muni dot cz.

daper

Mon, 2009-08-24 13:35

Thanks, your patch is included i nSVN.

Queuing files from command line would be a very nice feature.

mmilata

Wed, 2009-08-26 15:46

Ok, here is the patch:)

  1. This patch makes it possible to pass relative path to "-l" option.
  2. This patch adds option for queueing from command line.
  3. This patch removes some dead code i forgot in interface.c:queue_toggle_file(), allows to enqueue a stream and removes a lock which i think was unnecessary (since no playlist synchronization is taking place).

Is the choice of the option letter (-q) ok?

Regarding implementing queue as a playlist item flag: I don't know -- since you cannot have two instances of the same file in playlist it wouldn't be possible to "temporarily reorder" the sequence of files, which i sometimes find useful. Maybe if it was implemented as a linked list of playlist items (using playlist indices instead of pointers) ...

daper

Wed, 2009-09-09 07:52

Patches applied, thanks. -q is a great choice :)

Biga

Mon, 2009-11-09 16:41

How do I search and queue song? The problem is that when I use search, I can only play the found song (by Enter), any other thing won't work in the search mode.

I guess that this issue isn't related to queue patch, more likely to search functionality itself. One can't find a song and then queue it or jump to it without playing.

Any ideas?