Media control enablement and bug fixesneedlefish_13.93.0needlefish/13.93.0marlin_12.93.0marlin/12.93.0lamprey_12.1.6lamprey_12.1.5lamprey_12.1.4lamprey_12.1.3lamprey_12.1.2lamprey/12.1.6lamprey/12.1.5lamprey/12.1.4lamprey/12.1.3lamprey/12.1.213.93.012.93.012.1.612.1.512.1.412.1.312.1.2

Changes:
- Properly split out media control and transport callbacks, working
  on an implementation in a client pointed out some issues that
  needed to be fixed.
- Add new exported API functions bluez_get_media_control_properties
  and bluez_get_media_player_properties to simplify the ability of
  clients to determine the current state.  This is more required now
  for the usecase of standalone clients starting up separately as
  opposed to having a binding tracking the state for them.
- Added logic in bluez_devices_signal_callback to be paranoid about
  the input to the callback, as it became clear from debugging that
  having two different libraries use g_dbus_connection_signal_subscribe
  in the same process results in the separately registered callbacks
  all getting called with the superset of the filtered signal requests.
  At best this leads to confusion with respect to logging/debugging, at
  worst it opens the door to tripping over unexpected inputs in the
  callbacks.
- Fixed the device path being passed to bluez_autoconnect, this seems
  to re-enable the workaround wrt devices disconnecting after the
  initial connect.  Further investigation is required into what
  happens here, but the behavior now seems to match the pre-refactored
  code in the binding.
- Added a couple of DEBUG statements to allow vetting the arguments
  passed to g_dbus_connection_call in a couple of places.
- Fix install of the test program if building it is enabled with the
  meson option.

Bug-AGL: SPEC-4231

Signed-off-by: Scott Murray <scott.murray@konsulko.com>
Change-Id: Iba5bb6e4bde3a9f4172d11b9eefdddf8c5da2203

4 files changed, 149 insertions, 10 deletions

