Move player logic out of main #513

JasonLG1979 · 2017-09-13T23:35:15Z

Still a wip at the moment but I wanted to put in the PR so we could polish it up. currently this PR creates a custom player pipeline and abstracts as much out as possible as far as I can tell. It removes pretty much all gstreamer code from pithos.py. The proxy support and auto plugin install still needs tested.

TingPing · 2017-09-13T23:35:19Z

pithos/GstPlayer.py

+    _desired_state = PseudoGst.STOPPED
+    _actual_state = PseudoGst.STOPPED
+    _buffering_timer_id = 0
+    _update_proxy = True       


[flake8]

[W291] trailing whitespace

TingPing · 2017-09-13T23:35:20Z

pithos/GstPlayer.py

+        self._settings = settings
+        self._settings.connect('changed::proxy', lambda *ignore: setattr(self, '_update_proxy', True))
+
+        elements = [        


[flake8]

[W291] trailing whitespace

TingPing · 2017-09-13T23:35:20Z

pithos/GstPlayer.py

+        return GstPbutils.install_plugins_installation_in_progress()
+
+    def query_position(self):
+      if self.query(self.__query_position):


[flake8]

[E111] indentation is not a multiple of four

TingPing · 2017-09-13T23:35:21Z

pithos/GstPlayer.py

+
+    def query_position(self):
+      if self.query(self.__query_position):
+          return self.__query_position.parse_position()[1]


[flake8]

[E111] indentation is not a multiple of four

TingPing · 2017-09-13T23:35:21Z

pithos/GstPlayer.py

+    def query_position(self):
+      if self.query(self.__query_position):
+          return self.__query_position.parse_position()[1]
+      else:


[flake8]

[E111] indentation is not a multiple of four

TingPing · 2017-09-13T23:35:21Z

pithos/GstPlayer.py

+      if self.query(self.__query_position):
+          return self.__query_position.parse_position()[1]
+      else:
+          return 0


[flake8]

[E111] indentation is not a multiple of four

TingPing · 2017-09-13T23:35:22Z

pithos/GstPlayer.py

+        self._set_player_state(PseudoGst.STOPPED, change_gst_state=True)
+
+    def _query_duration(self):
+      if self.query(self.__query_duration):


[flake8]

[E111] indentation is not a multiple of four

TingPing · 2017-09-13T23:35:22Z

pithos/GstPlayer.py

+
+    def _query_duration(self):
+      if self.query(self.__query_duration):
+          return self.__query_duration.parse_duration()[1]


[flake8]

[E111] indentation is not a multiple of four

TingPing · 2017-09-13T23:35:22Z

pithos/GstPlayer.py

+        if GstPbutils.is_missing_plugin_message(message):
+            if GstPbutils.install_plugins_supported():
+                details = GstPbutils.missing_plugin_message_get_installer_detail(message)
+                GstPbutils.install_plugins_async([details,], None, self.on_gst_plugin_installed, None)


[flake8]

[E231] missing whitespace after ','

TingPing · 2017-09-13T23:35:23Z

pithos/GstPlayer.py

+                    _('Missing codec'),
+                    None,
+                    _('GStreamer is missing a plugin and it could not be automatically installed.'
+                            '\nEither manually install it or try another quality setting.'),


[flake8]

[E127] continuation line over-indented for visual indent

TingPing · 2017-09-13T23:35:23Z

pithos/pithos.py

 gi.require_version('GstPbutils', '1.0')
-from gi.repository import Gst, GstAudio, GstPbutils, GObject, Gtk, Gdk, Pango, GdkPixbuf, Gio, GLib
+from gi.repository import Gst, GstPbutils, GObject, Gtk, Gdk, Pango, GdkPixbuf, Gio, GLib


[flake8]

[F401] 'gi.repository.GstPbutils' imported but unused

TingPing · 2017-09-13T23:41:37Z

pithos/pithos.py

        # Edge case. We might get this singal while we're reconnecting to Pandora.
        # If so self.current_song will be None.
        if self.current_song is None:
            return
        # Fallback to using song.trackLength which is in seconds and converted to nanoseconds
-        self.current_song.duration = self.query_duration() or self.current_song.trackLength * Gst.SECOND
+        self.current_song.duration = duration or self.current_song.trackLength * Gst.SECOND


[flake8]

[F405] 'Gst' may be undefined, or defined from star imports: .pandora, .pandora.data

TingPing · 2017-09-16T13:02:33Z

pithos/GstPlayer.py

+        self.props.proxy_pw = password
+        logging.debug(
+            '\nSetting GstPlayer Proxy:'
+                '\nHTTP proxy server URI: {}'


[Flake8]

continuation line unaligned for hanging indent

TingPing · 2017-09-17T10:48:55Z

pithos/GstPlayer.py

+        elements[1].props.expose_all_streams = False
+        elements[2].props.use_buffering = True
+        elements[2].props.max_size_bytes = 0
+        elements[2].props.max_size_time = MAX_SIZE_TIME 


[Flake8]

trailing whitespace

TingPing · 2017-09-17T10:48:55Z

pithos/GstPlayer.py

+            lambda *ignore: self.emit('eos'),
+        )
+        self.bus.connect(
+            'message::error', 


[Flake8]

trailing whitespace

JasonLG1979 · 2017-09-21T14:19:41Z

@TingPing, I started over and switched back to just using playbin. I was getting random memory leaks with the custom pipeline?

TingPing · 2017-09-22T01:38:11Z

Maybe make a minimal reproducer and figure it out with upstream. Though if its a PyGObject problem that might be messier to figure out. I'm sure you wouldn't be interested in it but if the results are better using C is always acceptable IMO.

JasonLG1979 · 2017-09-22T07:23:26Z

So how would we do that? Write the player in C and write our own python bindings for it?

JasonLG1979 · 2017-09-22T07:26:18Z

My theory is that the buffer isn't always getting properly cleared. If you let songs finish on their own for the most part there's no real leak. But if you skip songs manually it will just eat memory most of the time.

JasonLG1979 · 2017-09-22T07:28:42Z

Playbin seems to workaround this because it generates a new pipeline for every song and I assume unrefs the entire old pipeline?

TingPing · 2017-10-01T15:22:53Z

pithos/GstPlayer.py

+        for prop, value, in props.items():
+            if value is not None and hasattr(soup, prop):
+                setattr(soup, prop, value)
+


[Flake8]

blank line at end of file

TingPing · 2017-10-08T18:55:35Z

pithos/GstPlayer.py

+                self,
+                'eq-band{}'.format(i),
+                GObject.BindingFlags.BIDIRECTIONAL | GObject.BindingFlags.SYNC_CREATE,
+        )


[Flake8]

continuation line missing indentation or outdented

TingPing · 2017-10-08T18:55:36Z

pithos/GstPlayer.py

+        self._set_player_state(PseudoGst.PAUSED)
+
+    def stop(self):
+        self._prerolled = False      


[Flake8]

trailing whitespace

TingPing · 2017-10-08T19:28:45Z

pithos/GstPlayer.py

+        # check to see if we're buffering. (we more than likely am)
+        if not self._prerolled:
+            self._prerolled = True
+            self._on_gst_buffering()        


[Flake8]

trailing whitespace

JasonLG1979 · 2017-10-08T19:30:59Z

@TingPing I think I figured out the memory leak. In my previous pipeline I had the queue2 element after the decodbin element. Moving it in front of decodebin seems to fixed it.

Also I think I found out why download buffering was locking up. In a pipeline it's possible to get buffering messages before the async-done message. The 1st async-done message going from NULL to PAUSED means that all elements in the pipeline have completed the state change and the pipeline is prerolled. In our case I think we were trying to go to PLAYING before the pipeline was prerolled every once in a great while. In this new pipeline I ignore all buffering messages until the pipeline is prerolled.

I'll do some testing but we may be able to add an option to enable download buffering as an option. an added benefit to the custom pipeline is that we can set the audio to download to the same folder as the cover art.

TingPing · 2017-10-20T15:22:29Z

pithos/pithos.py

+        try:
+            self.tempdir = tempfile.TemporaryDirectory(prefix='pithos-',
+                                                       dir=GLib.get_user_cache_dir())
+            logging.info("Created temporary directory %s" %self.tempdir.name)


[Flake8]

missing whitespace around operator

TingPing · 2017-10-26T12:25:32Z

pithos/GstPlayer.py

+        blurb='actual state of GstPlayer PseudoGst Enum',
+        flags=GObject.ParamFlags.READABLE,
+    )
+    def actual_state(self): 


