GNOME Bugzilla – Bug 596164
Totem forces pulseaudiosink
Last modified: 2009-11-27 13:25:03 UTC
Since commit a365d50c19b900911d7fe1bfdb24d22f8ab5bc98 it's impossible to use gconf configured audio sink (gstreamer-properties) - pulse audio sink is always chosen if available. It's confusing and in some cases - undesirable.
I am affected by this bug. Totem can't play audio on my system because I don't use PulseAudio. All other applications which uses gstreamer works fine. I'm using the Ubuntu 9.10 with OSSv4.
Created attachment 144775 [details] [review] Try to use the GConf audiosink rather than PA I wonder why Totem now tries to use PA directly, instead of just using the preferred audiosink defined in GConf. That doesn't make much sense to me. The attached patch stops trying to use pulseaudio over the preferred audiosink (I didn't fix the indentation to make it clearer where the changes are).
Except that will break the code a bit lower down when we check for the "volume" property on the audio-sink. You'd need to use gst_bin_get_by_name() to find whether the bin includes an element with a volume property.
(In reply to comment #3) > Except that will break the code a bit lower down when we check for the "volume" > property on the audio-sink. How come? Do you mean this code? + /* If we're using a sink that has a volume property, then that's what + * we need to modify, not playbin's one */ + if (g_object_class_find_property (G_OBJECT_GET_CLASS (audio_sink), "volume")) { + bvw->priv->pulse_audio_sink = g_object_ref (audio_sink); + g_signal_connect (G_OBJECT (bvw->priv->pulse_audio_sink), "notify::volume", + G_CALLBACK (notify_volume_cb), bvw); + } AFAICS that will still work. The if block will only be entered if audio_sink has a volume property, which could be the case e.g. if pulse was loaded. If it isn't, it won't be entered, but I can't see how that's a problem, and that could be the case without my patch anyway (e.g. if the current gst_element_factory_make ("pulsesink", "audio-sink") call returns NULL and it looks at gconf).
(In reply to comment #4) > (In reply to comment #3) > > Except that will break the code a bit lower down when we check for the "volume" > > property on the audio-sink. > > How come? Do you mean this code? > > + /* If we're using a sink that has a volume property, then that's what > + * we need to modify, not playbin's one */ > + if (g_object_class_find_property (G_OBJECT_GET_CLASS (audio_sink), > "volume")) { > + bvw->priv->pulse_audio_sink = g_object_ref (audio_sink); > + g_signal_connect (G_OBJECT (bvw->priv->pulse_audio_sink), > "notify::volume", > + G_CALLBACK (notify_volume_cb), bvw); > + } > > AFAICS that will still work. Did you test it? The audio_sink in this case won't have a volume property, because it's a gconfaudiosink bin, which will contain a pulsesink. > The if block will only be entered if audio_sink > has a volume property, which could be the case e.g. if pulse was loaded. If it > isn't, it won't be entered, but I can't see how that's a problem, and that > could be the case without my patch anyway (e.g. if the current > gst_element_factory_make ("pulsesink", "audio-sink") call returns NULL and it > looks at gconf). Yes, if pulsesink isn't present in the system we obviously won't have a volume property since no other audiosinks implement this property. That's expected.
(In reply to comment #5) > Did you test it? The audio_sink in this case won't have a volume property, > because it's a gconfaudiosink bin, which will contain a pulsesink. Ah I see, that makes sense. I've tested it but not with pulse, and it works fine of course, but that was expected... I'll look at fixing it, thanks for the insight.
Created attachment 144846 [details] [review] Try to use the GConf audiosink rather than PA, take 2 How about this one? Again I've tested it but not with Pulse. The change looks fine to me, but I'm not familiar with GStreamer/PA. It builds fine and there are no compiler warnings.
BTW I guess it would be better to try to look for the property and not for the name of the audiosink directly, in case others appear in the future that implement the volume property. But I don't know how to do that.
*** Bug 599948 has been marked as a duplicate of this bug. ***
(In reply to comment #7) > Created an attachment (id=144846) [details] [review] > Try to use the GConf audiosink rather than PA, take 2 > > How about this one? Again I've tested it but not with Pulse. The change looks > fine to me, but I'm not familiar with GStreamer/PA. It builds fine and there > are no compiler warnings. It will leak the pulsesink, you need to unref it after getting it from the bin. A better idea might be, to check either if there's an element that implements the GstStreamVolume interface inside the bin (gst_bin_get_by_interface) or by iterating all elements in the bin and look if it has a volume property (gst_bin_iterate_recurse). And really, why does it even matter for totem if pulsesink or something that has a volume property exists? It should simply set the volume property on playbin2, which will internally handle everything! (And it doesn't make any sense to only use cubic volumes if PA is used, that's very inconsistent... use it always or never)
Ok, I'm working on a good fix now... this will need the changes from bug #600027 though and another playbin2 fix.
Ok, patch for totem will follow soon after fixing the last issue. Patches to GStreamer that are necessary for this to work properly: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=de1db5ccbdc10a835a2dfdd5984892f3b0c9bcf4 http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=d85dadc122d40608ee304516702b479fb79c8ed7 http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=598c9376342b4f67012d1d418c090f432fc26d27 http://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=e72c3029c0e4674e3ce6d9770dc9de665e7a3793 http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=e4d6a2aa2c6b8b3d9137a38e10d28434eb31a580 http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=3f541452c4916b31a222043b172b3398647b8091 Best experience will be with pulseaudio >= 0.9.20 because of: http://git.0pointer.de/?p=pulseaudio.git;a=commit;h=f27a50691c8fe45bac7dd6b21fac91a359def3a1
Created attachment 146557 [details] [review] 0001-Don-t-special-case-pulseaudio-and-clean-up-the-volum.patch Apparently no other fixes are necessary. The only thing that is a bit annoying is, that clicking on the volume button in totem will show the slider and directly forward a click to the slider. Because the slider is positioned relative to the mouse cursor position, this will always lower the volume by a few % :) No idea how to fix this, GTK is not my area of knowledge. Apart from that the attached patch should provide perfect behaviour together with the changesets mentioned above and in the commit message. (Patch is against master, not gnome-2-28 branch)
As I mentioned on IRC, the patch doesn't handle _not_ restoring the volume when PulseAudio (or another audio sub-system) supports it. We don't want Totem to restore the volume when PA saves it for us just fine. So this should be hacked around for 2.28, and remove the volume restore code in Totem in master.
Created attachment 146559 [details] [review] 0002-Don-t-special-case-pulseaudio-and-clean-up-the-volum.patch New patch against 2.28, that additionally adds a volume restore hack as requested by Bastien.
Created attachment 146560 [details] [review] 0001-Don-t-special-case-pulseaudio-and-clean-up-the-volum.patch Patch against master
Created attachment 146561 [details] [review] 0002-Remove-volume-restoring.patch Patch against master to remove the volume restoring feature
Created attachment 146563 [details] [review] 0002-Remove-volume-restoring-feature.patch New patch against head, to remove the feature really completely
Ok, patches are pushed to master and gnome-2-28 now.
And marking as fixed. Thanks for the patches Sebastian.
*** Bug 603127 has been marked as a duplicate of this bug. ***