Skip to content
This repository

End of track cleanup #346

Closed
wants to merge 2 commits into from

3 participants

Stein Magnus Jodal Thomas Adamcik Ronald Hecht
Stein Magnus Jodal
Owner
jodal commented March 19, 2013

This pull request reintroduces the changes originally done in pull request #231 to properly separate end-of-track and end-of-stream handling.

These changes breaks some existing functionality in Mopidy, and was thus reverted from the develop branch. The following issues must be resolved before this can be merged again:

  • #316 EOT changes makes "next" deadlock Mopidy
  • #320 EOT changes breaks random playback
Thomas Adamcik
Owner

hechtus/mopidy-gmusic#2 has some relevant discussion on the issues reaming in this branch.

Ronald Hecht

The fact that, BasePlaybackProvider::change_track() always returns True leads to the assumption, that change_track() always succeed. I would like to see that BasePlaybackProvider::play() evaluates the return value of change_track() and returns False when change_track() returns False.

Ronald Hecht

Playing around with this patch I experienced, that ncmcpp (0.5.4) didn't got noticed about (gapless) track changes. It simply continues incrementing the time and does not change the current playing track in the current playlist. GMPC seems to got noticed. So, I don't know, if this is an issue of ncmcpp 0.5.4.

Ronald Hecht

In general, I think gapless playback of remote media will still be an issue. The about-to-finish message comes too late. I was trying to play short mp3 audio files from http://. Sometimes it works, but mostly there is a noticeable gap between the tracks. So, prefetching the next song is crucial for gapless playback of remote media (e.g. Google Music).

Thomas Adamcik
Owner

Should we perhaps just close this as a PR, as we obviously won't be using this code for anything but a reference at this point and move tracking to a proper bug?

Stein Magnus Jodal
Owner

Agree. I'm closing this, as well as the issues that are only present in this branch.

Stein Magnus Jodal jodal closed this October 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
22  docs/changes.rst
Source Rendered
@@ -10,6 +10,28 @@ v0.13.0 (in development)
10 10
 
11 11
 (in development)
12 12
 
  13
+**Audio sub-system**
  14
+
  15
+- EOT (end of track) and EOS (end of stream) handling has finally been fixed.
  16
+  Problem with the old code was that we simply used EOS for everything, and let
  17
+  the EOS propagate for the end of each track. What this means is basically that
  18
+  we would disrupt the pipeline at the end of every track. This work was
  19
+  concluded in the pull request :issue:`231`. Changes and issues fixed by this
  20
+  include:
  21
+
  22
+  - A new method ``change_track(track)`` for playback provider which is where
  23
+    doing everything like setting the URI and starting to push data should be
  24
+    done. The ``play(track)`` method and EOT track changing uses this new
  25
+    method.
  26
+
  27
+  - We've started using the about to finish signal from playbin2 to ensure
  28
+    gapless playback as part of our EOT cleanup. (Fixes: :issue:`160`)
  29
+
  30
+  - Mopidy now distinguishes between EOT and EOS properly. (Fixes: :issue:`222`)
  31
+
  32
+  - Since we no longer let EOS disrupt the pipeline streaming OGG via SHOUTcast
  33
+    now works. (Fixes: :issue:`168`)
  34
+
13 35
 **Spotify backend**
14 36
 
15 37
 - Let GStreamer handle time position tracking and seeks. (Fixes: :issue:`191`)
32  mopidy/audio/actor.py
@@ -20,7 +20,6 @@
20 20
 
21 21
 mixers.register_mixers()
22 22
 
23  
-
24 23
 MB = 1 << 20
25 24
 
26 25
 
@@ -50,6 +49,8 @@ def __init__(self):
50 49
         self._software_mixing = False
51 50
         self._volume_set = None
52 51
 
  52
+        self._end_of_track_callback = None
  53
+
53 54
         self._appsrc = None
54 55
         self._appsrc_caps = None
55 56
         self._appsrc_need_data_callback = None