[Flake8]

trailing whitespace

TingPing · 2017-10-26T12:25:32Z

pithos/GstPlayer.py

+        return GstPbutils.install_plugins_installation_in_progress()
+
+    def query_position(self):
+        if self.query(self.__query_position):          


[Flake8]

trailing whitespace

TingPing · 2017-10-26T12:25:33Z

pithos/GstPlayer.py

+
+    def _query_duration(self):
+        if self.query(self.__query_duration):
+            return self.__query_duration.parse_duration()[1] 


[Flake8]

trailing whitespace

JasonLG1979 · 2017-10-26T13:04:21Z

@TingPing I think I'm pretty much done with this if you could take a look at it please? It does use the downloadbuffer element but in using it myself over the last couple weeks it seems that whatever the problem was either has been resolved and/or my usage of the element is more "correct"?

In any case we could provide a pref to disable it for users of older Gstreamer versions if it causes them issues. It wouldn't be hard. My pipeline already falls back to stream buffering if the cache folder couldn't be created. (audio is cached to the same folder as the art).

JasonLG1979 · 2017-11-08T17:12:52Z

Yes but the volume shouldn't differ between the current player.

All things being equal I haven't noticed a difference? I'll do a quick test to make sure though...

JasonLG1979 · 2017-11-08T17:22:33Z

