---
.gitignore | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/.gitignore b/.gitignore
index 2ce99e7..c2fa9e9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -80,6 +80,7 @@ test/ipctest
test/btiotest
test/test-textfile
test/uuidtest
+test/mpris-player
compat/dund
compat/hidd
compat/pand
--
1.7.7
Hi Lucas,
On Wed, Oct 12, 2011, Lucas De Marchi wrote:
> Allow user to trigger TrackChanged and PropertyChanged. Calls made by
> remote side (coming from D-Bus) are printed to stderr while the ones to
> interact with user go to stdout.
> ---
> test/simple-player | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 67 insertions(+), 4 deletions(-)
Patches 5-12 have been applied also. Thanks.
Johan
Hi Lucas,
On Wed, Oct 12, 2011, Lucas De Marchi wrote:
> ---
> audio/media.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/audio/media.c b/audio/media.c
> index c9fe0f0..56913d5 100644
> --- a/audio/media.c
> +++ b/audio/media.c
> @@ -1438,6 +1438,11 @@ static gboolean parse_player_metadata(struct media_player *mp,
> if (g_hash_table_size(track) == 0) {
> g_hash_table_unref(track);
> track = NULL;
> + } else if (title == FALSE) {
> + struct metadata_value *value = g_new(struct metadata_value, 1);
> +
> + value->type = DBUS_TYPE_STRING;
> + value->value.str = g_strdup("");
> }
>
> if (mp->track != NULL)
This doesn't make any sense. You're allocating a new variable which is
only available inside the "else if" scope, and then doing nothing with
it. Additionally this memory is leaked once you exit the scope.
Johan
Hi Lucas,
On Wed, Oct 12, 2011, Lucas De Marchi wrote:
> If we use the same hash table to set the new metadata, we have 2
> undesired behaviors:
>
> 1) New track may contain fields from previous track if it didn't set all
> the fields
> 2) If we fail on parsing the signal, we will still change some of the
> fields
> ---
> audio/media.c | 41 ++++++++++++++++++++++++++---------------
> doc/media-api.txt | 6 ++++++
> 2 files changed, 32 insertions(+), 15 deletions(-)
The first two patches have been applied but this one doesn't compile:
audio/media.c: In function ‘parse_player_metadata’:
audio/media.c:1358:11: error: variable ‘title’ set but not used [-Werror=unused-but-set-variable]
Johan
---
audio/avrcp.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 6bddf5b..a7987fb 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -565,11 +565,10 @@ static uint8_t avrcp_handle_list_player_values(struct avrcp_player *player,
if (len != 1 || !player)
goto err;
- len = attr_get_max_val(pdu->params[0]);
- if (!len) {
- error("Attribute is invalid: %u", pdu->params[0]);
+ if (player_get_attribute(player, pdu->params[0]) < 0)
goto err;
- }
+
+ len = attr_get_max_val(pdu->params[0]);
for (i = 1; i <= len; i++)
pdu->params[i] = i;
--
1.7.7
Allow user to trigger TrackChanged and PropertyChanged. Calls made by
remote side (coming from D-Bus) are printed to stderr while the ones to
interact with user go to stdout.
---
test/simple-player | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/test/simple-player b/test/simple-player
index f483897..209fcdc 100755
--- a/test/simple-player
+++ b/test/simple-player
@@ -1,5 +1,7 @@
#!/usr/bin/python
+from __future__ import print_function
+import os
import sys
import dbus
import dbus.service
@@ -10,9 +12,68 @@ class Player(dbus.service.Object):
@dbus.service.method("org.bluez.MediaPlayer",
in_signature="sv", out_signature="")
def SetProperty(self, key, value):
- print "SetProperty (%s, %s)" % (key, value)
+ print("SetProperty (%s, %s)" % (key, value), file=sys.stderr)
return
+ @dbus.service.signal("org.bluez.MediaPlayer", signature="sv")
+ def PropertyChanged(self, setting, value):
+ """PropertyChanged(setting, value)
+
+ Send a PropertyChanged signal. 'setting' and 'value' are
+ string parameters as specified in doc/media-api.txt.
+ """
+ pass
+
+ @dbus.service.signal("org.bluez.MediaPlayer", signature="a{sv}")
+ def TrackChanged(self, metadata):
+ """TrackChanged(metadata)
+
+ Send a TrackChanged signal. 'metadata' parameter is a dictionary,
+ with values as defined in doc/media-api.txt.
+ """
+ pass
+
+ def help(self, func):
+ help(self.__class__.__dict__[func])
+
+class InputHandler:
+ commands = { 'TrackChanged': '(metadata)',
+ 'PropertyChanged': '(key, value)',
+ 'help': '(cmd)' }
+ def __init__(self, player):
+ self.player = player
+ print('\n\nAvailable commands:')
+ for cmd in self.commands:
+ print('\t', cmd, self.commands[cmd], sep='')
+
+ print("\nUse python syntax to pass arguments to available methods.\n" \
+ "E.g.: TrackChanged({'Title': 'My title', 'Album': 'my album' })")
+ self.prompt()
+
+ def prompt(self):
+ print('\n>>> ', end='')
+ sys.stdout.flush()
+
+ def handle(self, fd, condition):
+ s = os.read(fd.fileno(), 1024).strip()
+ try:
+ cmd = s[:s.find('(')]
+ if not cmd in self.commands:
+ print("Unknown command ", cmd)
+ except ValueError:
+ print("Malformed command")
+ return True
+
+ try:
+ exec "self.player.%s" % s
+ except Exception as e:
+ print(e)
+ pass
+ self.prompt()
+ return True
+
+
+
if __name__ == '__main__':
dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
@@ -39,8 +100,6 @@ if __name__ == '__main__':
"Status" : "playing",
"Position" : dbus.UInt32(0) })
- print properties
-
metadata = dbus.Dictionary({ "Title" : "Title",
"Artist" : "Artist",
"Album" : "Album",
@@ -49,7 +108,11 @@ if __name__ == '__main__':
"Number" : dbus.UInt32(1),
"Duration" : dbus.UInt32(10000) })
- print metadata
+ print('Register media player with:\n\tProperties: %s\n\tMetadata: %s' \
+ % (properties, metadata))
+
+ handler = InputHandler(player)
+ gobject.io_add_watch(sys.stdin, gobject.IO_IN, handler.handle)
media.RegisterPlayer(path, properties, metadata)
--
1.7.7
Refactor code when we are 'listing selected attributes' to share code
with 'listing all attributes'. This way we always keep the list of
attributes in a GList, and call player_get_media_attribute() in only one
place.
---
audio/avrcp.c | 55 ++++++++++++++++++++++---------------------------------
1 files changed, 22 insertions(+), 33 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 4e3ef97..ac7c8d4 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -591,11 +591,11 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
uint8_t transaction)
{
uint16_t len = ntohs(pdu->params_len);
- uint64_t *identifier = (void *) &pdu->params[0];
+ uint64_t *identifier = (uint64_t *) &pdu->params[0];
uint16_t pos;
uint8_t nattr;
int size;
- unsigned int i;
+ GList *l, *attr_ids;
if (len < 9 || *identifier != 0)
goto err;
@@ -605,53 +605,42 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
if (len < nattr * sizeof(uint32_t) + 1)
goto err;
- len = 0;
- pos = 1; /* Keep track of current position in reponse */
-
if (!nattr) {
/*
* Return all available information, at least
* title must be returned if there's a track selected.
*/
- GList *l, *attr_ids = player->cb->list_metadata(player->user_data);
-
- for (l = attr_ids; l != NULL; l = l->next) {
- uint32_t attr = GPOINTER_TO_UINT(l->data);
-
- size = player_get_media_attribute(player, attr,
- &pdu->params[pos],
- AVRCP_PDU_MTU - pos);
+ attr_ids = player->cb->list_metadata(player->user_data);
+ } else {
+ unsigned int i;
+ uint32_t *attr = (uint32_t *) &pdu->params[9];
- if (size > 0) {
- len++;
- pos += size;
- }
+ for (i = 0, attr_ids = NULL; i < nattr; i++, attr++) {
+ uint32_t id = ntohl(bt_get_unaligned(attr));
+ attr_ids = g_list_prepend(attr_ids,
+ GUINT_TO_POINTER(id));
}
- g_list_free(attr_ids);
- } else {
- uint32_t *attr_ids;
-
- attr_ids = g_memdup(&pdu->params[9], sizeof(uint32_t) * nattr);
+ attr_ids = g_list_reverse(attr_ids);
+ }
- for (i = 0; i < nattr; i++) {
- uint32_t attr = ntohl(attr_ids[i]);
+ for (l = attr_ids, len = 0, pos = 1; l != NULL; l = l->next) {
+ uint32_t attr = GPOINTER_TO_UINT(l->data);
- size = player_get_media_attribute(player, attr,
+ size = player_get_media_attribute(player, attr,
&pdu->params[pos],
AVRCP_PDU_MTU - pos);
- if (size > 0) {
- len++;
- pos += size;
- }
+ if (size >= 0) {
+ len++;
+ pos += size;
}
+ }
- g_free(attr_ids);
+ g_list_free(attr_ids);
- if (!len)
- goto err;
- }
+ if (!len)
+ goto err;
pdu->params[0] = len;
pdu->params_len = htons(pos);
--
1.7.7
When remote side requests all available metadata (i.e. it gives number
attributes equals to 0) do not return zero-length strings for
unavailable items. The only exception is title, that must be always present.
---
audio/avrcp.c | 12 +++++++++---
1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 8b46650..4e3ef97 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -611,10 +611,14 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
if (!nattr) {
/*
* Return all available information, at least
- * title must be returned.
+ * title must be returned if there's a track selected.
*/
- for (i = 1; i < AVRCP_MEDIA_ATTRIBUTE_LAST; i++) {
- size = player_get_media_attribute(player, i,
+ GList *l, *attr_ids = player->cb->list_metadata(player->user_data);
+
+ for (l = attr_ids; l != NULL; l = l->next) {
+ uint32_t attr = GPOINTER_TO_UINT(l->data);
+
+ size = player_get_media_attribute(player, attr,
&pdu->params[pos],
AVRCP_PDU_MTU - pos);
@@ -623,6 +627,8 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
pos += size;
}
}
+
+ g_list_free(attr_ids);
} else {
uint32_t *attr_ids;
--
1.7.7
If number of attributes remote side provided is larger than the length
we read, we would read garbage from stack memory.
---
audio/avrcp.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 0ca91a5..8b46650 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -597,12 +597,16 @@ static uint8_t avrcp_handle_get_element_attributes(struct avrcp_player *player,
int size;
unsigned int i;
- if (len < 8 || *identifier != 0)
+ if (len < 9 || *identifier != 0)
+ goto err;
+
+ nattr = pdu->params[8];
+
+ if (len < nattr * sizeof(uint32_t) + 1)
goto err;
len = 0;
pos = 1; /* Keep track of current position in reponse */
- nattr = pdu->params[8];
if (!nattr) {
/*
--
1.7.7
---
audio/avrcp.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index a7987fb..0ca91a5 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -427,6 +427,10 @@ static int player_get_media_attribute(struct avrcp_player *player,
DBG("Get media attribute: %u", id);
+ if (id == AVRCP_MEDIA_ATTRIBUTE_ILLEGAL ||
+ id > AVRCP_MEDIA_ATTRIBUTE_LAST)
+ return -ENOENT;
+
value = player->cb->get_metadata(id, player->user_data);
if (value == NULL) {
len = 0;
@@ -452,8 +456,6 @@ static int player_get_media_attribute(struct avrcp_player *player,
return -ENOBUFS;
memcpy(elem->val, valstr, len);
break;
- default:
- return -ENOENT;
}
done:
--
1.7.7
---
audio/avrcp.h | 1 +
audio/media.c | 13 +++++++++++++
2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/audio/avrcp.h b/audio/avrcp.h
index 66d09bc..360a80a 100644
--- a/audio/avrcp.h
+++ b/audio/avrcp.h
@@ -76,6 +76,7 @@ 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);
void *(*get_metadata) (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 56913d5..5a74fec 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1156,6 +1156,18 @@ static int set_setting(uint8_t attr, uint8_t val, void *user_data)
return 0;
}
+static GList *list_metadata(void *user_data)
+{
+ struct media_player *mp = user_data;
+
+ DBG("");
+
+ if (mp->track == NULL)
+ return NULL;
+
+ return g_hash_table_get_keys(mp->track);
+}
+
static void *get_metadata(uint32_t id, void *user_data)
{
struct media_player *mp = user_data;
@@ -1207,6 +1219,7 @@ static uint32_t get_position(void *user_data)
static struct avrcp_player_cb player_cb = {
.get_setting = get_setting,
.set_setting = set_setting,
+ .list_metadata = list_metadata,
.get_metadata = get_metadata,
.get_position = get_position,
.get_status = get_status
--
1.7.7
---
audio/avrcp.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index c36af5d..6bddf5b 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -474,9 +474,15 @@ static int player_set_attribute(struct avrcp_player *player,
static int player_get_attribute(struct avrcp_player *player, uint8_t attr)
{
- DBG("Get attribute: %u", attr);
+ int value;
- return player->cb->get_setting(attr, player->user_data);
+ DBG("attr %u", attr);
+
+ value = player->cb->get_setting(attr, player->user_data);
+ if (value < 0)
+ DBG("attr %u not supported by player", attr);
+
+ return value;
}
static uint8_t avrcp_handle_get_capabilities(struct avrcp_player *player,
@@ -535,10 +541,8 @@ static uint8_t avrcp_handle_list_player_attributes(struct avrcp_player *player,
goto done;
for (i = 1; i <= AVRCP_ATTRIBUTE_SCAN; i++) {
- if (player_get_attribute(player, i) < 0) {
- DBG("Ignoring setting %u: not supported by player", i);
+ if (player_get_attribute(player, i) < 0)
continue;
- }
len++;
pdu->params[len] = i;
@@ -681,11 +685,8 @@ static uint8_t avrcp_handle_get_current_player_value(struct avrcp_player *player
}
val = player_get_attribute(player, settings[i]);
- if (val < 0) {
- DBG("Ignoring %u: not supported by player",
- settings[i]);
+ if (val < 0)
continue;
- }
pdu->params[++len] = settings[i];
pdu->params[++len] = val;
--
1.7.7
---
audio/media.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/audio/media.c b/audio/media.c
index c9fe0f0..56913d5 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1438,6 +1438,11 @@ static gboolean parse_player_metadata(struct media_player *mp,
if (g_hash_table_size(track) == 0) {
g_hash_table_unref(track);
track = NULL;
+ } else if (title == FALSE) {
+ struct metadata_value *value = g_new(struct metadata_value, 1);
+
+ value->type = DBUS_TYPE_STRING;
+ value->value.str = g_strdup("");
}
if (mp->track != NULL)
--
1.7.7
If we use the same hash table to set the new metadata, we have 2
undesired behaviors:
1) New track may contain fields from previous track if it didn't set all
the fields
2) If we fail on parsing the signal, we will still change some of the
fields
---
audio/media.c | 41 ++++++++++++++++++++++++++---------------
doc/media-api.txt | 6 ++++++
2 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/audio/media.c b/audio/media.c
index ef595b3..c9fe0f0 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1353,6 +1353,7 @@ static gboolean parse_player_metadata(struct media_player *mp,
{
DBusMessageIter dict;
DBusMessageIter var;
+ GHashTable *track;
int ctype;
gboolean title = FALSE;
@@ -1362,6 +1363,9 @@ static gboolean parse_player_metadata(struct media_player *mp,
dbus_message_iter_recurse(iter, &dict);
+ track = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL,
+ metadata_value_free);
+
while ((ctype = dbus_message_iter_get_arg_type(&dict)) !=
DBUS_TYPE_INVALID) {
DBusMessageIter entry;
@@ -1370,21 +1374,21 @@ static gboolean parse_player_metadata(struct media_player *mp,
int id;
if (ctype != DBUS_TYPE_DICT_ENTRY)
- return FALSE;
+ goto parse_error;
dbus_message_iter_recurse(&dict, &entry);
if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING)
- return FALSE;
+ goto parse_error;
dbus_message_iter_get_basic(&entry, &key);
dbus_message_iter_next(&entry);
id = metadata_to_val(key);
if (id < 0)
- return FALSE;
+ goto parse_error;
if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_VARIANT)
- return FALSE;
+ goto parse_error;
dbus_message_iter_recurse(&entry, &var);
@@ -1399,7 +1403,7 @@ static gboolean parse_player_metadata(struct media_player *mp,
case AVRCP_MEDIA_ATTRIBUTE_GENRE:
if (value->type != DBUS_TYPE_STRING) {
g_free(value);
- return FALSE;
+ goto parse_error;
}
dbus_message_iter_get_basic(&var, &value->value.str);
@@ -1409,13 +1413,13 @@ static gboolean parse_player_metadata(struct media_player *mp,
case AVRCP_MEDIA_ATTRIBUTE_DURATION:
if (value->type != DBUS_TYPE_UINT32) {
g_free(value);
- return FALSE;
+ goto parse_error;
}
dbus_message_iter_get_basic(&var, &value->value.num);
break;
default:
- return FALSE;
+ goto parse_error;
}
switch (value->type) {
@@ -1427,24 +1431,31 @@ static gboolean parse_player_metadata(struct media_player *mp,
DBG("%s=%u", key, value->value.num);
}
- if (!mp->track)
- mp->track = g_hash_table_new_full(g_direct_hash,
- g_direct_equal, NULL,
- metadata_value_free);
-
- g_hash_table_replace(mp->track, GUINT_TO_POINTER(id), value);
+ g_hash_table_replace(track, GUINT_TO_POINTER(id), value);
dbus_message_iter_next(&dict);
}
- if (title == FALSE)
- return TRUE;
+ if (g_hash_table_size(track) == 0) {
+ g_hash_table_unref(track);
+ track = NULL;
+ }
+ if (mp->track != NULL)
+ g_hash_table_unref(mp->track);
+
+ mp->track = track;
mp->position = 0;
g_timer_start(mp->timer);
avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_CHANGED, NULL);
return TRUE;
+
+parse_error:
+ if (track)
+ g_hash_table_unref(track);
+
+ return FALSE;
}
static gboolean track_changed(DBusConnection *connection, DBusMessage *msg,
diff --git a/doc/media-api.txt b/doc/media-api.txt
index 7dc7661..b8dcdbd 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -144,6 +144,12 @@ Signals PropertyChanged(string setting, variant value)
TrackChanged(dict metadata)
+ This signal indicates that current track has changed.
+ All available metadata for the new track shall be set
+ at once in the metadata argument. Metadata cannot be
+ updated in parts, otherwise it will be interpreted as
+ multiple track changes.
+
Possible values:
string Title:
--
1.7.7
---
doc/media-api.txt | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/doc/media-api.txt b/doc/media-api.txt
index af4cfa0..7dc7661 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -133,7 +133,7 @@ Object path freely definable
Methods void SetProperty(string property, 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.
--
1.7.7