Merge lp:~cjcurran/indicator-sound/stable-service-with-unstable-pulse into lp:indicator-sound/0.1

[Conor Curran]

[Conor Curran (cjcurran)]

This work should make the service stable against a flapping pulseaudio daemon. Tested thoroughly with an eye on memory using pmap on the service pid (connecting and reconnecting to pulse can cause memory leaks).

Also caches any new sinks that become available during its lifespan. This may prevent a seg fault that was reported earlier this week.

[David Barth (dbarth)]

I'd like to see some unit tests that show how the code now handles corner cases properly, typically with pa not being available at startup, then appearing, the flipping ten times. Do you have a test environment that can simulate that situation?

To improve the readability of the merge proposals, can you also embed some emacs or vi tab setttings in your files?

review: Needs Information

=== modified file 'src/pulse-manager.c'
--- src/pulse-manager.c	2010-02-18 14:30:43 +0000
+++ src/pulse-manager.c	2010-02-26 14:45:26 +0000
@@ -31,7 +31,6 @@
 
 static GHashTable *sink_hash = NULL;
 static SoundServiceDbus *dbus_service = NULL;
-// Until we find a satisfactory default sink this index should remain < 0
 static gint DEFAULT_SINK_INDEX = -1;
 static gboolean pa_server_available = FALSE;
 // PA related
@@ -46,6 +45,7 @@
 static void pulse_source_info_callback(pa_context *c, const pa_source_info *i, int eol, void *userdata); 
 static void destroy_sink_info(void *value);
 static gboolean determine_sink_availability();
+static void reconnect_to_pulse();
 
 
 /**
@@ -65,14 +65,19 @@
 	g_assert(pulse_context);
     
     sink_hash = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, destroy_sink_info);
+
     // Establish event callback registration
 	pa_context_set_state_callback(pulse_context, context_state_callback, NULL);
-	pa_context_connect(pulse_context, NULL, PA_CONTEXT_NOAUTOSPAWN, NULL);    
+    // BUILD MENU ANYWHO - it will be updated
+    update_pa_state(FALSE, FALSE, FALSE, 0);
+
+	pa_context_connect(pulse_context, NULL, PA_CONTEXT_NOFAIL, NULL);    
 }
 
 void close_pulse_activites()
 {
-    if (pulse_context){
+    if (pulse_context != NULL){
+        g_debug("freeing the pulse context");
  	    pa_context_unref(pulse_context);
         pulse_context = NULL;
    	}
@@ -82,6 +87,30 @@
     g_debug("I just closed communication with Pulse");
 }
 
+/** 
+reconnect_to_pulse()
+In the event of Pulseaudio flapping in the wind handle gracefully without
+memory leaks !
+*/
+static void reconnect_to_pulse()
+{
+    // reset
+    if (pulse_context != NULL){
+        g_debug("freeing the pulse context");
+ 	    pa_context_unref(pulse_context);
+        pulse_context = NULL;
+   	}
+    g_hash_table_destroy(sink_hash);
+
+    // reconnect
+	pulse_context = pa_context_new(pa_glib_mainloop_get_api(pa_main_loop), "ayatana.indicator.sound");
+	g_assert(pulse_context);   
+    sink_hash = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, destroy_sink_info);
+    // Establish event callback registration
+	pa_context_set_state_callback(pulse_context, context_state_callback, NULL);
+    update_pa_state(FALSE, FALSE, FALSE, 0);
+	pa_context_connect(pulse_context, NULL, PA_CONTEXT_NOFAIL, NULL);        
+}
 
 static void destroy_sink_info(void *value)
 {
@@ -186,6 +215,8 @@
 */
 void set_sink_volume(gdouble percent)
 {
+    if(pa_server_available == FALSE)
+        return;   
     g_debug("in the pulse manager:set_sink_volume with percent %f", percent);
     if(DEFAULT_SINK_INDEX < 0)
     {
@@ -366,12 +397,20 @@
     }
     else
     {
-        // TODO ADD new sink - part of big refactor
-        g_debug("attempting to add new sink with name %s", info->name);
-        //sink_info *s;
-        //s = g_new0(sink_info, 1);                
-        //update the sinks hash with new sink.
-    }    
+        sink_info *value;
+        value = g_new0(sink_info, 1);
+        value->index = value->device_index = info->index;
+        value->name = g_strdup(info->name);
+        value->description = g_strdup(info->description);
+        value->icon_name = g_strdup(pa_proplist_gets(info->proplist, PA_PROP_DEVICE_ICON_NAME));
+        value->active_port = (info->active_port != NULL);
+        value->mute = !!info->mute;
+        value->volume = info->volume;
+        value->base_volume = info->base_volume;
+        value->channel_map = info->channel_map;
+        g_hash_table_insert(sink_hash, GINT_TO_POINTER(value->index), value);
+        g_debug("pulse-manager:update_sink_info -> After adding a new sink to our hash");
+   }    
 }
 
 
@@ -474,7 +513,7 @@
 			g_debug("unconnected");
 			break;
         case PA_CONTEXT_CONNECTING:
-			g_debug("connecting");
+			g_debug("connecting - waiting for the server to become available");
 			break;
         case PA_CONTEXT_AUTHORIZING:
 			g_debug("authorizing");
@@ -484,8 +523,8 @@
 			break;
         case PA_CONTEXT_FAILED:
 			g_warning("FAILED to retrieve context - Is PulseAudio Daemon running ?");
