GNOME Bugzilla – Bug 590768
GstPulseSrc should allow swapping the device used by the stream
Last modified: 2013-08-22 19:17:43 UTC
It should be able to change the device used by a GstPulseSrc, even when the stream is playing.
Created attachment 139890 [details] [review] Proposed patch By the way, I've made a typo in the bug description. I should have written "It should be possible..." not "It should be able..." :)
Created attachment 139896 [details] [review] Corrected patch Tester made me realize that some locking was needed.
What do you need the pa_stream for? "Changing device of stream %d to %s", pulsesrc->device, stream_index); ^^^^^^^^ this is bogus. Also, perhaps pulsesrc->device should be updated only when the moving is successful, dunno.
> What do you need the pa_stream for? An application that is using this new functionality will most likely want to interact with the pa_stream in order to work properly. For instance, it might want to call pa_stream_set_moved_callback() to see if the device switching went well. Exposing the pa_stream is the simplest way I found of addressing such an issue, but I'm open to suggestions. > "Changing device of stream %d to %s", pulsesrc->device, stream_index); > ^^^^^^^^ > this is bogus. Well, it was useful to me when I was debugging, but I can always remove it. > Also, perhaps pulsesrc->device should be updated only when the moving is > successful, dunno. Yeah, that makes sense. I'll change that too.
Created attachment 139956 [details] [review] Corrected patch
(In reply to comment #4) > > What do you need the pa_stream for? > > An application that is using this new functionality will most likely want to > interact with the pa_stream in order to work properly. For instance, it might > want to call pa_stream_set_moved_callback() to see if the device switching went > well. Exposing the pa_stream is the simplest way I found of addressing such an > issue, but I'm open to suggestions. > Right. Maybe you should have split the patch and say that. > > "Changing device of stream %d to %s", pulsesrc->device, stream_index); > > ^^^^^^^^ > > this is bogus. > > Well, it was useful to me when I was debugging, but I can always remove it. > That was not the point, it was about %d and %s being in wrong order. > > Also, perhaps pulsesrc->device should be updated only when the moving is > > successful, dunno. > > Yeah, that makes sense. I'll change that too. > I don't know if it's a common way in gstreamer - perhaps properties are just not the right interface for that. You could avoid the double dup() and the lock() in case pulsesrc->stream is NULL. I would do it the same way it was done before. otherwise, the patch looks ok to me
Created attachment 140662 [details] [review] Fixed patch > That was not the point, it was about %d and %s being in wrong order. Oh, I see! Well actually, it's the parameters who were in the wrong order. I've put it back then. > You could avoid the double dup() and the lock() in case pulsesrc->stream is > NULL. I would do it the same way it was done before. I don't know if I understand you correctly, but I don't think we can avoid the lock(). What if there's a race condition in which one thread is creating the stream and the other one is checking if it's NULL? Regarding the double dup(), I've made some small modification which gets rid of it.
I also think you should put the new property into a separate patch. Using g_param_spec_pointer() is quite nasty for bindings btw, we try to not use that at all. You could post a message on the bus to let the app know that the device change went well or not (and/or a warning message if it didn't).
I would leave out the property part for now too. Jonathan, any chance for spitted patch.
Jonathan, will you be able to provide patches in the format requested in comment#8 and comment#9 ?
Jonathan is not working with us since a while. Someone has to take over this bug/patch.
Anybody still interested in this or let's just close this?
I think I saw a patch by Olivier recently that does exactly this.
Created attachment 252644 [details] [review] pulsesrc: Implement changing the device while playing
Created attachment 252645 [details] [review] pulsesrc: De-duplicate code to get the current source output info
Created attachment 252646 [details] [review] pulsesrc: Add property to find out the device currently in use
I happened to implement this while doing device monitors. I propose adding a "current-device" property as the device change change from under our feet at any time. So the "device" property is the desired device, while "current-device" is the device actually in use.
Review of attachment 252644 [details] [review]: Maybe the same for pulsesink?
Committed, pulsesink will follow commit 691b04e5c949ba85aa8e6dc8f5259017c4f652af Author: Olivier Crête <olivier.crete@collabora.com> Date: Sun Aug 18 23:32:22 2013 -0400 pulsesrc: Add property to find out the device currently in use https://bugzilla.gnome.org/show_bug.cgi?id=590768 commit d56f4718c2349adade11adb0400e2ab8679fce7f Author: Olivier Crête <olivier.crete@collabora.com> Date: Sun Aug 18 23:31:15 2013 -0400 pulsesrc: De-duplicate code to get the current source output info https://bugzilla.gnome.org/show_bug.cgi?id=590768 commit c3642e3ecf925e24362e09d901d46ac9b0faacd3 Author: Olivier Crête <olivier.crete@collabora.com> Date: Sun Aug 18 22:27:37 2013 -0400 pulsesrc: Implement changing the device while playing https://bugzilla.gnome.org/show_bug.cgi?id=590768
And for pulsesink commit e00b8f0a4abbffc9b6597848d95406f4d519592c Author: Olivier Crête <olivier.crete@collabora.com> Date: Sun Aug 18 23:32:22 2013 -0400 pulsesink: Add property to find out the device currently in use https://bugzilla.gnome.org/show_bug.cgi?id=590768 commit d379e237c1b4366111cddab718a554d87bcaf779 Author: Olivier Crête <olivier.crete@collabora.com> Date: Sun Aug 18 23:31:15 2013 -0400 pulsesink: De-duplicate code to get the current sink input info https://bugzilla.gnome.org/show_bug.cgi?id=590768 commit 8f9fbfa99297bc54a20d964d5067490fd167b22f Author: Olivier Crête <olivier.crete@collabora.com> Date: Sun Aug 18 22:27:37 2013 -0400 pulsesink: Implement changing the device while playing https://bugzilla.gnome.org/show_bug.cgi?id=590768