Nope, I notice no difference. It's really hard to say one way or another without hearing what you're hearing but there's nothing really different going on with the volume. AFAIK the volume element is used for volume in playbin? It's also hard to measure objectively by ear the subjective apparent volume based on different songs?

TingPing · 2017-11-09T01:08:03Z

Toggling the normalization plugin isn't the cause btw. But it isn't a subtle difference, 100% on this branch is about as loud as 50% on master.

JasonLG1979 · 2017-11-09T08:37:27Z

Does the volume in the system audio settings slider match the volume controls in Pithos?

JasonLG1979 · 2017-11-09T08:43:18Z

Another option is to use pulsesink directly instead of autoaudiosink. Pulsesink has built in volume and since it's designed for pulseaudio it should work perfectly. It would also allow us to set the pulseprops.

JasonLG1979 · 2017-11-09T15:09:11Z

As a side note to my previous comment, we already assume pulseaudio in our volume value conversions.

TingPing · 2017-11-09T16:23:25Z

Does the volume in the system audio settings slider match the volume controls in Pithos?

Nope, master does but not this branch. So perhaps it is using a different audio backend and not pulseaudio.

JasonLG1979 · 2017-11-09T17:24:28Z

So perhaps it is using a different audio backend and not pulseaudio.

PulseAudio pretty much gobbles up all audio. The only other audio backend really is ALSA and it's not really a backend per say but the layer under Pulseaudio right above the hardware. PulseAudio even goes so far to pretend to be ALSA so that applications that aren't PulseAudio aware(not many these days) work with PulseAudio. The effect is that you have to be trying pretty hard to talk directly to ALSA otherwise you'll get Pulseaudio.

The autoaudiosink element will automatically create and use pulsesink if it detects PulseAudio otherwise it will use alsasink. pulsesink as I said has volume built in, alsasink does not. That's why I have a volume element in the pipeline.

TingPing · 2017-11-09T17:31:18Z

Right, I meant maybe its incorrectly loading alsasink here or something. I'm sure somebody would be mad if we depended upon Pulseaudio but I don't know if we should care.

JasonLG1979 · 2017-11-09T17:32:17Z

I'm sure somebody would be mad if we depended upon Pulseaudio but I don't know if we should care

It's not hard to detect pulse and act accordingly.

JasonLG1979 · 2017-11-09T17:34:45Z

adapted from quodlibet:

    def _pulse_is_running(self):
        # If we have a pulsesink we can get the server presence through
        # setting the ready state
        element = Gst.ElementFactory.make('pulsesink', None)
        if element is not None:
            element.set_state(Gst.State.READY)
            res = element.get_state(0)[0]
            element.set_state(Gst.State.NULL)
            return res != Gst.StateChangeReturn.FAILURE
        return False

JasonLG1979 · 2017-11-09T17:38:10Z

If pulse is running we use pulsesink with it's pulse friendly built in volume. If not we use a separate volume and autoaudiosink binding the player's volume prop accordingly. The rest of Pithos would be none the wiser.

JasonLG1979 · 2017-11-09T18:12:56Z

There @TingPing test the branch out again and see if this fixes your volume issue. The latest changes are:

No download buffering.

Auto detect pulseaudio.

TingPing · 2017-11-09T18:18:53Z

Worked

JasonLG1979 · 2017-11-09T18:27:38Z

Ok, I got a little bit a tweaking to do and it'll be ready I think.

TingPing · 2017-11-09T18:34:41Z

