Created attachment 128357 [details] [review]
Re-add AcmeVolume control support via extensive and ugly ifdefs

This patch can also be git am-ed if preferred.

First of all, I could see us adding (parts of) the old backend back. However:

* the only supported AcmeVolume backend was GStreamer. The others were unmaintained and untested, and I don't think it makes sense to put them back. The code could be simplified and reduced quite a bit if the AcmeVolume abstraction was dropped completely.

* the code needs to be cleanly separated from the new paths. An ifdef jungle makes the whole thing unmaintainable.

Is there any process for this bug? Media-key is also broken on Solaris which lacks PulseAudio.

(In reply to comment #3)
> Is there any process for this bug? Media-key is also broken on Solaris which
> lacks PulseAudio.
> 

There is some progress on the downstream bug[1], the contributor will post a cleaned-up patch here soon.


1. http://bugs.gentoo.org/show_bug.cgi?id=264883

> There is some progress on the downstream bug[1], the contributor will post a
> cleaned-up patch here soon.
> 
> 
> 1. http://bugs.gentoo.org/show_bug.cgi?id=264883
> 

Thanks Nirbheek. So I could copy the patch shamelessly and base my work on it instead of creating a new patch myself :)

Yeah , i'm removed acme-volume-[alsa|oss|dummy] module and clean-up acme-volume-gstreamer.c which i removed to acme-volume.c (because there is just one module now), i re-written his interface too (to avoiding to rewrite some parts of gs-media-keys-manager.c) i'll make some clean-up for gsd-media-keys-manager.c and i think that i could submit the patch this evening :)

Created attachment 132286 [details] [review]
patch001

The new patch is finished :

* I removed plugins/media-keys/actions/acme-volume-[alsa|oss|dummy].*
  and acme-volume ABC (Abstract Base Class)
* I cleaned-up code in plugins/media-keys/actions/acme-volume.c
  and re-written the interface indentically to acme-volume ABC (avoiding to re-write some parts in plugins/media-keys/gsd-media-keys-manager.c for nothing)

* I updated plugins/media-keys/actions/Makefile.am
* I updated the configure.ac to only support --enable-gstreamer, so --enable-[alsa|oss] have been removed
* I updated plugins/media-keys/Makefile.am indentically to the orig patches.
* I Fixed the bug which blocked the volume when mixer was muted
  (if we decreased the volume to zero, it was muted, but it was impossible to increase it, see do_sound_action function with a appropriated comment :) )

mrpouet

Ps: and i cleaned some ifdef (the most as possible) in plugins/media-keys/gsd-media-keys-manager.c  :p

(In reply to comment #7)
> Created an attachment (id=132286) [edit]
> the new patch cleaned-up and which work perfectly
> 

Verified to work, and is now used in our gnome-2.26 overlay.

This patch doesn't work it we execute configure script with --disable-gstreamer --disable-pulse options, this  is due to #else directive in code (#else to #ifdef HAVE_PULSE) , i'll fix it and submit the corrected patch :)

Comment on attachment 132286 [details] [review]
patch001

out-dated

Created attachment 132353 [details] [review]
The correct patches fixed (the last version)

the correct patch is submitted in attachment , sorry for the forget :)

Comment on attachment 132353 [details] [review]
The correct patches fixed (the last version)

outdated

Created attachment 132424 [details] [review]
new patch

new patch was attached, i cleaned some code, and fixed compilation failed without gstreamer support, i added AC_DEFINE call if gstreamer is enable (the macroconstant HAVE_GSTREAMER was not defined in config.h before)

mrpouet

Created attachment 132559 [details] [review]
gentoo gnome-overlay patch

patch was corrected due to an compilation error with pulseaudio support, and an run-time execution, tested and reviewed by the developpers.

you'll can find the new patch in attachment

Is it possible to have this included in 2.26.1? A lot of people would benefit from this :)

thanks

The rate of recent changes has kept me from even looking at the patch until now, and doesn't really suggest it's a good candidate for inclusion in a stable release just yet. So no, sorry.