@@ -93,21 +94,28 @@ def _setup_playbin(self):
93 94
         self._playbin = playbin
94 95
 
95 96
     def _on_about_to_finish(self, element):
96  
-        source, self._appsrc = self._appsrc, None
97  
-        if source is None:
98  
-            return
  97
+        # Cleanup appsrc related stuff.
  98
+        old_appsrc, self._appsrc = self._appsrc, None
  99
+
  100
+        self._disconnect(old_appsrc, 'need-data')
  101
+        self._disconnect(old_appsrc, 'enough-data')
  102
+        self._disconnect(old_appsrc, 'seek-data')
  103
+
99 104
         self._appsrc_caps = None
100 105
 
101  
-        self._disconnect(source, 'need-data')
102  
-        self._disconnect(source, 'enough-data')
103  
-        self._disconnect(source, 'seek-data')
  106
+        # Note that we can not let this function return until we have the next
  107
+        # URI set for gapless / EOS free playback. This means all the
  108
+        # subsequent remote calls to backends etc. down this code path need to
  109
+        # block.
  110
+        if self._end_of_track_callback:
  111
+            self._end_of_track_callback()
104 112
 
105 113
     def _on_new_source(self, element, pad):
106  
-        uri = element.get_property('uri')
107  
-        if not uri or not uri.startswith('appsrc://'):
  114
+        source = element.get_property('source')
  115
+
  116
+        if source.get_factory().get_name() != 'appsrc':
108 117
             return
109 118
 
110  
-        source = element.get_property('source')
111 119
         source.set_property('caps', self._appsrc_caps)
112 120
         source.set_property('format', b'time')
113 121
         source.set_property('stream-type', b'seekable')
@@ -280,6 +288,10 @@ def _on_end_of_stream(self):
280 288
         logger.debug('Triggering reached_end_of_stream event')
281 289
         AudioListener.send('reached_end_of_stream')
282 290
 
  291
+    def set_on_end_of_track(self, callback):
  292
+        """Set callback to be called on end of track."""
  293
+        self._end_of_track_callback = callback
  294
+
283 295
     def set_uri(self, uri):
284 296
         """
285 297
         Set URI of audio to be played.
21  mopidy/backends/base.py
@@ -88,8 +88,8 @@ def search(self, **query):
88 88
 
89 89
 class BasePlaybackProvider(object):
90 90
     """
91  
-    :param audio: the audio actor
92  
-    :type audio: actor proxy to an instance of :class:`mopidy.audio.Audio`
  91
+    :param audio: audio sub-system
  92
+    :type audio: actor proxy to a :class:`mopidy.audio.Audio` actor.
93 93
     :param backend: the backend
94 94
     :type backend: :class:`mopidy.backends.base.Backend`
95 95
     """
@@ -121,9 +121,24 @@ def play(self, track):
121 121
         :rtype: :class:`True` if successful, else :class:`False`
122 122
         """
123 123
         self.audio.prepare_change()
124  
-        self.audio.set_uri(track.uri).get()
  124
+        self.change_track(track)
125 125
         return self.audio.start_playback().get()
126 126
 
  127
+    def change_track(self, track):
  128
+        """
  129
+        Swith to provided track.
  130
+
  131
+        Used for handling of EOT and and in :meth:`play`.
  132
+
  133
+        *MAY be reimplemented by subclass.*
  134
+
  135
+        :param track: the track to play
  136
+        :type track: :class:`mopidy.models.Track`
  137
+        :rtype: :class:`True` if successful, else :class:`False`
  138
+        """
  139
+        self.audio.set_uri(track.uri).get()
  140
+        return True
  141
+
127 142
     def resume(self):
