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

Get correct track position on change events #1534

Merged
merged 3 commits into from Jul 25, 2016

Conversation

2 participants
@edran
Contributor

edran commented Jul 24, 2016

Playback callbacks now correctly handle the following cases:

  1. the track finishes and switches to the next one in the list;
  2. user presses next / previous

Fixes #1456 (and therefore should also close mopidy/mopidy-scrobbler#20).

Properly get track position before change events
In particular, this allows to send the right information when:

1. the track finishes and switches to the next one in the list;
2. user presses next / previous

The cases of EOS and stop event were already handled properly.

Note: we only have GStreamer's `about-to-finish` event to deal with the
end of a track, which usually happens a few seconds before the end of
the track. We set the position to the length of the track, which is not
overridden unless the user generates a relevant callback.
@edran

This comment has been minimized.

Contributor

edran commented Jul 24, 2016

Err... I think the build broke because of #1523.

(Edit: cc @adamcik )

@@ -251,6 +251,10 @@ def _on_about_to_finish(self):
if self._state == PlaybackState.STOPPED:
return
# Unless overridden by other calls (e.g. next / previous / stop) this
# will be the last position recorded until the track gets reassigned.
self._last_position = self._current_tl_track.track.length

This comment has been minimized.

@adamcik

adamcik Jul 24, 2016

Member

This might still fail if track.length isn't populated, but that's well beyond the scope of this PR as it would require changes in audio and various other places.

This comment has been minimized.

@edran

edran Jul 24, 2016

Contributor

I'm putting a TODO to throw some sort of relevant exception whenever we get around fixing this.

This comment has been minimized.

@edran

edran Jul 24, 2016

Contributor

Just to check, wouldn't the backend explode if track.length was None after a about-to-finish event?

This comment has been minimized.

@adamcik

adamcik Jul 24, 2016

Member

If memory serves we can set track length to None to signify a an "unlimited" length such as web streams. So thinking over this again a "bad" backend that doesn't set a length for a track will just look like a stream. And streaming has been working just fine so I don't think there is much we can or should do about this after all.

This comment has been minimized.

@edran

edran Jul 24, 2016

Contributor

Fair enough.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 24, 2016

Flake failure is preexisting due to a newer version of flake8 catching more things that the old one :(

Should _seek reset _last_position as well, that would be an other way to leave early. Or would it still work out by _last_position getting overriden again?

(It's been to long since I've looked at this in detail, so I'm missing a bit more state than I should)

@adamcik adamcik added this to the v2.0.1 - Bug fixes milestone Jul 24, 2016

@edran

This comment has been minimized.

Contributor

edran commented Jul 24, 2016

I've tried to debug seek, but the problem appears in such a small time frame that I'm not sure whether it's bugging out because the logic of seek / _seek is buggy.

If I keep seeking after the song I get all sort of unrelated traceback (plus buggy behaviour with ncmpcpp).

@edran

This comment has been minimized.

Contributor

edran commented Jul 24, 2016

I can confirm it works fine if you seek till the end of the track (both in EOS and when switching to a new track in the list).

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 24, 2016

There were still some rough edges in gapless that could triggered with last minute seeks. I had some notes somewhere about which cases I still had outstanding, this is likely one of them.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 24, 2016

So I think this looks good as fix for the specific issue you are targeting. If we want this in a 2.0.x release we should probably cherrypick it into the right branch, or replace this PR with one against the release-2.0 branch instead of develop. Either way is fine by me.

@edran

This comment has been minimized.

Contributor

edran commented Jul 24, 2016

Considering it's critical for mopidy-scrobbler, I suggest whichever makes the fix go upstream faster.

@adamcik adamcik merged commit 5b6632e into mopidy:develop Jul 25, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@edran edran deleted the edran:fix-scrobbling branch Jul 25, 2016

adamcik added a commit that referenced this pull request Jul 25, 2016

Merge pull request #1534 from edran/fix-scrobbling
Get correct track position on change events
@adamcik

This comment has been minimized.

Member

adamcik commented Jul 25, 2016

Thanks for the contribution, this has been backported to 2.0.x so we can hopefully do a point release with this really soon.

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