[PATCH 01/13] AVRCP: Update constants

[PATCH 01/13] AVRCP: Update constants

[Michal Labedzki]

Change all specification constants to enum type.
Also update constants for AVRCP 1.4.
---
 audio/avrcp.c |  187 ++++++++++++++++++++++++++++++++++++---------------------
 audio/avrcp.h |  108 +++++++++++++++++++--------------
 audio/media.c |    2 +-
 3 files changed, 183 insertions(+), 114 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 89ee112..196d6a5 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -58,56 +58,109 @@
 #include "sdpd.h"
 #include "dbus-common.h"
 
-/* Company IDs for vendor dependent commands */
-#define IEEEID_BTSIG		0x001958
-
-/* Error codes for metadata transfer */
-#define E_INVALID_COMMAND	0x00
-#define E_INVALID_PARAM		0x01
-#define E_PARAM_NOT_FOUND	0x02
-#define E_INTERNAL		0x03
-
-/* Packet types */
-#define AVRCP_PACKET_TYPE_SINGLE	0x00
-#define AVRCP_PACKET_TYPE_START		0x01
-#define AVRCP_PACKET_TYPE_CONTINUING	0x02
-#define AVRCP_PACKET_TYPE_END		0x03
-
-/* PDU types for metadata transfer */
-#define AVRCP_GET_CAPABILITIES		0x10
-#define AVRCP_LIST_PLAYER_ATTRIBUTES	0X11
-#define AVRCP_LIST_PLAYER_VALUES	0x12
-#define AVRCP_GET_CURRENT_PLAYER_VALUE	0x13
-#define AVRCP_SET_PLAYER_VALUE		0x14
-#define AVRCP_GET_PLAYER_ATTRIBUTE_TEXT	0x15
-#define AVRCP_GET_PLAYER_VALUE_TEXT	0x16
-#define AVRCP_DISPLAYABLE_CHARSET	0x17
-#define AVRCP_CT_BATTERY_STATUS		0x18
-#define AVRCP_GET_ELEMENT_ATTRIBUTES	0x20
-#define AVRCP_GET_PLAY_STATUS		0x30
-#define AVRCP_REGISTER_NOTIFICATION	0x31
-#define AVRCP_REQUEST_CONTINUING	0x40
-#define AVRCP_ABORT_CONTINUING		0x41
-#define AVRCP_SET_ABSOLUTE_VOLUME	0x50
-
-/* Capabilities for AVRCP_GET_CAPABILITIES pdu */
-#define CAP_COMPANY_ID		0x02
-#define CAP_EVENTS_SUPPORTED	0x03
-
 #define AVRCP_REGISTER_NOTIFICATION_PARAM_LENGTH 5
 
-#define AVRCP_FEATURE_CATEGORY_1	0x0001
-#define AVRCP_FEATURE_CATEGORY_2	0x0002
-#define AVRCP_FEATURE_CATEGORY_3	0x0004
-#define AVRCP_FEATURE_CATEGORY_4	0x0008
-#define AVRCP_FEATURE_PLAYER_SETTINGS	0x0010
+enum company_id {
+	IEEEID_BTSIG	 = 0x001958
+};
+
+enum status_code {
+	STATUS_INVALID_COMMAND		= 0x00,
+	STATUS_INVALID_PARAMETER	= 0x01,
+	STATUS_PARAMETER_NOT_FOUND	= 0x02,
+	STATUS_INTERNAL_ERROR		= 0x03,
+	STATUS_OK			= 0x04,
+	STATUS_UID_CHANGED		= 0x05,
+	STATUS_INVALID_DIRECTION	= 0x07,
+	STATUS_NOT_DIRECTORY		= 0x08,
+	STATUS_DOES_NOT_EXIST		= 0x09,
+	STATUS_INVALID_SCOPE		= 0x0A,
+	STATUS_RANGE_OUT_OF_BOUNDS	= 0x0B,
+	STATUS_UID_IS_DIRECTORY		= 0x0C,
+	STATUS_MEDIA_IN_USE		= 0x0D,
+	STATUS_NOW_PLAYING_LIST_FULL	= 0x0E,
+	STATUS_SEARCH_NOT_SUPPORTED	= 0x0F,
+	STATUS_SEARCH_IN_PROGRESS	= 0x10,
+	STATUS_INVALID_PLAYER_ID	= 0x11,
+	STATUS_PLAYER_NOT_BROWSABLE	= 0x12,
+	STATUS_PLAYER_NOT_ADDRESSED	= 0x13,
+	STATUS_NO_VALID_SEARCH_RESULTS	= 0x14,
+	STATUS_NO_AVAILABLE_PLAYERS	= 0x15,
+	STATUS_ADDRESSED_PLAYER_CHANGED	= 0x16
+};
+
+enum packet_type {
+	AVRCP_PACKET_TYPE_SINGLE	= 0x00,
+	AVRCP_PACKET_TYPE_START		= 0x01,
+	AVRCP_PACKET_TYPE_CONTINUING	= 0x02,
+	AVRCP_PACKET_TYPE_END		= 0x03
+};
+
+enum control_pdu_ieeeid_btsig {
+	AVRCP_GET_CAPABILITIES		= 0x10,
+	AVRCP_LIST_PLAYER_ATTRIBUTES	= 0X11,
+	AVRCP_LIST_PLAYER_VALUES	= 0x12,
+	AVRCP_GET_CURRENT_PLAYER_VALUE	= 0x13,
+	AVRCP_SET_PLAYER_VALUE		= 0x14,
+	AVRCP_GET_PLAYER_ATTRIBUTE_TEXT	= 0x15,
+	AVRCP_GET_PLAYER_VALUE_TEXT	= 0x16,
+	AVRCP_DISPLAYABLE_CHARSET	= 0x17,
+	AVRCP_CT_BATTERY_STATUS		= 0x18,
+	AVRCP_GET_ELEMENT_ATTRIBUTES	= 0x20,
+	AVRCP_GET_PLAY_STATUS		= 0x30,
+	AVRCP_REGISTER_NOTIFICATION	= 0x31,
+	AVRCP_REQUEST_CONTINUING	= 0x40,
+	AVRCP_ABORT_CONTINUING		= 0x41,
+	AVRCP_SET_ABSOLUTE_VOLUME	= 0x50,
+	AVRCP_SET_ADDRESSED_PLAYER	= 0x60,
+	AVRCP_PLAY_ITEM			= 0x74,
+	AVRCP_ADD_TO_NOW_PLAYING	= 0x90
+};
+
+enum browsing_pdu {
+	AVRCP_SET_BROWSED_PLAYER	= 0x70,
+	AVRCP_GET_FOLDER_ITEMS		= 0x71,
+	AVRCP_CHANGE_PATH		= 0x72,
+	AVRCP_GET_ITEM_ATTRIBUTES	= 0x73,
+	AVRCP_SEARCH			= 0x80,
+	AVRCP_GENERAL_REJECT		= 0xA0
+};
+
+enum capability {
+	CAP_COMPANY_ID		= 0x02,
+	CAP_EVENTS_SUPPORTED	= 0x03
+};
+
+enum sdp_feature {
+	AVRCP_FEATURE_CATEGORY_1	= 0x0001,
+	AVRCP_FEATURE_CATEGORY_2	= 0x0002,
+	AVRCP_FEATURE_CATEGORY_3	= 0x0004,
+	AVRCP_FEATURE_CATEGORY_4	= 0x0008,
+	AVRCP_FEATURE_PLAYER_SETTINGS	= 0x0010,
+	AVRCP_FEATURE_GROUP_NAVIGATION	= 0x0020,
+	AVRCP_FEATURE_BROWSING		= 0x0040,
+	AVRCP_FEATURE_MULTIPLE_PLAYERS	= 0x0080
+};
+
+enum scope {
+	AVRCP_SCOPE_MEDIA_PLAYER_LIST	= 0x00,
+	AVRCP_SCOPE_VFS			= 0x01,
+	AVRCP_SCOPE_SEARCH		= 0x02,
+	AVRCP_SCOPE_NOW_PLAYING		= 0x03
+};
 
 enum battery_status {
-	BATTERY_STATUS_NORMAL =		0,
-	BATTERY_STATUS_WARNING =	1,
-	BATTERY_STATUS_CRITICAL =	2,
-	BATTERY_STATUS_EXTERNAL =	3,
-	BATTERY_STATUS_FULL_CHARGE =	4,
+	BATTERY_STATUS_NORMAL		= 0,
+	BATTERY_STATUS_WARNING		= 1,
+	BATTERY_STATUS_CRITICAL		= 2,
+	BATTERY_STATUS_EXTERNAL		= 3,
+	BATTERY_STATUS_FULL_CHARGE	= 4
+};
+
+enum avrcp_version {
+	AVRCP_VERSION_UNKNOWN	= 0x0000,
+	AVRCP_VERSION_1_3	= 0x0103,
+	AVRCP_VERSION_1_4	= 0x0104
 };
 
 #if __BYTE_ORDER == __LITTLE_ENDIAN
@@ -396,7 +449,7 @@ int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
 	DBG("id=%u", id);
 
 	switch (id) {
-	case AVRCP_EVENT_STATUS_CHANGED:
+	case AVRCP_EVENT_PLAYBACK_STATUS_CHANGED:
 		size = 2;
 		pdu->params[1] = *((uint8_t *)data);
 
@@ -581,7 +634,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp_player *player,
 		return AVC_CTYPE_STABLE;
 	case CAP_EVENTS_SUPPORTED:
 		pdu->params[1] = 4;
-		pdu->params[2] = AVRCP_EVENT_STATUS_CHANGED;
+		pdu->params[2] = AVRCP_EVENT_PLAYBACK_STATUS_CHANGED;
 		pdu->params[3] = AVRCP_EVENT_TRACK_CHANGED;
 		pdu->params[4] = AVRCP_EVENT_TRACK_REACHED_START;
 		pdu->params[5] = AVRCP_EVENT_TRACK_REACHED_END;
@@ -592,7 +645,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp_player *player,
 
 err:
 	pdu->params_len = htons(1);
-	pdu->params[0] = E_INVALID_PARAM;
+	pdu->params[0] = STATUS_INVALID_PARAMETER;
 
 	return AVC_CTYPE_REJECTED;
 }
@@ -606,7 +659,7 @@ static uint8_t avrcp_handle_list_player_attributes(struct avrcp_player *player,
 
 	if (len != 0) {
 		pdu->params_len = htons(1);
-		pdu->params[0] = E_INVALID_PARAM;
+		pdu->params[0] = STATUS_INVALID_PARAMETER;
 		return AVC_CTYPE_REJECTED;
 	}
 
@@ -653,7 +706,7 @@ static uint8_t avrcp_handle_list_player_values(struct avrcp_player *player,
 
 err:
 	pdu->params_len = htons(1);
-	pdu->params[0] = E_INVALID_PARAM;
+	pdu->params[0] = STATUS_INVALID_PARAMETER;
 	return AVC_CTYPE_REJECTED;
 }
 
@@ -724,7 +777,7 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
 	return AVC_CTYPE_STABLE;
 err:
 	pdu->params_len = htons(1);
-	pdu->params[0] = E_INVALID_PARAM;
+	pdu->params[0] = STATUS_INVALID_PARAMETER;
 	return AVC_CTYPE_REJECTED;
 }
 
@@ -781,7 +834,7 @@ static uint8_t avrcp_handle_get_current_player_value(struct avrcp_player *player
 
 err:
 	pdu->params_len = htons(1);
-	pdu->params[0] = E_INVALID_PARAM;
+	pdu->params[0] = STATUS_INVALID_PARAMETER;
 
 	return AVC_CTYPE_REJECTED;
 }
