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

Gapless part 1 #1288

Merged
merged 34 commits into from Oct 7, 2015

Conversation

2 participants
@adamcik
Member

adamcik commented Sep 16, 2015

This is a major cleanup of core playback internals. Some important things to note:

  • We no longer emit the "silly" state changed events between tracks. This might confuse clients, so we could add playing -> playing type events.
  • play, next, prev are now async. Tests must take care to wait for core futures to finish. Flow is not that these calls, about about to finish set the pending track. Once stream changed comes in we switch. This might also confuse clients.
  • There is a race after about to finish and before stream changed were we can seek the wrong song. Fix postponed to later gapless followup branch.
  • play, next, prev still break streaming, we need to make all changes async by faking end of stream events. Postponed for now.
  • We should review my event spreadsheet and make sure we have things cleaned up. I also suspect we are missing some calls to add history.

adamcik added some commits Jan 21, 2015

backend: Change playback API (breaking change)
While trying to remove traces of stop calls in core to get gapless working I
found we had no way to switch to switch tracks without triggering a play. This
change fixes this by changing the backends playback provider API.

- play() now _only_ starts playback and does not take any arguments.
- prepare_change() has been added, this could have been avoided with a kwarg to
  change_track(track), but that would break more backends.
- core has been updated to call prepare_change+change_track+play as needed.
- tests have been updated to handle this change.

Longer term I hope to completely rework the playback API in backends, as 99% of
our backends only use change_track(track) to translate URIs. So we should make
simple case simple, and handle mopidy-spotify / appsrc in some other way.
core: Move core.playback.next off change_track helper
Note that since this doesn't use play via change_track we have to copy over the
tracking code and make sure it is only run for the playing case
core: Move core.playback.previous off change_track helper
Same fix as for core.playback.next and mostly the same caveats.
core: Move core.playback.on_end_of_track off change_track helper
Only handles the playing case, unlike the previous and next changes. It should
also be noted that this is just a temporary state on the road to making this
method handle gapless.
core: Enable gapless playback
There is still quite a bit to be done is this area:

- We need to start tracking pending tracks as there is time window when we are
  decoding a new track but still playing the old one
  - Currently this breaks seek, next, prev during this time window
- The old on_end_of_track code needs to die.
- Stream changes need to trigger playing events and switch the pending track to
  current.
core: Remove on_end_of_track from playback
This has been replaced by on_about_to_finish and on_end_of_stream.
Tests have been updated to reflect these changes.
core: Make sure current_tl_track changes on stream change
- Adds stream changed handler to core
- Moves playback started trigger to stream changed
- Made about to finish store next track in _pending_tl_track
- Set the pending track as current in stream changed
- Adds tests for all of this and fixes existing tests
core: Update pending track handling and fix consume / gapless issue
- Pending track should only be triggered by stream_changed if there is one.
- Tracklist changed was incorrectly calling stop breaking tests and gapless
- Tests have been updated to capture and replay audio events. This should avoid
  test deadlocks while still using the audio fakes.
Merge branch 'develop' into feature/implement-gapless
Conflicts:
	mopidy/backend.py
	mopidy/commands.py
	mopidy/core/actor.py
	mopidy/core/playback.py
	tests/audio/test_actor.py
	tests/core/test_playback.py
	tests/local/test_playback.py
Merge branch 'develop' into feature/implement-gapless
Conflicts:
	mopidy/commands.py
	mopidy/core/playback.py
	tests/core/test_playback.py
	tests/local/test_playback.py
listener: Kill off mopidy.listener.send_async
This is no longer needed as the plain send method makes sure to use tell to
queue actor message. Which has better performance, and avoids deadlocks.

A side effect of this is that assuming you have a core actor running and a
dummy audio in use audio events just work.
Merge branch 'develop' into feature/implement-gapless
Conflicts:
	tests/local/test_playback.py
tests: Make dummy backend use real playback provider if audio is pass…
…ed in

This is needed in order to make audio events propagate, to core and trigger
async state changes in tests.

