savonet / liquidsoap

Liquidsoap is a statically typed scripting general-purpose language with dedicated operators and backend for all thing media, streaming, file generation, automation, HTTP backend and more.
http://liquidsoap.info
GNU General Public License v2.0
1.4k stars 130 forks source link

Make playlist not pick the same file twice in a row (check_next) #197

Closed giflw closed 9 years ago

giflw commented 9 years ago

Playlist in random could not pick the same file twice in a row. This behavior could be default or setted by parameter. This is really interesting when the size of playlist is small, increasing the changes of repetition.

dbaelde commented 9 years ago

You seem to be looking for randomize instead of random. Am I right?

giflw commented 9 years ago

Hello

I hope make myself clear enough. Sorry for my english.

Lets suppose I have a script with this configuration:

songs=playlist(mode="random", "/path/to/songs")
jingles=playlist(mode="???", "/path/to/jingles")

radio=s = rotate(weights=[2,3], [jingles, songs])

So, I will have this flow:

┌►  (2 jingles), (3 songs)  ─┐
└────────────────────────────┘

Using normal mode:

┌►┌►  load playlist → play each file once and in order  ─┐ ────────────────────┐
│ └──────────────────────────────────────────────────────┘                     │
└─ reload playlist via telnet or socket with more or less files, even 0 files ─┘

Using randomize mode:

┌►┌►  load playlist → shuffle playlist → play each file once and in shuffled order  ─┐ ──┐
│ └──────────────────────────────────────────────────────────────────────────────────┘   │
└─ reload playlist via telnet or socket with more or less files, even 0 files ───────────┘

Using random mode:

┌►  load playlist → ┌►  choose 1 file → play choosen file once  ─┐ ────────────┐
│                   └────────────────────────────────────────────┘             │
└─ reload playlist via telnet or socket with more or less files, even 0 files ─┘

From those options above, I can have those scenarios:

  1. I have a large playlist: any mode will be acceptable, because the probability of each file to be played can be approximated by 1/n, where n is the number of files on playlist.
  2. I have a small playlist, even with 2 files: now, random and randomize modes still have 1/n probability to pick each file, but if I have 2 files, the probability is 50%. In this case, no matter which mode I use (random or randomize), a file have a huge probability to be played twice in a row.

For case 2, would be interesting to have liquidsoap testing if the choosen file to played is the same last played, and skip to the next or move it to the end of playlist.

So, the base flow will be:

def isTheSameFile()
    IF playlist has more than 1 file AND choosen file is NOT the same last played?
        return [NO]
    ELSE
        return [YES]
end

For random mode:

┌►  load playlist → ┌►  choose 1 file → isTheSameFile()? [NO] → play choosen file once  ─┐ ────────┐
│                   ├───────────────────── [YES] ──────┘                                 │         │
│                   └────────────────────────────────────────────────────────────────────┘         │
└─ reload playlist via telnet or socket with more or less files, even 0 files ─────────────────────┘

For randomize mode:

┌►┌►  load playlist → shuffle playlist → isTheSameFile()? [NO] → play each file once and in shuffled order  ─┐ ──┐
│ │                                      └─[YES] ─► move first to last ↗                                     │   │
│ └──────────────────────────────────────────────────────────────────────────────────────────────────────────┘   │
└─ reload playlist via telnet or socket with more or less files, even 0 files ───────────────────────────────────┘

For normal mode:

┌►┌►  load playlist → isTheSameFile()? [NO] → play each file once and in order  ─┐ ──┐
│ │                   └─[YES] ─► move first to last ↗                            │   │
│ └──────────────────────────────────────────────────────────────────────────────┘   │
└─ reload playlist via telnet or socket with more or less files, even 0 files ───────┘

Would be nice to have a parameter to pass to playlist to enable this behavior, like check_same=true|false, and by default, the current behavior can be assumed (check_same=false)