(In reply to comment #21)
> The rate of recent changes has kept me from even looking at the patch until
> now, and doesn't really suggest it's a good candidate for inclusion in a stable
> release just yet. So no, sorry.
> 

No problem, we'll be including this in our release though, since we can't really force our users to use pulseaudio (the ensuing bug-flood would cripple us :).

We'll keep updating this bug with any fixes we find along the way.

Thanks!

Apparently there are some crashes caused by this patch:
http://bugs.debian.org/524164 http://bugs.debian.org/524165

I’m investigating this, but maybe Romain will have more clues.

Yeah, but if you have a look to the backtrace attached to the debian bugs, there is mainly causes apparently by gdk functions, the top of the stack is :

+ Trace 214484

my patch updates and add plugins/media-keys/actions/acme-volume.[c|h], in which an Gobject is added to simply use gstreamer volume control (control mixer and tracks)

it's also updates plugins/media-keys/gsd-media-keys-manager.c do add the appropriate handler to handle KEY_UP/KEY_DOWN/KEY_MUTE (which use acme_volume object). The only logical link that I see with gdk is eventually glade control volume panel, but i've not updated this part.


i could be wrong, of course ;) , can anyone could fully test this patch (for an other distribution if it's possible) , to report any problem or issue ?

Created attachment 132719 [details] [review]
patch more factorized and fix a bug compile

Created attachment 132720 [details] [review]
factorized and clean again

Thanks Romain, applied it to gnome 2.26 on Solaris.

wow cool :), i found the causes of the segfault :

it's apparently due when you defines a device for /desktop/gnome/sound/default_mixer_device (which was empty for me that's why i was unable to reproduice the problem), if i specifi "alsamixer:hw:0" into it, it's crashes apparently due to acme_volume_open.

i'll have a look to code, and find the problem, so please Mask this version, or put it to experimental branch (depending of the distribution) waiting i find a real issue.

thanks ;)

Created attachment 132788 [details] [review]
a new patch version, which normally should be work

anyone can test the new patch ? (if possible on differents systems)

thanks

mrpouet

Note about the last patch only works for gnome-settings-daemon 2.26.0, the patch can be of course apply for g-s-d 2.26.1 but doesn't work (any pop up window is displayed and control is not changed probably due to some changes in g-s-d 2.26.1 code).

i'll have a look to the code change and write a new version of this patch

This is was not due to a bug of code but to a forget.

The problem was ONLY due to media-keys.gnome-settings-daemon-plugin plugin
file which was not installed on the system, mainly due to this line in the
orig plugins/media-keys/Makefile.am :

if HAVE_PULSE
 plugin_DATA =
$(plugin_in_files:.gnome-setting-daemon-plugin.in:.gnome-settings-daemon-plugin)
fi

which is not just absurd for gstreamer support but in general, because
there are not ONLY volume keys bindings which are handled : HOME, MAIL,
EJECT...etc... (I don't criticize anyone, it just a comment ;) )

and finally it's work, the patch just update this part.

mrpouet ;)

Created attachment 132912 [details] [review]
gnome-settings-daemon-2.26.1-readd-gst-vol-control-support.patch

the patch which solves the problem :)

Stuff like that just can't be right:

+#ifdef HAVE_PULSE
+        if (vol_step <= 0 || vol_step > 100) {
+#else
+        if (error) {
+#endif

And the changes to the manager code would need to be cleaned up:
- indentation doesn't match the existing code
- replacing int with gint gains us nothing

-#include "gsd-media-keys-window.h"
-
 #ifdef HAVE_PULSE
 #include "gvc-mixer-control.h"
+#elif defined(HAVE_GSTREAMER)
+#include "actions/acme-volume.h"
 #endif /* HAVE_PULSE */
+#include "gsd-media-keys-window.h"

Why do you move things around like that?

-                if (screen == NULL) {
+                if (screen == NULL)
                         continue;
-                }

Don't make changes like that, if somebody touches that code, they'll remove the braces.

+#ifdef HAVE_PULSE
+        guint norm_vol_step;
         if (manager->priv->stream == NULL)
+#else
+        if (manager->priv->volume == NULL)
+#endif

Put the conditional and its branch inside the ifdef, otherwise it's unreadable.

-
-        /* FIXME: this is racy */
         vol = gvc_mixer_stream_get_volume (manager->priv->stream);
         muted = gvc_mixer_stream_get_is_muted (manager->priv->stream);
+

Why do you modify the pulse code?

-}
then
+        }

Broken indentation

-

Don't remove linefeeds for no reason.

Regarding braces and style.  g-s-d used the GDM style guidelines in my original separate module.  Fizz may have different rules now though...

http://git.gnome.org/cgit/gdm/plain/HACKING