@jodal jodal self-assigned this Sep 16, 2015

@jodal jodal added this to the v1.2 - Gapless milestone Sep 16, 2015

self._pending_tl_track = None
if self._audio:
self._audio.set_about_to_finish_callback(self._on_about_to_finish)

This comment has been minimized.

@jodal

jodal Oct 5, 2015

Member

Won't this cause the audio actor to run core's code and alter core state?

If you make the callback public, you can probably fix this by using self.core.actor_ref.proxy().playback.on_about_to_finish as the callback.

This comment has been minimized.

@adamcik

adamcik Oct 5, 2015

Member

The call path should be something like the following:

about-to-finish --> audio --> core callback --> backend --> audio 

The goal here is to block the about to finish until an audio.set_uri type call has made it out. (This is also one of the reasons I want backends out of this path). The about-to-finish should be coming from some gst internal thread, so our audio code will also run there, and then from there core would probably still be in the same context to how the callback is done. We den call into the backend via actors, after which the backend calls into audio via actors. So we must ensure that we can't deadlock that last call, or else the about-to-finish never returns.

This comment has been minimized.

@jodal

jodal Oct 5, 2015

Member

Not exactly easy to follow what's happening here. Are you confident in how this is implemented now, or do I have a point? :-)

This comment has been minimized.

@adamcik

adamcik Oct 5, 2015

Member

I'm fairly sure what I described is correct, and it was intentional to do things outside of actors due to deadlocks that I no longer remember the details of. And also with respect to the circular deps between audio and core setup.

This comment has been minimized.

@adamcik

adamcik Oct 5, 2015

Member

self.core.actor_ref.proxy().playback.on_about_to_finish inside the init actually deadlocks as you are trying to create proxy to yourself while still setting yourself up...

This comment has been minimized.

@adamcik

adamcik Oct 5, 2015

Member

If I give it the actor, and then let it create the proxy on the fly in audio it works at least. And I've not manged to deadlock it yet. I still want to think this over a bit more and do some more testing.

This comment has been minimized.

@jodal

jodal Oct 5, 2015

Member

You should be able to create the proxy in on_start(). __init__() is executed in the actor's parent before the actor is actually started, thus there is no ActorRef object to create an ActorProxy from.

self.stop()
self._on_end_of_stream() # pretend and EOS happend for cleanup

This comment has been minimized.

@jodal

jodal Oct 5, 2015

Member

s/and EOS happend/an EOS happened/

# TODO: split into smaller easier to follow tests. setup is way to complex.
# TODO: just mock tracklist?
class CorePlaybackTest(unittest.TestCase):
# TODO: Replace this with dummy_backend no that it uses a real playbackprovider

This comment has been minimized.

@jodal

jodal Oct 5, 2015

Member

s/no that/now that/

@jodal

This comment has been minimized.

Member

jodal commented Oct 5, 2015

  • Update changelog

adamcik added some commits Oct 5, 2015

core: Make sure the about-to-finish callback gets run in the actor.
When about to finish gets called we are running in some GStreamer thread. Our
audio code then calls the shim core callback which is responsible for
transferring our execution to the core actor thread and waiting for the
response. From this point we do normal actor calls to the backend(s) which in
turn call into the audio actor. Since the initial audio code that was called is
outside the actor this should never deadlock due to this loop.
@adamcik

This comment has been minimized.

Member

adamcik commented Oct 6, 2015

There, now the core code that gets called is in the actor.

@adamcik

This comment has been minimized.

Member

adamcik commented Oct 6, 2015

  • Other followup should probably also be moving certain events out of about-to-finish and into stream changed.
@adamcik

This comment has been minimized.

Member

adamcik commented Oct 7, 2015

Currently blocked on mopidy-spotify / appsrc deadlocking with these changes.

jodal added a commit that referenced this pull request Oct 7, 2015

@jodal jodal merged commit 93899c8 into mopidy:develop Oct 7, 2015

1 check passed

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

@adamcik adamcik deleted the adamcik:feature/implement-gapless branch Jul 27, 2016

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