-            //Update the indicator to show PA either is not ready or has no available sink
-            update_pa_state(FALSE, FALSE, TRUE, 0); 
+            pa_server_available = FALSE;
+            reconnect_to_pulse();
 			break;
         case PA_CONTEXT_TERMINATED:
 			g_debug("context terminated");

=== modified file 'src/sound-service.c'
--- src/sound-service.c	2010-02-18 11:11:38 +0000
+++ src/sound-service.c	2010-02-26 14:45:26 +0000
@@ -37,12 +37,13 @@
 static gboolean b_sink_available = FALSE;
 static gboolean b_all_muted = FALSE;
 static gboolean b_pulse_ready = FALSE;
+static gboolean b_startup = TRUE;
 static gdouble volume_percent = 0.0;
 
 static void set_global_mute_from_ui();
 static gboolean idle_routine (gpointer data);
 static void rebuild_sound_menu(DbusmenuMenuitem *root, SoundServiceDbus *service);
-
+static void refresh_menu();
 
 /**********************************************************************************************************************/
 //    Init functions (GTK and DBUS)
@@ -66,7 +67,8 @@
     }
 }
 /**
-Build the DBus menu items. For now Mute all/Unmute is the only available option
+rebuild_sound_menu:
+Build the DBus menu items, mute/unmute, slider, separator and sound preferences 'link'
 **/
 static void rebuild_sound_menu(DbusmenuMenuitem *root, SoundServiceDbus *service)
 {
@@ -80,10 +82,18 @@
     volume_slider_menuitem = slider_menu_item_new(b_sink_available, volume_percent);
     dbusmenu_menuitem_child_append(root, mute_all_menuitem);
     dbusmenu_menuitem_child_append(root, DBUSMENU_MENUITEM(volume_slider_menuitem));
-
+    dbusmenu_menuitem_property_set_bool(DBUSMENU_MENUITEM(volume_slider_menuitem),
+                                        DBUSMENU_MENUITEM_PROP_ENABLED,
+                                        b_sink_available);       
+    dbusmenu_menuitem_property_set_bool(DBUSMENU_MENUITEM(volume_slider_menuitem),
+                                        DBUSMENU_MENUITEM_PROP_VISIBLE,
+                                        b_sink_available);   
+    // Separator
     DbusmenuMenuitem *separator = dbusmenu_menuitem_new();
     dbusmenu_menuitem_property_set(separator, DBUSMENU_MENUITEM_PROP_TYPE, DBUSMENU_CLIENT_TYPES_SEPARATOR);
     dbusmenu_menuitem_child_append(root, separator);
+
+    // Sound preferences dialog
     DbusmenuMenuitem *settings_mi = dbusmenu_menuitem_new();
     dbusmenu_menuitem_property_set(settings_mi, DBUSMENU_MENUITEM_PROP_LABEL,
                                                                    _("Sound Preferences..."));
@@ -127,8 +137,8 @@
 	if (mainloop != NULL) {
 		g_debug("Service shutdown !");
         // TODO: uncomment for release !!
-        close_pulse_activites();
-        g_main_loop_quit(mainloop);
+/*        close_pulse_activites();*/
+/*        g_main_loop_quit(mainloop);*/
 	}
 	return;
 }
@@ -140,12 +150,49 @@
     b_pulse_ready = pa_state;
     volume_percent = percent;
 	g_debug("update pa state with state %i, availability of %i, mute value of %i and a volume percent is %f", pa_state, sink_available, sink_muted, volume_percent);
+    // Only rebuild the menu on start up...
+    if(b_startup == TRUE){
+        rebuild_sound_menu(root_menuitem, dbus_interface);
+        b_startup = FALSE;
+    }
+    else{
+        refresh_menu();
+    }
+    // Emit the signals after the menus are setup/torn down
     sound_service_dbus_update_sink_volume(dbus_interface, percent); 
     sound_service_dbus_update_sink_mute(dbus_interface, sink_muted); 
 
-    // Only rebuild the menu on start up...
-    if(volume_slider_menuitem == NULL)
-        rebuild_sound_menu(root_menuitem, dbus_interface);
+}
+
+static void refresh_menu()
+{
+    g_debug("in the refresh menu method");
+    if(b_sink_available == FALSE || b_pulse_ready == FALSE)
+    {
+
+        dbusmenu_menuitem_property_set_bool(DBUSMENU_MENUITEM(volume_slider_menuitem), 
+                                            DBUSMENU_MENUITEM_PROP_ENABLED,
+                                            FALSE);   
+        dbusmenu_menuitem_property_set_bool(DBUSMENU_MENUITEM(volume_slider_menuitem), 
+                                            DBUSMENU_MENUITEM_PROP_VISIBLE,
+                                            FALSE);   
+        dbusmenu_menuitem_property_set_bool(mute_all_menuitem, 
+                                            DBUSMENU_MENUITEM_PROP_ENABLED,
+                                            FALSE);
+
+    }
+    else if(b_sink_available == TRUE  && b_pulse_ready == TRUE){
+
+        dbusmenu_menuitem_property_set_bool(DBUSMENU_MENUITEM(volume_slider_menuitem), 
+                                            DBUSMENU_MENUITEM_PROP_ENABLED,
+                                            TRUE);   
+        dbusmenu_menuitem_property_set_bool(DBUSMENU_MENUITEM(volume_slider_menuitem), 
+                                            DBUSMENU_MENUITEM_PROP_VISIBLE,
+                                            TRUE);   
+        dbusmenu_menuitem_property_set_bool(mute_all_menuitem, 
+                                            DBUSMENU_MENUITEM_PROP_ENABLED,
+                                            TRUE);        
+    }
 }