Rearranging a shuffled queue moves songs to unexpected positions

unrenowned commented 2 weeks ago

Describe the Bug/Crash

N.B. This currently only affects the latest commit on the dev branch, but I had some questions about the code in the latest release too so thought I'd mention both in this bug.

When shuffle is enabled, rearranging the queue causes songs to jump to unexpected positions. To reproduce:

Shuffle an album/artist/playlist
Check that the current and next song in the queue are not adjacent in the unshuffled playlist.
Swap the current and next song in the queue

https://github.com/OxygenCobalt/Auxio/assets/163776820/6ce27ef0-9fe0-4414-ad6b-0f0255669668

Describe the intended behavior

The next song should move so it sits behind the current song.

What android version do you use?

Android 13

What device model do you use?

Pixel 2 (Android Emulator -- sorry, I don't have a spare device to test on at the moment!)

Logs

I added some extra debug logs to the code to try and work out what's going on. Here are those logs for the video above:

22260-22260 ExoPlaybackStateHolder  org.oxycblt.auxio.debug              D  shuffle order = 6, 4, 1, 2, 10, 5, 9, 8, 0, 3, 7
22260-22260 ExoPlaybackStateHolder  org.oxycblt.auxio.debug              D  old timeline = 0. Intro, 1. VCR, 2. Crystalised, 3. Islands, 4. Heart Skipped A Beat, 5. Fantasy, 6. Shelter, 7. Basic Space, 8. Infinity, 9. Night Time, 10. Stars
22260-22260 ExoPlaybackStateHolder  org.oxycblt.auxio.debug              D  current index = 6. Shelter
22260-22260 ExoPlaybackStateHolder  org.oxycblt.auxio.debug              D  next item = 4. Heart Skipped A Beat
22260-22260 ExoPlaybackStateHolder  org.oxycblt.auxio.debug              D  from = 1, to = 0
22260-22260 ExoPlaybackStateHolder  org.oxycblt.auxio.debug              D  trueFrom = 4, trueTo = 6
22260-22260 ExoPlaybackStateHolder  org.oxycblt.auxio.debug              D  new timeline = 0. Intro, 1. VCR, 2. Crystalised, 3. Islands, 4. Fantasy, 5. Shelter, 6. Heart Skipped A Beat, 7. Basic Space, 8. Infinity, 9. Night Time, 10. Stars
...
22260-22260 ExoPlaybackStateHolder  org.oxycblt.auxio.debug              D  new current index = 5. Shelter
22260-22260 ExoPlaybackStateHolder  org.oxycblt.auxio.debug              D  new next item = 9. Night Time

You can see that Auxio is moving the next song in the shuffle order (Heart Skipped A Beat) to the index of the current song (Shelter), shifting it one index to the left. Because the shuffle order stays constant, that bumps the current song to a completely different position in the shuffle order.

Duplicates

[x] I have checked the Troubleshooting page.
[x] #771
[x] I have checked this issue occurs on the lastest version.
[x] I agree to the Contribution Guidelines.

Root Cause

It looks like the code in ExoPlaybackStateHolder.move() changed when you broke up the playback package. Originally it swapped the positions of the songs, now it just moves the song in the from index to the to index.

However, I don't think reverting to this will work in all cases either. If abs(to - from) > 1, you'll get other unexpected results as the songs swap position in the queue/shuffle order and skip over the songs in between them. In practice this doesn't seem to happen unless you drag a song to a different position very quickly, but I thought it was worth mentioning in case this is a problem.

I think the simplest solution would be to keep the current code for unshuffled queues, but when shuffle is enabled just rearrange the shuffle order and leave the actual timeline alone. I can write a PR for this if you'd like, but I wanted to check first as I'm not sure if/how you want the underlying timeline to be affected when someone rearranges a shuffled queue.

OxygenCobalt commented 1 week ago

Thank you for the extremely detailed report @unrenowned. I actually made this change because:

Media3's interface requires a more standard "move" operation rather than my swap behavior.
I thought the swapping was actually a no-op since I forgot about this index shifting edge case. I'm pretty sure this is why I was swapping in the first place to more or less hack around it.

The underlying timeline is altered when the shuffled queue is re-arranged, and mutating/replacing the shuffle order itself is a huge performance hit since it triggers a full reload of the player state. I need to find a way to avoid re-introducing the swap behavior since it can no longer work when external apps do a direct move but Auxio internally is only expecting swaps.

OxygenCobalt commented 16 hours ago

LOL, I figured out this issue months ago and thought it was my problem: https://github.com/androidx/media/issues/954

Yeah. It legit does not update the shuffle order when you move it, I have to do it. I wouldn't know how to patch this in myself since ExoPlayer internals are so nightmarishly difficult to use.

OxygenCobalt commented 16 hours ago

Re-added old swap code. I don't care if larger moves break it, Android Auto probably doesn't make those anyway. Will wait for them to do https://github.com/androidx/media/issues/1381

OxygenCobalt / Auxio