From: Luiz Augusto von Dentz <[email protected]>
This replaces get_metadata callback with get_string and get_uint32
which uses proper types as return.
---
audio/avrcp.c | 32 ++++++++++++++++++--------------
audio/avrcp.h | 3 ++-
audio/media.c | 31 ++++++++++++++++++++++++-------
3 files changed, 44 insertions(+), 22 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 2f5df21..5a18cb4 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -501,23 +501,29 @@ static uint16_t player_write_media_attribute(struct avrcp_player *player,
uint16_t len;
uint16_t attr_len;
char valstr[20];
- void *value;
+ const char *value = NULL;
+ uint32_t num;
DBG("%u", id);
- value = player->cb->get_metadata(id, player->user_data);
- if (value == NULL) {
- *offset = 0;
- return 0;
- }
-
switch (id) {
case AVRCP_MEDIA_ATTRIBUTE_TRACK:
case AVRCP_MEDIA_ATTRIBUTE_N_TRACKS:
case AVRCP_MEDIA_ATTRIBUTE_DURATION:
- snprintf(valstr, 20, "%u", GPOINTER_TO_UINT(value));
+ num = player->cb->get_uint32(id, player->user_data);
+ if (num == 0)
+ break;
+
+ snprintf(valstr, 20, "%u", num);
value = valstr;
break;
+ default:
+ value = player->cb->get_string(id, player->user_data);
+ }
+
+ if (value == NULL) {
+ *offset = 0;
+ return 0;
}
attr_len = strlen(value);
@@ -946,7 +952,6 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp *session,
uint16_t len = ntohs(pdu->params_len);
uint32_t position;
uint32_t duration;
- void *pduration;
if (len != 0 || player == NULL) {
pdu->params_len = htons(1);
@@ -955,14 +960,13 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp *session,
}
position = player->cb->get_position(player->user_data);
- pduration = player->cb->get_metadata(AVRCP_MEDIA_ATTRIBUTE_DURATION,
+ duration = player->cb->get_uint32(AVRCP_MEDIA_ATTRIBUTE_DURATION,
player->user_data);
- if (pduration != NULL)
- duration = htonl(GPOINTER_TO_UINT(pduration));
- else
- duration = htonl(UINT32_MAX);
+ if (duration == 0)
+ duration = UINT32_MAX;
position = htonl(position);
+ duration = htonl(duration);
memcpy(&pdu->params[0], &duration, 4);
memcpy(&pdu->params[4], &position, 4);
diff --git a/audio/avrcp.h b/audio/avrcp.h
index 6c651dd..1a1e111 100644
--- a/audio/avrcp.h
+++ b/audio/avrcp.h
@@ -80,7 +80,8 @@ struct avrcp_player_cb {
int (*get_setting) (uint8_t attr, void *user_data);
int (*set_setting) (uint8_t attr, uint8_t value, void *user_data);
uint64_t (*get_uid) (void *user_data);
- void *(*get_metadata) (uint32_t id, void *user_data);
+ uint32_t (*get_uint32) (uint32_t id, void *user_data);
+ const char *(*get_string) (uint32_t id, void *user_data);
GList *(*list_metadata) (void *user_data);
uint8_t (*get_status) (void *user_data);
uint32_t (*get_position) (void *user_data);
diff --git a/audio/media.c b/audio/media.c
index f2b5b2f..116e2cd 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1281,7 +1281,7 @@ static uint64_t get_uid(void *user_data)
return 0;
}
-static void *get_metadata(uint32_t id, void *user_data)
+static const char *get_string(uint32_t id, void *user_data)
{
struct media_player *mp = user_data;
struct metadata_value *value;
@@ -1295,16 +1295,32 @@ static void *get_metadata(uint32_t id, void *user_data)
if (!value)
return NULL;
- switch (value->type) {
- case DBUS_TYPE_STRING:
+ if (value->type == DBUS_TYPE_STRING)
return value->value.str;
- case DBUS_TYPE_UINT32:
- return GUINT_TO_POINTER(value->value.num);
- }
return NULL;
}
+static uint32_t get_uint32(uint32_t id, void *user_data)
+{
+ struct media_player *mp = user_data;
+ struct metadata_value *value;
+
+ DBG("%s", metadata_to_str(id));
+
+ if (mp->track == NULL)
+ return 0;
+
+ value = g_hash_table_lookup(mp->track, GUINT_TO_POINTER(id));
+ if (!value)
+ return 0;
+
+ if (value->type == DBUS_TYPE_UINT32)
+ return value->value.num;
+
+ return 0;
+}
+
static uint8_t get_status(void *user_data)
{
struct media_player *mp = user_data;
@@ -1360,7 +1376,8 @@ static struct avrcp_player_cb player_cb = {
.set_setting = set_setting,
.list_metadata = list_metadata,
.get_uid = get_uid,
- .get_metadata = get_metadata,
+ .get_uint32 = get_uint32,
+ .get_string = get_string,
.get_position = get_position,
.get_status = get_status,
.set_volume = set_volume
--
1.7.11.4
Hi Lucas,
On Tue, Oct 16, 2012 at 3:58 PM, Lucas De Marchi
<[email protected]> wrote:
> On Tue, Oct 16, 2012 at 8:58 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> Some devices w.g. Sony MW600 will stop using certain commands if an
>> error happen, so the code now just fake a player and once a real
>> player is registered it takes place of the fake one.
>> ---
>> audio/avrcp.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 74 insertions(+), 17 deletions(-)
>>
>> diff --git a/audio/avrcp.c b/audio/avrcp.c
>> index 5a18cb4..cd57358 100644
>> --- a/audio/avrcp.c
>> +++ b/audio/avrcp.c
>> @@ -493,6 +493,28 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
>> return;
>> }
>>
>> +static const char *player_get_string(struct avrcp_player *player, uint32_t attr)
>> +{
>
> Instead of adding these wrapper functions, did you consider having a
> default_avrcp_player with all hooks set and player pointing to it if
> there's no player registered?
It is probably more code, besides I would not like to mix const
pointers with dynamic allocated ones.
--
Luiz Augusto von Dentz
Hi Lucas,
On Tue, Oct 16, 2012 at 3:56 PM, Lucas De Marchi
<[email protected]> wrote:
> Hi Luiz
>
> On Tue, Oct 16, 2012 at 8:58 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> This replaces get_metadata callback with get_string and get_uint32
>> which uses proper types as return.
>> ---
>
> I fail to see what this commit is fixing. It seems more like
> refactoring void* to explicit types.
>
>
>> audio/avrcp.c | 32 ++++++++++++++++++--------------
>> audio/avrcp.h | 3 ++-
>> audio/media.c | 31 ++++++++++++++++++++++++-------
>> 3 files changed, 44 insertions(+), 22 deletions(-)
>>
>> diff --git a/audio/avrcp.c b/audio/avrcp.c
>> index 2f5df21..5a18cb4 100644
>> --- a/audio/avrcp.c
>> +++ b/audio/avrcp.c
>> @@ -501,23 +501,29 @@ static uint16_t player_write_media_attribute(struct avrcp_player *player,
>> uint16_t len;
>> uint16_t attr_len;
>> char valstr[20];
>> - void *value;
>> + const char *value = NULL;
>> + uint32_t num;
>>
>> DBG("%u", id);
>>
>> - value = player->cb->get_metadata(id, player->user_data);
>> - if (value == NULL) {
>> - *offset = 0;
>> - return 0;
>> - }
>> -
>> switch (id) {
>> case AVRCP_MEDIA_ATTRIBUTE_TRACK:
>> case AVRCP_MEDIA_ATTRIBUTE_N_TRACKS:
>> case AVRCP_MEDIA_ATTRIBUTE_DURATION:
>> - snprintf(valstr, 20, "%u", GPOINTER_TO_UINT(value));
>> + num = player->cb->get_uint32(id, player->user_data);
>> + if (num == 0)
>> + break;
>> +
>> + snprintf(valstr, 20, "%u", num);
>
> The downside here is that we loose the ability to differentiate
> attribute not present from the value 0. 0 doesn't make sense for
> duration, but is a valid value for the other 2.
Which ones? You mean track and track number, I guess we can skip those
if they are 0.
--
Luiz Augusto von Dentz
On Tue, Oct 16, 2012 at 8:58 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> Some devices w.g. Sony MW600 will stop using certain commands if an
> error happen, so the code now just fake a player and once a real
> player is registered it takes place of the fake one.
> ---
> audio/avrcp.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 74 insertions(+), 17 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 5a18cb4..cd57358 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -493,6 +493,28 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
> return;
> }
>
> +static const char *player_get_string(struct avrcp_player *player, uint32_t attr)
> +{
Instead of adding these wrapper functions, did you consider having a
default_avrcp_player with all hooks set and player pointing to it if
there's no player registered?
Lucas De Marchi
Hi Luiz
On Tue, Oct 16, 2012 at 8:58 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> This replaces get_metadata callback with get_string and get_uint32
> which uses proper types as return.
> ---
I fail to see what this commit is fixing. It seems more like
refactoring void* to explicit types.
> audio/avrcp.c | 32 ++++++++++++++++++--------------
> audio/avrcp.h | 3 ++-
> audio/media.c | 31 ++++++++++++++++++++++++-------
> 3 files changed, 44 insertions(+), 22 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 2f5df21..5a18cb4 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -501,23 +501,29 @@ static uint16_t player_write_media_attribute(struct avrcp_player *player,
> uint16_t len;
> uint16_t attr_len;
> char valstr[20];
> - void *value;
> + const char *value = NULL;
> + uint32_t num;
>
> DBG("%u", id);
>
> - value = player->cb->get_metadata(id, player->user_data);
> - if (value == NULL) {
> - *offset = 0;
> - return 0;
> - }
> -
> switch (id) {
> case AVRCP_MEDIA_ATTRIBUTE_TRACK:
> case AVRCP_MEDIA_ATTRIBUTE_N_TRACKS:
> case AVRCP_MEDIA_ATTRIBUTE_DURATION:
> - snprintf(valstr, 20, "%u", GPOINTER_TO_UINT(value));
> + num = player->cb->get_uint32(id, player->user_data);
> + if (num == 0)
> + break;
> +
> + snprintf(valstr, 20, "%u", num);
The downside here is that we loose the ability to differentiate
attribute not present from the value 0. 0 doesn't make sense for
duration, but is a valid value for the other 2.
> value = valstr;
> break;
> + default:
> + value = player->cb->get_string(id, player->user_data);
> + }
> +
> + if (value == NULL) {
> + *offset = 0;
> + return 0;
> }
>
> attr_len = strlen(value);
> @@ -946,7 +952,6 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp *session,
> uint16_t len = ntohs(pdu->params_len);
> uint32_t position;
> uint32_t duration;
> - void *pduration;
>
> if (len != 0 || player == NULL) {
> pdu->params_len = htons(1);
> @@ -955,14 +960,13 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp *session,
> }
>
> position = player->cb->get_position(player->user_data);
> - pduration = player->cb->get_metadata(AVRCP_MEDIA_ATTRIBUTE_DURATION,
> + duration = player->cb->get_uint32(AVRCP_MEDIA_ATTRIBUTE_DURATION,
> player->user_data);
> - if (pduration != NULL)
> - duration = htonl(GPOINTER_TO_UINT(pduration));
> - else
> - duration = htonl(UINT32_MAX);
> + if (duration == 0)
> + duration = UINT32_MAX;
>
> position = htonl(position);
> + duration = htonl(duration);
>
> memcpy(&pdu->params[0], &duration, 4);
> memcpy(&pdu->params[4], &position, 4);
> diff --git a/audio/avrcp.h b/audio/avrcp.h
> index 6c651dd..1a1e111 100644
> --- a/audio/avrcp.h
> +++ b/audio/avrcp.h
> @@ -80,7 +80,8 @@ struct avrcp_player_cb {
> int (*get_setting) (uint8_t attr, void *user_data);
> int (*set_setting) (uint8_t attr, uint8_t value, void *user_data);
> uint64_t (*get_uid) (void *user_data);
> - void *(*get_metadata) (uint32_t id, void *user_data);
> + uint32_t (*get_uint32) (uint32_t id, void *user_data);
> + const char *(*get_string) (uint32_t id, void *user_data);
> GList *(*list_metadata) (void *user_data);
> uint8_t (*get_status) (void *user_data);
> uint32_t (*get_position) (void *user_data);
> diff --git a/audio/media.c b/audio/media.c
> index f2b5b2f..116e2cd 100644
> --- a/audio/media.c
> +++ b/audio/media.c
> @@ -1281,7 +1281,7 @@ static uint64_t get_uid(void *user_data)
> return 0;
> }
>
> -static void *get_metadata(uint32_t id, void *user_data)
> +static const char *get_string(uint32_t id, void *user_data)
> {
> struct media_player *mp = user_data;
> struct metadata_value *value;
> @@ -1295,16 +1295,32 @@ static void *get_metadata(uint32_t id, void *user_data)
> if (!value)
> return NULL;
>
> - switch (value->type) {
> - case DBUS_TYPE_STRING:
> + if (value->type == DBUS_TYPE_STRING)
> return value->value.str;
> - case DBUS_TYPE_UINT32:
> - return GUINT_TO_POINTER(value->value.num);
> - }
>
> return NULL;
> }
>
> +static uint32_t get_uint32(uint32_t id, void *user_data)
> +{
> + struct media_player *mp = user_data;
> + struct metadata_value *value;
> +
> + DBG("%s", metadata_to_str(id));
> +
> + if (mp->track == NULL)
> + return 0;
> +
> + value = g_hash_table_lookup(mp->track, GUINT_TO_POINTER(id));
> + if (!value)
> + return 0;
> +
> + if (value->type == DBUS_TYPE_UINT32)
> + return value->value.num;
> +
> + return 0;
> +}
> +
> static uint8_t get_status(void *user_data)
> {
> struct media_player *mp = user_data;
> @@ -1360,7 +1376,8 @@ static struct avrcp_player_cb player_cb = {
> .set_setting = set_setting,
> .list_metadata = list_metadata,
> .get_uid = get_uid,
> - .get_metadata = get_metadata,
> + .get_uint32 = get_uint32,
> + .get_string = get_string,
> .get_position = get_position,
> .get_status = get_status,
> .set_volume = set_volume
> --
> 1.7.11.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Luiz Augusto von Dentz <[email protected]>
Some devices w.g. Sony MW600 will stop using certain commands if an
error happen, so the code now just fake a player and once a real
player is registered it takes place of the fake one.
---
audio/avrcp.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 74 insertions(+), 17 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 5a18cb4..cd57358 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -493,6 +493,28 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data)
return;
}
+static const char *player_get_string(struct avrcp_player *player, uint32_t attr)
+{
+ if (player != NULL)
+ return player->cb->get_string(attr, player->user_data);
+
+ if (attr == AVRCP_MEDIA_ATTRIBUTE_TITLE)
+ return "";
+
+ return NULL;
+}
+
+static uint32_t player_get_uint32(struct avrcp_player *player, uint32_t attr)
+{
+ if (player != NULL)
+ return player->cb->get_uint32(attr, player->user_data);
+
+ if (attr == AVRCP_MEDIA_ATTRIBUTE_DURATION)
+ return UINT32_MAX;
+
+ return 0;
+}
+
static uint16_t player_write_media_attribute(struct avrcp_player *player,
uint32_t id, uint8_t *buf,
uint16_t *pos,
@@ -510,7 +532,7 @@ static uint16_t player_write_media_attribute(struct avrcp_player *player,
case AVRCP_MEDIA_ATTRIBUTE_TRACK:
case AVRCP_MEDIA_ATTRIBUTE_N_TRACKS:
case AVRCP_MEDIA_ATTRIBUTE_DURATION:
- num = player->cb->get_uint32(id, player->user_data);
+ num = player_get_uint32(player, id);
if (num == 0)
break;
@@ -518,7 +540,7 @@ static uint16_t player_write_media_attribute(struct avrcp_player *player,
value = valstr;
break;
default:
- value = player->cb->get_string(id, player->user_data);
+ value = player_get_string(player, id);
}
if (value == NULL) {
@@ -610,6 +632,9 @@ static int player_set_attribute(struct avrcp_player *player,
{
DBG("Change attribute: %u %u", attr, val);
+ if (player == NULL)
+ return -ENOENT;
+
return player->cb->set_setting(attr, val, player->user_data);
}
@@ -619,6 +644,9 @@ static int player_get_attribute(struct avrcp_player *player, uint8_t attr)
DBG("attr %u", attr);
+ if (player == NULL)
+ return -ENOENT;
+
value = player->cb->get_setting(attr, player->user_data);
if (value < 0)
DBG("attr %u not supported by player", attr);
@@ -675,7 +703,7 @@ static uint8_t avrcp_handle_list_player_attributes(struct avrcp *session,
uint16_t len = ntohs(pdu->params_len);
unsigned int i;
- if (len != 0 || player == NULL) {
+ if (len != 0) {
pdu->params_len = htons(1);
pdu->params[0] = AVRCP_STATUS_INVALID_PARAM;
return AVC_CTYPE_REJECTED;
@@ -707,7 +735,7 @@ static uint8_t avrcp_handle_list_player_values(struct avrcp *session,
uint16_t len = ntohs(pdu->params_len);
unsigned int i;
- if (len != 1 || player == NULL)
+ if (len != 1)
goto err;
if (player_get_attribute(player, pdu->params[0]) < 0)
@@ -729,6 +757,15 @@ err:
return AVC_CTYPE_REJECTED;
}
+static GList *player_list_metadata(struct avrcp_player *player)
+{
+ if (player != NULL)
+ return player->cb->list_metadata(player->user_data);
+
+ return g_list_prepend(NULL,
+ GUINT_TO_POINTER(AVRCP_MEDIA_ATTRIBUTE_TITLE));
+}
+
static uint8_t avrcp_handle_get_element_attributes(struct avrcp *session,
struct avrcp_header *pdu,
uint8_t transaction)
@@ -741,7 +778,7 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp *session,
GList *attr_ids;
uint16_t offset;
- if (len < 9 || identifier != 0 || player == NULL)
+ if (len < 9 || identifier != 0)
goto err;
nattr = pdu->params[8];
@@ -754,7 +791,7 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp *session,
* Return all available information, at least
* title must be returned if there's a track selected.
*/
- attr_ids = player->cb->list_metadata(player->user_data);
+ attr_ids = player_list_metadata(player);
len = g_list_length(attr_ids);
} else {
unsigned int i;
@@ -810,8 +847,7 @@ static uint8_t avrcp_handle_get_current_player_value(struct avrcp *session,
uint8_t *settings;
unsigned int i;
- if (player == NULL || len <= 1 || pdu->params[0] != len - 1 ||
- player == NULL)
+ if (len <= 1 || pdu->params[0] != len - 1)
goto err;
/*
@@ -944,6 +980,14 @@ err:
return AVC_CTYPE_REJECTED;
}
+static uint32_t player_get_position(struct avrcp_player *player)
+{
+ if (player == NULL)
+ return 0;
+
+ return player->cb->get_position(player->user_data);
+}
+
static uint8_t avrcp_handle_get_play_status(struct avrcp *session,
struct avrcp_header *pdu,
uint8_t transaction)
@@ -953,17 +997,14 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp *session,
uint32_t position;
uint32_t duration;
- if (len != 0 || player == NULL) {
+ if (len != 0) {
pdu->params_len = htons(1);
pdu->params[0] = AVRCP_STATUS_INVALID_PARAM;
return AVC_CTYPE_REJECTED;
}
- position = player->cb->get_position(player->user_data);
- duration = player->cb->get_uint32(AVRCP_MEDIA_ATTRIBUTE_DURATION,
- player->user_data);
- if (duration == 0)
- duration = UINT32_MAX;
+ position = player_get_position(player);
+ duration = player_get_uint32(player, AVRCP_MEDIA_ATTRIBUTE_DURATION);
position = htonl(position);
duration = htonl(duration);
@@ -977,6 +1018,22 @@ static uint8_t avrcp_handle_get_play_status(struct avrcp *session,
return AVC_CTYPE_STABLE;
}
+static uint64_t player_get_uid(struct avrcp_player *player)
+{
+ if (player == NULL)
+ return UINT64_MAX;
+
+ return player->cb->get_status(player->user_data);
+}
+
+static uint8_t player_get_status(struct avrcp_player *player)
+{
+ if (player == NULL)
+ return AVRCP_PLAY_STATUS_STOPPED;
+
+ return player->cb->get_status(player->user_data);
+}
+
static uint8_t avrcp_handle_register_notification(struct avrcp *session,
struct avrcp_header *pdu,
uint8_t transaction)
@@ -990,18 +1047,18 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
* one is applicable only for EVENT_PLAYBACK_POS_CHANGED. See AVRCP
* 1.3 spec, section 5.4.2.
*/
- if (len != 5 || player == NULL)
+ if (len != 5)
goto err;
switch (pdu->params[0]) {
case AVRCP_EVENT_STATUS_CHANGED:
len = 2;
- pdu->params[1] = player->cb->get_status(player->user_data);
+ pdu->params[1] = player_get_status(player);
break;
case AVRCP_EVENT_TRACK_CHANGED:
len = 9;
- uid = player->cb->get_uid(player->user_data);
+ uid = player_get_uid(player);
memcpy(&pdu->params[1], &uid, sizeof(uint64_t));
break;
--
1.7.11.4