128 143
         """
129 144
         Resume playback at the same time position playback was paused.
25  mopidy/backends/spotify/playback.py
@@ -35,11 +35,9 @@ def __init__(self, *args, **kwargs):
35 35
         super(SpotifyPlaybackProvider, self).__init__(*args, **kwargs)
36 36
         self._first_seek = False
37 37
 
38  
-    def play(self, track):
39  
-        if track.uri is None:
40  
-            return False
41  
-
  38
+    def change_track(self, track):
42 39
         spotify_backend = self.backend.actor_ref.proxy()
  40
+
43 41
         need_data_callback_bound = functools.partial(
44 42
             need_data_callback, spotify_backend)
45 43
         enough_data_callback_bound = functools.partial(
@@ -49,21 +47,18 @@ def play(self, track):
49 47
 
50 48
         self._first_seek = True
51 49
 
  50
+        self.audio.set_appsrc(
  51
+            self._caps,
  52
+            need_data=need_data_callback_bound,
  53
+            enough_data=enough_data_callback_bound,
  54
+            seek_data=seek_data_callback_bound)
  55
+        self.audio.set_metadata(track)
  56
+
52 57
         try:
53 58
             self.backend.spotify.session.load(
54 59
                 Link.from_string(track.uri).as_track())
55  
-            self.backend.spotify.session.play(1)
56 60
             self.backend.spotify.buffer_timestamp = 0
57  
-
58  
-            self.audio.prepare_change()
59  
-            self.audio.set_appsrc(
60  
-                self._caps,
61  
-                need_data=need_data_callback_bound,
62  
-                enough_data=enough_data_callback_bound,
63  
-                seek_data=seek_data_callback_bound)
64  
-            self.audio.start_playback()
65  
-            self.audio.set_metadata(track)
66  
-
  61
+            self.backend.spotify.session.play(1)
67 62
             return True
68 63
         except SpotifyError as e:
69 64
             logger.info('Playback of %s failed: %s', track.uri, e)
7  mopidy/core/actor.py
@@ -46,6 +46,11 @@ def __init__(self, audio=None, backends=None):
46 46
 
47 47
         self.tracklist = TracklistController(core=self)
48 48
 
  49
+        if audio:
  50
+            # Hook up blocking on end of track handler to audio sub-system.
  51
+            audio.set_on_end_of_track(
  52
+                lambda: self.actor_ref.proxy().playback.on_end_of_track().get())
  53
+
49 54
     def get_uri_schemes(self):
50 55
         futures = [b.uri_schemes for b in self.backends]
51 56
         results = pykka.get_all(futures)
@@ -56,7 +61,7 @@ def get_uri_schemes(self):
56 61
     """List of URI schemes we can handle"""
57 62
 
58 63
     def reached_end_of_stream(self):
59  
-        self.playback.on_end_of_track()
  64
+        self.playback.on_end_of_stream()
60 65
 
61 66
     def state_changed(self, old_state, new_state):
62 67
         # XXX: This is a temporary fix for issue #232 while we wait for a more
14  mopidy/core/playback.py
@@ -314,6 +314,11 @@ def change_track(self, tl_track, on_error_step=1):
314 314
         elif old_state == PlaybackState.PAUSED:
315 315
             self.pause()
316 316
 
  317
+    def on_end_of_stream(self):
  318
+        self._trigger_track_playback_ended()
  319
+        self.state = PlaybackState.STOPPED
  320
+        self.current_tl_track = None
  321
+
317 322
     def on_end_of_track(self):
318 323
         """
319 324
         Tell the playback controller that end of track is reached.
@@ -325,11 +330,14 @@ def on_end_of_track(self):
325 330
 
326 331
         original_tl_track = self.current_tl_track
327 332
 
  333
+        # As noted in mopidy.audio which calls this code, we need to make sure
  334
+        # the calls to the backend are blocking or gapless / EOS free playback
  335
+        # will break.
328 336
         if self.tl_track_at_eot:
329 337
             self._trigger_track_playback_ended()
330  
-            self.play(self.tl_track_at_eot)
331  
-        else:
332  
-            self.stop(clear_current_track=True)
  338
+            self.current_tl_track = self.tl_track_at_eot
  339
+            self._get_backend().playback.change_track(self.current_track).get()
  340
+            self._trigger_track_playback_started()
333 341
 
