Skip to content
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.

Sign up for GitHub

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

Fix gapless playback #269

Closed
wants to merge 5 commits into from
Closed

Fix gapless playback #269

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0423b5e
Fix gapless playback
Jul 24, 2020
9e8520a
Add buffer holding
Jul 25, 2020
fe5b0c0
bugfix
Jul 26, 2020
e7d2733
Improve comments
Aug 25, 2020
7bdb72d
Move buffer clearing to avoid race conditions
Aug 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions mopidy_spotify/playback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
# Extra log level with lower importance than DEBUG=10 for noisy debug logging
TRACE_LOG_LEVEL = 5

# Last audio data sent to the buffer, currently on hold.
# This data is held because libspotify sends a single empty buffer before ending the track. It is discarded
# the moment a new track starts so a smooth transition between songs can be made.
_held_buffer = None


class SpotifyPlaybackProvider(backend.PlaybackProvider):
def __init__(self, *args, **kwargs):
Expand Down Expand Up @@ -50,6 +55,8 @@ def _connect_events(self):
def change_track(self, track):
self._connect_events()

global _held_buffer

if track.uri is None:
return False

Expand All @@ -73,6 +80,9 @@ def change_track(self, track):
self._first_seek = True
self._end_of_track_event.clear()

# Discard held buffer
_held_buffer = None

try:
sp_track = self.backend._session.get_track(track.uri)
sp_track.load(self._timeout)
Expand Down Expand Up @@ -162,6 +172,8 @@ def music_delivery_callback(
# This is called from an internal libspotify thread.
# Ideally, nothing here should block.

global _held_buffer

if seeking_event.is_set():
# A seek has happened, but libspotify hasn't confirmed yet, so
# we're dropping all audio data from libspotify.
Expand All @@ -187,8 +199,15 @@ def music_delivery_callback(
bytes(frames), timestamp=buffer_timestamp.get(), duration=duration
)

# We must block here to know if the buffer was consumed successfully.
consumed = audio_actor.emit_data(buffer_).get()
# If there is any held buffer, send it
if _held_buffer:
consumed = audio_actor.emit_data(_held_buffer).get()
else:
# No buffer, we assume successful consumption
consumed = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this should be true when there isn't a held buffer, as a follow on effect would be increasing the duration incorrectly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set it as true, because we take the current buffer and put it on hold, which (if my understanding of frame consumption is right) means we did consume the frames — even if we'll push it to Mopidy later.

On the other hand, I'm not sure if the buffer_timestamp is increased correctly if its done before emitting data.

Copy link
Member

@kingosticks kingosticks Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem weird to attach what we know is the wrong timestamp to the buffer we are emitting to GST (although the error will be very small). Should we be doing something more like buffer_timestamp.increase(_held_buffer.duration)?

EDIT: Sorry, confused myself. The timestamp attached to the buffer is obviously correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine. It doesn't matter when we increment buffer_timestamp as long as it's before we use in the next delivery.


# Hold the buffer for a while, so we can filter out the single empty buffer after the track
_held_buffer = buffer_

if consumed:
buffer_timestamp.increase(duration)
Expand All @@ -208,6 +227,9 @@ def end_of_track_callback(session, end_of_track_event, audio_actor):
end_of_track_event.set()
audio_actor.emit_data(None)

# Stop the track to prevent receiving empty audio data
session.player.unload()


class BufferTimestamp:
"""Wrapper around an int to serialize access by multiple threads.
Expand Down