mopidy / mopidy-spotify

Mopidy extension for playing music from Spotify
https://mopidy.com/ext/spotify/
Apache License 2.0
937 stars 109 forks source link

Possible race in timestamp handling #88

Closed adamcik closed 1 month ago

adamcik commented 8 years ago

I have a hunch that the music delivery callback and the seek callbacks can be interleaved in such a way that the buffer timestamp will be wrong. Likely we need to make these two function lock each other out.

Using the following patch and dragging the progress bar back and forth in mopidy-mobile I managed to get:

mopidy.audio.actor Timestamp did not match: got 150432000000, expected 143864000000

Which indicates there is a chance that there is race in the code, though possibly not the one I am looking for.

diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py
index 92ccb44..ea1dab4 100644
--- a/mopidy/audio/actor.py
+++ b/mopidy/audio/actor.py
@@ -57,6 +57,7 @@ class _Appsrc(object):
         self._need_data_callback = need_data
         self._seek_data_callback = seek_data
         self._enough_data_callback = enough_data
+        self._expected_timestamp = 0

     def configure(self, source):
         """Configure the supplied source for use.
@@ -73,7 +74,7 @@ class _Appsrc(object):
             self._signals.connect(source, 'need-data', self._on_signal,
                                   self._need_data_callback)
         if self._seek_data_callback:
-            self._signals.connect(source, 'seek-data', self._on_signal,
+            self._signals.connect(source, 'seek-data', self._on_seek_data,
                                   self._seek_data_callback)
         if self._enough_data_callback:
             self._signals.connect(source, 'enough-data', self._on_signal, None,
@@ -91,7 +92,20 @@ class _Appsrc(object):
             return result == Gst.FlowReturn.OK
         else:
             result = self._source.emit('push-buffer', buffer_)
-            return result == Gst.FlowReturn.OK
+            if result != Gst.FlowReturn.OK:
+                return False
+
+            if self._expected_timestamp != buffer_.pts:
+                logger.error('Timestamp did not match: got %s, expected %s',
+                             buffer_.pts, self._expected_timestamp)
+            self._expected_timestamp = buffer_.pts + buffer_.duration
+            return True
+
+    def _on_seek_data(self, element, clocktime, func):
+        self._expected_timestamp = clocktime
+        logger.warning('Expecting next timestamp to be: %s', clocktime)
+        func(utils.clocktime_to_millisecond(clocktime))
+        return True

     def _on_signal(self, element, clocktime, func):
         # This shim is used to ensure we always return true, and also handles
kingosticks commented 1 month ago

appsrc is now gone.