I would like to get a 1.4.1 bugfix release out soon (will wait for a fix the mediakeys issue I commented on) and then we can merge this and work towards 1.5.0.

JasonLG1979 · 2017-11-09T18:37:01Z

OK. I'm fine with that. I'd like 1.5 to be a pretty big release. This, UI redesign and main logic clean up.

JasonLG1979 · 2017-11-10T13:28:26Z

@TingPing, on the subject of download buffering; what about just doing a version check?

Something like:

GST_MAJOR_VERSION, GST_MINOR_VERSION = Gst.version()[0:2]
_USE_DOWNLOADBUFFER = GST_MAJOR_VERSION > 1 or GST_MINOR_VERSION > 11

I really don't want to test every version of Gstreamer 1.x to see where it's fixed, but I know that download buffering in 1.12 works. This seems to solve your objection of exposing internal details?

TingPing · 2017-11-10T22:58:46Z

I really don't want to test every version of Gstreamer 1.x to see where it's fixed, but I know that download buffering in 1.12 works.

I believe due to our build dependencies we are only installable on distros with GStreamer 1.8 and newer anyway.

If you really don't want to test that anyway I suppose I can live with this.

JasonLG1979 · 2017-11-11T23:35:53Z

I believe due to our build dependencies we are only installable on distros with GStreamer 1.8 and newer anyway.

I know it's broken in 1.8.x, at least on Ubuntu 16.04. That really only leaves 1.10.x. Which comes with the current Debian stable and Ubuntu 17.04 neither of which I really care about or care to test.(one being crusty and the other only supported for 9 months and not current) It will work with flatpak and non-stagnant distros. I'd like to make a reasonable effort to not break Pithos on crusty distros but my thought is that I really don't want to be held back by them either on the master branch. If a user wants the latest and greatest version of Pithos they can install the flatpak otherwise they can install the version that comes with their distro of choice. If they aren't happy with that version they need to talk to the Pithos package maintainer for that distro about backporting bug fixes and features and dealing with dependencies.

TingPing · 2017-11-11T23:47:10Z

pithos/GstPlayer.py

+        duration = self._query_duration()
+        if duration or final:
+            self._got_duration = True
+            self.emit('new-stream-duration', duration) 


[Flake8]

trailing whitespace

TingPing · 2017-11-11T23:47:10Z

pithos/pithos.py

+        try:
+            self.tempdir = tempfile.TemporaryDirectory(prefix='pithos-',
+                                                       dir=GLib.get_user_cache_dir())
+            logging.info("Created temporary directory %s" %self.tempdir.name)


[Flake8]

missing whitespace around operator

JasonLG1979 · 2017-11-13T15:05:42Z

Well @TingPing, I've been abusing the pipeline for the last couple days by changing songs machine gun style and even in Gstreamer 1.12 I've been able to lockup the downloadbuffer a couple times so download buffering is still out :'(

On the up side I've been able to improve other aspects of the pipeline. Here's a rundown on what I've got so far:

Short circuit typefinding in decodebin. We can get the file extension from the url and force the stream caps if it's a known file type/extension. Currently Pandora only streams AAC's (caps = 'application/x-3gp, profile=(string)basic') and MP3's (caps = 'audio/mpeg, mpegversion=(int)1, layer=(int)3, parsed=(boolean)false') so we don't need decodebin to waste time every song doing a typefind. In the case of mp3 streams it also allows us to skip adding the id3demux element to decodebin. typefind reports the caps of mp3 streams as 'application/x-id3' more often than not because it's reading the tags at the beginning of the MP3 leading to the addition of id3demux to the pipeline. As Pandora streams contain no useful tags there's no point in adding it. If for some reason in the future Pandora decides to stream something other than AAC's or MP3's typefind would work in the usual default manner. The net effect is that prerolling is much faster so songs start sooner.
Cache a list of element factories to be used in the 'autoplug-factories' signal handler(qtdemux, mpegaudioparse, a mp3 decoder and a aac decoder) and optimize the order of the mp3 elements. mpegaudioparse's sink caps are 'audio/mpeg, mpegversion=(int)1', so it will match both parsed and unparsed mp3 stream caps. mpg123audiodec and flump3dec sink caps do not match unparsed mp3 streams. We want them before mpegaudioparse so that for parsed streams they are tried 1st and not mpegaudioparse. avdec_mp3* and mad(?) sink caps also match both parsed and unparsed mp3 stream caps so mpegaudioparse must always be before them otherwise they will be plugged to unparsed streams and break the pipeline. This speeds up the autoplugging process(and thus prerolling), and stops aacparse from being added to decodebin for aac's unnecessary. If again for some reason Pandora decides to stream something other than AAc's or MP3's autoplugging would work in the usual default manner.
Dynamically switch between souphttpsrc and giosrc for our source element depending upon if we need proxy support. giosrc uses less resources (around 10MB less RAM or so on my system) then souphttpsrc and seems to be faster in general at loading songs. Again songs start sooner.(with giosrc anyway)
The above changes have allowed for a drastic increase in the buffer size(from 3 to 10 secs) while not increasing the total delay between loading a url and the beginning of playback since more time can be spent actually buffer before the song begins instead of wasting time prerolling. This should greatly decrease the amount of buffer "fluttering" and improve network resilience(although not as much as download buffering would have :/ grumble, grumble).
On the fly song quality changes. Not a performance increase but a neat feature none the less. Pretty much what it sounds like. If you change the quality in the prefs the stream is changed in place and seeked to where you left off.