As previously reported here (http://bugzilla.gnome.org/show_bug.cgi?id=583075)
when I try to mute volume by hotkeys (Fn+F10) a notify is shown on screen
but inputmix-mute (codec1 jack green) is toggled instead of expected
vmix0-outvol. The result is that volume is not muted.
No problem when toggling volume-applet icon or when increasing / decreasing
volume with Fn + F11 / Fn + F12 instead.
I get two different logs when launching gnome-volume-control --gst-debug=5 from
shell.
- muting from Hotkeys:
http://pastebin.com/m445fb04
- muting from Applet:
http://pastebin.com/m5016c2c5

Additional Info:
Distro ArchLinux
oss 4.1_1052-1
gnome-settings-daemon 2.26.1 built with a patch to use gstreamer volume
(http://repos.archlinux.org/viewvc.cgi/gnome-settings-daemon/repos/extra-i686/07_use_gstreamer_volume.patch?view=log).

Created attachment 140745 [details] [review]
Updated patch for 2.27.5

The attached patch has been updated for 2.27.5, and addresses the problems with the earlier patch listed by Bastien. Can we have this for 2.28 please? :)

I don't like the build changes one bit. And things like this are very very broken:
+if HAVE_PULSE
 DIST_SUBDIRS = cut-n-paste
+else
+DIST_SUBDIRS = actions
+endif

DIST_SUBDIRS can't be conditional. Otherwise you end up adding one directory and not the other in the tarballs.

The acme-volume.c should be renamed to something else, and make it clear it's the GStreamer backend. It also doesn't need to go in a sub-directory.

plugins/media-keys/actions/Makefile.am most certainly doesn't work as you think it does.

You're moving stuff around again, and removing empty lines for no reason.
-        guint vol, norm_vol_step;
         int vol_step;
-
+        GError  *error = NULL;
+        guint vol;

You're still putting conditional in ifdefs, and not documenting your code. This still looks wrong.
+#ifdef HAVE_PULSE
+        if (vol_step <= 0 || vol_step > 100) {
+#else
+        if (error) {
+#endif

I stopped reviewing there because it's clear you didn't fix the problems I mentioned in the earlier review.

Created attachment 140751 [details] [review]
Patch with mentioned problems fixed

(In reply to comment #38)
> DIST_SUBDIRS can't be conditional. Otherwise you end up adding one directory
> and not the other in the tarballs.
> 

Fixed

> The acme-volume.c should be renamed to something else, and make it clear it's
> the GStreamer backend. It also doesn't need to go in a sub-directory.
> 

Done

> plugins/media-keys/actions/Makefile.am most certainly doesn't work as you think
> it does.
> 

That was taken from the old file in svn, fixed now (merged into cut-n-paste)

> You're moving stuff around again, and removing empty lines for no reason.
> -        guint vol, norm_vol_step;
>          int vol_step;
> -
> +        GError  *error = NULL;
> +        guint vol;
> 

This was partially fixed in the previous patch, and is now fully fixed.

> You're still putting conditional in ifdefs, and not documenting your code. This
> still looks wrong.
> +#ifdef HAVE_PULSE
> +        if (vol_step <= 0 || vol_step > 100) {
> +#else
> +        if (error) {
> +#endif
> 

That's how it was in the old code (checking "error"), but apparently the new way works too.

> I stopped reviewing there because it's clear you didn't fix the problems I
> mentioned in the earlier review.
> 

Well, except for the conditional in ifdef, all the other problems you mentioned last time were fixed this time.

Do not be discouraged by lots of problems, tell me what they are and I'll fix them :)

Some pieces of the patch assume that either Gstreamer or Pulse is available, but this is not enforced in configure.in.

(In reply to comment #40)
> Some pieces of the patch assume that either Gstreamer or Pulse is available,
> but this is not enforced in configure.in.
> 

Okay, which way do you want it? Force one of the two or support having neither of the two? (easiest will be forcing one of the two)

(In reply to comment #39)
> > plugins/media-keys/actions/Makefile.am most certainly doesn't work as you think
> > it does.
> > 
> 
> That was taken from the old file in svn, fixed now (merged into cut-n-paste)

But it's not cut-n-paste code, put it in the media-keys directory.

> > I stopped reviewing there because it's clear you didn't fix the problems I
> > mentioned in the earlier review.
> > 
> 
> Well, except for the conditional in ifdef, all the other problems you mentioned
> last time were fixed this time.
> 
> Do not be discouraged by lots of problems, tell me what they are and I'll fix
> them :)

Yes, but my comments seem to fall on deaf ears. Like this piece of code, when I mentioned "You're still putting conditional in ifdefs" in comment 38.

+#ifdef HAVE_PULSE
         if (manager->priv->stream == NULL)
+#else
+        if (manager->priv->volume == NULL)
+#endif
                 return;

This should be:
#ifdef HAVE_PULSE
         if (manager->priv->stream == NULL)
                 return;
#else
        if (manager->priv->volume == NULL)
                 return;
#endif

You could also just rename stream to volume in the struct, and avoid this construct altogether.

-SUBDIRS =
-plugin_LTLIBRARIES =
-
-if HAVE_PULSE
-SUBDIRS += cut-n-paste
-plugin_LTLIBRARIES += libmedia-keys.la
-endif
+SUBDIRS = cut-n-paste
+plugin_LTLIBRARIES = libmedia-keys.la

Those changes are broken if you're building without PA and without GStreamer (as mentioned by Josselin), and they shouldn't be made requirements either.

And don't prefix the code with gvc- when it doesn't come from gnome-volume-control.

Created attachment 145145 [details] [review]
Updated patch for g-s-d 2.28.0

This patch is updated to apply against g-s-d 2.28.0. I haven't fixed all the issues mentioned before though.

We are suffering a small bug with this patch: 

Buttons "raiseVolume" "lowerVolume" work only on the second time, if
they are pressed after one another (i.e. if I, for instance, press raiseVolume
repeatedly, and then start pressing lowerVolume) the first time the volume will
actually increase, as if I pressed raiseVolume, and only on the second hit it
will start actually decrease.

Created attachment 165566 [details] [review]
updated patch


Here is an updated patch that applies against gnome-settings-daemon 2.30.1 code.

Aside from making the patch work against the latest code, I made two changes to the code:

1) I reduced the TIMEOUT defined in 
   plugins/media-keys/cut-n-paste/gvc-gstreamer-acme-vol.c from 4 seconds to
   2 seconds.  

   This TIMEOUT is used to make the media keys plugin avoid rechecking for the 
   device if the user hits a media key more than once before the timeout.  So
   if a user hits media keys multiple times (which is common), the code avoids
   re-checking for the device and just assumes it will use the last one used.
   In my testing, a 4 second timeout was just too long.  It was too easy to
   change the device, and then hit a media key within 4 seconds and have it not 
   work right.  Since the timeout is reset each time you hit a media key, this 
   code forces you to not hit a media key for 4 seconds before it will re-check
   the device and start using the correct one.  I found changing the timeout to
   2 seconds made the code work much more nicely and not so confusingly.
  
2) The media-keys plugin just uses whatever device it finds first.  If the user
   has multiple audio devices, this can cause the media keys to control the 
   wrong device.  This can seem like the media keys are not working if the
   second audio device is not actually hooked up to speakers, for example.

   I fixed the /plugins/media-keys/cut-n-paste/gvc-gstreamer-acme-vol.c code to 
   check the gnome-volume-control GConf key:
   /apps/gnome-volume-control/active-element
   And the media-keys plugin will use the device that gnome-volume-control is
   using if the GConf key is set to a valid value.
 
These issues are discussed in the following OpenSolaris bug report, and comment 
#13 shows the exact code changes associated with these changes:

   https://defect.opensolaris.org/bz/show_bug.cgi?id=13512#c13

Thanks a lot :-D, it works fine for me.

Only a note, I will probably need to run:
echo "plugins/media-keys/cut-n-paste/gvc-gstreamer-acme-vol.c" >> po/POTFILES.in

to prevent tests failure due:

Making check in po
make[1]: Entering directory `/var/tmp/portage/gnome-base/gnome-settings-daemon-2.30.2/work/gnome-settings-daemon-2.30.2/po'
INTLTOOL_EXTRACT=/usr/bin/intltool-extract srcdir=. /usr/bin/intltool-update --gettext-package gnome-settings-daemon --pot
rm -f missing notexist
srcdir=. /usr/bin/intltool-update -m
The following files contain translations and are currently not in use. Please
consider adding these to the POTFILES.in file, located in the po/ directory.

plugins/media-keys/cut-n-paste/gvc-gstreamer-acme-vol.c

If some of these files are left out on purpose then please add them to
POTFILES.skip instead of POTFILES.in. A file 'missing' containing this list
of left out files has been written in the current directory.
Please report to http://bugzilla.gnome.org/enter_bug.cgi?product=gnome-settings-daemon
if [ -r missing -o -r notexist ]; then \
          exit 1; \
        fi
make[1]: *** [check] Error 1

(In reply to comment #45)
> Created an attachment (id=165566) [details] [review]
> updated patch
> 
> 
> Here is an updated patch that applies against gnome-settings-daemon 2.30.1
> code.
> 

It also seems to break when compiling with as-needed:
test_media_keys-gsd-media-keys-manager.o: In function `acme_filter_events':
gsd-media-keys-manager.c:(.text+0x158b): undefined reference to
`acme_volume_get_threshold'
gsd-media-keys-manager.c:(.text+0x15b6): undefined reference to
`acme_volume_get_volume'
gsd-media-keys-manager.c:(.text+0x15c5): undefined reference to
`acme_volume_get_mute'
gsd-media-keys-manager.c:(.text+0x15e3): undefined reference to
`acme_volume_mute_toggle'
gsd-media-keys-manager.c:(.text+0x15ef): undefined reference to
`acme_volume_get_mute'
gsd-media-keys-manager.c:(.text+0x15fe): undefined reference to
`acme_volume_get_volume'
gsd-media-keys-manager.c:(.text+0x1892): undefined reference to
`acme_volume_set_volume'
gsd-media-keys-manager.c:(.text+0x18ae): undefined reference to
`acme_volume_set_volume'
gsd-media-keys-manager.c:(.text+0x18c3): undefined reference to
`acme_volume_mute_toggle'
gsd-media-keys-manager.c:(.text+0x18e9): undefined reference to
`acme_volume_set_mute'
test_media_keys-gsd-media-keys-manager.o: In function
`gsd_media_keys_manager_start':
gsd-media-keys-manager.c:(.text+0x1ec6): undefined reference to
`acme_volume_new'
collect2: ld returned 1 exit status

Thanks for your work :-)

Hope this gstreamer backend can change the control devices.

like "Giorgio DG" said, i also have the problem.

gnome-settings-daemon and gnome-mixer-applet control the different device.

(In reply to comment #45)
> Created an attachment (id=165566) [details] [review]
> updated patch
> 
> 
> Here is an updated patch that applies against gnome-settings-daemon 2.30.1
> code.
> 
> Aside from making the patch work against the latest code, I made two changes to
> the code:
> 
> 1) I reduced the TIMEOUT defined in 
>    plugins/media-keys/cut-n-paste/gvc-gstreamer-acme-vol.c from 4 seconds to
>    2 seconds.  
> 
>    This TIMEOUT is used to make the media keys plugin avoid rechecking for the 
>    device if the user hits a media key more than once before the timeout.  So
>    if a user hits media keys multiple times (which is common), the code avoids
>    re-checking for the device and just assumes it will use the last one used.
>    In my testing, a 4 second timeout was just too long.  It was too easy to
>    change the device, and then hit a media key within 4 seconds and have it not 
>    work right.  Since the timeout is reset each time you hit a media key, this 
>    code forces you to not hit a media key for 4 seconds before it will re-check
>    the device and start using the correct one.  I found changing the timeout to
>    2 seconds made the code work much more nicely and not so confusingly.
> 
> 2) The media-keys plugin just uses whatever device it finds first.  If the user
>    has multiple audio devices, this can cause the media keys to control the 
>    wrong device.  This can seem like the media keys are not working if the
>    second audio device is not actually hooked up to speakers, for example.
> 
>    I fixed the /plugins/media-keys/cut-n-paste/gvc-gstreamer-acme-vol.c code to 
>    check the gnome-volume-control GConf key:
>    /apps/gnome-volume-control/active-element
>    And the media-keys plugin will use the device that gnome-volume-control is
>    using if the GConf key is set to a valid value.
> 
> These issues are discussed in the following OpenSolaris bug report, and comment 
> #13 shows the exact code changes associated with these changes:
> 
>    https://defect.opensolaris.org/bz/show_bug.cgi?id=13512#c13

This introduced a regression as reported downstream on http://bugs.gentoo.org/show_bug.cgi?id=339732 :
"I am unable to change pulseaudio's default output device from my laptop's
onboard sound chip to my Logitech ClearChat USB headset if the onboard sound's
volume is set to 0%. I have to raise the volume before I am able to change the
default device.

Reproducible: Always

Steps to Reproduce:
1. Open gnome-volume-control
2. Go to "Output" tab
3. Try to click the radio button beside "Logitech USB Headset Analog Stereo"
(4.) Sometimes the default will switch to the headset but will switch back to
onboard within ~10 seconds. This step does not always happen.
Actual Results:  
Default output returned to onboard sound

Expected Results:  
Default device should be changed to USB headset"

(In reply to comment #45)
> Created an attachment (id=165566) [details] [review]
> updated patch
> 
> 
> Here is an updated patch that applies against gnome-settings-daemon 2.30.1
> code.

Tried this with gnome-settings-daemon 2.32.1 and it works fine for me.

Trying to create a desktop environment based on pieces of string is over. Feel free to get your distribution to ship this if they cannot be bothered to ship modern components to power this desktop.

fwiw, Gentoo dropped this patch months ago, and has been pulling in Pulseaudio unconditionally for GNOME 3.