stevenleeg / geemusic

A bridge between Google Music and Amazon's Alexa
GNU General Public License v3.0
664 stars 181 forks source link

Last.fm not implemented properly #276

Closed RandyCupic closed 5 years ago

RandyCupic commented 5 years ago

This project suffers from the same issue as many other audio playing apps, including Deezer (and that's mostly why I stopped using it because the developers weren't open to solve the issue). It is very important to somehow scrobble only if at least half of the track has been played. Otherwise, it happens that it will generate fake/false scrobbles every time we're skipping tracks, and I do that very often.

I looked into the code and found where the scrobbling is being executed; it's in the controller action, which is executed when starting a new song. And this is the cause of the issue; it's a bad place to do scrobbling because in that moment, it's now known whether we will really listen to this song or just skip it. But the problem is even worse: for some reason, it adds 3 scrobbles for each track! It seems that this action is being called 3 times, for some reason. You can see it on this image:

image

Also I noticed that it scrobbles in advance: when I took this photo, I was listening to the song Move Your Body (D.J.Gabry Ponte Original Radio Edit) but it has already scrobbled the next track Too Much of Heaven (album mix), which came right after this one. It happened with all previous tracks as well.

I will try my best to find some solution for this issue. If I succeed at that, I will create a PR. But in the meantime, do you have any idea what would be the best place to do the scrobble? I was thinking about the on_playback_nearly_finished callback in the intents/playback.py file. Is it possible to get the info about the current track playing there? I will try to do some tests and let you know if it works.

fergyfresh commented 5 years ago

Why so feisty? It's not a core feature of the app. I don't use that feature at all. I know @huberf implemented this and might have some input he can provide, but I don't know how to solve that.

It is an open source project to work around the fact that Alexa doesn't allow you to play google play music in a regular app. Any features that don't exist or don't work precisely as you'd like them are open to be forked and/or fixed.

Wish you best of luck in trying to fix it yourself! Thanks for pointing out the issue.

huberf commented 5 years ago

@RandyCupic The last.fm scrobbling feature can certainly be improved. As most GeeMusic users don't use the feature, I wanted it to be as minimal as possible but adding a more proper implementation shouldn't be a problem. To rearchitect it, you are right that https://github.com/stevenleeg/geemusic/blob/master/geemusic/intents/playback.py#L30 would be a logical place to add the scrobbling logic and you could use queue.current() or something similar to extract the current song ID and scrobble that. If you want to take care of adding this, that would be great and I'm happy to help answer any questions about the implementation.

RandyCupic commented 5 years ago

@fergyfresh I'm sorry if I sounded feisty, this was not my intention at all. I just wanted to point out at thee issue and see if I could get some useful info which would help me to try to solve it. I know that last.fm is not so popular and that not many people use it but to us who are using it it's very important to be working properly. I ditched all devices and applications which are not supporting scrobbling because I want all my tracks to get tracked.

My intention with this issue was to see if there might be something in the process of implementation already or to get some useful info. I think that it's always good to open an issue first and then try to implement it itself, if it's possible.

In this case, infro from @huberf will help me a lot. Thank you!

RandyCupic commented 5 years ago

Just to let you know: I've managed to improve the scrobbling feature a little bit. First, I tried to put scrobbling right after this line: https://github.com/stevenleeg/geemusic/blob/5754e5745631c88f14fd20e155533078ea014f9b/geemusic/intents/playback.py#L31 as I mentioned. It was working much better than before; there was no more duplicate scrobbles (3 scrobbles for each track) and there was no more scrobbling of the next track instead of current one, but it was still acting weird: it would scrobble the song right after it started playing, so it was still vulnerable to creating fake scrobbles when skipping songs. It was weird to me, because this code should by invoked before the current song finished playing, not when it starts. Then I noticed that I accidentally put the scrobbling after the next_id = queue.up_next() line. So yes, I was obviously scrobbling the next track. But even when I fixed this, nothing has changed.

Then I moved scrobbling right after this line: https://github.com/stevenleeg/geemusic/blob/5754e5745631c88f14fd20e155533078ea014f9b/geemusic/intents/playback.py#L42 And now it works almost perfectly. It will scrobble the song once it finished playing or better said, only if you listen to the whole song. So there's no chance that we will get fake scrobbles anymore. But why "almost" perfect? Well, there has been a lot of discussions when the song should be considered as listened to and when to scrobble. It mostly depends on the preferences of the individual persons; some people like to scrobble only when 100% of the song has been played, but some agree that 50% is enough to assume that the track has been listened to. But overall conclusion is that 50% is minimum. That's why most scrobblers have a slider to set the point between 50% to 100% when the song will be scrobbled. I'm mostly using 50% because I often tend to skip songs before they end, due to, for example, having long and boring outro.

But this is definitely a good enough behavior for now so I'm planning to do a pull request. And once when I get some more time, I will see if it's somehow possible to improve this even more. As far as I know, this app doesn't support seek through the song, so I could hook into next() and stop()commands and check there if elapsed_time / track_duration percentage is bigger than the value set in the, for example, environment variable -> and then scrobble the track. In that way, we would get scrobble even when we skip the song, if we've been listening to it long enough.

@fergyfresh @huberf is it somehow possible to get elapsed time and track duration from the queue/api/player?

And thank you once more for all your support and help!

EDIT: I've found why it was not working properly with the playbackNearlyFinished event:

PlaybackNearlyFinished is sent when your client is ready to buffer/download the next stream in your playback queue. Many implementations send this event shortly after PlaybackStarted to start buffering and reduce lag between playback of streams.

Guys, I'm sorry for making a philosophy over a simple feature like last.fm scrobbling and over a feature which is not being used by many users. But I'll try to improve this by myself and I will appreciate any of your help :)

fergyfresh commented 5 years ago

@RandyCupic is this one good to go?

RandyCupic commented 5 years ago

@fergyfresh My PR has solved this in an acceptable way but it still has to be solved better. Currently, it will only scrobble if the track has been listened 100%, which is wrong since most users have setting on their scrobbling apps to scrobble after 50%.

But since I currently don't have time to do this better, and since there were no interests for this feature except me, I think that the issue can be closed. Once I got the time to look better into it, I may implement some more advanced way for handling this and submit a PR. Currently I don't even use Geemusic since I'm having tons of issues with it and it's very unstable when using with Amazon AWS, it crashes all the time. And I didn't have the time nor the chance to look into it and try the local server option.