diff --git a/include/bluez-glib.h b/include/bluez-glib.h
index 25abecb..257effb 100644
--- a/include/bluez-glib.h
+++ b/include/bluez-glib.h
@@ -75,11 +75,17 @@ typedef void (*bluez_device_event_cb_t)(gchar *adapter,
 
 typedef void (*bluez_media_control_event_cb_t)(gchar *adapter,
 					       gchar *device,
-					       gchar *endpoint,
 					       bluez_event_t event,
 					       GVariant *properties,
 					       gpointer user_data);
 
+typedef void (*bluez_media_transport_event_cb_t)(gchar *adapter,
+						 gchar *device,
+						 gchar *endpoint,
+						 bluez_event_t event,
+						 GVariant *properties,
+						 gpointer user_data);
+
 typedef void (*bluez_media_player_event_cb_t)(gchar *adapter,
 					      gchar *device,
 					      gchar *player,
@@ -104,6 +110,8 @@ void bluez_add_media_control_event_callback(bluez_media_control_event_cb_t cb, g
 
 void bluez_add_media_player_event_callback(bluez_media_player_event_cb_t cb, gpointer user_data);
 
+void bluez_add_media_transport_event_callback(bluez_media_transport_event_cb_t cb, gpointer user_data);
+
 void bluez_add_agent_event_callback(bluez_agent_event_cb_t cb, gpointer user_data);
 
 void bluez_set_log_level(bluez_log_level_t level);
@@ -154,6 +162,10 @@ gboolean bluez_device_avrcp_controls(const char *device, bluez_media_control_t a
 
 gboolean bluez_set_pincode(const char *pincode);
 
+gboolean bluez_get_media_control_properties(const char *device, GVariant **reply);
+
+gboolean bluez_get_media_player_properties(const char *device, GVariant **reply);
+
 #ifdef __cplusplus
 } // extern "C"
 #endif
diff --git a/src/api.c b/src/api.c
index 04c3dc2..39f1f39 100644
--- a/src/api.c
+++ b/src/api.c
@@ -51,6 +51,7 @@ callback_list_t bluez_adapter_callbacks;
 callback_list_t bluez_device_callbacks;
 callback_list_t bluez_media_control_callbacks;
 callback_list_t bluez_media_player_callbacks;
+callback_list_t bluez_media_transport_callbacks;
 
 // The global handler thread and BlueZ state
 static GThread *g_bluez_thread;
@@ -175,7 +176,7 @@ static void run_media_callbacks(callback_list_t *callbacks,
 {
 	GSList *list;
 
-	if (!(endpoint || player))
+	if (!(adapter || device))
 		return;
 
 	g_mutex_lock(&callbacks->mutex);
@@ -185,9 +186,12 @@ static void run_media_callbacks(callback_list_t *callbacks,
 			if (player) {
 				bluez_media_player_event_cb_t cb = (bluez_media_player_event_cb_t) entry->callback;
 				(*cb)(adapter, device, player, event, properties, entry->user_data);
+			} else if (endpoint) {
+				bluez_media_transport_event_cb_t cb = (bluez_media_transport_event_cb_t) entry->callback;
+				(*cb)(adapter, device, endpoint, event, properties, entry->user_data);
 			} else {
 				bluez_media_control_event_cb_t cb = (bluez_media_control_event_cb_t) entry->callback;
-				(*cb)(adapter, device, endpoint, event, properties, entry->user_data);
+				(*cb)(adapter, device, event, properties, entry->user_data);
 			}
 		}
 	}
@@ -226,6 +230,14 @@ EXPORT void bluez_add_media_player_event_callback(bluez_media_player_event_cb_t
 	callback_add(&bluez_media_player_callbacks, cb, user_data);
 }
 
+EXPORT void bluez_add_media_transport_event_callback(bluez_media_player_event_cb_t cb, gpointer user_data)
+{
+	if (!cb)
+		return;
+
+	callback_add(&bluez_media_transport_callbacks, cb, user_data);
+}
+
 static void mediaplayer1_set_path(struct bluez_state *ns, const char *path)
 {
 	if (ns->mediaplayer_path)
@@ -254,6 +266,18 @@ static void bluez_devices_signal_callback(GDBusConnection *connection,
 	DEBUG("parameters: %s", g_variant_print(parameters, TRUE));
 #endif
 
+	// Be paranoid to avoid any potential issues from unexpected signals,
+	// as glib seems to do some unexpected reuse of the D-Bus signal
+	// mechanism if there is more than one subscriber in the same process,
+	// and we will see signals we did not register for. :(
+	if (!((g_str_has_prefix(object_path, "/org/bluez/") &&
+	       g_strcmp0(interface_name, "org.freedesktop.DBus.Properties") == 0) ||
+	      (g_strcmp0(object_path, "/") == 0 &&
+	       g_strcmp0(interface_name, "org.freedesktop.DBus.ObjectManager") == 0))) {
+		// Not an expected signal
+		return;
+	}
+
 	if (!g_strcmp0(signal_name, "InterfacesAdded")) {
 
 		g_variant_get(parameters, "(&oa{sa{sv}})", &path, &array);
@@ -284,7 +308,7 @@ static void bluez_devices_signal_callback(GDBusConnection *connection,
 				DEBUG("media endpoint added!");
 				DEBUG("media endpoint path %s, key %s, endpoint %s",
 				      path, key, endpoint);
-				run_media_callbacks(&bluez_media_control_callbacks,
+				run_media_callbacks(&bluez_media_transport_callbacks,
 						    adapter,
 						    device,
 						    endpoint,
@@ -299,7 +323,7 @@ static void bluez_devices_signal_callback(GDBusConnection *connection,
 				gchar *adapter = bluez_return_adapter(path);
 				gchar *device = bluez_return_device(path);
 				gchar *player = find_index(path, 5);
-				DEBUG("media player removed!");
+				DEBUG("media player added!");
 				DEBUG("media player = %s", player);
 				run_media_callbacks(&bluez_media_player_callbacks,
 						    adapter,
@@ -388,6 +412,20 @@ static void bluez_devices_signal_callback(GDBusConnection *connection,
 			run_callbacks(&bluez_adapter_callbacks, adapter, NULL, BLUEZ_EVENT_CHANGE, var);
 			g_free(adapter);
 
+		} else if (!g_strcmp0(path, BLUEZ_MEDIACONTROL_INTERFACE)) {
+			gchar *adapter = bluez_return_adapter(object_path);
+			gchar *device = bluez_return_device(object_path);
+			DEBUG("media control changed!");
+			run_media_callbacks(&bluez_media_control_callbacks,
+					    adapter,
+					    device,
+					    NULL,
+					    NULL,
+					    BLUEZ_EVENT_CHANGE,
+					    var);
+			g_free(adapter);
+			g_free(device);
+
 		} else if (!g_strcmp0(path, BLUEZ_MEDIAPLAYER_INTERFACE)) {
 			gchar *adapter = bluez_return_adapter(object_path);
 			gchar *device = bluez_return_device(object_path);
@@ -411,12 +449,12 @@ static void bluez_devices_signal_callback(GDBusConnection *connection,
 			gchar *endpoint = bluez_return_endpoint(object_path);
 			DEBUG("media endpoint changed!");
 			DEBUG("media endpoint %s", endpoint);
-			run_media_callbacks(&bluez_media_control_callbacks,
+			run_media_callbacks(&bluez_media_transport_callbacks,
 					    adapter,
 					    device,
 					    endpoint,
 					    NULL,
-					    BLUEZ_EVENT_REMOVE,
+					    BLUEZ_EVENT_CHANGE,
 					    var);
 			g_free(adapter);
 			g_free(device);
@@ -1541,3 +1579,87 @@ EXPORT gboolean bluez_set_pincode(const char *pincode)
 
 	return rc;
 }
+
+EXPORT gboolean bluez_get_media_control_properties(const char *device, GVariant **reply)
+{
+	struct bluez_state *ns = bluez_get_state();
+	GVariant *properties = NULL;
+	GError *error = NULL;
+
+	if (!ns) {
+		ERROR("Invalid state");
+		return FALSE;
+	}
+	if (!reply)
+		return FALSE;
+
+	gchar *device_path = get_bluez_path(NULL, device);
+	properties = bluez_get_properties(ns,
+					  BLUEZ_AT_MEDIACONTROL,
+					  device_path,
+					  &error);
+	if (error) {
+		ERROR("bluez_get_properties error: %s", error->message);
+		g_clear_error(&error);
+		*reply = NULL;
+	}
+
+	// Pull properties out of tuple so caller does not have to
+	g_variant_get(properties, "(@a{sv})", reply);
+	g_variant_unref(properties);
+
+	return TRUE;
+}
+
+EXPORT gboolean bluez_get_media_player_properties(const char *device, GVariant **reply)
+{
+	struct bluez_state *ns = bluez_get_state();
+	GVariant *properties = NULL;
+	GError *error = NULL;
+
+	if (!ns) {
+		ERROR("Invalid state");
+		return FALSE;
+	}
+	if (!reply)
+		return FALSE;
+
+	gchar *player = NULL;
+	gchar *device_path = get_bluez_path(NULL, device);
+	if (device_path) {
+		// TODO: handle multiple players per device
+		GVariant *val = bluez_get_property(ns, BLUEZ_AT_MEDIACONTROL, device_path, "Player", NULL);
+		if (val) {
+			player = g_variant_get_string(val, NULL);
+			g_variant_unref(val);
+		}
+		if (!player)
+			player = g_strconcat(device_path, "/", BLUEZ_DEFAULT_PLAYER, NULL);
+
+		g_free(device_path);
+	} else {
+		player = g_strdup(ns->mediaplayer_path);
+	}
+
+	if (!player) {
+		ERROR("No path given");
+		return FALSE;
+	}
+
+	DEBUG("Using player %s", player);
+	properties = bluez_get_properties(ns,
+					  BLUEZ_AT_MEDIAPLAYER,
+					  player,
+					  &error);
+	if (error) {
+		ERROR("bluez_get_properties error: %s", error->message);
+		g_clear_error(&error);
+		*reply = NULL;
+	}
+
+	// Pull properties out of tuple so caller does not have to
+	g_variant_get(properties, "(@a{sv})", reply);
+	g_variant_unref(properties);
+
+	return TRUE;
+}
diff --git a/src/bluez-call.c b/src/bluez-call.c
index 86f9b9c..5c0fc77 100644
--- a/src/bluez-call.c
+++ b/src/bluez-call.c
@@ -229,6 +229,8 @@ bluez_call_async(struct bluez_state *ns,
 	}
 	cpw->callback = callback;
 
+	DEBUG("calling %s:%s:%s on %s:%s",
+	      interface, method, params, BLUEZ_SERVICE, path);
 	g_dbus_connection_call(ns->conn,
 			       BLUEZ_SERVICE, path, interface, method, params,
 			       NULL,	/* reply type */
@@ -281,6 +283,8 @@ GVariant *bluez_get_properties(struct bluez_state *ns,
 		return NULL;
 	}
 
+	DEBUG("calling %s:%s:%s on %s:%s",
+	      interface, method, interface2, BLUEZ_SERVICE, path);
 	reply = g_dbus_connection_call_sync(ns->conn,
 					    BLUEZ_SERVICE, path, interface, method,
 					    interface2 ? g_variant_new("(s)", interface2) : NULL,
@@ -443,8 +447,8 @@ gboolean bluez_autoconnect(gpointer data)
 
 		gboolean paired = FALSE;
 		if (g_variant_dict_lookup(props_dict, "Paired", "b", &paired) && paired) {
-			gchar *path = g_strconcat("/org/bluez/", adapter, "/", key, NULL);
-			GVariant *connect_reply = bluez_call(ns, "device", path, "Connect", NULL, NULL);
+			gchar *path = g_strconcat(adapter, "/", key, NULL);
+			GVariant *connect_reply = bluez_call(ns, BLUEZ_AT_DEVICE, path, "Connect", NULL, NULL);
 			g_free(path);
 			if (!connect_reply)
 				continue;
diff --git a/src/meson.build b/src/meson.build
index 2ce6cb0..e774d21 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -14,6 +14,7 @@ if get_option('build-tester')
     executable('bluez-glib-test',
                'test.c',
                include_directories: inc,
-               dependencies: [systemd_dep, glib_deps, lib_dep])
+               dependencies: [systemd_dep, glib_deps, lib_dep],
+               install: true)
 endif