@@ -802,7 +855,7 @@ static uint8_t avrcp_handle_set_player_value(struct avrcp_player *player,
 	 * and set the existent ones. Sec. 5.2.4 is not clear however how to
 	 * indicate that a certain ID was not accepted. If at least one
 	 * attribute is valid, we respond with no parameters. Otherwise an
-	 * E_INVALID_PARAM is sent.
+	 * STATUS_INVALID_PARAMETER is sent.
 	 */
 	for (len = 0, i = 0, param = &pdu->params[1]; i < pdu->params[0];
 							i++, param += 2) {
@@ -820,7 +873,7 @@ static uint8_t avrcp_handle_set_player_value(struct avrcp_player *player,
 
 err:
 	pdu->params_len = htons(1);
-	pdu->params[0] = E_INVALID_PARAM;
+	pdu->params[0] = STATUS_INVALID_PARAMETER;
 	return AVC_CTYPE_REJECTED;
 }
 
@@ -832,7 +885,7 @@ static uint8_t avrcp_handle_displayable_charset(struct avrcp_player *player,
 
 	if (len < 3) {
 		pdu->params_len = htons(1);
-		pdu->params[0] = E_INVALID_PARAM;
+		pdu->params[0] = STATUS_INVALID_PARAMETER;
 		return AVC_CTYPE_REJECTED;
 	}
 
@@ -864,7 +917,7 @@ static uint8_t avrcp_handle_ct_battery_status(struct avrcp_player *player,
 
 err:
 	pdu->params_len = htons(1);
-	pdu->params[0] = E_INVALID_PARAM;
+	pdu->params[0] = STATUS_INVALID_PARAMETER;
 	return AVC_CTYPE_REJECTED;
 }
 
@@ -879,7 +932,7 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp_player *player,
 
 	if (len != 0) {
 		pdu->params_len = htons(1);
-		pdu->params[0] = E_INVALID_PARAM;
+		pdu->params[0] = STATUS_INVALID_PARAMETER;
 		return AVC_CTYPE_REJECTED;
 	}
 
@@ -918,7 +971,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
 		goto err;
 
 	switch (pdu->params[0]) {
-	case AVRCP_EVENT_STATUS_CHANGED:
+	case AVRCP_EVENT_PLAYBACK_STATUS_CHANGED:
 		len = 2;
 		pdu->params[1] = player->cb->get_status(player->user_data);
 
@@ -948,7 +1001,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
 
 err:
 	pdu->params_len = htons(1);
-	pdu->params[0] = E_INVALID_PARAM;
+	pdu->params[0] = STATUS_INVALID_PARAMETER;
 	return AVC_CTYPE_REJECTED;
 }
 
@@ -988,7 +1041,7 @@ static uint8_t avrcp_handle_request_continuing(struct avrcp_player *player,
 	return AVC_CTYPE_STABLE;
 err:
 	pdu->params_len = htons(1);
-	pdu->params[0] = E_INVALID_PARAM;
+	pdu->params[0] = STATUS_INVALID_PARAMETER;
 	return AVC_CTYPE_REJECTED;
 }
 
@@ -1014,7 +1067,7 @@ static uint8_t avrcp_handle_abort_continuing(struct avrcp_player *player,
 
 err:
 	pdu->params_len = htons(1);
-	pdu->params[0] = E_INVALID_PARAM;
+	pdu->params[0] = STATUS_INVALID_PARAMETER;
 	return AVC_CTYPE_REJECTED;
 }
 
@@ -1079,7 +1132,7 @@ static size_t handle_vendordep_pdu(struct avctp *session, uint8_t transaction,
 	pdu->rsvd = 0;
 
 	if (operand_count < AVRCP_HEADER_LENGTH) {
-		pdu->params[0] = E_INVALID_COMMAND;
+		pdu->params[0] = STATUS_INVALID_COMMAND;
 		goto err_metadata;
 	}
 
@@ -1089,12 +1142,12 @@ static size_t handle_vendordep_pdu(struct avctp *session, uint8_t transaction,
 	}
 
 	if (!handler || handler->code != *code) {
-		pdu->params[0] = E_INVALID_COMMAND;
+		pdu->params[0] = STATUS_INVALID_COMMAND;
 		goto err_metadata;
 	}
 
 	if (!handler->func) {
-		pdu->params[0] = E_INVALID_PARAM;
+		pdu->params[0] = STATUS_INVALID_PARAMETER;
 		goto err_metadata;
 	}
 
@@ -1122,7 +1175,7 @@ size_t avrcp_handle_vendor_reject(uint8_t *code, uint8_t *operands)
 
     *code = AVC_CTYPE_REJECTED;
     pdu->params_len = htons(1);
-    pdu->params[0] = E_INTERNAL;
+    pdu->params[0] = STATUS_INTERNAL_ERROR;
 
     DBG("rejecting AVRCP PDU 0x%02X, company 0x%06X len 0x%04X",
             pdu->pdu_id, company_id, pdu->params_len);
@@ -1236,7 +1289,7 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
 
 		desc = list->data;
 
-		if (desc && desc->version >= 0x0104)
+		if (desc && desc->version >= AVRCP_VERSION_1_4)
 			register_volume_notification(player);
 
 		sdp_list_free(list, free);
diff --git a/audio/avrcp.h b/audio/avrcp.h
index bf11a6c..0045a2c 100644
--- a/audio/avrcp.h
+++ b/audio/avrcp.h
@@ -22,59 +22,75 @@
  *
  */
 
-/* player attributes */
-#define AVRCP_ATTRIBUTE_ILEGAL		0x00
-#define AVRCP_ATTRIBUTE_EQUALIZER	0x01
-#define AVRCP_ATTRIBUTE_REPEAT_MODE	0x02
-#define AVRCP_ATTRIBUTE_SHUFFLE		0x03
-#define AVRCP_ATTRIBUTE_SCAN		0x04
+enum player_attribute {
+	AVRCP_ATTRIBUTE_ILEGAL		= 0x00,
+	AVRCP_ATTRIBUTE_EQUALIZER	= 0x01,
+	AVRCP_ATTRIBUTE_REPEAT_MODE	= 0x02,
+	AVRCP_ATTRIBUTE_SHUFFLE		= 0x03,
+	AVRCP_ATTRIBUTE_SCAN		= 0x04
+};
 
-/* equalizer values */
-#define AVRCP_EQUALIZER_OFF		0x01
-#define AVRCP_EQUALIZER_ON		0x02
+enum equalizer_value {
+	AVRCP_EQUALIZER_OFF		= 0x01,
+	AVRCP_EQUALIZER_ON		= 0x02
+};
 
-/* repeat mode values */
-#define AVRCP_REPEAT_MODE_OFF		0x01
-#define AVRCP_REPEAT_MODE_SINGLE	0x02
-#define AVRCP_REPEAT_MODE_ALL		0x03
-#define AVRCP_REPEAT_MODE_GROUP		0x04
+enum repeat_mode_value {
+	AVRCP_REPEAT_MODE_OFF		= 0x01,
+	AVRCP_REPEAT_MODE_SINGLE	= 0x02,
+	AVRCP_REPEAT_MODE_ALL		= 0x03,
+	AVRCP_REPEAT_MODE_GROUP		= 0x04
+};
 
-/* shuffle values */
-#define AVRCP_SHUFFLE_OFF		0x01
-#define AVRCP_SHUFFLE_ALL		0x02
-#define AVRCP_SHUFFLE_GROUP		0x03
+enum shuffle_value {
+	AVRCP_SHUFFLE_OFF		= 0x01,
+	AVRCP_SHUFFLE_ALL		= 0x02,
+	AVRCP_SHUFFLE_GROUP		= 0x03
+};
 
-/* scan values */
-#define AVRCP_SCAN_OFF			0x01
-#define AVRCP_SCAN_ALL			0x02
-#define AVRCP_SCAN_GROUP		0x03
+enum scan_value {
+	AVRCP_SCAN_OFF			= 0x01,
+	AVRCP_SCAN_ALL			= 0x02,
+	AVRCP_SCAN_GROUP		= 0x03
+};
 
-/* media attributes */
-#define AVRCP_MEDIA_ATTRIBUTE_ILLEGAL	0x00
-#define AVRCP_MEDIA_ATTRIBUTE_TITLE	0x01
-#define AVRCP_MEDIA_ATTRIBUTE_ARTIST	0x02
-#define AVRCP_MEDIA_ATTRIBUTE_ALBUM	0x03
-#define AVRCP_MEDIA_ATTRIBUTE_TRACK	0x04
-#define AVRCP_MEDIA_ATTRIBUTE_N_TRACKS	0x05
-#define AVRCP_MEDIA_ATTRIBUTE_GENRE	0x06
-#define AVRCP_MEDIA_ATTRIBUTE_DURATION	0x07
-#define AVRCP_MEDIA_ATTRIBUTE_LAST	AVRCP_MEDIA_ATTRIBUTE_DURATION
+enum media_attribute {
+	AVRCP_MEDIA_ATTRIBUTE_ILLEGAL	= 0x00,
+	AVRCP_MEDIA_ATTRIBUTE_TITLE	= 0x01,
+	AVRCP_MEDIA_ATTRIBUTE_ARTIST	= 0x02,
+	AVRCP_MEDIA_ATTRIBUTE_ALBUM	= 0x03,
+	AVRCP_MEDIA_ATTRIBUTE_TRACK	= 0x04,
+	AVRCP_MEDIA_ATTRIBUTE_N_TRACKS	= 0x05,
+	AVRCP_MEDIA_ATTRIBUTE_GENRE	= 0x06,
+	AVRCP_MEDIA_ATTRIBUTE_DURATION	= 0x07,
+	AVRCP_MEDIA_ATTRIBUTE_LAST	= AVRCP_MEDIA_ATTRIBUTE_DURATION
+};
 
-/* play status */
-#define AVRCP_PLAY_STATUS_STOPPED	0x00
-#define AVRCP_PLAY_STATUS_PLAYING	0x01
-#define AVRCP_PLAY_STATUS_PAUSED	0x02
-#define AVRCP_PLAY_STATUS_FWD_SEEK	0x03
-#define AVRCP_PLAY_STATUS_REV_SEEK	0x04
-#define AVRCP_PLAY_STATUS_ERROR		0xFF
+enum play_status {
+	AVRCP_PLAY_STATUS_STOPPED	= 0x00,
+	AVRCP_PLAY_STATUS_PLAYING	= 0x01,
+	AVRCP_PLAY_STATUS_PAUSED	= 0x02,
+	AVRCP_PLAY_STATUS_FWD_SEEK	= 0x03,
+	AVRCP_PLAY_STATUS_REV_SEEK	= 0x04,
+	AVRCP_PLAY_STATUS_ERROR		= 0xFF
+};
 
-/* Notification events */
-#define AVRCP_EVENT_STATUS_CHANGED	0x01
-#define AVRCP_EVENT_TRACK_CHANGED	0x02
-#define AVRCP_EVENT_TRACK_REACHED_END	0x03
-#define AVRCP_EVENT_TRACK_REACHED_START	0x04
-#define AVRCP_EVENT_VOLUME_CHANGED	0x0d
-#define AVRCP_EVENT_LAST		AVRCP_EVENT_VOLUME_CHANGED
+enum notification_event {
+	AVRCP_EVENT_PLAYBACK_STATUS_CHANGED		= 0x01,
+	AVRCP_EVENT_TRACK_CHANGED			= 0x02,
+	AVRCP_EVENT_TRACK_REACHED_END			= 0x03,
+	AVRCP_EVENT_TRACK_REACHED_START			= 0x04,
+	AVRCP_EVENT_PLAYBACK_POS_CHANGED		= 0x05,
+	AVRCP_EVENT_BATT_STATUS_CHANGED			= 0x06,
+	AVRCP_EVENT_SYSTEM_STATUS_CHANGED		= 0x07,
+	AVRCP_EVENT_PLAYER_APPLICATION_SETTING_CHANGED	= 0x08,
+	AVRCP_EVENT_NOW_PLAYING_CONTENT_CHANGED		= 0x09,
+	AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED		= 0x0A,
+	AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED		= 0x0B,
+	AVRCP_EVENT_UIDS_CHANGED			= 0x0C,
+	AVRCP_EVENT_VOLUME_CHANGED			= 0x0D,
+	AVRCP_EVENT_LAST				= AVRCP_EVENT_VOLUME_CHANGED
+};
 
 struct avrcp_player_cb {
 	int (*get_setting) (uint8_t attr, void *user_data);
diff --git a/audio/media.c b/audio/media.c
index 1956653..4e23273 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1414,7 +1414,7 @@ static gboolean set_status(struct media_player *mp, DBusMessageIter *iter)
 
 	mp->status = val;
 
-	avrcp_player_event(mp->player, AVRCP_EVENT_STATUS_CHANGED, &val);
+	avrcp_player_event(mp->player, AVRCP_EVENT_PLAYBACK_STATUS_CHANGED, &val);
 
 	return TRUE;
 }
-- 
on behalf of ST-Ericsson

[PATCH 02/13] AVRCP: Use name "addressed" instead of "active"

[Michal Labedzki]

AVRCP 1.4 named currently controlled player as "addressed", so use
this name instead of "active". In future there where be more "browsing"
player that will be active, but not addressed.
---
 audio/avrcp.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 196d6a5..7576e19 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -199,7 +199,7 @@ struct avrcp_server {
 	uint32_t tg_record_id;
 	uint32_t ct_record_id;
 	GSList *players;
-	struct avrcp_player *active_player;
+	struct avrcp_player *addressed_player;
 };
 
 struct pending_pdu {
@@ -1253,7 +1253,7 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
 	if (!server)
 		return;
 
-	player = server->active_player;
+	player = server->addressed_player;
 	if (!player)
 		return;
 
@@ -1446,7 +1446,7 @@ struct avrcp_player *avrcp_register_player(const bdaddr_t *src,
 	player->destroy = destroy;
 
 	if (!server->players)
-		server->active_player = player;
+		server->addressed_player = player;
 
 	if (!avctp_id)
 		avctp_id = avctp_add_state_cb(state_changed, NULL);
@@ -1462,8 +1462,8 @@ void avrcp_unregister_player(struct avrcp_player *player)
 
 	server->players = g_slist_remove(server->players, player);
 
-	if (server->active_player == player)
-		server->active_player = g_slist_nth_data(server->players, 0);
+	if (server->addressed_player == player)
+		server->addressed_player = g_slist_nth_data(server->players, 0);
 
 	player_destroy(player);
 }
@@ -1498,7 +1498,7 @@ int avrcp_set_volume(struct audio_device *dev, uint8_t volume)
 	if (server == NULL)
 		return -EINVAL;
 
-	player = server->active_player;
+	player = server->addressed_player;
 	if (player == NULL)
 		return -ENOTSUP;
 
-- 
on behalf of ST-Ericsson

[PATCH 03/13] media: Rename set_property to set_player_property

[Michal Labedzki]

This is to avoid conflicts with DBus interface convention
used in BlueZ.
---
 audio/media.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/audio/media.c b/audio/media.c
index 4e23273..cc8ac37 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1454,7 +1454,7 @@ static gboolean set_position(struct media_player *mp, DBusMessageIter *iter)
 	return TRUE;
 }
 
-static gboolean set_property(struct media_player *mp, const char *key,
+static gboolean set_player_property(struct media_player *mp, const char *key,
 							DBusMessageIter *entry)
 {
 	DBusMessageIter var;
@@ -1515,7 +1515,7 @@ static gboolean property_changed(DBusConnection *connection, DBusMessage *msg,
 
 	dbus_message_iter_next(&iter);
 
-	set_property(mp, property, &iter);
+	set_player_property(mp, property, &iter);
 
 	return TRUE;
 }
@@ -1747,7 +1747,7 @@ static gboolean parse_player_properties(struct media_player *mp,
 		dbus_message_iter_get_basic(&entry, &key);
 		dbus_message_iter_next(&entry);
 
-		if (set_property(mp, key, &entry) == FALSE)
+		if (set_player_property(mp, key, &entry) == FALSE)
 			return FALSE;
 
 		dbus_message_iter_next(&dict);
-- 
on behalf of ST-Ericsson

[PATCH 04/13] AVRCP: Keep AVRCP version of connected device in server

[Michal Labedzki]

This can be used to improve compatibility between
AVRCP 1.3 vs AVRCP 1.4 devices and implementation in stack.
---
 audio/avrcp.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 7576e19..9ff0700 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -200,6 +200,7 @@ struct avrcp_server {
 	uint32_t ct_record_id;
 	GSList *players;
 	struct avrcp_player *addressed_player;
+	uint16_t version;
 };
 
 struct pending_pdu {
@@ -1288,8 +1289,12 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
 			return;
 
 		desc = list->data;
+		if (desc)
+			server->version = desc->version;
+		else
+			server->version = AVRCP_VERSION_UNKNOWN;
 
-		if (desc && desc->version >= AVRCP_VERSION_1_4)
+		if (server->version >= AVRCP_VERSION_1_4)
 			register_volume_notification(player);
 
 		sdp_list_free(list, free);
@@ -1380,6 +1385,8 @@ int avrcp_register(DBusConnection *conn, const bdaddr_t *src, GKeyFile *config)
 
 	bacpy(&server->src, src);
 
+	server->version = AVRCP_VERSION_UNKNOWN;
+
 	servers = g_slist_append(servers, server);
 
 	return 0;
-- 
on behalf of ST-Ericsson

[PATCH 05/13] AVRCP: Register AVRCP before MEDIA interface

[Michal Labedzki]

Register AVRCP before MEDIA interface to avoid searching for or
accessing non-existent AVRCP server.
---
 audio/manager.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/audio/manager.c b/audio/manager.c
index d442d1d..076346a 100644
--- a/audio/manager.c
+++ b/audio/manager.c
@@ -1238,6 +1238,9 @@ proceed:
 	if (enabled.socket)
 		unix_init();
 
+	if (enabled.control)
+		btd_register_adapter_driver(&avrcp_server_driver);
+
 	if (enabled.media)
 		btd_register_adapter_driver(&media_server_driver);
 
@@ -1250,9 +1253,6 @@ proceed:
 	if (enabled.source || enabled.sink)
 		btd_register_adapter_driver(&a2dp_server_driver);
 
-	if (enabled.control)
-		btd_register_adapter_driver(&avrcp_server_driver);
-
 	btd_register_device_driver(&audio_driver);
 
 	*enable_sco = (enabled.gateway || enabled.headset);
-- 
on behalf of ST-Ericsson

[PATCH 06/13] media: Add Properties stubs to Media interface

[Michal Labedzki]

Add Properties stubs to Media interface.
---
 audio/media.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/audio/media.c b/audio/media.c
index cc8ac37..5010ea5 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1821,6 +1821,55 @@ static DBusMessage *unregister_player(DBusConnection *conn, DBusMessage *msg,
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
+static DBusMessage *get_properties(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct media_adapter *adapter = data;
+	DBusMessage *reply;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+	GSList *l;
+
+	reply = dbus_message_new_method_return(msg);
+	if (!reply)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+			DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+			DBUS_TYPE_STRING_AS_STRING DBUS_TYPE_VARIANT_AS_STRING
+			DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &dict);
+
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
+
+static DBusMessage *set_property(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct media_adapter *adapter = data;
+	DBusMessageIter iter;
+	DBusMessageIter sub;
+	const char *property;
+
+	if (!dbus_message_iter_init(msg, &iter))
+		return btd_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+		return btd_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&iter, &property);
+	dbus_message_iter_next(&iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
+		return btd_error_invalid_args(msg);
+	dbus_message_iter_recurse(&iter, &sub);
+
+	return btd_error_invalid_args(msg);
+}
+
 static const GDBusMethodTable media_methods[] = {
 	{ GDBUS_METHOD("RegisterEndpoint",
 		GDBUS_ARGS({ "endpoint", "o" }, { "properties", "a{sv}" }),
@@ -1833,6 +1882,11 @@ static const GDBusMethodTable media_methods[] = {
 		NULL, register_player) },
 	{ GDBUS_METHOD("UnregisterPlayer",
 		GDBUS_ARGS({ "player", "o" }), NULL, unregister_player) },
+	{ GDBUS_METHOD("GetProperties",
+		NULL, GDBUS_ARGS({ "properties", "a{sv}" }), get_properties) },
+	{ GDBUS_ASYNC_METHOD("SetProperty",
+		GDBUS_ARGS({ "name", "s" }, { "value", "v" }),
+		NULL, set_property) },
 	{ },
 };
 
-- 
on behalf of ST-Ericsson

[PATCH 07/13] media: Add support for local changing addressed player

[Michal Labedzki]

There is possibility to register more than one player. This patch
allows to change currently selected player from the list of previously
registered players.

This patch extends Media interface for Addressed Player procedure and
adds Default Dummy Player to avoid confusion with AVRCP 1.3
devices.
---
 audio/media.c |  146 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 144 insertions(+), 2 deletions(-)

diff --git a/audio/media.c b/audio/media.c
index 5010ea5..3455496 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -54,6 +54,8 @@
 #define MEDIA_ENDPOINT_INTERFACE "org.bluez.MediaEndpoint"
 #define MEDIA_PLAYER_INTERFACE "org.bluez.MediaPlayer"
 
+#define DUMMY_PLAYER "/dummy"
+
 #define REQUEST_TIMEOUT (3 * 1000)		/* 3 seconds */
 
 struct media_adapter {
@@ -62,6 +64,7 @@ struct media_adapter {
 	DBusConnection		*conn;		/* Adapter connection */
 	GSList			*endpoints;	/* Endpoints list */
 	GSList			*players;	/* Players list */
+	struct media_player	*addressed;	/* Addressed Player */
 };
 
 struct endpoint_request {
@@ -102,6 +105,7 @@ struct media_player {
 	uint8_t			status;
 	uint32_t		position;
 	uint8_t			volume;
+	uint16_t		id;
 	GTimer			*timer;
 };
 
@@ -115,6 +119,11 @@ struct metadata_value {
 
 static GSList *adapters = NULL;
 
+static struct media_player *media_player_create(struct media_adapter *adapter,
+						const char *sender,
+						const char *path,
+						int *err);
+
 static void endpoint_request_free(struct endpoint_request *request)
 {
 	if (request->call)
@@ -961,6 +970,25 @@ static struct media_player *media_adapter_find_player(
 	return NULL;
 }
 
+static struct media_player *media_adapter_find_player_by_id(
+						struct media_adapter *adapter,
+						const char *sender,
+						uint16_t player_id) {
+	GSList *l;
+
+	for (l = adapter->players; l; l = l->next) {
+		struct media_player *mp = l->data;
+
+		if (sender && g_strcmp0(mp->sender, sender) != 0)
+			continue;
+
+		if (player_id == mp->id)
+			return mp;
+	}
+
+	return NULL;
+}
+
 static void release_player(struct media_player *mp)
 {
 	DBusMessage *msg;
@@ -1370,6 +1398,51 @@ static void set_volume(uint8_t volume, struct audio_device *dev, void *user_data
 	}
 }
 
+static void media_update_players(struct media_adapter *adapter)
+{
+	char **players;
+	int i;
+	GSList *l;
+	struct media_player *mp = NULL;
+
+	players = g_new0(char *, g_slist_length(adapter->players) + 1);
+	for (i = 0, l = adapter->players; l; l = l->next, i++) {
+		mp = l->data;
+		players[i] = mp->path;
+	}
+
+	emit_array_property_changed(adapter->conn, adapter->path,
+					MEDIA_INTERFACE, "Players",
+					DBUS_TYPE_OBJECT_PATH, &players, i);
+	g_free(players);
+}
+
+static gboolean media_set_addressed_player(struct media_adapter *adapter,
+					    const char *sender, const char *path,
+					    gboolean local)
+{
+	struct media_player *mp;
+
+	if (adapter->addressed &&
+		g_strcmp0(path, adapter->addressed->path) == 0)
+		return FALSE;
+
+	mp = media_adapter_find_player(adapter, NULL, path);
+	if (!mp && g_strcmp0(path, DUMMY_PLAYER) == 0)
+		mp = media_player_create(adapter, NULL, DUMMY_PLAYER, NULL);
+
+	if (!mp)
+		return FALSE;
+
+	if (adapter->addressed &&
+		g_strcmp0(adapter->addressed->path, DUMMY_PLAYER) == 0)
+		media_player_remove(adapter->addressed);
+
+	adapter->addressed = mp;
+
+	return TRUE;
+}
+
 static struct avrcp_player_cb player_cb = {
 	.get_setting = get_setting,
 	.set_setting = set_setting,
@@ -1673,6 +1746,17 @@ static gboolean track_changed(DBusConnection *connection, DBusMessage *msg,
 	return TRUE;
 }
 
+/* Need at least one free id to avoid infinite loop */
+static uint16_t get_free_player_id(struct media_adapter *adapter,
+						const char *sender)
+{
+	static uint16_t last_id = 0;
+
+	while (media_adapter_find_player_by_id(adapter, sender, last_id++));
+
+	return last_id - 1;
+}
+
 static struct media_player *media_player_create(struct media_adapter *adapter,
 						const char *sender,
 						const char *path,
@@ -1680,6 +1764,12 @@ static struct media_player *media_player_create(struct media_adapter *adapter,
 {
 	struct media_player *mp;
 
+	if (g_slist_length(adapter->players) >= UINT16_MAX) {
+		if (err)
+			*err = -EINVAL;
+		return NULL;
+	}
+
 	mp = g_new0(struct media_player, 1);
 	mp->adapter = adapter;
 	mp->sender = g_strdup(sender);
@@ -1709,10 +1799,11 @@ static struct media_player *media_player_create(struct media_adapter *adapter,
 	}
 
 	mp->settings = g_hash_table_new(g_direct_hash, g_direct_equal);
+	mp->id = get_free_player_id(adapter, sender);
 
 	adapter->players = g_slist_append(adapter->players, mp);
 
-	info("Player registered: sender=%s path=%s", sender, path);
+	info("Player registered: sender=%s path=%s id=%d", sender, path, mp->id);
 
 	if (err)
 		*err = 0;
@@ -1762,7 +1853,8 @@ static DBusMessage *register_player(DBusConnection *conn, DBusMessage *msg,
 	struct media_adapter *adapter = data;
 	struct media_player *mp;
 	DBusMessageIter args;
-	const char *sender, *path;
+	const char *sender;
+	char *path;
 	int err;
 
 	sender = dbus_message_get_sender(msg);
@@ -1772,6 +1864,8 @@ static DBusMessage *register_player(DBusConnection *conn, DBusMessage *msg,
 	dbus_message_iter_get_basic(&args, &path);
 	dbus_message_iter_next(&args);
 
+	DBG("Registering player sender=%s path=%s", sender, path);
+
 	if (media_adapter_find_player(adapter, sender, path) != NULL)
 		return btd_error_already_exists(msg);
 
@@ -1795,6 +1889,11 @@ static DBusMessage *register_player(DBusConnection *conn, DBusMessage *msg,
 		return btd_error_invalid_args(msg);
 	}
 
+	if (g_strcmp0(adapter->addressed->path, DUMMY_PLAYER) == 0)
+		media_set_addressed_player(adapter, sender, path, TRUE);
+
+	media_update_players(adapter);
+
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
@@ -1812,12 +1911,19 @@ static DBusMessage *unregister_player(DBusConnection *conn, DBusMessage *msg,
 
 	sender = dbus_message_get_sender(msg);
 
+	DBG("Unregistering player sender=%s path=%s", sender, path);
+
 	player = media_adapter_find_player(adapter, sender, path);
 	if (player == NULL)
 		return btd_error_does_not_exist(msg);
 
 	media_player_remove(player);
 
+	if (g_strcmp0(adapter->addressed->path, path) == 0)
+		media_set_addressed_player(adapter, sender, DUMMY_PLAYER, TRUE);
+
+	media_update_players(adapter);
+
 	return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
 }
 
@@ -1825,9 +1931,12 @@ static DBusMessage *get_properties(DBusConnection *conn,
 					DBusMessage *msg, void *data)
 {
 	struct media_adapter *adapter = data;
+	const char *property;
 	DBusMessage *reply;
 	DBusMessageIter iter;
 	DBusMessageIter dict;
+	char **players;
+	int i;
 	GSList *l;
 
 	reply = dbus_message_new_method_return(msg);
@@ -1841,6 +1950,21 @@ static DBusMessage *get_properties(DBusConnection *conn,
 			DBUS_TYPE_STRING_AS_STRING DBUS_TYPE_VARIANT_AS_STRING
 			DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &dict);
 
+	/* Players */
+	players = g_new0(char *, g_slist_length(adapter->players) + 1);
+	for (i = 0, l = adapter->players; l; l = l->next, i++) {
+		struct media_player *mp = l->data;
+
+		players[i] = mp->path;
+	}
+	dict_append_array(&dict, "Players", DBUS_TYPE_OBJECT_PATH,
+								&players, i);
+	g_free(players);
+
+	/* AddressedPlayer */
+	property = adapter->addressed->path;
+	dict_append_entry(&dict, "AddressedPlayer", DBUS_TYPE_OBJECT_PATH, &property);
+
 	dbus_message_iter_close_container(&iter, &dict);
 
 	return reply;
@@ -1853,6 +1977,9 @@ static DBusMessage *set_property(DBusConnection *conn,
 	DBusMessageIter iter;
 	DBusMessageIter sub;
 	const char *property;
+	const char *sender;
+
+	sender = dbus_message_get_sender(msg);
 
 	if (!dbus_message_iter_init(msg, &iter))
 		return btd_error_invalid_args(msg);
@@ -1867,6 +1994,19 @@ static DBusMessage *set_property(DBusConnection *conn,
 		return btd_error_invalid_args(msg);
 	dbus_message_iter_recurse(&iter, &sub);
 
+	if (g_str_equal("AddressedPlayer", property)) {
+		const char *path;
+
+		if (dbus_message_iter_get_arg_type(&sub) != DBUS_TYPE_OBJECT_PATH)
+			return btd_error_invalid_args(msg);
+		dbus_message_iter_get_basic(&sub, &path);
+
+		if (!media_set_addressed_player(adapter, sender, path, TRUE))
+		    return btd_error_invalid_args(msg);
+
+		return dbus_message_new_method_return(msg);
+	}
+
 	return btd_error_invalid_args(msg);
 }
 
@@ -1913,6 +2053,8 @@ int media_register(DBusConnection *conn, const char *path, const bdaddr_t *src)
 	adapter->conn = dbus_connection_ref(conn);
 	bacpy(&adapter->src, src);
 	adapter->path = g_strdup(path);
+	adapter->addressed = NULL;
+	media_set_addressed_player(adapter, NULL, DUMMY_PLAYER, TRUE);
 
 	if (!g_dbus_register_interface(conn, path, MEDIA_INTERFACE,
 					media_methods, NULL, NULL,
-- 
on behalf of ST-Ericsson

[PATCH 08/13] AVRCP: Implement notification AVAILABLE_PLAYERS_CHANGED

[Michal Labedzki]

Notify remote controller about change in available players
when player is being registered or unregistered.
---
 audio/avrcp.c |    5 ++++-
 audio/media.c |    2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 9ff0700..895001e 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -462,6 +462,7 @@ int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
 		break;
 	case AVRCP_EVENT_TRACK_REACHED_END:
 	case AVRCP_EVENT_TRACK_REACHED_START:
+	case AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED:
 		size = 1;
 		break;
 	default:
@@ -634,11 +635,12 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp_player *player,
 
 		return AVC_CTYPE_STABLE;
 	case CAP_EVENTS_SUPPORTED:
-		pdu->params[1] = 4;
+		pdu->params[1] = 5;
 		pdu->params[2] = AVRCP_EVENT_PLAYBACK_STATUS_CHANGED;
 		pdu->params[3] = AVRCP_EVENT_TRACK_CHANGED;
 		pdu->params[4] = AVRCP_EVENT_TRACK_REACHED_START;
 		pdu->params[5] = AVRCP_EVENT_TRACK_REACHED_END;
+		pdu->params[6] = AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED;
 
 		pdu->params_len = htons(2 + pdu->params[1]);
 		return AVC_CTYPE_STABLE;
@@ -985,6 +987,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
 		break;
 	case AVRCP_EVENT_TRACK_REACHED_END:
 	case AVRCP_EVENT_TRACK_REACHED_START:
+	case AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED:
 		len = 1;
 		break;
 	default:
diff --git a/audio/media.c b/audio/media.c
index 3455496..6ee8955 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1411,6 +1411,8 @@ static void media_update_players(struct media_adapter *adapter)
 		players[i] = mp->path;
 	}
 
+	avrcp_player_event(adapter->addressed->player, AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED, NULL);
+
 	emit_array_property_changed(adapter->conn, adapter->path,
 					MEDIA_INTERFACE, "Players",
 					DBUS_TYPE_OBJECT_PATH, &players, i);
-- 
on behalf of ST-Ericsson

[PATCH 09/13] AVRCP: Implement setting AddressedPlayer with notifications completion

[Michal Labedzki]

For AVRCP 1.4 complete notifications according to the specification
by rejecting all registered notifications with coresponding
transaction id.

For AVRCP 1.3 complete events by reusing already registered events.
This allows to change media player and notify remote controller
about metadata changed.
---
 audio/avrcp.c |  101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 audio/avrcp.h |    1 +
 audio/media.c |    9 +++++
 3 files changed, 111 insertions(+)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index 895001e..a10b3c1 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -58,6 +58,8 @@
 #include "sdpd.h"
 #include "dbus-common.h"
 
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
 #define AVRCP_REGISTER_NOTIFICATION_PARAM_LENGTH 5
 
 enum company_id {
@@ -224,6 +226,21 @@ struct avrcp_player {
 	GDestroyNotify destroy;
 };
 
+struct status_reply {
+	struct avrcp_header pdu;
+	uint8_t status;
+} __attribute__ ((packed));
+
+static const uint8_t events_to_complete[] = {
+	AVRCP_EVENT_PLAYBACK_STATUS_CHANGED,
+	AVRCP_EVENT_TRACK_CHANGED,
+	AVRCP_EVENT_TRACK_REACHED_END,
+	AVRCP_EVENT_TRACK_REACHED_START,
+	AVRCP_EVENT_PLAYBACK_POS_CHANGED,
+	AVRCP_EVENT_PLAYER_APPLICATION_SETTING_CHANGED,
+	AVRCP_EVENT_NOW_PLAYING_CONTENT_CHANGED
+};
+
 static GSList *servers = NULL;
 static unsigned int avctp_id = 0;
 
@@ -1529,3 +1546,87 @@ int avrcp_set_volume(struct audio_device *dev, uint8_t volume)
 					AVC_SUBUNIT_PANEL, buf, sizeof(buf),
 					avrcp_handle_set_volume, player);
 }
+
+static void avrcp_events_completion(struct avrcp_player *addressed)
+{
+	struct status_reply event;
+	unsigned int i;
+	uint8_t id;
+	int err;
+
+	memset(&event, 0, sizeof(event));
+	set_company_id(event.pdu.company_id, IEEEID_BTSIG);
+	event.pdu.pdu_id = AVRCP_REGISTER_NOTIFICATION;
+	event.pdu.params_len = htons(1);
+	event.status = STATUS_ADDRESSED_PLAYER_CHANGED;
+
+	for (i = 0; i < ARRAY_SIZE(events_to_complete); i++) {
+		id = 1 << events_to_complete[i];
+
+		if (!(addressed->registered_events & id))
+			continue;
+
+		err = avctp_send_vendordep(addressed->session,
+						addressed->transaction_events[events_to_complete[i]],
+						AVC_CTYPE_REJECTED, AVC_SUBUNIT_PANEL,
+						(uint8_t *) &event, sizeof(event));
+		if (err < 0)
+			DBG("Cannot complete event %u: %s (%d)",
+				events_to_complete[i], strerror(-err), err);
+
+		addressed->registered_events ^= id;
+	}
+}
+
+static void avrcp_events_reusing(struct avrcp_player *player)
+{
+	struct avrcp_player *addressed = player->server->addressed_player;
+	uint8_t play_status;
+	uint64_t uid;
+
+	/* Reusing previously registered events for AVRCP 1.3 devices */
+
+	play_status = player->cb->get_status(player->user_data);
+	uid = player->cb->get_uid(player->user_data);
+
+	if (play_status != addressed->cb->get_status(addressed->user_data))
+		avrcp_player_event(addressed,
+					AVRCP_EVENT_PLAYBACK_STATUS_CHANGED,
+					&play_status);
+
+	avrcp_player_event(addressed, AVRCP_EVENT_TRACK_CHANGED, &uid);
+}
+
+gboolean avrcp_set_addressed_player(struct avrcp_player *player, gboolean local)
+{
+	struct avrcp_server *server = player->server;
+
+	if (player == server->addressed_player)
+		return TRUE;
+
+	/* if triggered from remote, try to complete events */
+	if (local) {
+		if (server->version == AVRCP_VERSION_1_3)
+			avrcp_events_reusing(player);
+		else
+			avrcp_events_completion(server->addressed_player);
+	}
+
+	player_abort_pending_pdu(server->addressed_player);
+	player->registered_events = server->addressed_player->registered_events;
+	memcpy(player->transaction_events,
+		server->addressed_player->transaction_events, AVRCP_EVENT_LAST);
+	player->session = server->addressed_player->session;
+	player->dev = server->addressed_player->dev;
+
+	if (server->addressed_player->handler) {
+		avctp_unregister_pdu_handler(server->addressed_player->handler);
+		server->addressed_player->handler = 0;
+	}
+	player->handler = avctp_register_pdu_handler(AVC_OP_VENDORDEP,
+							handle_vendordep_pdu, player);
+
+	server->addressed_player = player;
+
+	return TRUE;
+}
diff --git a/audio/avrcp.h b/audio/avrcp.h
index 0045a2c..82f75d4 100644
--- a/audio/avrcp.h
+++ b/audio/avrcp.h
@@ -116,6 +116,7 @@ struct avrcp_player *avrcp_register_player(const bdaddr_t *src,
 						void *user_data,
 						GDestroyNotify destroy);
 void avrcp_unregister_player(struct avrcp_player *player);
+gboolean avrcp_set_addressed_player(struct avrcp_player *player, gboolean local);
 
 int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data);
 
diff --git a/audio/media.c b/audio/media.c
index 6ee8955..cac2378 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1440,8 +1440,17 @@ static gboolean media_set_addressed_player(struct media_adapter *adapter,
 		g_strcmp0(adapter->addressed->path, DUMMY_PLAYER) == 0)
 		media_player_remove(adapter->addressed);
 
+	if (!avrcp_set_addressed_player(mp->player, local))
+		return FALSE;
+
 	adapter->addressed = mp;
 
+	DBG("Set addressed player id=%u to %s", mp->id, path);
+
+	emit_property_changed(adapter->conn, adapter->path,
+				MEDIA_INTERFACE, "AddressedPlayer",
+				DBUS_TYPE_OBJECT_PATH, &path);
+
 	return TRUE;
 }
 
-- 
on behalf of ST-Ericsson

[PATCH 10/13] AVRCP: Implement notification ADDRESSED_PLAYER_CHANGED

[Michal Labedzki]

It is triggered by local change of addressed player on TG.
---
 audio/avrcp.c |   36 +++++++++++++++++++++++++++++++++++-
 audio/avrcp.h |    2 ++
 audio/media.c |   19 ++++++++++++++++++-
 3 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index a10b3c1..b095884 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -241,6 +241,11 @@ static const uint8_t events_to_complete[] = {
 	AVRCP_EVENT_NOW_PLAYING_CONTENT_CHANGED
 };
 
+struct addressed_param {
+	uint16_t player_id;
+	uint16_t uid_counter;
+} __attribute__ ((packed));
+
 static GSList *servers = NULL;
 static unsigned int avctp_id = 0;
 
@@ -477,6 +482,14 @@ int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
 		memcpy(&pdu->params[1], data, sizeof(uint64_t));
 
 		break;
+	case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
+		size = 5;
+		/* player id */
+		memcpy(&pdu->params[1], data, sizeof(uint16_t));
+		/* uid counter */
+		memcpy(&pdu->params[3], data + 2, sizeof(uint16_t));
+
+		break;
 	case AVRCP_EVENT_TRACK_REACHED_END:
 	case AVRCP_EVENT_TRACK_REACHED_START:
 	case AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED:
@@ -652,12 +665,13 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp_player *player,
 
 		return AVC_CTYPE_STABLE;
 	case CAP_EVENTS_SUPPORTED:
-		pdu->params[1] = 5;
+		pdu->params[1] = 6;
 		pdu->params[2] = AVRCP_EVENT_PLAYBACK_STATUS_CHANGED;
 		pdu->params[3] = AVRCP_EVENT_TRACK_CHANGED;
 		pdu->params[4] = AVRCP_EVENT_TRACK_REACHED_START;
 		pdu->params[5] = AVRCP_EVENT_TRACK_REACHED_END;
 		pdu->params[6] = AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED;
+		pdu->params[7] = AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED;
 
 		pdu->params_len = htons(2 + pdu->params[1]);
 		return AVC_CTYPE_STABLE;
@@ -981,6 +995,8 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
 {
 	uint16_t len = ntohs(pdu->params_len);
 	uint64_t uid;
+	uint16_t uid_counter;
+	uint16_t player_id;
 
 	/*
 	 * 1 byte for EventID, 4 bytes for Playback interval but the latest
@@ -1002,6 +1018,16 @@ static uint8_t avrcp_handle_register_notification(struct avrcp_player *player,
 		memcpy(&pdu->params[1], &uid, sizeof(uint64_t));
 
 		break;
+	case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
+		len = 5;
+
+		player_id = htons(player->cb->get_player_id(player->user_data));
+		memcpy(&pdu->params[1], &player_id, sizeof(player_id));
+
+		uid_counter = htons(player->cb->get_uid_counter(player->user_data));
+		memcpy(&pdu->params[3], &uid_counter, sizeof(uid_counter));
+
+		break;
 	case AVRCP_EVENT_TRACK_REACHED_END:
 	case AVRCP_EVENT_TRACK_REACHED_START:
 	case AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED:
@@ -1600,12 +1626,20 @@ static void avrcp_events_reusing(struct avrcp_player *player)
 gboolean avrcp_set_addressed_player(struct avrcp_player *player, gboolean local)
 {
 	struct avrcp_server *server = player->server;
+	struct addressed_param parameters;
 
 	if (player == server->addressed_player)
 		return TRUE;
 
 	/* if triggered from remote, try to complete events */
 	if (local) {
+		parameters.player_id = htons(player->cb->get_player_id(player->user_data));
+		parameters.uid_counter = htons(player->cb->get_uid_counter(player->user_data));
+
+		avrcp_player_event(server->addressed_player,
+					AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED,
+					&parameters);
+
 		if (server->version == AVRCP_VERSION_1_3)
 			avrcp_events_reusing(player);
 		else
diff --git a/audio/avrcp.h b/audio/avrcp.h
index 82f75d4..e83e2de 100644
--- a/audio/avrcp.h
+++ b/audio/avrcp.h
@@ -102,6 +102,8 @@ struct avrcp_player_cb {
 	uint32_t (*get_position) (void *user_data);
 	void (*set_volume) (uint8_t volume, struct audio_device *dev,
 							void *user_data);
+	uint16_t (*get_player_id) (void *user_data);
+	uint16_t (*get_uid_counter) (void *user_data);
 };
 
 int avrcp_register(DBusConnection *conn, const bdaddr_t *src, GKeyFile *config);
diff --git a/audio/media.c b/audio/media.c
index cac2378..516f12e 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -106,6 +106,7 @@ struct media_player {
 	uint32_t		position;
 	uint8_t			volume;
 	uint16_t		id;
+	uint16_t		uid_counter;
 	GTimer			*timer;
 };
 
@@ -1398,6 +1399,20 @@ static void set_volume(uint8_t volume, struct audio_device *dev, void *user_data
 	}
 }
 
+static uint16_t get_player_id(void *user_data)
+{
+	struct media_player *mp = user_data;
+
+	return mp->id;
+}
+
+static uint16_t get_uid_counter(void *user_data)
+{
+	struct media_player *mp = user_data;
+
+	return mp->uid_counter;
+}
+
 static void media_update_players(struct media_adapter *adapter)
 {
 	char **players;
@@ -1462,7 +1477,9 @@ static struct avrcp_player_cb player_cb = {
 	.get_metadata = get_metadata,
 	.get_position = get_position,
 	.get_status = get_status,
-	.set_volume = set_volume
+	.set_volume = set_volume,
+	.get_player_id = get_player_id,
+	.get_uid_counter = get_uid_counter
 };
 
 static void media_player_exit(DBusConnection *connection, void *user_data)
-- 
on behalf of ST-Ericsson

[PATCH 11/13] AVRCP: Handle SetAddressedPlayer command from remote controller

[Michal Labedzki]

Set player selected by controller as addressed player.
---
 audio/avrcp.c |   33 +++++++++++++++++++++++++++++++++
 audio/avrcp.h |    1 +
 audio/media.c |   15 ++++++++++++++-
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index b095884..249ed8e 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -1118,6 +1118,37 @@ err:
 	return AVC_CTYPE_REJECTED;
 }
 
+static uint8_t avrcp_handle_set_addressed_player(struct avrcp_player *player,
+							struct avrcp_header *pdu,
+							uint8_t transaction)
+{
+	uint16_t len = ntohs(pdu->params_len);
+	uint16_t player_id;
+
+	if (len != sizeof(player_id)) {
+		pdu->params[0] = STATUS_INVALID_PARAMETER;
+		return AVC_CTYPE_REJECTED;
+	}
+
+	player_id = bt_get_be16(pdu->params);
+
+	DBG("Remote controller set addressed player id=%u", player_id);
+
+	pdu->params_len = htons(1);
+
+	if (!player->cb->set_addressed_player(player_id, player->user_data)) {
+		if (g_slist_length(player->server->players) == 0)
+			pdu->params[0] = STATUS_NO_AVAILABLE_PLAYERS;
+		else
+			pdu->params[0] = STATUS_INVALID_PLAYER_ID;
+
+		return AVC_CTYPE_REJECTED;
+	}
+
+	pdu->params[0] = STATUS_OK;
+	return AVC_CTYPE_ACCEPTED;
+}
+
 static struct pdu_handler {
 	uint8_t pdu_id;
 	uint8_t code;
@@ -1153,6 +1184,8 @@ static struct pdu_handler {
 					avrcp_handle_request_continuing },
 		{ AVRCP_ABORT_CONTINUING, AVC_CTYPE_CONTROL,
 					avrcp_handle_abort_continuing },
+		{ AVRCP_SET_ADDRESSED_PLAYER, AVC_CTYPE_CONTROL,
+					avrcp_handle_set_addressed_player },
 		{ },
 };
 
diff --git a/audio/avrcp.h b/audio/avrcp.h
index e83e2de..d6b62e2 100644
--- a/audio/avrcp.h
+++ b/audio/avrcp.h
@@ -104,6 +104,7 @@ struct avrcp_player_cb {
 							void *user_data);
 	uint16_t (*get_player_id) (void *user_data);
 	uint16_t (*get_uid_counter) (void *user_data);
+	gboolean (*set_addressed_player) (uint16_t player_id, void *user_data);
 };
 
 int avrcp_register(DBusConnection *conn, const bdaddr_t *src, GKeyFile *config);
diff --git a/audio/media.c b/audio/media.c
index 516f12e..c59997b 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1469,6 +1469,18 @@ static gboolean media_set_addressed_player(struct media_adapter *adapter,
 	return TRUE;
 }
 
+static gboolean set_addressed_player(uint16_t player_id, void *user_data)
+{
+	struct media_player *player = user_data;
+	struct media_player *mp;
+
+	mp = media_adapter_find_player_by_id(player->adapter, NULL, player_id);
+	if (!mp)
+		return FALSE;
+
+	return media_set_addressed_player(player->adapter, player->sender, mp->path, FALSE);
+}
+
 static struct avrcp_player_cb player_cb = {
 	.get_setting = get_setting,
 	.set_setting = set_setting,
@@ -1479,7 +1491,8 @@ static struct avrcp_player_cb player_cb = {
 	.get_status = get_status,
 	.set_volume = set_volume,
 	.get_player_id = get_player_id,
-	.get_uid_counter = get_uid_counter
+	.get_uid_counter = get_uid_counter,
+	.set_addressed_player = set_addressed_player
 };
 
 static void media_player_exit(DBusConnection *connection, void *user_data)
-- 
on behalf of ST-Ericsson

[PATCH 12/13] media-api: Update documentation with setting Addressed Player

[Michal Labedzki]

Media API for Addressed Player allow to change addressed player
to one from already registered players. If no players are registered
default dummy player is used.
---
 doc/media-api.txt |   28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/doc/media-api.txt b/doc/media-api.txt
index 4446439..2e19417 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -8,7 +8,19 @@ Service		org.bluez
 Interface	org.bluez.Media
 Object path	[variable prefix]/{hci0,hci1,...}
 
-Methods		void RegisterEndpoint(object endpoint, dict properties)
+Methods		dict GetProperties()
+
+			Returns all properties. See the
+			properties section for available properties.
+
+		void SetProperty(string name, variant value)
+
+			Changes the value of the specified property. Only
+			properties that are listed as read-write are changeable.
+
+			On success this will emit a PropertyChanged signal.
+
+		void RegisterEndpoint(object endpoint, dict properties)
 
 			Register a local end point to sender, the sender can
 			register as many end points as it likes.
@@ -123,6 +135,20 @@ Methods		void RegisterEndpoint(object endpoint, dict properties)
 
 			Unregister sender media player.
 
+Signals		PropertyChanged(string name, variant value)
+
+			This signal indicates a changed value of the given
+			property.
+
+Properties	array{object} Players [readonly]
+
+			List of registered players.
+
+		object AddressedPlayer [readwrite]
+
+			Set active media player. Only one active player may be
+			used by remote controller.
+
 MediaPlayer hierarchy
 =====================
 
-- 
on behalf of ST-Ericsson

[PATCH 13/13] doc: Fix typo

[Michal Labedzki]

This patch fix simple typo in documentation of API.
---
 doc/adapter-api.txt   |    2 +-
 doc/audio-api.txt     |    2 +-
 doc/device-api.txt    |    2 +-
 doc/media-api.txt     |    2 +-
 doc/proximity-api.txt |    2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
index dccb6bf..916e941 100644
--- a/doc/adapter-api.txt
+++ b/doc/adapter-api.txt
@@ -24,7 +24,7 @@ Methods		dict GetProperties()
 		void SetProperty(string name, variant value)
 
 			Changes the value of the specified property. Only
-			properties that are listed a read-write are changeable.
+			properties that are listed as read-write are changeable.
 			On success this will emit a PropertyChanged signal.
 
 			Possible Errors: org.bluez.Error.InvalidArguments
diff --git a/doc/audio-api.txt b/doc/audio-api.txt
index 02291fd..9b1737d 100644
--- a/doc/audio-api.txt
+++ b/doc/audio-api.txt
@@ -122,7 +122,7 @@ Methods		void Connect()
 		void SetProperty(string name, variant value)
 
 			Changes the value of the specified property. Only
-			properties that are listed a read-write are changeable.
+			properties that are listed as read-write are changeable.
 			On success this will emit a PropertyChanged signal.
 
 			Possible Errors: org.bluez.Error.DoesNotExist
diff --git a/doc/device-api.txt b/doc/device-api.txt
index 4a170e4..0f34210 100644
--- a/doc/device-api.txt
+++ b/doc/device-api.txt
@@ -25,7 +25,7 @@ Methods		dict GetProperties()
 		void SetProperty(string name, variant value)
 
 			Changes the value of the specified property. Only
-			properties that are listed a read-write are changeable.
+			properties that are listed as read-write are changeable.
 			On success this will emit a PropertyChanged signal.
 
 			Possible Errors: org.bluez.Error.DoesNotExist
diff --git a/doc/media-api.txt b/doc/media-api.txt
index 2e19417..0299333 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -315,7 +315,7 @@ Methods		dict GetProperties()
 		void SetProperty(string name, variant value)
 
 			Changes the value of the specified property. Only
-			properties that are listed a read-write can be changed.
+			properties that are listed as read-write can be changed.
 
 			On success this will emit a PropertyChanged signal.
 
diff --git a/doc/proximity-api.txt b/doc/proximity-api.txt
index 0c60d47..c8eae50 100644
--- a/doc/proximity-api.txt
+++ b/doc/proximity-api.txt
@@ -19,7 +19,7 @@ Methods		dict GetProperties()
 		void SetProperty(string name, variant value)
 
 			Changes the value of the specified property. Only
-			properties that are listed a read-write are changeable.
+			properties that are listed as read-write are changeable.
 			On success this will emit a PropertyChanged signal.
 
 			Possible Errors: org.bluez.Error.InvalidArguments
-- 
on behalf of ST-Ericsson

Re: [PATCH 01/13] AVRCP: Update constants

[Lucas De Marchi]

Hi Michal,

On Wed, Jun 6, 2012 at 1:05 PM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> Change all specification constants to enum type.

Nack from my side. WHY?

> Also update constants for AVRCP 1.4.

It's already set for AVRCP 1.4. The other constants are used nowhere.


Lucas De Marchi

Re: [PATCH 02/13] AVRCP: Use name "addressed" instead of "active"

[Luiz Augusto von Dentz]

Hi Michal,

On Wed, Jun 6, 2012 at 7:06 PM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> AVRCP 1.4 named currently controlled player as "addressed", so use
> this name instead of "active". In future there where be more "browsing"
> player that will be active, but not addressed.
> ---
>  audio/avrcp.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 196d6a5..7576e19 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -199,7 +199,7 @@ struct avrcp_server {
>        uint32_t tg_record_id;
>        uint32_t ct_record_id;
>        GSList *players;
> -       struct avrcp_player *active_player;
> +       struct avrcp_player *addressed_player;
>  };
>
>  struct pending_pdu {
> @@ -1253,7 +1253,7 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
>        if (!server)
>                return;
>
> -       player = server->active_player;
> +       player = server->addressed_player;
>        if (!player)
>                return;
>
> @@ -1446,7 +1446,7 @@ struct avrcp_player *avrcp_register_player(const bdaddr_t *src,
>        player->destroy = destroy;
>
>        if (!server->players)
> -               server->active_player = player;
> +               server->addressed_player = player;
>
>        if (!avctp_id)
>                avctp_id = avctp_add_state_cb(state_changed, NULL);
> @@ -1462,8 +1462,8 @@ void avrcp_unregister_player(struct avrcp_player *player)
>
>        server->players = g_slist_remove(server->players, player);
>
> -       if (server->active_player == player)
> -               server->active_player = g_slist_nth_data(server->players, 0);
> +       if (server->addressed_player == player)
> +               server->addressed_player = g_slist_nth_data(server->players, 0);
>
>        player_destroy(player);
>  }
> @@ -1498,7 +1498,7 @@ int avrcp_set_volume(struct audio_device *dev, uint8_t volume)
>        if (server == NULL)
>                return -EINVAL;
>
> -       player = server->active_player;
> +       player = server->addressed_player;
>        if (player == NULL)
>                return -ENOTSUP;
>
> --
> on behalf of ST-Ericsson


The concept of active and addressed are a bit different, the active
means the player can be addressed but not necessary in use, this is to
allow bluetoothd to control the addressed player without another
component setting it. Iirc I have said this before in irc, but Im not
sure if was you or someone else, we can detect if a player is active
e.g by checking its status, this way we can probably pick the first
player active so they don't have to compete to set themselves as
addressed or anything like that, then if the player is stopped/become
inactive we can pick the next active, same goes if the remote device
change the addressed player we just change it, the player or other
components don't need to know about the details of which is being
addressed.

Btw, do you realize that addressed player is controlled by the remote
device, as far audio is concern it can be mixed with several different
playbacks in the same stream, so it more a matter of which player's
metadata the remote would like to display, which is probably idiotic
since you limit the events and commands to only the addressed and push
some complexity to the target where it could just have the signalling
working to as many as the controller can handle.

-- 
Luiz Augusto von Dentz

RE: [PATCH 02/13] AVRCP: Use name "addressed" instead of "active"

[Michal.Labedzki]

Hi Luiz,

# Dummy player
Whenever a device presents itself as the device which supports AVRCP (at least)
one player should be provided. Current AVRCP 1.3 implementation does not provide
any AVRCP player but - at the same time - registers SDP record assuming AVRCP
service is working however it is not the case; if you do not register a player the
AVRCP service will not work.

For the above reason we shall introduce the Default Player (specification 6.9)
which is sort of Dummy Player doing nothing but presents as a player.
Providing the Dummy Player is meant to avoid the situation in which service
registers itself without an available player; without such Dummy Player the AVRCP
service returns some errors which cause major issues with some market devices -
AVRCP stops working due to unexpected REJECT response (one of the device where such
behaviour could be observed: MW600).

# AddressedPlayer changed locally
Specification says (6.10.2.1):
"The Play Status field can be used by the CT to learn which players are currently
playing without having to switch the Addressed Player between all players to
explicitly request each player’s status separately (Note that on some TGs,
switching the Addressed Player might result in stopping a player’s audio stream)"

According to above more than one player can play music at the same time and an
audio stream should be switched on changing the addressed player but should NOT
result in stopping the media player. To my understanding this should be done by
"struct audio_device" in each player).

So an user may run 10 players. Then on all players play is pushed (locally).
But AVRCP controls only one player. I guess if for example when player #5 is playing,
then should keep playing whenever it is addressed or not.

For the above reasons the AVRCP 1.4 implementation should allow changing the
AddressedPlayer locally (by an user).
This indicates that the MediaPlayer will not implement the way to automatically
set itself as addressed. Only an user should have the possibility to change
the addressed player.
(To my understanding section 26.10 in the spec indicates mentioned above
interpretation).


I am adding some side notes to be little bit more elaborate in here:
1. An user works on a PC, but listen to the music using BT headset with AVRCP.
2. The user does something (whatever... write SDP code in LibreOffice)
   while the mediaplayer is in background.
3. Music ends... and the user starts the second media player (Instance #2 or separated player).
4. Somebody interrupts the user asking about anything. The user presses headset button to
   stop the player...

In this case user is working on a PC, so changing addressed player locally on PC,
but at the same time using remote controller for basic operation like volume,
play/stop. Wired headsets usually do not used PC for controlling those
actions. So naturally user want to changing addressed player locally.
Remote controller must be sync with PC.

According to point 3. user should change addressed player in some way.
It can be external component or maybe something in player, but think about
audio services like radio, TV. Each one all time playing, but should not be
controlled by remote controller. Which player should be used as addressed?
I think one player should was chosen by user. All other player should be treat as
"local player", so should be usable on local device. I think about use case where
one user use headset to listen the music from TG and there is second user which
watching video on TG too using internal screen/speaker. (so local device, maybe PC,
maybe smartphone in "Media Center" role)

Why not allow user to playing audio stream from few players? User may want that.
If not then it should stopped players one by one. This is not bad, because it must
push "play" on all player early.



Next thing:

----------------------------------
Players are separated per adapter (and partially per "DBus sender")

Adapter #1

mp1
mp2
mp3
mp4


Adapter #2

mp5
mp6
mp7


----------------------------------
There is a lot of players and a lot of audio outputs (sound card 1,
sound card 2...) and finally a lot of users...

Adapter #1

mp1: playing for remote controller (BT)
mp2: stopped
mp3: playing for local user XXX1 (PC/phone...)
mp4: paused

Adapter #2

mp5: playing for remote user XXX2 (client-server)
mp6: playing for another remote controller (BT)
mp7: stopped


---------------------------------- 

Summary:

1. Remote controller should be treated as part of the system, like a keyboard,
so no less permission to "break environment" (for example: pick up player control someone
local user)

2. Then a local user should do everything what remote controller can do, so changing
addressed player in external application like "Virtual TV Remote Controller".

3. More than one mediaplayer may be playing at the same time, maybe different
output, maybe not (use case: user wants to mix audio streams: voice and music).
I am against limiting user options. Limiting by adding option seems to be OK
(like "Simple Pairing Mode"). So one could extend current implementation by limiting
"playing players" to only one player by additional switch in audio.conf.

My view on AVRCP (1.4) is as follow: AVRCP 1.4 is only the interface between
MediaPlayer applications and controllers, so nothing special to implement
(think about callback for "GetFolderItems", it should be a pipe). And mediaplayer presents
"audio services", like radio, TV which are stream based,
so mediaplayer may be all time in playing state.


Regards / Pozdrawiam
-------------------------------------------------------------------------------------------------------------
Michał Łabędzki
e-mail: michal.labedzki@tieto.com
office communicator: michal.labedzki@tieto.com
location: Poland, Wrocław, Legnicka 55F
room: 315
phone: +48 717 740 340
(Michal Labedzki)

Tieto Poland spółka z ograniczoną odpowiedzialnością z siedzibą w Szczecinie, ul. Malczewskiego 26. Zarejestrowana w Sądzie Rejonowym Szczecin-Centrum w Szczecinie, XIII Wydział Gospodarczy Krajowego Rejestru Sądowego pod numerem 0000124858. NIP: 8542085557. REGON: 812023656. Kapitał zakładowy:  4 271 500 PLN.

Re: [PATCH 02/13] AVRCP: Use name "addressed" instead of "active"

[Luiz Augusto von Dentz]

Hi Michal,

On Wed, Jun 13, 2012 at 6:18 PM,  <Michal.Labedzki@tieto.com> wrote:
> Hi Luiz,
>
> # Dummy player
> Whenever a device presents itself as the device which supports AVRCP (at least)
> one player should be provided. Current AVRCP 1.3 implementation does not provide
> any AVRCP player but - at the same time - registers SDP record assuming AVRCP
> service is working however it is not the case; if you do not register a player the
> AVRCP service will not work.
>
> For the above reason we shall introduce the Default Player (specification 6.9)
> which is sort of Dummy Player doing nothing but presents as a player.
> Providing the Dummy Player is meant to avoid the situation in which service
> registers itself without an available player; without such Dummy Player the AVRCP
> service returns some errors which cause major issues with some market devices -
> AVRCP stops working due to unexpected REJECT response (one of the device where such
> behaviour could be observed: MW600).

This is a completely different issue, yes we might have to emulate a
player until one is register to avoid this dumb behavior by the
headset.

> # AddressedPlayer changed locally
> Specification says (6.10.2.1):
> "The Play Status field can be used by the CT to learn which players are currently
> playing without having to switch the Addressed Player between all players to
> explicitly request each player’s status separately (Note that on some TGs,
> switching the Addressed Player might result in stopping a player’s audio stream)"
>
> According to above more than one player can play music at the same time and an
> audio stream should be switched on changing the addressed player but should NOT
> result in stopping the media player. To my understanding this should be done by
> "struct audio_device" in each player).
>
> So an user may run 10 players. Then on all players play is pushed (locally).
> But AVRCP controls only one player. I guess if for example when player #5 is playing,
> then should keep playing whenever it is addressed or not.
>
> For the above reasons the AVRCP 1.4 implementation should allow changing the
> AddressedPlayer locally (by an user).
> This indicates that the MediaPlayer will not implement the way to automatically
> set itself as addressed. Only an user should have the possibility to change
> the addressed player.
> (To my understanding section 26.10 in the spec indicates mentioned above
> interpretation).
>
>
> I am adding some side notes to be little bit more elaborate in here:
> 1. An user works on a PC, but listen to the music using BT headset with AVRCP.
> 2. The user does something (whatever... write SDP code in LibreOffice)
>   while the mediaplayer is in background.
> 3. Music ends... and the user starts the second media player (Instance #2 or separated player).
> 4. Somebody interrupts the user asking about anything. The user presses headset button to
>   stop the player...
>
> In this case user is working on a PC, so changing addressed player locally on PC,
> but at the same time using remote controller for basic operation like volume,
> play/stop. Wired headsets usually do not used PC for controlling those
> actions. So naturally user want to changing addressed player locally.
> Remote controller must be sync with PC.
>
> According to point 3. user should change addressed player in some way.
> It can be external component or maybe something in player, but think about
> audio services like radio, TV. Each one all time playing, but should not be
> controlled by remote controller. Which player should be used as addressed?
> I think one player should was chosen by user. All other player should be treat as
> "local player", so should be usable on local device. I think about use case where
> one user use headset to listen the music from TG and there is second user which
> watching video on TG too using internal screen/speaker. (so local device, maybe PC,
> maybe smartphone in "Media Center" role)

This is already possible by using PulseAudio, you can select the
output per application, it is really a non-issue.

> Why not allow user to playing audio stream from few players? User may want that.
> If not then it should stopped players one by one. This is not bad, because it must
> push "play" on all player early.

It can already do that, what is your point here, the addressed is
plain english, addressed _by_ the controller. Besides the user cannot
know all the player it has in the system, because the players may
belong to different users.

In this case you are trying to duplicate the same UI the controller
could have to control the addressed player locally, lets not
over-engineer things the same way the AVRCP did, the controller will
be able to address different players and the user has the ability to
close/stop the addressed player to start another one, that cover
pretty much everything in on the market.

>
>
> Next thing:
>
> ----------------------------------
> Players are separated per adapter (and partially per "DBus sender")
>
> Adapter #1
>
> mp1
> mp2
> mp3
> mp4
>
>
> Adapter #2
>
> mp5
> mp6
> mp7
>

The media interface is per-adapter already...

> ----------------------------------
> There is a lot of players and a lot of audio outputs (sound card 1,
> sound card 2...) and finally a lot of users...
>
> Adapter #1
>
> mp1: playing for remote controller (BT)
> mp2: stopped
> mp3: playing for local user XXX1 (PC/phone...)
> mp4: paused
>
> Adapter #2
>
> mp5: playing for remote user XXX2 (client-server)
> mp6: playing for another remote controller (BT)
> mp7: stopped
>
>
> ----------------------------------
>
> Summary:
>
> 1. Remote controller should be treated as part of the system, like a keyboard,
> so no less permission to "break environment" (for example: pick up player control someone
> local user)

If the player register itself to be controlled they it can be
addressed otherwise it doesn't, that is the permission.

> 2. Then a local user should do everything what remote controller can do, so changing
> addressed player in external application like "Virtual TV Remote Controller".

It can already, we just don't allow to overwrite the addressed player,
it is like a universal remote control where the tv don't overwrite its
configuration to tell it should control the dvd player.

> 3. More than one mediaplayer may be playing at the same time, maybe different
> output, maybe not (use case: user wants to mix audio streams: voice and music).
> I am against limiting user options. Limiting by adding option seems to be OK
> (like "Simple Pairing Mode"). So one could extend current implementation by limiting
> "playing players" to only one player by additional switch in audio.conf.
>
> My view on AVRCP (1.4) is as follow: AVRCP 1.4 is only the interface between
> MediaPlayer applications and controllers, so nothing special to implement
> (think about callback for "GetFolderItems", it should be a pipe). And mediaplayer presents
> "audio services", like radio, TV which are stream based,
> so mediaplayer may be all time in playing state.

This is all possible, the only thing that is not possible is changing
the addressed player, this would just complicate things more than
necessary, also one important thing that you forgot is that the only
use for changing the addressed is to the controller to be able to send
the commands to a different player, but the point is to use the
Bluetooth (wireless) why do you thing that going back to the PC/Phone
interface to change the addressed player would be that useful?


-- 
Luiz Augusto von Dentz

RE: [PATCH 02/13] AVRCP: Use name "addressed" instead of "active"

[Michal.Labedzki]

Hi Luiz,

> This is already possible by using PulseAudio, you can select the
> output per application, it is really a non-issue.

Do not forget about "mplayer".


> Why not allow user to playing audio stream from few players? User may want that.
> If not then it should stopped players one by one. This is not bad, because it must
> push "play" on all player early.

> It can already do that, what is your point here, the addressed is
> plain english, addressed _by_ the controller. Besides the user cannot
> know all the player it has in the system, because the players may
> belong to different users.

Wrong. User may not know all the players in system, because every player should registering at "boot time". 
So you have a list of available player. Then all player supported AVRCP 1.3/1.4 will be accessible for remote controller or local controller.
According to the specification player may be not currently running (specification 6.9). Like Android, 
music player is registered, but it starting only on PLAY (other) button. Of course this case is not implemented in those patches.

Addressed_by_controller is not truth, because you have event designed to be registered by remote controller - AddressedPlayerChanged.
This mean that you can change addressedplayer on local device and this event inform remote controller about this change.
Look that browsing feature  does not have corresponding event. (SetAddressedPlayer & AddressedPlayerChanged vs SetBrowsedPlayer)


> In this case you are trying to duplicate the same UI the controller
> could have to control the addressed player locally, lets not
> over-engineer things the same way the AVRCP did, the controller will
> be able to address different players and the user has the ability to
> close/stop the addressed player to start another one, that cover
> pretty much everything in on the market.

Do you mean that local device should be a poor and have limited functionality than remote controller?
This is not good idea. Everything what is possible on remote controller should be possible on local device too (in this case this is not the problem)



>> My view on AVRCP (1.4) is as follow: AVRCP 1.4 is only the interface between
>> MediaPlayer applications and controllers, so nothing special to implement
>> (think about callback for "GetFolderItems", it should be a pipe). And mediaplayer presents
>> "audio services", like radio, TV which are stream based,
>> so mediaplayer may be all time in playing state.
>
> This is all possible, the only thing that is not possible is changing
> the addressed player, this would just complicate things more than
> necessary, also one important thing that you forgot is that the only
> use for changing the addressed is to the controller to be able to send
> the commands to a different player, but the point is to use the
> Bluetooth (wireless) why do you thing that going back to the PC/Phone
> interface to change the addressed player would be that useful?
> --
> Luiz Augusto von Dentz

In my opinion changing addressed player locally is necessary. 

Case 1: Let think about you run 2 player. Music player and radio player. You listen the music over headset.
            But your headset is  AVRCP 1.4 without AddressedPlayer feature (like MW600). Then you use local device (for example phone) to change player to radio.

Case 2: You have headset with AVRCP 1.4 . So you should still support for case 1, but you should not remove player from the player list, because you will 
            broken functionality on remote controller - incomplete player list. Done by "GetFolderItems" on MediaPlayerList scopes 
            implies that you need registered more player at the same time, you should not unregistered anymore.

Case 3: Start thinking about onscreen metadata reader (like KDE plasmoid, notification, etc.). User should be able to choose used mediaplayer for using  metadata from the player list. API from this patchset allow to check every player (the list is available), also can chech which player is currently addressed (very user experience, because you should expect the same metadata on screen and on remote controller)  

Case 4: ...this API allow to testing this feature. How do you want to check AddressedPlayerChanged without major changed in environment (unregistered player is not the case, changing play status too, because you will send unwanted frames to testing tool)


Try think about full support of AVRCP 1.4. Without cut some functionality.

Regards / Pozdrawiam
-------------------------------------------------------------------------------------------------------------
Michał Łabędzki
ASCII: Michal Labedzki
e-mail: michal.labedzki@tieto.com
office communicator: michal.labedzki@tieto.com
location: Poland, Wrocław, Legnicka 55F
room: 315
phone: +48 717 740 340
---
Tieto Corporation / Tieto Poland
http://www.tieto.com / http://www.tieto.pl
---
Tieto Poland spółka z ograniczoną odpowiedzialnością z siedzibą w Szczecinie, ul. Malczewskiego 26. Zarejestrowana w Sądzie Rejonowym Szczecin-Centrum w Szczecinie, XIII Wydział Gospodarczy Krajowego Rejestru Sądowego pod numerem 0000124858. NIP: 8542085557. REGON: 812023656. Kapitał zakładowy: 4 271500 PLN
________________________________________

Re: [PATCH 02/13] AVRCP: Use name "addressed" instead of "active"

[Luiz Augusto von Dentz]

Hi Michal,

On Thu, Jun 14, 2012 at 12:42 PM,  <Michal.Labedzki@tieto.com> wrote:
> Hi Luiz,
>
>> This is already possible by using PulseAudio, you can select the
>> output per application, it is really a non-issue.
>
> Do not forget about "mplayer".
>
>
>> Why not allow user to playing audio stream from few players? User may want that.
>> If not then it should stopped players one by one. This is not bad, because it must
>> push "play" on all player early.
>
>> It can already do that, what is your point here, the addressed is
>> plain english, addressed _by_ the controller. Besides the user cannot
>> know all the player it has in the system, because the players may
>> belong to different users.
>
> Wrong. User may not know all the players in system, because every player should registering at "boot time".
> So you have a list of available player. Then all player supported AVRCP 1.3/1.4 will be accessible for remote controller or local controller.
> According to the specification player may be not currently running (specification 6.9). Like Android,
> music player is registered, but it starting only on PLAY (other) button. Of course this case is not implemented in those patches.

Register at boot time? Are you serious you want players to be started
during boot? If the player is not running it cannot be registered
since it cannot be connected to D-Bus an thus cannot receive any
messages, an using an agent to register itself is also a bad idea
since you would have to forward message which is considered a very bad
design.

> Addressed_by_controller is not truth, because you have event designed to be registered by remote controller - AddressedPlayerChanged.
> This mean that you can change addressedplayer on local device and this event inform remote controller about this change.
> Look that browsing feature  does not have corresponding event. (SetAddressedPlayer & AddressedPlayerChanged vs SetBrowsedPlayer)

AddressedPlayerChanged is a notification that the player is no longer
available, and the browsed player reacts to addressed player, btw once
you change the addressed player the controller has to register
everything again, it is a very bad design no matter how you look at
it.

>
>> In this case you are trying to duplicate the same UI the controller
>> could have to control the addressed player locally, lets not
>> over-engineer things the same way the AVRCP did, the controller will
>> be able to address different players and the user has the ability to
>> close/stop the addressed player to start another one, that cover
>> pretty much everything in on the market.
>
> Do you mean that local device should be a poor and have limited functionality than remote controller?
> This is not good idea. Everything what is possible on remote controller should be possible on local device too (in this case this is not the problem)

I disagree, exposing this will just bloat the UI and probably fragment
the testing of this feature since each environment may implement this
differently, not to mention with multiuser environment this simple
doesn't work, we should really concentrate on having something that
works reliable with one player active at time which is most likely
what the user gonna be doing most of the time.

> In my opinion changing addressed player locally is necessary.
>
> Case 1: Let think about you run 2 player. Music player and radio player. You listen the music over headset.
>            But your headset is  AVRCP 1.4 without AddressedPlayer feature (like MW600). Then you use local device (for example phone) to change player to radio.

The player interfaces may have a disable/enable bluetooth button where
it unregister/register itself, that way you can control what metadata
will be displayed.

> Case 2: You have headset with AVRCP 1.4 . So you should still support for case 1, but you should not remove player from the player list, because you will
>            broken functionality on remote controller - incomplete player list. Done by "GetFolderItems" on MediaPlayerList scopes
>            implies that you need registered more player at the same time, you should not unregistered anymore.

Then don't unregister the player just let it mark itself as
non-addressable, so the metadata policy is directly controlled by the
player itself not an external tool.

> Case 3: Start thinking about onscreen metadata reader (like KDE plasmoid, notification, etc.). User should be able to choose used mediaplayer for using  metadata from the player list. API from this patchset allow to check every player (the list is available), also can chech which player is currently addressed (very user experience, because you should expect the same metadata on screen and on remote controller)

In a multiuser environment this doesn't work, we cannot list players
from other users, so at this point this became completely invalid use
case.

> Case 4: ...this API allow to testing this feature. How do you want to check AddressedPlayerChanged without major changed in environment (unregistered player is not the case, changing play status too, because you will send unwanted frames to testing tool)
>

If the player unregister, leaves the bus or mark itself as non-addressable

> Try think about full support of AVRCP 1.4. Without cut some functionality.

Im not cutting anything, all the use cases are covered, you just can't
overwrite the addressed player with an external tool because we have
no idea what user will be running that tool and if it has permission
to change the addressed player, and that is by design to avoid having
to write a lot of code to manage this when the great majority of users
will just have one player active at time, thus making this useless.

Note, this is nothing we cannot add in future, but it is always better
to start we something simple so there is less risk break API, we did
this a couple of times and it is always hard to deprecate things.

-- 
Luiz Augusto von Dentz

RE: [PATCH 02/13] AVRCP: Use name "addressed" instead of "active"

[Michal.Labedzki]

Hi Luiz,

> Register at boot time? Are you serious you want players to be started
> during boot?

Not only system-stable players, like main player.

> If the player is not running it cannot be registered
> since it cannot be connected to D-Bus an thus cannot receive any
> messages, an using an agent to register itself is also a bad idea
> since you would have to forward message which is considered a very bad
> design.

"Registration" and "Player" should be separated. So player can be registered by external tool, like bootscripts. Then starting player by dbus call.

Offtopic: forwarding messages are not that bad. It is only an issue introduce by bad interface design, which are too poor to do something directly.

> AddressedPlayerChanged is a notification that the player is no longer
> available

No, no, no. For available player you got separated event: AvailablePlayersChanged, and after that you should download list of the players again, so that is for detecting available players. On the other hand you got event AddressedPlayerChanged which is designed for detecting currently controlled player by local user.

Specification 6.9:
"The player to which the AV/C Panel Subunit [2] shall route control commands is the
addressed player. This may be changed locally on the TG, or by the CT using the SetAddressedPlayer command."


> , and the browsed player reacts to addressed player

What? Do you not try to mix these functionalities? AddressedPlayer and BrowsedPlayer can be separated. Player decide of that. May implement OnlyBrowsableWhenAddressed or no.

>, btw once
> you change the addressed player the controller has to register
> everything again, it is a very bad design no matter how you look at
> it.

No. It is not bad. It is AVRCP 1.4 feature based on notification nature. Look to specification 6.9.2.2
"On completion of the Addressed Player Changed notification the TG shall complete all
player specific notifications with AV/C C-Type REJECTED with error code Addressed
Player Changed." - so controller will probably registering events again. This allow to update content of mediaplayer on controller.

> I disagree, exposing this will just bloat the UI and probably fragment
> the testing of this feature since each environment may implement this
> differently, not to mention with multiuser environment this simple
> doesn't work, we should really concentrate on having something that
> works reliable with one player active at time which is most likely
> what the user gonna be doing most of the time.

What about indication which player are currently addressed? I think that one player may be indicated by icon, etc.


> The player interfaces may have a disable/enable bluetooth button where
> it unregister/register itself, that way you can control what metadata
> will be displayed.
>
> Then don't unregister the player just let it mark itself as
> non-addressable, so the metadata policy is directly controlled by the
> player itself not an external tool.

This is already done by this patchset. Players are registrated and rarely unregistered.
Player may use any of BlueZ interface anytime, so can implementing that by
using property AddressedPlayer (but should not that automaticaly, only by "button")


> In a multiuser environment this doesn't work, we cannot list players
> from other users, so at this point this became completely invalid use
> case.

In patchset we can print player list by adapters. No any user/dbus sender relation.



> Im not cutting anything, all the use cases are covered, you just can't
> overwrite the addressed player with an external tool because we have
> no idea what user will be running that tool and if it has permission
> to change the addressed player, and that is by design to avoid having
> to write a lot of code to manage this when the great majority of users
> will just have one player active at time, thus making this useless.

Ther is no any code to managment, right? This patches only implements the possibility to implement
the managment tool. By player or external tool. Seems to user can disconnect device by BlueZ/Dbus
so changing addressed player can be done in the same way. Do I not miss something?


> Note, this is nothing we cannot add in future, but it is always better
> to start we something simple so there is less risk break API, we did
> this a couple of times and it is always hard to deprecate things.

So maybe good idea is partially merge? Are everything without media interface "AddressedPlayer" ok for you? (but within "Players" property)
Without whole new interface?


Summary
1. Regarding to the playerList feature, we cannot unregister any player
2. AddressedPlayer can be changed on local device (not only remote controller)
3. More than one player may be in playing state at time same time. Audio stream should be switched with addressed player ("routing").


Regards / Pozdrawiam
-------------------------------------------------------------------------------------------------------------
Michał Łabędzki
ASCII: Michal Labedzki
e-mail: michal.labedzki@tieto.com
office communicator: michal.labedzki@tieto.com
location: Poland, Wrocław, Legnicka 55F
room: 315
phone: +48 717 740 340
---
Tieto Corporation / Tieto Poland
http://www.tieto.com / http://www.tieto.pl
---
Tieto Poland spółka z ograniczoną odpowiedzialnością z siedzibą w Szczecinie, ul. Malczewskiego 26. Zarejestrowana w Sądzie Rejonowym Szczecin-Centrum w Szczecinie, XIII Wydział Gospodarczy Krajowego Rejestru Sądowego pod numerem 0000124858. NIP: 8542085557. REGON: 812023656. Kapitał zakładowy: 4 271500 PLN

Re: [PATCH 02/13] AVRCP: Use name "addressed" instead of "active"

[Luiz Augusto von Dentz]

Hi Michal,

On Fri, Jun 15, 2012 at 10:47 AM,  <Michal.Labedzki@tieto.com> wrote:
> Hi Luiz,
>
>> Register at boot time? Are you serious you want players to be started
>> during boot?
>
> Not only system-stable players, like main player.

What is a main player? Where I can find one?

>> If the player is not running it cannot be registered
>> since it cannot be connected to D-Bus an thus cannot receive any
>> messages, an using an agent to register itself is also a bad idea
>> since you would have to forward message which is considered a very bad
>> design.
>
> "Registration" and "Player" should be separated. So player can be registered by external tool, like bootscripts. Then starting player by dbus call.

Do you really want that each player install a file saying it needs to
be registered during the boot? And you call that a good design?
Besides we cannot start the player like that, we are in the system bus
the player needs to be activated on the session bus which we don't
have access to.

> Offtopic: forwarding messages are not that bad. It is only an issue introduce by bad interface design, which are too poor to do something directly.

You got to be kidding, any forward message adds another timeout
handler, pending call handler, not to mention latency, tracking the
sender, just about everything got more complicated. It is a bad
design, so bad that motivates D-Bus on kernel to get rid of the D-Bus
daemon forwarding messages.

>> AddressedPlayerChanged is a notification that the player is no longer
>> available
>
> No, no, no. For available player you got separated event: AvailablePlayersChanged, and after that you should download list of the players again, so that is for detecting available players. On the other hand you got event AddressedPlayerChanged which is designed for detecting currently controlled player by local user.

That doesn't mean we need to expose this to the user and in most of
cases the user don't really switch the addressed player because it has
only one anyway.

> Specification 6.9:
> "The player to which the AV/C Panel Subunit [2] shall route control commands is the
> addressed player. This may be changed locally on the TG, or by the CT using the SetAddressedPlayer command."
>
>
>> , and the browsed player reacts to addressed player
>
> What? Do you not try to mix these functionalities? AddressedPlayer and BrowsedPlayer can be separated. Player decide of that. May implement OnlyBrowsableWhenAddressed or no.

Why you want the player to decide that? It doesn't need to know about
this details, specially if there is only one player in the system,
which is what we should be focusing.

>>, btw once
>> you change the addressed player the controller has to register
>> everything again, it is a very bad design no matter how you look at
>> it.
>
> No. It is not bad. It is AVRCP 1.4 feature based on notification nature. Look to specification 6.9.2.2
> "On completion of the Addressed Player Changed notification the TG shall complete all
> player specific notifications with AV/C C-Type REJECTED with error code Addressed
> Player Changed." - so controller will probably registering events again. This allow to update content of mediaplayer on controller.

Yeah, that indeed is a very good design to reject everything, takes a
few message and some time, and register everything again, another few
messages, then in the meantime the user select another player, nobody
need to be a genius to know this is racy and probably will cause quite
a few IOP problems.

>> I disagree, exposing this will just bloat the UI and probably fragment
>> the testing of this feature since each environment may implement this
>> differently, not to mention with multiuser environment this simple
>> doesn't work, we should really concentrate on having something that
>> works reliable with one player active at time which is most likely
>> what the user gonna be doing most of the time.
>
> What about indication which player are currently addressed? I think that one player may be indicated by icon, etc.

You can indicate that by a method call e.g.
org.bluez.MediaPlayer.SetProperty("Addressed", TRUE), so far I haven't
seen any use for this but perhaps to update the audio routing it would
be acceptable.

>
>> The player interfaces may have a disable/enable bluetooth button where
>> it unregister/register itself, that way you can control what metadata
>> will be displayed.
>>
>> Then don't unregister the player just let it mark itself as
>> non-addressable, so the metadata policy is directly controlled by the
>> player itself not an external tool.
>
> This is already done by this patchset. Players are registrated and rarely unregistered.
> Player may use any of BlueZ interface anytime, so can implementing that by
> using property AddressedPlayer (but should not that automaticaly, only by "button")
>
>
>> In a multiuser environment this doesn't work, we cannot list players
>> from other users, so at this point this became completely invalid use
>> case.
>
> In patchset we can print player list by adapters. No any user/dbus sender relation.
>

Exactly, but this is the problem, it should be able to list the
players only for the same user, otherwise one can use this to gain
some access to other user players and change the addressed.

>
>> Im not cutting anything, all the use cases are covered, you just can't
>> overwrite the addressed player with an external tool because we have
>> no idea what user will be running that tool and if it has permission
>> to change the addressed player, and that is by design to avoid having
>> to write a lot of code to manage this when the great majority of users
>> will just have one player active at time, thus making this useless.
>
> Ther is no any code to managment, right? This patches only implements the possibility to implement
> the managment tool. By player or external tool. Seems to user can disconnect device by BlueZ/Dbus
> so changing addressed player can be done in the same way. Do I not miss something?

Which is only necessary if we implement all things you are suggesting,
right? And the worst part is that you don't even have an example of
such external tool, and nobody else seems to care either.

>
>> Note, this is nothing we cannot add in future, but it is always better
>> to start we something simple so there is less risk break API, we did
>> this a couple of times and it is always hard to deprecate things.
>
> So maybe good idea is partially merge? Are everything without media interface "AddressedPlayer" ok for you? (but within "Players" property)
> Without whole new interface?

I don't want the Player nor the AddressedPlayer, that being clear I do
see some other changes might be okay, we definable need a dummy player
until one is registered for 1.3 and 1.4 if AvailablePlayersChanged is
not used.

> Summary
> 1. Regarding to the playerList feature, we cannot unregister any player

Yes we can, that is what AvailablePlayersChanged is for.

> 2. AddressedPlayer can be changed on local device (not only remote controller)

Only by the player itself, so it is being addressed and mark itself
non-addressable/unregister/disconnect from D-Bus then the addressed
player is changed.

> 3. More than one player may be in playing state at time same time. Audio stream should be switched with addressed player ("routing").

The less the player know about Bluetooth the better, that is the point
of integrating with PulseAudio so that we don't need to invent our own
API, the same applies here to routing, we won't create any specific
policy for this in bluetoothd, the player may tag the stream to use
bluetooth but that is up to the player.

-- 
Luiz Augusto von Dentz

RE: [PATCH 02/13] AVRCP: Use name "addressed" instead of "active"

[Michal.Labedzki]

Hello again Luiz,

> What is a main player? Where I can find one?

Main player? Your favourite player. Frequently used.
For example in my system may be usable four separated player.:
1. mplayer for music/video [main]
2. mplayer for radio
3. mplayer for cam
4. vlc for Internet video streaming

I expect all four players should be available all time via RemoteController.

>> If the player is not running it cannot be registered
>> since it cannot be connected to D-Bus an thus cannot receive any
>> messages, an using an agent to register itself is also a bad idea
>> since you would have to forward message which is considered a very bad
>> design.
>
> "Registration" and "Player" should be separated. So player can be registered by external tool, like bootscripts. Then starting player by dbus call.

> Do you really want that each player install a file saying it needs to
> be registered during the boot? And you call that a good design?
> Besides we cannot start the player like that, we are in the system bus
> the player needs to be activated on the session bus which we don't
> have access to.

Yes, it is good design. User experience Luiz. Boot time is only example. You can do that any time. On login or run player. 

Session bus? But how run obexd by dbus now? Cannot use session bus? (maybe through external component) 

> Offtopic: forwarding messages are not that bad. It is only an issue introduce by bad interface design, which are too poor to do something directly.

> You got to be kidding, any forward message adds another timeout
> handler, pending call handler, not to mention latency, tracking the
> sender, just about everything got more complicated. It is a bad
> design, so bad that motivates D-Bus on kernel to get rid of the D-Bus
> daemon forwarding messages.

No, no. You look wrong on that. What do you want to choose? System without feature or system with little overload but within feature? Costs vs Features.
AVRCP may not generate output like HID or A2DP/VDP. Also there is a cache on Controller side. 
Take it easy :)



> No, no, no. For available player you got separated event: AvailablePlayersChanged, and after that you should download list of the players again, so that is for detecting available players. On the other hand you got event AddressedPlayerChanged which is designed for detecting currently controlled player by local user.

> That doesn't mean we need to expose this to the user and in most of
> cases the user don't really switch the addressed player because it has
> only one anyway

This is your opinion. In my opinion this will be very usable. If you do not want that, please do not use that. But we cannot use that if we do not have that in BlueZ, right?
Think about you keyboard. If you use keyboard then you expect the same functionality on onscreen keyboard, right?

>> Specification 6.9:
>> "The player to which the AV/C Panel Subunit [2] shall route control commands is the
>> addressed player. This may be changed locally on the TG, or by the CT using the SetAddressedPlayer command."
>>
>> What? Do you not try to mix these functionalities? AddressedPlayer and BrowsedPlayer can be separated. Player decide of that. May implement OnlyBrowsableWhenAddressed or no.
>
> Why you want the player to decide that?
> It doesn't need to know about
> this details, specially if there is only one player in the system,
> which is what we should be focusing.

Do you mean feature "OnlyBrowsableWhenAddressed" in PlayerFeatureBitmask? Name of this field describing what player can very well.

Specification 5.9:
"The media player selection functionality allows a TG device to support a range of media
players with varying functionality." (so one mediaplayer has "browsing", other no, etc.)


>>  No. It is not bad. It is AVRCP 1.4 feature based on notification nature. Look to specification 6.9.2.2
>>  "On completion of the Addressed Player Changed notification the TG shall complete all
>>  player specific notifications with AV/C C-Type REJECTED with error code Addressed
> > Player Changed." - so controller will probably registering events again. This allow to update content of mediaplayer on controller.

> Yeah, that indeed is a very good design to reject everything, takes a
> few message and some time, and register everything again, another few
> messages, then in the meantime the user select another player, nobody
> need to be a genius to know this is racy and probably will cause quite
> a few IOP problems.

This base on external AV specification  from 1998 :)


> >What about indication which player are currently addressed? I think that one player may be indicated by icon, etc.
> You can indicate that by a method call e.g.
> org.bluez.MediaPlayer.SetProperty("Addressed", TRUE), so far I haven't
> seen any use for this but perhaps to update the audio routing it would
> be acceptable.

Is that not bad design? Only one player can be addressed, but what when you done?:
/player1 org.bluez.MediaPlayer.SetProperty("Addressed", TRUE)
/player2 org.bluez.MediaPlayer.SetProperty("Addressed", TRUE)

Expected be  two player to be Addressed (two separated object by ObjectPath)




>> So maybe good idea is partially merge? Are everything without media interface "AddressedPlayer" ok for you? (but within "Players" property)
>> Without whole new interface?
> I don't want the Player nor the AddressedPlayer, that being clear I do
> see some other changes might be okay, we definable need a dummy player
> until one is registered for 1.3 and 1.4 if AvailablePlayersChanged is
> not used.

Ok. I will prepare new patchset.



>> Summary
>> 1. Regarding to the playerList feature, we cannot unregister any player
> Yes we can, that is what AvailablePlayersChanged is for.

Yes, but I thinking about normal use case. I you want to change current player, you need two or more, so you cannot unregister player anytime.

Specification 6.9:
"Note that available means that a player can be accessed via
AVRCP with no user interaction locally on the TG. It does not imply that the media
player application must be currently running."


>> 2. AddressedPlayer can be changed on local device (not only remote controller)
> Only by the player itself, so it is being addressed and mark itself
> non-addressable/unregister/disconnect from D-Bus then the addressed
> player is changed.

Why player should have ability to mark itself as non-addressed? 

I see to way to set player as addressed:
1. User decision, by mediaplayer or external component like VirtualOnscreenRemoteController.
2. On playing state (this implies war if you start more then one player and push play... last one will be addressed, but some player may do sequence stoppend/playing on the trackchanged, so metadata may blinking...)


> 3. More than one player may be in playing state at time same time. Audio stream should be switched with addressed player ("routing").

The less the player know about Bluetooth the better, that is the point
of integrating with PulseAudio so that we don't need to invent our own
API, the same applies here to routing, we won't create any specific
policy for this in bluetoothd, the player may tag the stream to use
bluetooth but that is up to the player.

What about system without PulseAudio? And how ALSA will know that one stream is from player A, other one from player B and how it will make decision which should be routed to the RemoteController (Headset)
By the way, what can be an alternative for PulseAudio?



Regards / Pozdrawiam
-------------------------------------------------------------------------------------------------------------
Michał Łabędzki
ASCII: Michal Labedzki
e-mail: michal.labedzki@tieto.com
office communicator: michal.labedzki@tieto.com
location: Poland, Wrocław, Legnicka 55F
room: 315
phone: +48 717 740 340
---
Tieto Corporation / Tieto Poland
http://www.tieto.com / http://www.tieto.pl
---
Tieto Poland spółka z ograniczoną odpowiedzialnością z siedzibą w Szczecinie, ul. Malczewskiego 26. Zarejestrowana w Sądzie Rejonowym Szczecin-Centrum w Szczecinie, XIII Wydział Gospodarczy Krajowego Rejestru Sądowego pod numerem 0000124858. NIP: 8542085557. REGON: 812023656. Kapitał zakładowy: 4 271500 PLN

Re: [PATCH 05/13] AVRCP: Register AVRCP before MEDIA interface

[Luiz Augusto von Dentz]

Hi Michal,

On Wed, Jun 6, 2012 at 7:06 PM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> Register AVRCP before MEDIA interface to avoid searching for or
> accessing non-existent AVRCP server.
> ---
>  audio/manager.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/audio/manager.c b/audio/manager.c
> index d442d1d..076346a 100644
> --- a/audio/manager.c
> +++ b/audio/manager.c
> @@ -1238,6 +1238,9 @@ proceed:
>        if (enabled.socket)
>                unix_init();
>
> +       if (enabled.control)
> +               btd_register_adapter_driver(&avrcp_server_driver);
> +
>        if (enabled.media)
>                btd_register_adapter_driver(&media_server_driver);
>
> @@ -1250,9 +1253,6 @@ proceed:
>        if (enabled.source || enabled.sink)
>                btd_register_adapter_driver(&a2dp_server_driver);
>
> -       if (enabled.control)
> -               btd_register_adapter_driver(&avrcp_server_driver);
> -
>        btd_register_device_driver(&audio_driver);
>
>        *enable_sco = (enabled.gateway || enabled.headset);
> --
> on behalf of ST-Ericsson

Ack


-- 
Luiz Augusto von Dentz

Re: [PATCH 04/13] AVRCP: Keep AVRCP version of connected device in server

[Luiz Augusto von Dentz]

Hi Michal,

On Wed, Jun 6, 2012 at 7:06 PM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> This can be used to improve compatibility between
> AVRCP 1.3 vs AVRCP 1.4 devices and implementation in stack.
> ---
>  audio/avrcp.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 7576e19..9ff0700 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -200,6 +200,7 @@ struct avrcp_server {
>        uint32_t ct_record_id;
>        GSList *players;
>        struct avrcp_player *addressed_player;
> +       uint16_t version;
>  };
>
>  struct pending_pdu {
> @@ -1288,8 +1289,12 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
>                        return;
>
>                desc = list->data;
> +               if (desc)
> +                       server->version = desc->version;
> +               else
> +                       server->version = AVRCP_VERSION_UNKNOWN;
>
> -               if (desc && desc->version >= AVRCP_VERSION_1_4)
> +               if (server->version >= AVRCP_VERSION_1_4)
>                        register_volume_notification(player);
>
>                sdp_list_free(list, free);
> @@ -1380,6 +1385,8 @@ int avrcp_register(DBusConnection *conn, const bdaddr_t *src, GKeyFile *config)
>
>        bacpy(&server->src, src);
>
> +       server->version = AVRCP_VERSION_UNKNOWN;
> +
>        servers = g_slist_append(servers, server);
>
>        return 0;
> --
> on behalf of ST-Ericsson

The idea is fine, but this is not the server version, it is the
version used in the connection so it probably need to be stored in
avrcp_player. If there remote doesn't support AVRCP 1.3 or latter we
don't need to assign a player.


-- 
Luiz Augusto von Dentz

RE: [PATCH 04/13] AVRCP: Keep AVRCP version of connected device in server

[Michal.Labedzki]

Hi Luiz,

> The idea is fine, but this is not the server version, it is the
> version used in the connection so it probably need to be stored in
> avrcp_player. If there remote doesn't support AVRCP 1.3 or latter we
> don't need to assign a player.

Ok. This is "remote_version" - remote controller version of AVRCP.Therefore on (new) connection this version will be updated.
avrcp_player is not good place for that, because connection is only one, so version is the same for all players, but players may be for example 67.
So maybe only fix the name will be ok. What do you think?


Regards / Pozdrawiam
-------------------------------------------------------------------------------------------------------------
Michał Łabędzki
ASCII: Michal Labedzki
e-mail: michal.labedzki@tieto.com
office communicator: michal.labedzki@tieto.com
location: Poland, Wrocław, Legnicka 55F
room: 315
phone: +48 717 740 340
---
Tieto Corporation / Tieto Poland
http://www.tieto.com / http://www.tieto.pl
---
Tieto Poland spółka z ograniczoną odpowiedzialnością z siedzibą w Szczecinie, ul. Malczewskiego 26. Zarejestrowana w Sądzie Rejonowym Szczecin-Centrum w Szczecinie, XIII Wydział Gospodarczy Krajowego Rejestru Sądowego pod numerem 0000124858. NIP: 8542085557. REGON: 812023656. Kapitał zakładowy: 4 271500 PLN

Re: [PATCH 04/13] AVRCP: Keep AVRCP version of connected device in server

[Luiz Augusto von Dentz]

Hi Michal,

On Mon, Jun 18, 2012 at 1:30 PM,  <Michal.Labedzki@tieto.com> wrote:
> Hi Luiz,
>
>> The idea is fine, but this is not the server version, it is the
>> version used in the connection so it probably need to be stored in
>> avrcp_player. If there remote doesn't support AVRCP 1.3 or latter we
>> don't need to assign a player.
>
> Ok. This is "remote_version" - remote controller version of AVRCP.Therefore on (new) connection this version will be updated.
> avrcp_player is not good place for that, because connection is only one, so version is the same for all players, but players may be for example 67.
> So maybe only fix the name will be ok. What do you think?

Then perhaps have another structure e.g. avrcp_session that represents
the connection where this information can be stored.

-- 
Luiz Augusto von Dentz