334 342
         if self.consume:
335 343
             self.core.tracklist.remove(tlid=original_tl_track.tlid)
16  tests/backends/base/playback.py
@@ -115,6 +115,7 @@ def test_play_skips_to_next_track_on_failure(self):
115 115
     def test_current_track_after_completed_playlist(self):
116 116
         self.playback.play(self.tracklist.tl_tracks[-1])
117 117
         self.playback.on_end_of_track()
  118
+        self.playback.on_end_of_stream()
118 119
         self.assertEqual(self.playback.state, PlaybackState.STOPPED)
119 120
         self.assertEqual(self.playback.current_track, None)
120 121
 
@@ -343,6 +344,8 @@ def test_end_of_track_at_end_of_playlist(self):
343 344
 
344 345
             self.playback.on_end_of_track()
345 346
 
  347
+        self.playback.on_end_of_stream()
  348
+
346 349
         self.assertEqual(self.playback.state, PlaybackState.STOPPED)
347 350
 
348 351
     @populate_tracklist
@@ -351,6 +354,7 @@ def test_end_of_track_until_end_of_playlist_and_play_from_start(self):
351 354
 
352 355
         for _ in self.tracks:
353 356
             self.playback.on_end_of_track()
  357
+        self.playback.on_end_of_stream()
354 358
 
355 359
         self.assertEqual(self.playback.current_track, None)
356 360
         self.assertEqual(self.playback.state, PlaybackState.STOPPED)
@@ -364,16 +368,6 @@ def test_end_of_track_for_empty_playlist(self):
364 368
         self.assertEqual(self.playback.state, PlaybackState.STOPPED)
365 369
 
366 370
     @populate_tracklist
367  
-    def test_end_of_track_skips_to_next_track_on_failure(self):
368  
-        # If backend's play() returns False, it is a failure.
369  
-        self.backend.playback.play = lambda track: track != self.tracks[1]
370  
-        self.playback.play()
371  
-        self.assertEqual(self.playback.current_track, self.tracks[0])
372  
-        self.playback.on_end_of_track()
373  
-        self.assertNotEqual(self.playback.current_track, self.tracks[1])
374  
-        self.assertEqual(self.playback.current_track, self.tracks[2])
375  
-
376  
-    @populate_tracklist
377 371
     def test_end_of_track_track_before_play(self):
378 372
         self.assertEqual(self.playback.tl_track_at_next, self.tl_tracks[0])
379 373
 
@@ -515,6 +509,7 @@ def test_tracklist_position_after_next(self):
515 509
     def test_tracklist_position_at_end_of_playlist(self):
516 510
         self.playback.play(self.tracklist.tl_tracks[-1])
517 511
         self.playback.on_end_of_track()
  512
+        self.playback.on_end_of_stream()
518 513
         self.assertEqual(self.playback.tracklist_position, None)
519 514
 
520 515
     def test_on_tracklist_change_gets_called(self):
@@ -820,6 +815,7 @@ def test_end_of_song_with_single_and_repeat_starts_same(self):
820 815
     def test_end_of_playlist_stops(self):
821 816
         self.playback.play(self.tracklist.tl_tracks[-1])
822 817
         self.playback.on_end_of_track()
  818
+        self.playback.on_end_of_stream()
823 819
         self.assertEqual(self.playback.state, PlaybackState.STOPPED)
824 820
 
825 821
     def test_repeat_off_by_default(self):
2  tests/backends/base/tracklist.py
@@ -17,7 +17,7 @@ class TracklistControllerTest(object):
17 17
     def setUp(self):
18 18
         self.audio = audio.DummyAudio.start().proxy()
19 19
         self.backend = self.backend_class.start(audio=self.audio).proxy()
20  
-        self.core = core.Core(audio=audio, backends=[self.backend])
  20
+        self.core = core.Core(audio=self.audio, backends=[self.backend])
21 21
         self.controller = self.core.tracklist
22 22
         self.playback = self.core.playback
23 23
 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.