search

calzoneman / sync

Node.JS Server and JavaScript/HTML Client for synchronizing online media
Other
1.46k stars 235 forks source link

Shuffle playlist isn't very random #836

Open mikedesu opened 4 years ago

mikedesu commented 4 years ago

Shuffling the playlist does not seem to use a very good random implementation. Reproducible sequences of playlist shuffling on subsequent website reloads and shuffling of new playlists is possible. The RNG seed seems to be some bad default. I'd fix it myself but that takes time. cytu.be needs to be reloaded once y'all patch it.

calzoneman commented 4 years ago

Did this start happening recently? If so, I wonder if there is a bug in https://github.com/calzoneman/sync/commit/1ec3eab0dc312e505dc725119cbbd8c784ed6d9d (introduced by #812).

Something doesn't add up here, shuffle uses Math.random(), certainly not a CSPRNG, but shouldn't be trivially predicable either. It's shared across each instance of the application, so I'm not sure why reloading a channel (which doesn't restart the process) would cause reproducible sequences.

calzoneman commented 4 years ago

Can you provide more information about the "reproducible sequences"? I tested a few times and didn't notice any obvious patterns.

mikedesu commented 4 years ago

Sure thing.

I just loaded up a playlist of mine with 13 episodes, then shuffled, wrote down the sequence, then cleared the list. I did this 3 times. The sequences I got were:

1,6,13,10,9,5,12,2,4,11,3,7,8 1,11,13,4,12,8,10,2,9,3,5,6,7 1,4,11,13,8,12,9,10,5,2,6,7,3

Notice how close together 13, 7, 2, 12 appear relative to their positions in the list. Also, notice that 1 is locked at front because this is new functionality. At some point, the shuffle no longer fully shuffles the playlist, but instead continues to play whatever is currently playing (in essence, shuffling everything in the playlist except for whatever is currently playing). I don't know but it just feels like the rng sucks now and I didn't use to feel this way. Also, I'd like the old shuffle back. When I hit shuffle, I want a completely different item on the playlist to start playing immediately. It's annoying and it wasn't like that before.

Comment edited: please refrain from attaching advertisement signatures in your GitHub comments --calzoneman

On Sun, Nov 3, 2019 at 10:13 PM Calvin Montgomery notifications@github.com wrote:

Can you provide more information about the "reproducible sequences"? I tested a few times and didn't notice any obvious patterns.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/calzoneman/sync/issues/836?email_source=notifications&email_token=AABILA6SX6ASULU6L53YKITQR6ANRA5CNFSM4JIOMCQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC6FJIY#issuecomment-549213347, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABILA2T7VDHUVI6RNVXKKDQR6ANRANCNFSM4JIOMCQQ .

calzoneman commented 4 years ago

The patterns in shuffle results could be influenced by bias introduced by multiplying Math.random() (which returns a floating point number in [0, 1)) by the array length and flooring it, which does not guarantee uniform distribution. This behavior has been the same since shuffle was implemented, but it's possibly more noticeable when shorter playlists are shuffled.

As for the currently playing item retaining its position, that was introduced by #812, although I have now received feedback from a few people who aren't happy with it so perhaps that feature was accepted too hastily. At the least it should be possible to opt out of it.

Xaekai commented 4 years ago

Absolutely agree that 812 was hasty. Should be a boolean channel option defaulted to on, but I personally would have it off.