2011-08-22 19:55:47

by Lucas De Marchi

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

On Mon, Aug 22, 2011 at 1:42 PM, [email protected]
<[email protected]> wrote:
> Hello Lucas
>
> On August 22, 2011 at 10:42 AM Lucas De Marchi
> <[email protected]> wrote:
>
>> 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&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


2011-08-23 03:02:38

by David Stockwell

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

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