[PATCH 3/3] AVRCP: Corrected metadata: Playing Time

[PATCH 3/3] AVRCP: Corrected metadata: Playing Time

[David Stockwell]

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 <dstockwell@frequency-one.com>
---
 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

Re: [PATCH 3/3] AVRCP: Corrected metadata: Playing Time

[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 <dstockwell@frequency-one.com>

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

Re: [PATCH 3/3] AVRCP: Corrected metadata: Playing Time

[David Stockwell]

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 <dstockwell@frequency-one.com>

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 

Re: [PATCH 3/3] AVRCP: Corrected metadata: Playing Time

[Lucas De Marchi]

Hi David,

On Mon, Aug 22, 2011 at 8:58 AM, David Stockwell
<dstockwell@frequency-one.com> 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

Re: [PATCH 3/3] AVRCP: Corrected metadata: Playing Time

[Lucas De Marchi]

On Mon, Aug 22, 2011 at 1:42 PM, dstockwell@frequency-one.com
<dstockwell@frequency-one.com> wrote:
> Hello Lucas
>
> On August 22, 2011 at 10:42 AM Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>
>> Hi David,
>>
>> On Mon, Aug 22, 2011 at 8:58 AM, David Stockwell
>> <dstockwell@frequency-one.com> 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&apos;ll probably send it by tomorrow. If you are into it and want to
>> send
>> a fix, I&apos;m ok with that.
>
>
>
> If you already have a fix for that function, go ahead and submit it.
>
>
>
> Wondering what you mean by "PDU-continuation pending", though.  Does it have
>
> to do with AVRCP-level RequestContinuingResponse (and Abort)?  Or
> AVCTP-layer
>
> fragmentation?


AVRCP-level RequestContinuingResponse (and Abort)


regards,
Lucas De Marchi

Re: [PATCH 3/3] AVRCP: Corrected metadata: Playing Time

[David Stockwell]

Hi Lucas, (back to the lame client):
-----Original Message----- 
From: Lucas De Marchi
>
> Wondering what you mean by "PDU-continuation pending", though.  Does it 
> have
>
> to do with AVRCP-level RequestContinuingResponse (and Abort)?  Or
> AVCTP-layer
>
> fragmentation?


AVRCP-level RequestContinuingResponse (and Abort)

+++++

Yes, I know about that one, and am not sure it is really necessary.  And, I 
think it is a bit messy to support since you also have to make sure that 
only metadata is processed between CT and TG, but at the same time allow for 
passthroughs.

As you know from the spec, an AV/C packet can be up to 512 bytes long. 
There are only seven possible Metadata attributes, of which three are 
number-strings (only a few bytes each).  Even if all seven 
attributes/elements are set, there are 56 bytes of element headers, the 
two-byte AVCTP header, the 10-byte AVRCP (PDU) header, etc.

This leaves at least 400 bytes of metadata, divided across the track title, 
the artist name, the album name, and the genre. For most popular music, 
these are kept as short as possible (so customers/fans can remember them), 
so a typical track name, is much less than 40 bytes (satellite radio only 
transmits 16 bytes).  Same with the artist name, etc.

I suggest a good way to handle this is to guarantee that the elements are 
not pathologically long, limiting each to 80 bytes or so.  Or maybe, as we 
scan the metadata, keep a counter to make sure that, in aggregate, we do not 
exceed the 512-byte limit.

The bigger issue for the future is AVCTP-level fragmentation, which becomes 
an issue if either MTU drops below the default 672 bytes (noisy environment, 
weak signal, etc.), or we implement 1.4 with browsing.  If/when we go in 
that direction, we will probably need to layer the handling of AVCTP and 
AVRCP.  I have code ready for that, but needs some cleanup and testing.

Cheers,
David

regards,
Lucas De Marchi 

Re: [PATCH 3/3] AVRCP: Corrected metadata: Playing Time

[Lucas De Marchi]

Hi Johan

On Mon, Aug 22, 2011 at 7:36 AM, Johan Hedberg <johan.hedberg@gmail.com> 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

Re: [PATCH 3/3] AVRCP: Corrected metadata: Playing Time

[Lucas De Marchi]

Hi David,

On Sat, Aug 20, 2011 at 7:53 PM, David Stockwell
<dstockwell@frequency-one.com> 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