Just to comment, I have an open issue for reloading a playlist with 0 files (#196), wich I need to be an emtpy playlist until I reload it again with some files. This could be another parameter to playlist.

Regards

dbaelde commented 9 years ago

I understand your comment. When the playlist is false, randomize is not of much help. So you're asking for a new feature, a random with some non-repetition guarantees. I could hack something up quite quickly but I'm not sure what's the right approach there, and I really don't have much time these days. I would welcome contributions though.

Regarding check_same I guess I could implement this. I need to think and test a bit, check that it does not break anything, but that seems like a good feature request.

giflw commented 9 years ago

I'm trying to learn ocaml, but it can take some time....

I think that the best approach would be add a new optional parameter (check_same, for instance). Doing so, the current behaviour will not be affected and the new behaviour will be available only when the optional parameter is given with value true.

Would be nice have this option in randomize mode too.

dbaelde commented 9 years ago

Your suggestion is good, very much in the general style of liquidsoap. I have proposed an implementation of it in the commit referenced just above. The check is ran before resolution, which is less powerful in the case of non-local requests, but allowed me to code the feature in a less invasive way. Please try it and comment before I integrate it in master. Here is an example check_next function that you can use:

prev = ref("")
def check_next(r)
  metadata = request.metadata(r)
  if metadata["filename"] == "" then true else
    if metadata["filename"] == !prev then false else
      prev := metadata["filename"]
      true
    end
  end
end

Note that this function is too simple if you're using playlists that may contain a single item, as it will repeatedly reject that item after the first playout -- which leads to a nasty loop, as explained in the doc of the new parameter. A better function would have a counter for not rejecting attempts more than X times in row.

giflw commented 9 years ago

How can I build the liquidsoap on that commit? I tried ./configure, but there is no file with that name.

And another question: is there a way to get the playlist size in check_next function? If its possible, I could make a check in the size of it before checking the file to be played, like this:

...
def check_next(playlist, r)
  if playlist.size > 1then
    ... check file ...
  end
end
giflw commented 9 years ago

Hi!

I finally had some time to compile and test this. I used your suggestion for check_next, but when it returns false, the playlist do not try to load a new track, but freezes instead. There was 2 songs on the playlist. Below is my test script:

set("log.file.path", "/tmp/music.log")
%include "~/Binaries/LiquidSoap/liquidsoap-full/liquidsoap/scripts/utils.liq"

prev = ref("")
def check_next(r)
  metadata = request.metadata(r)
  print("PLAYLIST #{prev} -> #{metadata['filename']}")
  if metadata["filename"] == "" then true else
    if metadata["filename"] == !prev then false else
      prev := metadata["filename"]
      true
    end
  end
end

out(playlist(check_next=check_next, "/tmp/music"))

And the log:

2015/04/10 02:44:47 >>> LOG START
...
2015/04/10 02:44:47 [main:3] Liquidsoap 1.1.1+scm (git://github.com/savonet/liquidsoap.git@081ccc2519f404da9a5b2e895c39ea45650d842e:20150410:022330)
2015/04/10 02:44:47 [main:3] Using: graphics=[distributed with Ocaml] pcre=7.0.4 dtools=0.3.1 duppy=0.5.1 duppy.syntax=0.5.1 cry=0.3.0 mm=0.3.0 xmlplaylist=0.1.3 lastfm=0.3.0 ogg=0.5.0 vorbis=0.6.2 opus=0.1.1 mad=0.4.4 flac=0.1.2 flac.ogg=0.1.2 dynlink=[distributed with Ocaml] lame=0.3.2 gstreamer=0.2.0 theora=0.3.1 gavl=0.1.5 alsa=0.2.1 ao=0.2.0 samplerate=0.1.2 taglib=0.3.1 camomile=0.8.4 pulseaudio=0.1.2 ladspa=0.1.4 lo=0.1.0
...
2015/04/10 02:44:47 [dynamic.loader:3] Could not find dynamic module for fdkaac encoder.
2015/04/10 02:44:47 [dynamic.loader:3] Could not find dynamic module for aacplus encoder.
2015/04/10 02:44:47 [dynamic.loader:2] Could not load plugins in directory /usr/local/lib/liquidsoap/scm/plugins.
...
2015/04/10 02:44:47 [music:3] Loading playlist...
2015/04/10 02:44:47 [music:3] Playlist is a directory.
...
2015/04/10 02:44:47 [decoder:3] Method "MAD" accepted "/tmp/music/06 - Anthony Hamilton - Freedom.mp3".
2015/04/10 02:44:47 [music:3] Prepared "/tmp/music/06 - Anthony Hamilton - Freedom.mp3" (RID 2).
2015/04/10 02:44:47 [mksafe:3] Switch to music with transition.
2015/04/10 02:44:51 [server:3] New client: localhost.
2015/04/10 02:44:52 [pulse_out(liquidsoap:):3] Performing user-requested skip
2015/04/10 02:44:52 [music:3] Finished with "/tmp/music/06 - Anthony Hamilton - Freedom.mp3".
2015/04/10 02:44:52 [mksafe:3] Switch to safe_blank with forgetful transition.
2015/04/10 02:44:52 [decoder:3] Method "MAD" accepted "/tmp/music/02 - Luis Bacalov - Django.mp3".
2015/04/10 02:44:52 [music:3] Prepared "/tmp/music/02 - Luis Bacalov - Django.mp3" (RID 4).
2015/04/10 02:44:52 [mksafe:3] Switch to music with transition.
2015/04/10 02:44:54 [pulse_out(liquidsoap:):3] Performing user-requested skip
2015/04/10 02:44:54 [music:3] Finished with "/tmp/music/02 - Luis Bacalov - Django.mp3".
2015/04/10 02:44:54 [mksafe:3] Switch to safe_blank with forgetful transition.
2015/04/10 02:44:54 [decoder:3] Method "MAD" accepted "/tmp/music/06 - Anthony Hamilton - Freedom.mp3".
2015/04/10 02:44:55 [music:3] Prepared "/tmp/music/06 - Anthony Hamilton - Freedom.mp3" (RID 7).
2015/04/10 02:44:55 [mksafe:3] Switch to music with transition.
2015/04/10 02:44:57 [pulse_out(liquidsoap:):3] Performing user-requested skip
2015/04/10 02:44:57 [music:3] Finished with "/tmp/music/06 - Anthony Hamilton - Freedom.mp3".
2015/04/10 02:44:57 [mksafe:3] Switch to safe_blank with forgetful transition.
2015/04/10 02:44:57 [decoder:3] Method "MAD" accepted "/tmp/music/02 - Luis Bacalov - Django.mp3".
2015/04/10 02:44:57 [music:3] Prepared "/tmp/music/02 - Luis Bacalov - Django.mp3" (RID 8).
2015/04/10 02:44:57 [mksafe:3] Switch to music with transition.
2015/04/10 02:44:59 [pulse_out(liquidsoap:):3] Performing user-requested skip
2015/04/10 02:44:59 [music:3] Finished with "/tmp/music/02 - Luis Bacalov - Django.mp3".
2015/04/10 02:44:59 [mksafe:3] Switch to safe_blank with forgetful transition.
2015/04/10 02:44:59 [decoder:3] Method "MAD" accepted "/tmp/music/02 - Luis Bacalov - Django.mp3".
**2015/04/10 02:44:59 [music:3] Request (RID 10) rejected by check_next!**
2015/04/10 02:45:29 [server:3] Client localhost disconnected.
dbaelde commented 9 years ago

Sorry for the delay in getting back to you. I cannot reproduce the problem yet, but I think I see the problem: thanks.

dbaelde commented 9 years ago

For the record I could actually reproduce easily by rejecting every other request.

prev = ref(false)
def check_next(r)
  prev := not(!prev)
  !prev
end
out(playlist(check_next=check_next,"/tmp/playlist"))

I also used two files in the playlist directory, of reasonable length: about two minutes. The key is to skip, as in your example: in my first attempt I used short files and let them end normally. After the first skip it freezes systematically. I also noted that reloading the playlist (via telnet) unfreezes it.

dbaelde commented 9 years ago

Okay the issue is fixed in a new commit in the LS-197 branch. Tell me if it works for you, anyway I'll have to double check this after some sleep. Also, this issue makes me think that it may be more natural to implement check_next using deeper hooks, which will have the side effect of working not only for local files.

By the way, I do not like the idea of adding a parameter for check_next that indicates the length of the playlist: it feels ad-hoc. However, we could consider putting this kind of info as part of the request metadata?

giflw commented 9 years ago

Is this new option added to playlist.reloadable to?

dbaelde commented 9 years ago

The playlist.reloadable source builds on request.dynamic, to which I did not add check_next.

I would not consider adding the same check_next as for playlists: this check is performed on files before they are preloaded, and for request.dynamic the user already has full control over the process at this stage.

In other words this feature could be obtained by tweaking the code of playlist.reloadable directly in utils.liq. I'm not sure I want to do this now. There seems to be a fair amount of redundancy already between playlist and playlist.reloadable and I don't like adding more. In fact I'd ask instead why keep playlist.reloadable? What does it do that you can't do with playlist? Shouldn't we consider merging the two sources?

If I were to extend check_next and implement it at the level of request resolution, then it'd be more powerful because you could filter requests after that they are preloaded, with all metadata available. It would then also make sense to make it available to all sources that preload requests. It seems to me that this feature may be implement as a specific protocol, but this is a bit complex and I don't think I'll do it for the next release.

giflw commented 9 years ago

I think I got your point.

For your comment 2 days ago, I think its usefull to add the playlist lenght to request metadata, so one can prevents a playlist of one item stop playing.

For your last comment:

dbaelde commented 9 years ago

Thanks for your comment, @giflw. You might consider yourself a novice but you're using liquidsoap more than I do these days, and you're being really helpful reporting and fixing problems.

I'll add metadata fields to playlist requests, I don't see why it wouldn't work. I propose "playlist_length" and "playlist_position", both containing integers. After this I will consider merging the branch and closing the issue. Did you get a chance to play again with the fixed implementation of check_next?

Edit: Note that the branch has been rebased and my two commits merged, I hope this does not create any trouble for you. The commit to test right at the moment is 7d60025.

Regarding other comments:

dbaelde commented 9 years ago

FYI the metadata has been added in the LS-195 branch because this will make merging easier.

dbaelde commented 9 years ago

I have rebased again on top of the integrated features from #195, and added a test. (For reference LS-197 should now be at 4029d52.) I think check_next is ready for integration into master, but I'll merge only tomorrow: talk now or stay silent forever :)

giflw commented 9 years ago

Thanks!

I have just teste with your last example. Its seems to be working fine. I'll have some time to test using a more sofisticated check_next like your first example in this weekend. I'll try to test playlist_position and playlist_lenght to.

dbaelde commented 9 years ago

The check_next feature has been integrated into master, closing the issue now.