Banshee.GStreamer: Deinit gst before shutdown #2
Conversation
There was a problem hiding this comment.
Looks good, I'm all for proper resource disposal 👍
EDIT: Just one more thing - if we hope to get things merged upstream (eventually) we'll need to follow GNOME's commit message guidelines, in our case I think that just means switching out the project name you've used as a tag, gstreamer or even just gst would be fine.
| @@ -66,6 +66,7 @@ typedef void (* BansheeLogHandler) (BansheeLogType type, const gchar *component, | |||
|
|
|||
| MYEXPORT void | |||
| gstreamer_initialize (gboolean debugging, BansheeLogHandler log_handler); | |||
| void gstreamer_deinitialize (); | |||
There was a problem hiding this comment.
I think this will need MYEXPORT as above for the initialize function - I don't think its lack will bother us on GNU systems but maybe the windows folk need it?
| return; | ||
| } | ||
|
|
||
| bp_debug ("Deinitializing GStreamer"); |
There was a problem hiding this comment.
Any reason not to use banshee_log_debug below?
There was a problem hiding this comment.
Right. Didn't pay enough attention when reading the code. 😄
| @@ -209,9 +209,11 @@ protected override void Initialize () | |||
|
|
|||
| public override void Dispose () | |||
| { | |||
| bp_stop (handle, true); | |||
There was a problem hiding this comment.
Yes. gst_deinit blocks until all GStreamer tasks are finished, so we need to stop playback before calling.
There was a problem hiding this comment.
I'm still unsure we need this one since it appears to be called from bp_destroy, with this line commented out I still get this log when closing the program:
[1 Debug 15:01:55.078] (libbanshee:player) bp_stop: setting state to GST_STATE_NULL
[1 Debug 15:01:55.084] Player state change: Playing -> Idle
[1 Debug 15:01:55.087] (libbanshee:player) bp_destroy: disposed player
[1 Debug 15:01:55.087] (libbanshee:gst) Deinitializing GStreamer
[1 Debug 15:01:55.089] Service disposed (PlayerEngine)
[1 Debug 15:01:55.098] Service disposed (SourceManager)
[1 Debug 15:01:55.101] Service disposed (DbConnection)
EDIT: hmm, not from bp_destroy (I just looked) but definitely from somewhere.
EDIT2: found it... the base.Dispose call is calling back out to the Close function here which executes bp_stop - bit weird but let's leave that as is for now.
There was a problem hiding this comment.
It's called from PlayerEngine.Close. I had the deinit call previously somewhere else where it didn't work without bp_stop.
I'll remove the call.
Technically this isn't really necessary but it helps to keep valgrind logs cleaner
| @@ -39,6 +39,7 @@ | |||
| #include <gst/pbutils/pbutils.h> | |||
|
|
|||
| #include "banshee-gst.h" | |||
| #include "banshee-player-private.h" | |||
There was a problem hiding this comment.
Just one last thing, I don't think we need this line :)
|
Thanks. Still trying to get accustomed to the code base, so excuse me for the lengthy discussion. On a side note: GitHub has the feature to squash the commits directly during the merge. Look at the arrow on the merge button. |
|
Knew it - thing put the pull request title in the commit message for the squash. I really hate that button - just force-pushed the right commit (4f39dd5) for this back out. |
|
Don't worry about going through a bit of discussion - that's what issues and pull requests are for, thanks again for the input 👍 |
Should I create an issue to track this? |
|
Perhaps, yes, I noticed you'd removed the GStreamerSharp backend - I think your message said the bindings library was unmaintained |
|
Exactly. It's unmaintained, doesn't ship with any large distro and it couldn't play a track. |
|
Agreed, did you get that behaviour even with their master branch? I remember experimentally trying it out - had to make some changes to Banshee to get it to compile, see bb1ac56, think I tried it out and it was working at the time. |
Technically this isn't really necessary but it help to keep valgrind
logs cleaner