Metadata item #7 should return total playing time of the track (TrackDuration)
in msec, not current position within the track.
Signed-off-by: David Stockwell <[email protected]>
---
audio/control.c | 22 ++++++++--------------
1 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/audio/control.c b/audio/control.c
index 4e10cac..047e6ac 100644
--- a/audio/control.c
+++ b/audio/control.c
@@ -196,7 +196,7 @@ enum media_info_id {
MEDIA_INFO_TRACK = 4,
MEDIA_INFO_N_TRACKS = 5,
MEDIA_INFO_GENRE = 6,
- MEDIA_INFO_CURRENT_POSITION = 7,
+ MEDIA_INFO_PLAYING_TIME = 7,
};
static DBusConnection *connection = NULL;
@@ -979,19 +979,13 @@ static int mp_get_media_attribute(struct media_player
*mp,
len = strlen(valstr);
memcpy(elem->val, valstr, len);
break;
- case MEDIA_INFO_CURRENT_POSITION:
- if (mi->elapsed != 0xFFFFFFFF) {
- uint32_t elapsed;
-
- mp_get_playback_status(mp, NULL, &elapsed, NULL);
-
- snprintf(valstr, 20, "%u", elapsed);
- len = strlen(valstr);
- memcpy(elem->val, valstr, len);
- } else {
+ case MEDIA_INFO_PLAYING_TIME:
+ if (!mi->track_len)
return -ENOENT;
- }
-
+
+ snprintf(valstr, 20, "%u", mi->track_len);
+ len = strlen(valstr);
+ memcpy(elem->val, valstr, len);
break;
default:
return -EINVAL;
@@ -1179,7 +1173,7 @@ static int avrcp_handle_get_element_attributes(struct
control *control,
* Return all available information, at least
* title must be returned.
*/
- for (i = 1; i <= MEDIA_INFO_CURRENT_POSITION; i++) {
+ for (i = 1; i <= MEDIA_INFO_PLAYING_TIME; i++) {
size = mp_get_media_attribute(control->mp, i,
&pdu->params[pos]);
--
1.7.3.4
Hi David,
On Mon, Aug 22, 2011 at 8:58 AM, David Stockwell
<[email protected]> wrote:
> Btw, it looked like this avrcp_handle_get_element_attributes function
> might not be properly checking the amount of actually received data in
> all necessary places before accessing the buffer (i.e. having the risk
> of remotely triggered buffer overflows). Could you please verify this
> and fix it if the issue really exists.
>
> +++++ I will take a look this afternoon and either send a fix, or send a
> note that it looks OK.
As I answered to Johan before seeing your response, it does have this
problem. I have the PDU-continuation pending here in which I fix this.
I'll probably send it by tomorrow. If you are into it and want to send
a fix, I'm ok with that.
regards,
Lucas De Marchi
Hi Johan
On Mon, Aug 22, 2011 at 7:36 AM, Johan Hedberg <[email protected]> wrote:
>> @@ -196,7 +196,7 @@ enum media_info_id {
>> ? ? ? MEDIA_INFO_TRACK = ? ? ? ? ? ? ?4,
>> ? ? ? MEDIA_INFO_N_TRACKS = ? ? ? ? ? 5,
>> ? ? ? MEDIA_INFO_GENRE = ? ? ? ? ? ? ?6,
>> - ? ? MEDIA_INFO_CURRENT_POSITION = ? 7,
>> + ? ? MEDIA_INFO_PLAYING_TIME = ? ? ? 7,
>> ?};
>
> Would it make sense to add a MEDIA_INFO_LAST to the end of the above
> list and then instead of the following:
It makes sense since the spec reserve these values for future use.
>
>> - ? ? ? ? ? ? for (i = 1; i <= MEDIA_INFO_CURRENT_POSITION; i++) {
>> + ? ? ? ? ? ? for (i = 1; i <= MEDIA_INFO_PLAYING_TIME; i++) {
>> ? ? ? ? ? ? ? ? ? ? ? size = mp_get_media_attribute(control->mp, i,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &pdu->params[pos]);
>
> You'd have:
>
> ? ? ? ?for (i = 1; i < MEDIA_INFO_LAST; i++) {
>
> Seems more readable to me at least and it'd make it easier to add new
> MEDIA_INFO types in the future (you only need to change the enum
> definition and protect yourself against forgetting to update both
> places).
right.
> Btw, it looked like this avrcp_handle_get_element_attributes function
> might not be properly checking the amount of actually received data in
> all necessary places before accessing the buffer (i.e. having the risk
> of remotely triggered buffer overflows). Could you please verify this
> and fix it if the issue really exists.
Yes, this is true. We can just fix this the easy way, but the right
approach would be to add the PDU continuation. The
avrcp_handle_get_element_attributes() is the only one as of now that
might trigger the buffer overflow. I'm adding this support and will
send it soon.
Thanks
Lucas De Marchi
Hi David,
On Sat, Aug 20, 2011 at 7:53 PM, David Stockwell
<[email protected]> wrote:
> Metadata item #7 should return total playing time of the track (TrackDuration)
> in msec, not current position within the track.
Humn... seems like I misread the meaning of "playing time". And
apparently Luiz had the same interpretation of the spec as of mine,
while implementing the parser on hcidump. He named this field
AVRCP_MEDIA_ATTRIBUTE_PROGRESS. Looking at the spec again, it does
seems like we have to return the track duration, but I'm not sure.
Luiz, what do you think?
Lucas De Marchi
Hello, Johan
-----Original Message-----
From: Johan Hedberg
Hi David,
On Sat, Aug 20, 2011, David Stockwell wrote:
> Metadata item #7 should return total playing time of the track
> (TrackDuration)
> in msec, not current position within the track.
>
> Signed-off-by: David Stockwell <[email protected]>
Please remove the signed-off-by (same in the other patches)
++++++ OK, will do, and not add the line in the future. It is unnecessary,
but I noticed it in other submissions, and assumed that it was becoming
standard.
> ---
> audio/control.c | 22 ++++++++--------------
> 1 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/audio/control.c b/audio/control.c
> index 4e10cac..047e6ac 100644
> --- a/audio/control.c
> +++ b/audio/control.c
> @@ -196,7 +196,7 @@ enum media_info_id {
> MEDIA_INFO_TRACK = 4,
> MEDIA_INFO_N_TRACKS = 5,
> MEDIA_INFO_GENRE = 6,
> - MEDIA_INFO_CURRENT_POSITION = 7,
> + MEDIA_INFO_PLAYING_TIME = 7,
> };
Would it make sense to add a MEDIA_INFO_LAST to the end of the above
list and then instead of the following:
> - for (i = 1; i <= MEDIA_INFO_CURRENT_POSITION; i++) {
> + for (i = 1; i <= MEDIA_INFO_PLAYING_TIME; i++) {
> size = mp_get_media_attribute(control->mp, i,
> &pdu->params[pos]);
You'd have:
for (i = 1; i < MEDIA_INFO_LAST; i++) {
+++++ Yes, it does make sense. In fact in my "big bang" submission, that's
how I did it. Will make the change and resubmit.
Seems more readable to me at least and it'd make it easier to add new
MEDIA_INFO types in the future (you only need to change the enum
definition and protect yourself against forgetting to update both
places).
+++++ I agree...absolutely. FWIW, I do have a few MEDIA_INFO items that I
am sending to the BARB. Seems a waste to have a space of 4 bn possible
metadata elements, and only seven used, with all the rest "reserved". Not
even a space for vendor-defined elements.
Btw, it looked like this avrcp_handle_get_element_attributes function
might not be properly checking the amount of actually received data in
all necessary places before accessing the buffer (i.e. having the risk
of remotely triggered buffer overflows). Could you please verify this
and fix it if the issue really exists.
+++++ I will take a look this afternoon and either send a fix, or send a
note that it looks OK.
Cheers, David
Johan
Hi David,
On Sat, Aug 20, 2011, David Stockwell wrote:
> Metadata item #7 should return total playing time of the track (TrackDuration)
> in msec, not current position within the track.
>
> Signed-off-by: David Stockwell <[email protected]>
Please remove the signed-off-by (same in the other patches)
> ---
> audio/control.c | 22 ++++++++--------------
> 1 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/audio/control.c b/audio/control.c
> index 4e10cac..047e6ac 100644
> --- a/audio/control.c
> +++ b/audio/control.c
> @@ -196,7 +196,7 @@ enum media_info_id {
> MEDIA_INFO_TRACK = 4,
> MEDIA_INFO_N_TRACKS = 5,
> MEDIA_INFO_GENRE = 6,
> - MEDIA_INFO_CURRENT_POSITION = 7,
> + MEDIA_INFO_PLAYING_TIME = 7,
> };
Would it make sense to add a MEDIA_INFO_LAST to the end of the above
list and then instead of the following:
> - for (i = 1; i <= MEDIA_INFO_CURRENT_POSITION; i++) {
> + for (i = 1; i <= MEDIA_INFO_PLAYING_TIME; i++) {
> size = mp_get_media_attribute(control->mp, i,
> &pdu->params[pos]);
You'd have:
for (i = 1; i < MEDIA_INFO_LAST; i++) {
Seems more readable to me at least and it'd make it easier to add new
MEDIA_INFO types in the future (you only need to change the enum
definition and protect yourself against forgetting to update both
places).
Btw, it looked like this avrcp_handle_get_element_attributes function
might not be properly checking the amount of actually received data in
all necessary places before accessing the buffer (i.e. having the risk
of remotely triggered buffer overflows). Could you please verify this
and fix it if the issue really exists.
Johan