New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

time_position in track_playback_ended is 0 since 2.0 #1456

Closed
0nse opened this Issue Feb 18, 2016 · 6 comments

Comments

5 participants
@0nse

0nse commented Feb 18, 2016

I'm using

  • Mopidy 2.0.0 and
  • Mopidy-Spotify 3.0.0.

Since this major update, my Mopidy-Scrobbler will not scrobble anymore. It listens on the track_playback_ended(tl_track, time_position) event. However, the received time_position is always 0 instead of the actual time position.

I experience this problem with tracks from Spotify as well as using Mopidy's local backend.

@jodal jodal added the C-bug label Feb 19, 2016

@jodal jodal added this to the v2.0.1 - Bug fixes milestone Feb 19, 2016

@jodal

This comment has been minimized.

Member

jodal commented Feb 19, 2016

Sounds like a regression we clearly want to fix in 2.0.1.

@ghost

This comment has been minimized.

ghost commented Mar 2, 2016

is this as simple as changing time_position to get_time_position for track_playback_ended?

@trygveaa

This comment has been minimized.

Member

trygveaa commented Apr 10, 2016

@bigmittens: No, get_time_position gives the position for the current track, while in track_playback_ended you want the position for the track that ended.

There are four cases (as far as I can see) where we trigger track_playback_ended:

  1. Track ends and was the last
  2. Track ends and starts playing the next
  3. User presses next
  4. User presses stop

At 1. _on_end_of_stream is called. self.get_time_position() reports the same as the duration of the track, so this sends the correct position to track_playback_ended.

At 2. _on_stream_changed is called. self._last_position is None and self.get_time_position() is 0, so this sends 0 to track_playback_ended. The time position that should be sent to track_playback_ended is what it was right before the track ended, i.e. its duration. self._last_position should probably set somewhere before self.get_time_position() becomes 0. I looked at _on_about_to_finish which is called beforehand, but self.get_time_position() is not yet at the end of the song when that happens.

At 3. _on_stream_changed is called. self._last_position is None and self.get_time_position() is 0, so this sends 0 to track_playback_ended, just like at 2. Also like 2, the time position that should be sent here is what it was right before the track ended. However, in this case it is not the duration of the track. self._last_position should probably set somewhere before self.get_time_position() becomes 0 here as well. _change is called beforehand with the correct position, so maybe that should set self._last_position.

At 4. sometimes nothing happens, and sometimes _on_stream_changed. However, if _on_stream_changed wasn't called, it is when a song is started again. Not sure why it isn't always triggered immediately, but it probably should be. self._last_position was set by stop, so it reports the correct position to track_playback_ended.

The commit da7ec9b seems relevant.

@edran

This comment has been minimized.

Contributor

edran commented Jul 24, 2016

Setting self._last_position in _change and _on_about_to_finish does indeed fix the issue. The only thing is that the about-to-finish signal comes between 1 and 3 seconds before the end of the track.

I can make a pull request and open another issue related to this new problem? For now I'd rather have this problem and scrobbling working in 99.9% of the cases than what we currently have 😐

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 24, 2016

Could we have _on_about_to_finish set _last_position to duration, and then as long as the cases that would prematurely end the track (stop, next, etc) override _last_position we would be good?

@edran

This comment has been minimized.

Contributor

edran commented Jul 24, 2016

Yes, that also seems to be working. _last_position is correctly overridden by stop / next / previous.
Seems like a reasonable solution, I'll make a pull request in a moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment