* [PATCH ] audio/avrcp: Add Support for PLAYBACK_POS_CHANGED_EVENT @ 2014-10-06 15:49 Bharat Panda 2014-10-20 11:17 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 4+ messages in thread From: Bharat Panda @ 2014-10-06 15:49 UTC (permalink / raw) To: linux-bluetooth; +Cc: cpgs, Bharat Panda As per AVRCP 1.5 spec, 6.7.2, page 57. EVENT_PLAYBACK_POS_CHANGED shall be notified in the following conditions: 1. TG has reached the registered playback Interval time. 2. Changed PLAY STATUS. 3. Changed Current Track. 4. Reached end or beginning of track. --- profiles/audio/avrcp.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ profiles/audio/avrcp.h | 1 + profiles/audio/media.c | 23 ++++++++++++++--------- 3 files changed, 64 insertions(+), 9 deletions(-) diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c index 5c3c4f9..77f1dcb 100644 --- a/profiles/audio/avrcp.c +++ b/profiles/audio/avrcp.c @@ -198,6 +198,7 @@ struct pending_list_items { struct avrcp_player { struct avrcp_server *server; GSList *sessions; + uint16_t pos_timer; uint16_t id; uint8_t scope; uint64_t uid; @@ -627,6 +628,7 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, uint8_t buf[AVRCP_HEADER_LENGTH + 9]; struct avrcp_header *pdu = (void *) buf; uint16_t size; + uint32_t *pos = NULL; GSList *l; int attr; int val; @@ -673,6 +675,18 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, pdu->params[size++] = attr; pdu->params[size++] = val; break; + case AVRCP_EVENT_PLAYBACK_POS_CHANGED: + size = 5; + pos = (uint32_t *) data; + + be32toh(*pos); + memcpy(&pdu->params[1], pos, sizeof(uint32_t)); + + if (player->pos_timer > 0) { + g_source_remove(player->pos_timer); + player->pos_timer = 0; + } + break; default: error("Unknown event %u", id); return; @@ -1429,6 +1443,19 @@ static bool handle_passthrough(struct avctp *conn, uint8_t op, bool pressed, return handler->func(session); } +static gboolean interval_timeout(gpointer user_data) +{ + uint32_t pos; + struct avrcp_player *mp = user_data; + + pos = player_get_position(mp); + mp->pos_timer = 0; + avrcp_player_event(mp, AVRCP_EVENT_PLAYBACK_POS_CHANGED, + &pos); + + return FALSE; +} + static uint8_t avrcp_handle_register_notification(struct avrcp *session, struct avrcp_header *pdu, uint8_t transaction) @@ -1436,6 +1463,8 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session, struct avrcp_player *player = target_get_player(session); struct btd_device *dev = session->dev; uint16_t len = ntohs(pdu->params_len); + uint32_t interval; + uint32_t pos; uint64_t uid; GList *settings; @@ -1467,6 +1496,20 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session, case AVRCP_EVENT_TRACK_REACHED_START: len = 1; break; + case AVRCP_EVENT_PLAYBACK_POS_CHANGED: + len = 5; + + interval = htole32(pdu->params[1]); + player->pos_timer = g_timeout_add_seconds(interval, + interval_timeout, player); + + pos = player_get_position(player); + be32toh(pos); + /* Fill the value for sending INTERIM response */ + memcpy(&pdu->params[1], &pos, sizeof(uint32_t)); + + break; + case AVRCP_EVENT_SETTINGS_CHANGED: len = 1; settings = player_list_settings(player); @@ -2949,6 +2992,11 @@ static void player_destroy(gpointer data) if (player->destroy) player->destroy(player->user_data); + if (player->pos_timer != 0) { + g_source_remove(player->pos_timer); + player->pos_timer = 0; + } + g_slist_free(player->sessions); g_free(player->path); g_free(player->change_path); @@ -3407,6 +3455,7 @@ static void target_init(struct avrcp *session) (1 << AVRCP_EVENT_TRACK_CHANGED) | (1 << AVRCP_EVENT_TRACK_REACHED_START) | (1 << AVRCP_EVENT_TRACK_REACHED_END) | + (1 << AVRCP_EVENT_PLAYBACK_POS_CHANGED) | (1 << AVRCP_EVENT_SETTINGS_CHANGED); if (target->version < 0x0104) diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h index 6ac5294..4816e4b 100644 --- a/profiles/audio/avrcp.h +++ b/profiles/audio/avrcp.h @@ -74,6 +74,7 @@ #define AVRCP_EVENT_TRACK_CHANGED 0x02 #define AVRCP_EVENT_TRACK_REACHED_END 0x03 #define AVRCP_EVENT_TRACK_REACHED_START 0x04 +#define AVRCP_EVENT_PLAYBACK_POS_CHANGED 0x05 #define AVRCP_EVENT_SETTINGS_CHANGED 0x08 #define AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED 0x0a #define AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED 0x0b diff --git a/profiles/audio/media.c b/profiles/audio/media.c index ef7b910..cc19c61 100644 --- a/profiles/audio/media.c +++ b/profiles/audio/media.c @@ -1305,6 +1305,9 @@ static gboolean set_status(struct media_player *mp, DBusMessageIter *iter) mp->status = g_strdup(value); avrcp_player_event(mp->player, AVRCP_EVENT_STATUS_CHANGED, mp->status); + avrcp_player_event(mp->player, + AVRCP_EVENT_PLAYBACK_POS_CHANGED, + &mp->position); return TRUE; } @@ -1312,7 +1315,6 @@ static gboolean set_status(struct media_player *mp, DBusMessageIter *iter) static gboolean set_position(struct media_player *mp, DBusMessageIter *iter) { uint64_t value; - const char *status; if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_INT64) return FALSE; @@ -1321,11 +1323,6 @@ static gboolean set_position(struct media_player *mp, DBusMessageIter *iter) value /= 1000; - if (value > get_position(mp)) - status = "forward-seek"; - else - status = "reverse-seek"; - mp->position = value; g_timer_start(mp->timer); @@ -1334,6 +1331,9 @@ static gboolean set_position(struct media_player *mp, DBusMessageIter *iter) if (!mp->position) { avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_REACHED_START, NULL); + avrcp_player_event(mp->player, + AVRCP_EVENT_PLAYBACK_POS_CHANGED, + &mp->position); return TRUE; } @@ -1344,11 +1344,13 @@ static gboolean set_position(struct media_player *mp, DBusMessageIter *iter) if (mp->position == UINT32_MAX || mp->position >= mp->duration) { avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_REACHED_END, NULL); + avrcp_player_event(mp->player, + AVRCP_EVENT_PLAYBACK_POS_CHANGED, + &mp->position); return TRUE; } - - /* Send a status change to force resync the position */ - avrcp_player_event(mp->player, AVRCP_EVENT_STATUS_CHANGED, status); + avrcp_player_event(mp->player, AVRCP_EVENT_PLAYBACK_POS_CHANGED, + &mp->position); return TRUE; } @@ -1456,6 +1458,7 @@ static gboolean parse_player_metadata(struct media_player *mp, int ctype; gboolean title = FALSE; uint64_t uid; + uint32_t pos; ctype = dbus_message_iter_get_arg_type(iter); if (ctype != DBUS_TYPE_ARRAY) @@ -1521,9 +1524,11 @@ static gboolean parse_player_metadata(struct media_player *mp, mp->position = 0; g_timer_start(mp->timer); uid = get_uid(mp); + pos = get_position(mp); avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_CHANGED, &uid); avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_REACHED_START, NULL); + avrcp_player_event(mp->player, AVRCP_EVENT_PLAYBACK_POS_CHANGED, &pos); return TRUE; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH ] audio/avrcp: Add Support for PLAYBACK_POS_CHANGED_EVENT 2014-10-06 15:49 [PATCH ] audio/avrcp: Add Support for PLAYBACK_POS_CHANGED_EVENT Bharat Panda @ 2014-10-20 11:17 ` Luiz Augusto von Dentz 2014-10-20 13:11 ` Bharat Bhusan Panda 0 siblings, 1 reply; 4+ messages in thread From: Luiz Augusto von Dentz @ 2014-10-20 11:17 UTC (permalink / raw) To: Bharat Panda; +Cc: linux-bluetooth@vger.kernel.org, cpgs Hi, On Mon, Oct 6, 2014 at 6:49 PM, Bharat Panda <bharat.panda@samsung.com> wrote: > As per AVRCP 1.5 spec, 6.7.2, page 57. > > EVENT_PLAYBACK_POS_CHANGED shall be notified in the following conditions: > > 1. TG has reached the registered playback Interval time. > 2. Changed PLAY STATUS. > 3. Changed Current Track. > 4. Reached end or beginning of track. > --- > profiles/audio/avrcp.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > profiles/audio/avrcp.h | 1 + > profiles/audio/media.c | 23 ++++++++++++++--------- > 3 files changed, 64 insertions(+), 9 deletions(-) > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c > index 5c3c4f9..77f1dcb 100644 > --- a/profiles/audio/avrcp.c > +++ b/profiles/audio/avrcp.c > @@ -198,6 +198,7 @@ struct pending_list_items { > struct avrcp_player { > struct avrcp_server *server; > GSList *sessions; > + uint16_t pos_timer; > uint16_t id; > uint8_t scope; > uint64_t uid; > @@ -627,6 +628,7 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, > uint8_t buf[AVRCP_HEADER_LENGTH + 9]; > struct avrcp_header *pdu = (void *) buf; > uint16_t size; > + uint32_t *pos = NULL; > GSList *l; > int attr; > int val; > @@ -673,6 +675,18 @@ void avrcp_player_event(struct avrcp_player *player, uint8_t id, > pdu->params[size++] = attr; > pdu->params[size++] = val; > break; > + case AVRCP_EVENT_PLAYBACK_POS_CHANGED: > + size = 5; > + pos = (uint32_t *) data; > + > + be32toh(*pos); > + memcpy(&pdu->params[1], pos, sizeof(uint32_t)); > + > + if (player->pos_timer > 0) { > + g_source_remove(player->pos_timer); > + player->pos_timer = 0; > + } > + break; Note that it is meant to be playback interval, so if it is not playing we should not send anything. > default: > error("Unknown event %u", id); > return; > @@ -1429,6 +1443,19 @@ static bool handle_passthrough(struct avctp *conn, uint8_t op, bool pressed, > return handler->func(session); > } > > +static gboolean interval_timeout(gpointer user_data) > +{ > + uint32_t pos; > + struct avrcp_player *mp = user_data; > + > + pos = player_get_position(mp); > + mp->pos_timer = 0; > + avrcp_player_event(mp, AVRCP_EVENT_PLAYBACK_POS_CHANGED, > + &pos); > + > + return FALSE; > +} > + > static uint8_t avrcp_handle_register_notification(struct avrcp *session, > struct avrcp_header *pdu, > uint8_t transaction) > @@ -1436,6 +1463,8 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session, > struct avrcp_player *player = target_get_player(session); > struct btd_device *dev = session->dev; > uint16_t len = ntohs(pdu->params_len); > + uint32_t interval; > + uint32_t pos; > uint64_t uid; > GList *settings; > > @@ -1467,6 +1496,20 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session, > case AVRCP_EVENT_TRACK_REACHED_START: > len = 1; > break; > + case AVRCP_EVENT_PLAYBACK_POS_CHANGED: > + len = 5; > + > + interval = htole32(pdu->params[1]); > + player->pos_timer = g_timeout_add_seconds(interval, > + interval_timeout, player); > + > + pos = player_get_position(player); > + be32toh(pos); > + /* Fill the value for sending INTERIM response */ > + memcpy(&pdu->params[1], &pos, sizeof(uint32_t)); > + > + break; > + > case AVRCP_EVENT_SETTINGS_CHANGED: > len = 1; > settings = player_list_settings(player); > @@ -2949,6 +2992,11 @@ static void player_destroy(gpointer data) > if (player->destroy) > player->destroy(player->user_data); > > + if (player->pos_timer != 0) { > + g_source_remove(player->pos_timer); > + player->pos_timer = 0; > + } > + > g_slist_free(player->sessions); > g_free(player->path); > g_free(player->change_path); > @@ -3407,6 +3455,7 @@ static void target_init(struct avrcp *session) > (1 << AVRCP_EVENT_TRACK_CHANGED) | > (1 << AVRCP_EVENT_TRACK_REACHED_START) | > (1 << AVRCP_EVENT_TRACK_REACHED_END) | > + (1 << AVRCP_EVENT_PLAYBACK_POS_CHANGED) | > (1 << AVRCP_EVENT_SETTINGS_CHANGED); > > if (target->version < 0x0104) > diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h > index 6ac5294..4816e4b 100644 > --- a/profiles/audio/avrcp.h > +++ b/profiles/audio/avrcp.h > @@ -74,6 +74,7 @@ > #define AVRCP_EVENT_TRACK_CHANGED 0x02 > #define AVRCP_EVENT_TRACK_REACHED_END 0x03 > #define AVRCP_EVENT_TRACK_REACHED_START 0x04 > +#define AVRCP_EVENT_PLAYBACK_POS_CHANGED 0x05 > #define AVRCP_EVENT_SETTINGS_CHANGED 0x08 > #define AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED 0x0a > #define AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED 0x0b > diff --git a/profiles/audio/media.c b/profiles/audio/media.c > index ef7b910..cc19c61 100644 > --- a/profiles/audio/media.c > +++ b/profiles/audio/media.c > @@ -1305,6 +1305,9 @@ static gboolean set_status(struct media_player *mp, DBusMessageIter *iter) > mp->status = g_strdup(value); > > avrcp_player_event(mp->player, AVRCP_EVENT_STATUS_CHANGED, mp->status); > + avrcp_player_event(mp->player, > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, > + &mp->position); > > return TRUE; > } > @@ -1312,7 +1315,6 @@ static gboolean set_status(struct media_player *mp, DBusMessageIter *iter) > static gboolean set_position(struct media_player *mp, DBusMessageIter *iter) > { > uint64_t value; > - const char *status; > > if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_INT64) > return FALSE; > @@ -1321,11 +1323,6 @@ static gboolean set_position(struct media_player *mp, DBusMessageIter *iter) > > value /= 1000; > > - if (value > get_position(mp)) > - status = "forward-seek"; > - else > - status = "reverse-seek"; > - Not sure what is reasoning of removing these, the remote side may not register for position change and in that case it would not get any seek event to synchronize the position. > mp->position = value; > g_timer_start(mp->timer); > > @@ -1334,6 +1331,9 @@ static gboolean set_position(struct media_player *mp, DBusMessageIter *iter) > if (!mp->position) { > avrcp_player_event(mp->player, > AVRCP_EVENT_TRACK_REACHED_START, NULL); > + avrcp_player_event(mp->player, > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, > + &mp->position); > return TRUE; > } > > @@ -1344,11 +1344,13 @@ static gboolean set_position(struct media_player *mp, DBusMessageIter *iter) > if (mp->position == UINT32_MAX || mp->position >= mp->duration) { > avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_REACHED_END, > NULL); > + avrcp_player_event(mp->player, > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, > + &mp->position); > return TRUE; > } > - > - /* Send a status change to force resync the position */ > - avrcp_player_event(mp->player, AVRCP_EVENT_STATUS_CHANGED, status); > + avrcp_player_event(mp->player, AVRCP_EVENT_PLAYBACK_POS_CHANGED, > + &mp->position); > > return TRUE; > } > @@ -1456,6 +1458,7 @@ static gboolean parse_player_metadata(struct media_player *mp, > int ctype; > gboolean title = FALSE; > uint64_t uid; > + uint32_t pos; > > ctype = dbus_message_iter_get_arg_type(iter); > if (ctype != DBUS_TYPE_ARRAY) > @@ -1521,9 +1524,11 @@ static gboolean parse_player_metadata(struct media_player *mp, > mp->position = 0; > g_timer_start(mp->timer); > uid = get_uid(mp); > + pos = get_position(mp); > > avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_CHANGED, &uid); > avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_REACHED_START, NULL); > + avrcp_player_event(mp->player, AVRCP_EVENT_PLAYBACK_POS_CHANGED, &pos); > > return TRUE; > } > -- > 1.9.1 Overall EVENT_PLAYBACK_POS_CHANGED cause more problems than it solves, because position is already part of the play status and that normally has to be queried anyway, also self synchronizing is way more efficient since we can't estimate the speed when the playback is fast-foward or rewind, and even if we could that would generate a lot of traffic depending on what interval the remote side wants to be updated. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH ] audio/avrcp: Add Support for PLAYBACK_POS_CHANGED_EVENT 2014-10-20 11:17 ` Luiz Augusto von Dentz @ 2014-10-20 13:11 ` Bharat Bhusan Panda 2014-10-20 13:46 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 4+ messages in thread From: Bharat Bhusan Panda @ 2014-10-20 13:11 UTC (permalink / raw) To: 'Luiz Augusto von Dentz'; +Cc: linux-bluetooth, cpgs Hi Luiz, > > As per AVRCP 1.5 spec, 6.7.2, page 57. > > > > EVENT_PLAYBACK_POS_CHANGED shall be notified in the following > conditions: > > > > 1. TG has reached the registered playback Interval time. > > 2. Changed PLAY STATUS. > > 3. Changed Current Track. > > 4. Reached end or beginning of track. > > --- > > profiles/audio/avrcp.c | 49 > > +++++++++++++++++++++++++++++++++++++++++++++++++ > > profiles/audio/avrcp.h | 1 + > > profiles/audio/media.c | 23 ++++++++++++++--------- > > 3 files changed, 64 insertions(+), 9 deletions(-) > > > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c index > > 5c3c4f9..77f1dcb 100644 > > --- a/profiles/audio/avrcp.c > > +++ b/profiles/audio/avrcp.c > > @@ -198,6 +198,7 @@ struct pending_list_items { struct avrcp_player { > > struct avrcp_server *server; > > GSList *sessions; > > + uint16_t pos_timer; > > uint16_t id; > > uint8_t scope; > > uint64_t uid; > > @@ -627,6 +628,7 @@ void avrcp_player_event(struct avrcp_player > *player, uint8_t id, > > uint8_t buf[AVRCP_HEADER_LENGTH + 9]; > > struct avrcp_header *pdu = (void *) buf; > > uint16_t size; > > + uint32_t *pos = NULL; > > GSList *l; > > int attr; > > int val; > > @@ -673,6 +675,18 @@ void avrcp_player_event(struct avrcp_player > *player, uint8_t id, > > pdu->params[size++] = attr; > > pdu->params[size++] = val; > > break; > > + case AVRCP_EVENT_PLAYBACK_POS_CHANGED: > > + size = 5; > > + pos = (uint32_t *) data; > > + > > + be32toh(*pos); > > + memcpy(&pdu->params[1], pos, sizeof(uint32_t)); > > + > > + if (player->pos_timer > 0) { > > + g_source_remove(player->pos_timer); > > + player->pos_timer = 0; > > + } > > + break; > > Note that it is meant to be playback interval, so if it is not playing we should > not send anything. Yes, will change it accordingly. > > > default: > > error("Unknown event %u", id); > > return; > > @@ -1429,6 +1443,19 @@ static bool handle_passthrough(struct avctp > *conn, uint8_t op, bool pressed, > > return handler->func(session); } > > > > +static gboolean interval_timeout(gpointer user_data) { > > + uint32_t pos; > > + struct avrcp_player *mp = user_data; > > + > > + pos = player_get_position(mp); > > + mp->pos_timer = 0; > > + avrcp_player_event(mp, AVRCP_EVENT_PLAYBACK_POS_CHANGED, > > + &pos); > > + > > + return FALSE; > > +} > > + > > static uint8_t avrcp_handle_register_notification(struct avrcp *session, > > struct avrcp_header *pdu, > > uint8_t transaction) > > @@ -1436,6 +1463,8 @@ static uint8_t > avrcp_handle_register_notification(struct avrcp *session, > > struct avrcp_player *player = target_get_player(session); > > struct btd_device *dev = session->dev; > > uint16_t len = ntohs(pdu->params_len); > > + uint32_t interval; > > + uint32_t pos; > > uint64_t uid; > > GList *settings; > > > > @@ -1467,6 +1496,20 @@ static uint8_t > avrcp_handle_register_notification(struct avrcp *session, > > case AVRCP_EVENT_TRACK_REACHED_START: > > len = 1; > > break; > > + case AVRCP_EVENT_PLAYBACK_POS_CHANGED: > > + len = 5; > > + > > + interval = htole32(pdu->params[1]); > > + player->pos_timer = g_timeout_add_seconds(interval, > > + > > + interval_timeout, player); > > + > > + pos = player_get_position(player); > > + be32toh(pos); > > + /* Fill the value for sending INTERIM response */ > > + memcpy(&pdu->params[1], &pos, sizeof(uint32_t)); > > + > > + break; > > + > > case AVRCP_EVENT_SETTINGS_CHANGED: > > len = 1; > > settings = player_list_settings(player); @@ -2949,6 > > +2992,11 @@ static void player_destroy(gpointer data) > > if (player->destroy) > > player->destroy(player->user_data); > > > > + if (player->pos_timer != 0) { > > + g_source_remove(player->pos_timer); > > + player->pos_timer = 0; > > + } > > + > > g_slist_free(player->sessions); > > g_free(player->path); > > g_free(player->change_path); > > @@ -3407,6 +3455,7 @@ static void target_init(struct avrcp *session) > > (1 << AVRCP_EVENT_TRACK_CHANGED) | > > (1 << AVRCP_EVENT_TRACK_REACHED_START) | > > (1 << AVRCP_EVENT_TRACK_REACHED_END) | > > + (1 << > > + AVRCP_EVENT_PLAYBACK_POS_CHANGED) | > > (1 << AVRCP_EVENT_SETTINGS_CHANGED); > > > > if (target->version < 0x0104) > > diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h index > > 6ac5294..4816e4b 100644 > > --- a/profiles/audio/avrcp.h > > +++ b/profiles/audio/avrcp.h > > @@ -74,6 +74,7 @@ > > #define AVRCP_EVENT_TRACK_CHANGED 0x02 > > #define AVRCP_EVENT_TRACK_REACHED_END 0x03 > > #define AVRCP_EVENT_TRACK_REACHED_START 0x04 > > +#define AVRCP_EVENT_PLAYBACK_POS_CHANGED 0x05 > > #define AVRCP_EVENT_SETTINGS_CHANGED 0x08 > > #define AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED 0x0a > > #define AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED 0x0b > > diff --git a/profiles/audio/media.c b/profiles/audio/media.c index > > ef7b910..cc19c61 100644 > > --- a/profiles/audio/media.c > > +++ b/profiles/audio/media.c > > @@ -1305,6 +1305,9 @@ static gboolean set_status(struct media_player > *mp, DBusMessageIter *iter) > > mp->status = g_strdup(value); > > > > avrcp_player_event(mp->player, AVRCP_EVENT_STATUS_CHANGED, > > mp->status); > > + avrcp_player_event(mp->player, > > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, > > + &mp->position); > > > > return TRUE; > > } > > @@ -1312,7 +1315,6 @@ static gboolean set_status(struct media_player > > *mp, DBusMessageIter *iter) static gboolean set_position(struct > > media_player *mp, DBusMessageIter *iter) { > > uint64_t value; > > - const char *status; > > > > if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_INT64) > > return FALSE; > > @@ -1321,11 +1323,6 @@ static gboolean set_position(struct > > media_player *mp, DBusMessageIter *iter) > > > > value /= 1000; > > > > - if (value > get_position(mp)) > > - status = "forward-seek"; > > - else > > - status = "reverse-seek"; > > - > > Not sure what is reasoning of removing these, the remote side may not > register for position change and in that case it would not get any seek event > to synchronize the position. Yes, PLAYBACK_STATUS_CHANGED and POSITION_CHANGED both should be sent in this case. If only PLAYBACK_STATUS_CHANGED is sent, the CT will query for position if it didn’t receive " POSITION_CHANGED " event after PLAYBACK_STATUS_CHANGED. > > > mp->position = value; > > g_timer_start(mp->timer); > > > > @@ -1334,6 +1331,9 @@ static gboolean set_position(struct media_player > *mp, DBusMessageIter *iter) > > if (!mp->position) { > > avrcp_player_event(mp->player, > > > > AVRCP_EVENT_TRACK_REACHED_START, NULL); > > + avrcp_player_event(mp->player, > > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, > > + &mp->position); > > return TRUE; > > } > > > > @@ -1344,11 +1344,13 @@ static gboolean set_position(struct > media_player *mp, DBusMessageIter *iter) > > if (mp->position == UINT32_MAX || mp->position >= mp->duration) { > > avrcp_player_event(mp->player, > AVRCP_EVENT_TRACK_REACHED_END, > > > > NULL); > > + avrcp_player_event(mp->player, > > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, > > + &mp->position); > > return TRUE; > > } > > - > > - /* Send a status change to force resync the position */ > > - avrcp_player_event(mp->player, AVRCP_EVENT_STATUS_CHANGED, > status); > > + avrcp_player_event(mp->player, > AVRCP_EVENT_PLAYBACK_POS_CHANGED, > > + > > + &mp->position); > > > > return TRUE; > > } > > @@ -1456,6 +1458,7 @@ static gboolean parse_player_metadata(struct > media_player *mp, > > int ctype; > > gboolean title = FALSE; > > uint64_t uid; > > + uint32_t pos; > > > > ctype = dbus_message_iter_get_arg_type(iter); > > if (ctype != DBUS_TYPE_ARRAY) > > @@ -1521,9 +1524,11 @@ static gboolean parse_player_metadata(struct > media_player *mp, > > mp->position = 0; > > g_timer_start(mp->timer); > > uid = get_uid(mp); > > + pos = get_position(mp); > > > > avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_CHANGED, > &uid); > > avrcp_player_event(mp->player, > > AVRCP_EVENT_TRACK_REACHED_START, NULL); > > + avrcp_player_event(mp->player, > > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, &pos); > > > > return TRUE; > > } > > -- > > 1.9.1 > > Overall EVENT_PLAYBACK_POS_CHANGED cause more problems than it > solves, because position is already part of the play status and that normally > has to be queried anyway, also self synchronizing is way more efficient since > we can't estimate the speed when the playback is fast-foward or rewind, and > even if we could that would generate a lot of traffic depending on what > interval the remote side wants to be updated. Playback position was not a part of play status though, it was trying resync the position by sending forward-seek or reverse seek as status changed event. In some control devices, it expects a playback_pos_changed event followed by TRACK_REACHED_START, TRACK_REACHED_END, PLAYBACK_STATUS_CHANGED and also when interval of registered for getting the playback position. Your input on the same will be highly valuable. Best Regards, Bharat ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH ] audio/avrcp: Add Support for PLAYBACK_POS_CHANGED_EVENT 2014-10-20 13:11 ` Bharat Bhusan Panda @ 2014-10-20 13:46 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 4+ messages in thread From: Luiz Augusto von Dentz @ 2014-10-20 13:46 UTC (permalink / raw) To: Bharat Bhusan Panda; +Cc: linux-bluetooth@vger.kernel.org, cpgs Hi, On Mon, Oct 20, 2014 at 4:11 PM, Bharat Bhusan Panda <bharat.panda@samsung.com> wrote: > Hi Luiz, > >> > As per AVRCP 1.5 spec, 6.7.2, page 57. >> > >> > EVENT_PLAYBACK_POS_CHANGED shall be notified in the following >> conditions: >> > >> > 1. TG has reached the registered playback Interval time. >> > 2. Changed PLAY STATUS. >> > 3. Changed Current Track. >> > 4. Reached end or beginning of track. >> > --- >> > profiles/audio/avrcp.c | 49 >> > +++++++++++++++++++++++++++++++++++++++++++++++++ >> > profiles/audio/avrcp.h | 1 + >> > profiles/audio/media.c | 23 ++++++++++++++--------- >> > 3 files changed, 64 insertions(+), 9 deletions(-) >> > >> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c index >> > 5c3c4f9..77f1dcb 100644 >> > --- a/profiles/audio/avrcp.c >> > +++ b/profiles/audio/avrcp.c >> > @@ -198,6 +198,7 @@ struct pending_list_items { struct avrcp_player { >> > struct avrcp_server *server; >> > GSList *sessions; >> > + uint16_t pos_timer; >> > uint16_t id; >> > uint8_t scope; >> > uint64_t uid; >> > @@ -627,6 +628,7 @@ void avrcp_player_event(struct avrcp_player >> *player, uint8_t id, >> > uint8_t buf[AVRCP_HEADER_LENGTH + 9]; >> > struct avrcp_header *pdu = (void *) buf; >> > uint16_t size; >> > + uint32_t *pos = NULL; >> > GSList *l; >> > int attr; >> > int val; >> > @@ -673,6 +675,18 @@ void avrcp_player_event(struct avrcp_player >> *player, uint8_t id, >> > pdu->params[size++] = attr; >> > pdu->params[size++] = val; >> > break; >> > + case AVRCP_EVENT_PLAYBACK_POS_CHANGED: >> > + size = 5; >> > + pos = (uint32_t *) data; >> > + >> > + be32toh(*pos); >> > + memcpy(&pdu->params[1], pos, sizeof(uint32_t)); >> > + >> > + if (player->pos_timer > 0) { >> > + g_source_remove(player->pos_timer); >> > + player->pos_timer = 0; >> > + } >> > + break; >> >> Note that it is meant to be playback interval, so if it is not playing we should >> not send anything. > Yes, will change it accordingly. >> >> > default: >> > error("Unknown event %u", id); >> > return; >> > @@ -1429,6 +1443,19 @@ static bool handle_passthrough(struct avctp >> *conn, uint8_t op, bool pressed, >> > return handler->func(session); } >> > >> > +static gboolean interval_timeout(gpointer user_data) { >> > + uint32_t pos; >> > + struct avrcp_player *mp = user_data; >> > + >> > + pos = player_get_position(mp); >> > + mp->pos_timer = 0; >> > + avrcp_player_event(mp, AVRCP_EVENT_PLAYBACK_POS_CHANGED, >> > + &pos); >> > + >> > + return FALSE; >> > +} >> > + >> > static uint8_t avrcp_handle_register_notification(struct avrcp *session, >> > struct avrcp_header *pdu, >> > uint8_t transaction) >> > @@ -1436,6 +1463,8 @@ static uint8_t >> avrcp_handle_register_notification(struct avrcp *session, >> > struct avrcp_player *player = target_get_player(session); >> > struct btd_device *dev = session->dev; >> > uint16_t len = ntohs(pdu->params_len); >> > + uint32_t interval; >> > + uint32_t pos; >> > uint64_t uid; >> > GList *settings; >> > >> > @@ -1467,6 +1496,20 @@ static uint8_t >> avrcp_handle_register_notification(struct avrcp *session, >> > case AVRCP_EVENT_TRACK_REACHED_START: >> > len = 1; >> > break; >> > + case AVRCP_EVENT_PLAYBACK_POS_CHANGED: >> > + len = 5; >> > + >> > + interval = htole32(pdu->params[1]); >> > + player->pos_timer = g_timeout_add_seconds(interval, >> > + >> > + interval_timeout, player); >> > + >> > + pos = player_get_position(player); >> > + be32toh(pos); >> > + /* Fill the value for sending INTERIM response */ >> > + memcpy(&pdu->params[1], &pos, sizeof(uint32_t)); >> > + >> > + break; >> > + >> > case AVRCP_EVENT_SETTINGS_CHANGED: >> > len = 1; >> > settings = player_list_settings(player); @@ -2949,6 >> > +2992,11 @@ static void player_destroy(gpointer data) >> > if (player->destroy) >> > player->destroy(player->user_data); >> > >> > + if (player->pos_timer != 0) { >> > + g_source_remove(player->pos_timer); >> > + player->pos_timer = 0; >> > + } >> > + >> > g_slist_free(player->sessions); >> > g_free(player->path); >> > g_free(player->change_path); >> > @@ -3407,6 +3455,7 @@ static void target_init(struct avrcp *session) >> > (1 << AVRCP_EVENT_TRACK_CHANGED) | >> > (1 << AVRCP_EVENT_TRACK_REACHED_START) | >> > (1 << AVRCP_EVENT_TRACK_REACHED_END) | >> > + (1 << >> > + AVRCP_EVENT_PLAYBACK_POS_CHANGED) | >> > (1 << AVRCP_EVENT_SETTINGS_CHANGED); >> > >> > if (target->version < 0x0104) >> > diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h index >> > 6ac5294..4816e4b 100644 >> > --- a/profiles/audio/avrcp.h >> > +++ b/profiles/audio/avrcp.h >> > @@ -74,6 +74,7 @@ >> > #define AVRCP_EVENT_TRACK_CHANGED 0x02 >> > #define AVRCP_EVENT_TRACK_REACHED_END 0x03 >> > #define AVRCP_EVENT_TRACK_REACHED_START 0x04 >> > +#define AVRCP_EVENT_PLAYBACK_POS_CHANGED 0x05 >> > #define AVRCP_EVENT_SETTINGS_CHANGED 0x08 >> > #define AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED 0x0a >> > #define AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED 0x0b >> > diff --git a/profiles/audio/media.c b/profiles/audio/media.c index >> > ef7b910..cc19c61 100644 >> > --- a/profiles/audio/media.c >> > +++ b/profiles/audio/media.c >> > @@ -1305,6 +1305,9 @@ static gboolean set_status(struct media_player >> *mp, DBusMessageIter *iter) >> > mp->status = g_strdup(value); >> > >> > avrcp_player_event(mp->player, AVRCP_EVENT_STATUS_CHANGED, >> > mp->status); >> > + avrcp_player_event(mp->player, >> > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, >> > + &mp->position); >> > >> > return TRUE; >> > } >> > @@ -1312,7 +1315,6 @@ static gboolean set_status(struct media_player >> > *mp, DBusMessageIter *iter) static gboolean set_position(struct >> > media_player *mp, DBusMessageIter *iter) { >> > uint64_t value; >> > - const char *status; >> > >> > if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_INT64) >> > return FALSE; >> > @@ -1321,11 +1323,6 @@ static gboolean set_position(struct >> > media_player *mp, DBusMessageIter *iter) >> > >> > value /= 1000; >> > >> > - if (value > get_position(mp)) >> > - status = "forward-seek"; >> > - else >> > - status = "reverse-seek"; >> > - >> >> Not sure what is reasoning of removing these, the remote side may not >> register for position change and in that case it would not get any seek event >> to synchronize the position. > > Yes, PLAYBACK_STATUS_CHANGED and POSITION_CHANGED both should be sent in this case. > If only PLAYBACK_STATUS_CHANGED is sent, the CT will query for position if it didn’t receive " POSITION_CHANGED " event after PLAYBACK_STATUS_CHANGED. Exactly, and that means with POSITION_CHANGED you have more traffic because it first send a changed which is normally followed by register notification and interim response versus only get play status and response. >> >> > mp->position = value; >> > g_timer_start(mp->timer); >> > >> > @@ -1334,6 +1331,9 @@ static gboolean set_position(struct media_player >> *mp, DBusMessageIter *iter) >> > if (!mp->position) { >> > avrcp_player_event(mp->player, >> > >> > AVRCP_EVENT_TRACK_REACHED_START, NULL); >> > + avrcp_player_event(mp->player, >> > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, >> > + &mp->position); >> > return TRUE; >> > } >> > >> > @@ -1344,11 +1344,13 @@ static gboolean set_position(struct >> media_player *mp, DBusMessageIter *iter) >> > if (mp->position == UINT32_MAX || mp->position >= mp->duration) { >> > avrcp_player_event(mp->player, >> AVRCP_EVENT_TRACK_REACHED_END, >> > >> > NULL); >> > + avrcp_player_event(mp->player, >> > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, >> > + &mp->position); >> > return TRUE; >> > } >> > - >> > - /* Send a status change to force resync the position */ >> > - avrcp_player_event(mp->player, AVRCP_EVENT_STATUS_CHANGED, >> status); >> > + avrcp_player_event(mp->player, >> AVRCP_EVENT_PLAYBACK_POS_CHANGED, >> > + >> > + &mp->position); >> > >> > return TRUE; >> > } >> > @@ -1456,6 +1458,7 @@ static gboolean parse_player_metadata(struct >> media_player *mp, >> > int ctype; >> > gboolean title = FALSE; >> > uint64_t uid; >> > + uint32_t pos; >> > >> > ctype = dbus_message_iter_get_arg_type(iter); >> > if (ctype != DBUS_TYPE_ARRAY) >> > @@ -1521,9 +1524,11 @@ static gboolean parse_player_metadata(struct >> media_player *mp, >> > mp->position = 0; >> > g_timer_start(mp->timer); >> > uid = get_uid(mp); >> > + pos = get_position(mp); >> > >> > avrcp_player_event(mp->player, AVRCP_EVENT_TRACK_CHANGED, >> &uid); >> > avrcp_player_event(mp->player, >> > AVRCP_EVENT_TRACK_REACHED_START, NULL); >> > + avrcp_player_event(mp->player, >> > + AVRCP_EVENT_PLAYBACK_POS_CHANGED, &pos); >> > >> > return TRUE; >> > } >> > -- >> > 1.9.1 >> >> Overall EVENT_PLAYBACK_POS_CHANGED cause more problems than it >> solves, because position is already part of the play status and that normally >> has to be queried anyway, also self synchronizing is way more efficient since >> we can't estimate the speed when the playback is fast-foward or rewind, and >> even if we could that would generate a lot of traffic depending on what >> interval the remote side wants to be updated. > > Playback position was not a part of play status though, it was trying resync the position by sending forward-seek or reverse seek as status changed event. > In some control devices, it expects a playback_pos_changed event followed by TRACK_REACHED_START, TRACK_REACHED_END, PLAYBACK_STATUS_CHANGED and also when interval of registered for getting the playback position. > Your input on the same will be highly valuable. It is part of GetPlayStatus which also contains the duration, anyway afaik position changed event is not mandatory, so the remote control has to deal with stacks that do not have it which in the end leads to synchronize via state changes by querying play status, if it cannot do it you might have IOP problems with a lot stacks including iOS and Android which does not support position changed. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-20 13:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-06 15:49 [PATCH ] audio/avrcp: Add Support for PLAYBACK_POS_CHANGED_EVENT Bharat Panda 2014-10-20 11:17 ` Luiz Augusto von Dentz 2014-10-20 13:11 ` Bharat Bhusan Panda 2014-10-20 13:46 ` Luiz Augusto von Dentz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.