JasonLG1979 · 2017-11-13T15:13:34Z

On a side note it might be nice to expose the buffer size in secs as a user pref. I know you're not a fan of exposing internal details but I don't think it's unreasonable to expose a buffer size setting for a network streaming app. It would allow the user to find the sweet spot for their connection. A reasonable default could be between 3 and 5 secs and a reasonable max could be 10 secs and reasonable min would be 2.

TingPing · 2017-11-14T22:29:06Z

Flake8 detected 11 issues on 772c6e3
Visit https://sideci.com/gh/4059604/pull_requests/513 to review the issues.

As described in #513 (comment)

TingPing · 2018-03-19T13:34:01Z

@JasonLG1979 Maybe worth looking into: http://base-art.net/Articles/gstreamers-playbin3-overview-for-application-developers/

TingPing reviewed Sep 13, 2017

View reviewed changes

TingPing reviewed Sep 16, 2017

View reviewed changes

TingPing reviewed Sep 17, 2017

View reviewed changes

TingPing reviewed Oct 1, 2017

View reviewed changes

TingPing reviewed Oct 8, 2017

View reviewed changes

TingPing reviewed Oct 20, 2017

View reviewed changes

TingPing reviewed Oct 26, 2017

View reviewed changes

TingPing reviewed Nov 11, 2017

View reviewed changes

JasonLG1979 mentioned this pull request Nov 15, 2017

Default to High Quality instead of Medium #538

Closed

JasonLG1979 added 3 commits November 26, 2017 13:21

Move the Player logic of the main class

885958f

Replace playbin with a mostly static custom Gst.Pipeline

fddb7db

Optimize pipeline and allow for on the fly stream quality changes.

148103f

As described in #513 (comment)

Move player logic out of main #513

Are you sure you want to change the base?

Move player logic out of main #513

Conversation

JasonLG1979 commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonLG1979 commented Sep 21, 2017 • edited

TingPing commented Sep 22, 2017

JasonLG1979 commented Sep 22, 2017

JasonLG1979 commented Sep 22, 2017

JasonLG1979 commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonLG1979 commented Oct 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonLG1979 commented Oct 26, 2017

JasonLG1979 commented Nov 8, 2017 • edited

JasonLG1979 commented Nov 8, 2017

TingPing commented Nov 9, 2017

JasonLG1979 commented Nov 9, 2017

JasonLG1979 commented Nov 9, 2017

JasonLG1979 commented Nov 9, 2017

TingPing commented Nov 9, 2017

JasonLG1979 commented Nov 9, 2017

TingPing commented Nov 9, 2017

JasonLG1979 commented Nov 9, 2017

JasonLG1979 commented Nov 9, 2017

JasonLG1979 commented Nov 9, 2017

JasonLG1979 commented Nov 9, 2017

TingPing commented Nov 9, 2017

JasonLG1979 commented Nov 9, 2017

TingPing commented Nov 9, 2017

JasonLG1979 commented Nov 9, 2017

JasonLG1979 commented Nov 10, 2017

TingPing commented Nov 10, 2017

JasonLG1979 commented Nov 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonLG1979 commented Nov 13, 2017

JasonLG1979 commented Nov 13, 2017

TingPing commented Nov 14, 2017

TingPing commented Mar 19, 2018

JasonLG1979 commented Sep 21, 2017 •

edited

JasonLG1979 commented Nov 8, 2017 •

edited

JasonLG1979 commented Nov 11, 2017 •

edited