devgianlu / librespot-android

A demo app that runs librespot-java on Android
33 stars 13 forks source link

Clicking on 'Play' multiple times crashes the player #16

Closed vhaudiquet closed 1 year ago

vhaudiquet commented 2 years ago

Clicking on the play button rapidly for 4-5 times instantly crashes the player, with this stack trace :

2022-01-08 16:19:02.149 7076-7555/xyz.gianlu.librespot.android E/AndroidRuntime: FATAL EXCEPTION: player-queue-132206105
    Process: xyz.gianlu.librespot.android, PID: 7076
    java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
        at java.util.ArrayList.get(ArrayList.java:437)
        at xyz.gianlu.librespot.player.StateWrapper.getCurrentTrack(StateWrapper.java:843)
        at xyz.gianlu.librespot.player.StateWrapper.metadataFor(StateWrapper.java:655)
        at xyz.gianlu.librespot.player.Player$3.metadataFor(Player.java:471)
        at xyz.gianlu.librespot.player.playback.PlayerSession.metadataFor(PlayerSession.java:200)
        at xyz.gianlu.librespot.player.playback.PlayerQueueEntry.load(PlayerQueueEntry.java:132)
        at xyz.gianlu.librespot.player.playback.PlayerQueueEntry.run(PlayerQueueEntry.java:276)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:923)
2022-01-08 16:19:02.165 7076-7555/xyz.gianlu.librespot.android I/Process: Sending signal. PID: 7076 SIG: 9

This can be considered a bug in librespot-java (the 'Player' does not accept multiple load requests in a short time) or a bug in librespot-android (if it is not a bug in librespot-java, then we should implement a way of not sending the request if the player is already processing requests).

Anyway, i'd like to hear what should be done on this, so that i know if I should implement a request queue and wait for player readyness or if this should be done directly into librespot-java in a next release.

mitschwimmer commented 2 years ago

I would not consider this a bug in librespot-java, although it surely could be more robust in this case. From my point of view this should be handled in the UI layer. If this was not a call to a backing library but, let's say, a service calling someones backend via network, it might be a bit more obvious. But I think this is more or less the same situation. You could, for example, implement a state machine to decide what to allow a user to do at which given state. I will put this issue in the README as an example of one of the requirements librespot-android is lacking as an integration showcase in contrast to being an app meant to be used by real users.

vhaudiquet commented 2 years ago

I tried implementing such a thing, but even doing waitReady() calls and surrounding the function with a volatile boolean global variable to allow only one execution at a time does not seem enough. I think the API does not allow to get the state of the player properly so it is not really possible to do that... Even waiting for everything, i get (in my app) to another crash :

2022-01-09 23:10:00.820 3346-3425/v.blade E/AndroidRuntime: FATAL EXCEPTION: pool-2-thread-4
    Process: v.blade, PID: 3346
    java.lang.IllegalStateException
        at xyz.gianlu.librespot.player.StateWrapper$TracksKeeper.initializeStart(StateWrapper.java:1096)
        at xyz.gianlu.librespot.player.StateWrapper.loadContext(StateWrapper.java:503)
        at xyz.gianlu.librespot.player.Player.load(Player.java:255)
        at v.blade.sources.spotify.SpotifyPlayer.playSong(SpotifyPlayer.java:258)
        at v.blade.player.MediaSessionCallback.lambda$onPlay$1$v-blade-player-MediaSessionCallback(MediaSessionCallback.java:63)
        at v.blade.player.MediaSessionCallback$$ExternalSyntheticLambda1.run(Unknown Source:4)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:923)

I can't seem to find a way to implement something where the user can spam click. This is not a major issue tho, as with those 'fixes' (waiting with a while loop inside the function), the user has to spam click really really fast, which i don't think would happen on a normal use scenario.

iscle commented 2 years ago

I tried implementing such a thing, but even doing waitReady() calls and surrounding the function with a volatile boolean global variable to allow only one execution at a time does not seem enough. I think the API does not allow to get the state of the player properly so it is not really possible to do that... Even waiting for everything, i get (in my app) to another crash :

In this case I think you you should open an issue on the librespot-java repo and I will try to take a look at it if devgianlu does not have time :)