[RFC] android/hal-audio: Fix wrong memory access

[RFC] android/hal-audio: Fix wrong memory access

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

downmix_buf is allocated to have buffer size FIXED_BUFFER_SIZE / 2, when
we access it as (int16_t *) we shall device index by 2.
---
 android/hal-audio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/hal-audio.c b/android/hal-audio.c
index 7305bb6..96fa5c3 100644
--- a/android/hal-audio.c
+++ b/android/hal-audio.c
@@ -984,7 +984,7 @@ static void downmix_to_mono(struct a2dp_stream_out *out, const uint8_t *buffer,
 	int16_t *output = (void *) out->downmix_buf;
 	size_t i;
 
-	for (i = 0; i < bytes / 2; i++) {
+	for (i = 0; i < bytes / (2 * sizeof(int16_t)); i++) {
 		int16_t l = le16_to_cpu(get_unaligned(&input[i * 2]));
 		int16_t r = le16_to_cpu(get_unaligned(&input[i * 2 + 1]));
 
-- 
1.8.3.2

Re: [RFC] android/hal-audio: Fix wrong memory access

[Andrzej Kaczmarek]

Hi Andrei,

On 22 May 2014 15:07, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> downmix_buf is allocated to have buffer size FIXED_BUFFER_SIZE / 2, when
> we access it as (int16_t *) we shall device index by 2.
> ---
>  android/hal-audio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/android/hal-audio.c b/android/hal-audio.c
> index 7305bb6..96fa5c3 100644
> --- a/android/hal-audio.c
> +++ b/android/hal-audio.c
> @@ -984,7 +984,7 @@ static void downmix_to_mono(struct a2dp_stream_out *out, const uint8_t *buffer,
>         int16_t *output = (void *) out->downmix_buf;
>         size_t i;
>
> -       for (i = 0; i < bytes / 2; i++) {
> +       for (i = 0; i < bytes / (2 * sizeof(int16_t)); i++) {
>                 int16_t l = le16_to_cpu(get_unaligned(&input[i * 2]));
>                 int16_t r = le16_to_cpu(get_unaligned(&input[i * 2 + 1]));

Fix is correct, but commit message does not explain properly why this
is required. Basically we need to downmix X frames from input buffer
and this number is "bytes / (number_of_channels * sample_size)" - so
we were missing "sample_size" here. I think adding inline comment to
explain this would be also good.

BR,
Andrzej

Re: [RFC] android/hal-audio: Fix wrong memory access

[Andrei Emeltchenko]

Hi Andrzej,

On Thu, May 22, 2014 at 03:54:34PM +0200, Andrzej Kaczmarek wrote:
> Hi Andrei,
> 
> On 22 May 2014 15:07, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > downmix_buf is allocated to have buffer size FIXED_BUFFER_SIZE / 2, when
> > we access it as (int16_t *) we shall device index by 2.
> > ---
> >  android/hal-audio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/android/hal-audio.c b/android/hal-audio.c
> > index 7305bb6..96fa5c3 100644
> > --- a/android/hal-audio.c
> > +++ b/android/hal-audio.c
> > @@ -984,7 +984,7 @@ static void downmix_to_mono(struct a2dp_stream_out *out, const uint8_t *buffer,
> >         int16_t *output = (void *) out->downmix_buf;
> >         size_t i;
> >
> > -       for (i = 0; i < bytes / 2; i++) {
> > +       for (i = 0; i < bytes / (2 * sizeof(int16_t)); i++) {
> >                 int16_t l = le16_to_cpu(get_unaligned(&input[i * 2]));
> >                 int16_t r = le16_to_cpu(get_unaligned(&input[i * 2 + 1]));
> 
> Fix is correct, but commit message does not explain properly why this
> is required. Basically we need to downmix X frames from input buffer
> and this number is "bytes / (number_of_channels * sample_size)" - so
> we were missing "sample_size" here. I think adding inline comment to
> explain this would be also good.

I my code I use frame_num instead of bytes.

Best regards 
Andrei Emeltchenko 

Re: [RFC] android/hal-audio: Fix wrong memory access

[Marcel Holtmann]

Hi Andrei,

> downmix_buf is allocated to have buffer size FIXED_BUFFER_SIZE / 2, when
> we access it as (int16_t *) we shall device index by 2.
> ---
> android/hal-audio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/android/hal-audio.c b/android/hal-audio.c
> index 7305bb6..96fa5c3 100644
> --- a/android/hal-audio.c
> +++ b/android/hal-audio.c
> @@ -984,7 +984,7 @@ static void downmix_to_mono(struct a2dp_stream_out *out, const uint8_t *buffer,
> 	int16_t *output = (void *) out->downmix_buf;
> 	size_t i;
> 
> -	for (i = 0; i < bytes / 2; i++) {
> +	for (i = 0; i < bytes / (2 * sizeof(int16_t)); i++) {
> 		int16_t l = le16_to_cpu(get_unaligned(&input[i * 2]));
> 		int16_t r = le16_to_cpu(get_unaligned(&input[i * 2 + 1]));

I wonder actually what this get_unaligned is doing here? You cast the const void into const int16_t buffer. Is this really needed? Where is our input and output buffer coming from? Aren’t these aligned anyway? Meaning aren’t they allocated?

I also wonder why we are not doing the unaligned access directly on the void buffer and do proper offset calculation into the stream. Instead of casting it to an int16 array.

Regards

Marcel

Re: [RFC] android/hal-audio: Fix wrong memory access

[Andrzej Kaczmarek]

Hi Marcel,

On 22 May 2014 16:21, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andrei,
>
>> downmix_buf is allocated to have buffer size FIXED_BUFFER_SIZE / 2, when
>> we access it as (int16_t *) we shall device index by 2.
>> ---
>> android/hal-audio.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/android/hal-audio.c b/android/hal-audio.c
>> index 7305bb6..96fa5c3 100644
>> --- a/android/hal-audio.c
>> +++ b/android/hal-audio.c
>> @@ -984,7 +984,7 @@ static void downmix_to_mono(struct a2dp_stream_out *=
out, const uint8_t *buffer,
>>       int16_t *output =3D (void *) out->downmix_buf;
>>       size_t i;
>>
>> -     for (i =3D 0; i < bytes / 2; i++) {
>> +     for (i =3D 0; i < bytes / (2 * sizeof(int16_t)); i++) {
>>               int16_t l =3D le16_to_cpu(get_unaligned(&input[i * 2]));
>>               int16_t r =3D le16_to_cpu(get_unaligned(&input[i * 2 + 1])=
);
>
> I wonder actually what this get_unaligned is doing here? You cast the con=
st void into const int16_t buffer. Is this really needed? Where is our inpu=
t and output buffer coming from? Aren=E2=80=99t these aligned anyway? Meani=
ng aren=E2=80=99t they allocated?

We have this buffer from AudioFlinger so we don't actually know
alignment or if this is pointer to beginning of some internal buffer
(it's most probably both, but I don't think we should assume this).

> I also wonder why we are not doing the unaligned access directly on the v=
oid buffer and do proper offset calculation into the stream. Instead of cas=
ting it to an int16 array.

Only to have buffer indexed by samples rather than by bytes.

BR,
Andrzej

Re: [RFC] android/hal-audio: Fix wrong memory access

[Szymon Janc]

Hi Andrei,

On Thursday 22 of May 2014 16:07:02 Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> downmix_buf is allocated to have buffer size FIXED_BUFFER_SIZE / 2, when
> we access it as (int16_t *) we shall device index by 2.
> ---
>  android/hal-audio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/android/hal-audio.c b/android/hal-audio.c
> index 7305bb6..96fa5c3 100644
> --- a/android/hal-audio.c
> +++ b/android/hal-audio.c
> @@ -984,7 +984,7 @@ static void downmix_to_mono(struct a2dp_stream_out *out, const uint8_t *buffer,
>  	int16_t *output = (void *) out->downmix_buf;
>  	size_t i;
>  
> -	for (i = 0; i < bytes / 2; i++) {
> +	for (i = 0; i < bytes / (2 * sizeof(int16_t)); i++) {
>  		int16_t l = le16_to_cpu(get_unaligned(&input[i * 2]));
>  		int16_t r = le16_to_cpu(get_unaligned(&input[i * 2 + 1]));
>  

Although RFC I've applied this patch but modified commit message and added
a local 'frames' variable with comment where this calculation comes from.
Thanks.

-- 
Best regards, 
Szymon Janc

Re: [RFC] android/hal-audio: Fix wrong memory access

[Marcel Holtmann]

Hi Andrzej,

>>> downmix_buf is allocated to have buffer size FIXED_BUFFER_SIZE / 2, when
>>> we access it as (int16_t *) we shall device index by 2.
>>> ---
>>> android/hal-audio.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/android/hal-audio.c b/android/hal-audio.c
>>> index 7305bb6..96fa5c3 100644
>>> --- a/android/hal-audio.c
>>> +++ b/android/hal-audio.c
>>> @@ -984,7 +984,7 @@ static void downmix_to_mono(struct a2dp_stream_out *out, const uint8_t *buffer,
>>>      int16_t *output = (void *) out->downmix_buf;
>>>      size_t i;
>>> 
>>> -     for (i = 0; i < bytes / 2; i++) {
>>> +     for (i = 0; i < bytes / (2 * sizeof(int16_t)); i++) {
>>>              int16_t l = le16_to_cpu(get_unaligned(&input[i * 2]));
>>>              int16_t r = le16_to_cpu(get_unaligned(&input[i * 2 + 1]));
>> 
>> I wonder actually what this get_unaligned is doing here? You cast the const void into const int16_t buffer. Is this really needed? Where is our input and output buffer coming from? Aren’t these aligned anyway? Meaning aren’t they allocated?
> 
> We have this buffer from AudioFlinger so we don't actually know
> alignment or if this is pointer to beginning of some internal buffer
> (it's most probably both, but I don't think we should assume this).

and audio system that does not give you a guarantee here on the alignment is utterly screwed up. Seriously, that is just bad for performance. Especially bad on ARM CPUs, so I doubt that they have not thought about this.

Regards

Marcel

Re: [RFC] android/hal-audio: Fix wrong memory access

[Andrzej Kaczmarek]

Hi Marcel,

On 26 May 2014 14:48, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andrzej,
>
>>>> downmix_buf is allocated to have buffer size FIXED_BUFFER_SIZE / 2, wh=
en
>>>> we access it as (int16_t *) we shall device index by 2.
>>>> ---
>>>> android/hal-audio.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/android/hal-audio.c b/android/hal-audio.c
>>>> index 7305bb6..96fa5c3 100644
>>>> --- a/android/hal-audio.c
>>>> +++ b/android/hal-audio.c
>>>> @@ -984,7 +984,7 @@ static void downmix_to_mono(struct a2dp_stream_out=
 *out, const uint8_t *buffer,
>>>>      int16_t *output =3D (void *) out->downmix_buf;
>>>>      size_t i;
>>>>
>>>> -     for (i =3D 0; i < bytes / 2; i++) {
>>>> +     for (i =3D 0; i < bytes / (2 * sizeof(int16_t)); i++) {
>>>>              int16_t l =3D le16_to_cpu(get_unaligned(&input[i * 2]));
>>>>              int16_t r =3D le16_to_cpu(get_unaligned(&input[i * 2 + 1]=
));
>>>
>>> I wonder actually what this get_unaligned is doing here? You cast the c=
onst void into const int16_t buffer. Is this really needed? Where is our in=
put and output buffer coming from? Aren=E2=80=99t these aligned anyway? Mea=
ning aren=E2=80=99t they allocated?
>>
>> We have this buffer from AudioFlinger so we don't actually know
>> alignment or if this is pointer to beginning of some internal buffer
>> (it's most probably both, but I don't think we should assume this).
>
> and audio system that does not give you a guarantee here on the alignment=
 is utterly screwed up. Seriously, that is just bad for performance. Especi=
ally bad on ARM CPUs, so I doubt that they have not thought about this.

This is only to be on safe side and in terms of performance it does
not really matter here since this code won't probably be used at all -
who has mono A2DP headset nowadays? But of course we can change this
with proper comment. Actually I use it like this in aptX where samples
are accessed from the same buffer.

BR,
Andrzej

Re: [RFC] android/hal-audio: Fix wrong memory access

[Marcel Holtmann]

Hi Andrzej,

>>>>> downmix_buf is allocated to have buffer size FIXED_BUFFER_SIZE / 2, when
>>>>> we access it as (int16_t *) we shall device index by 2.
>>>>> ---
>>>>> android/hal-audio.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/android/hal-audio.c b/android/hal-audio.c
>>>>> index 7305bb6..96fa5c3 100644
>>>>> --- a/android/hal-audio.c
>>>>> +++ b/android/hal-audio.c
>>>>> @@ -984,7 +984,7 @@ static void downmix_to_mono(struct a2dp_stream_out *out, const uint8_t *buffer,
>>>>>     int16_t *output = (void *) out->downmix_buf;
>>>>>     size_t i;
>>>>> 
>>>>> -     for (i = 0; i < bytes / 2; i++) {
>>>>> +     for (i = 0; i < bytes / (2 * sizeof(int16_t)); i++) {
>>>>>             int16_t l = le16_to_cpu(get_unaligned(&input[i * 2]));
>>>>>             int16_t r = le16_to_cpu(get_unaligned(&input[i * 2 + 1]));
>>>> 
>>>> I wonder actually what this get_unaligned is doing here? You cast the const void into const int16_t buffer. Is this really needed? Where is our input and output buffer coming from? Aren’t these aligned anyway? Meaning aren’t they allocated?
>>> 
>>> We have this buffer from AudioFlinger so we don't actually know
>>> alignment or if this is pointer to beginning of some internal buffer
>>> (it's most probably both, but I don't think we should assume this).
>> 
>> and audio system that does not give you a guarantee here on the alignment is utterly screwed up. Seriously, that is just bad for performance. Especially bad on ARM CPUs, so I doubt that they have not thought about this.
> 
> This is only to be on safe side and in terms of performance it does
> not really matter here since this code won't probably be used at all -
> who has mono A2DP headset nowadays? But of course we can change this
> with proper comment. Actually I use it like this in aptX where samples
> are accessed from the same buffer.

I have no idea what that means. The important part is that the buffer itself is aligned. With binary codecs like aptX this is even more important since we have no idea if aptX does the unaligned access or not.

Regards

Marcel

Re: [RFC] android/hal-audio: Fix wrong memory access

[Andrei Emeltchenko]

On Mon, May 26, 2014 at 02:59:31PM +0200, Andrzej Kaczmarek wrote:
> Hi Marcel,
> 
> On 26 May 2014 14:48, Marcel Holtmann <marcel@holtmann.org> wrote:
> > Hi Andrzej,
> >
> >>>> downmix_buf is allocated to have buffer size FIXED_BUFFER_SIZE / 2, when
> >>>> we access it as (int16_t *) we shall device index by 2.
> >>>> ---
> >>>> android/hal-audio.c | 2 +-
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/android/hal-audio.c b/android/hal-audio.c
> >>>> index 7305bb6..96fa5c3 100644
> >>>> --- a/android/hal-audio.c
> >>>> +++ b/android/hal-audio.c
> >>>> @@ -984,7 +984,7 @@ static void downmix_to_mono(struct a2dp_stream_out *out, const uint8_t *buffer,
> >>>>      int16_t *output = (void *) out->downmix_buf;
> >>>>      size_t i;
> >>>>
> >>>> -     for (i = 0; i < bytes / 2; i++) {
> >>>> +     for (i = 0; i < bytes / (2 * sizeof(int16_t)); i++) {
> >>>>              int16_t l = le16_to_cpu(get_unaligned(&input[i * 2]));
> >>>>              int16_t r = le16_to_cpu(get_unaligned(&input[i * 2 + 1]));
> >>>
> >>> I wonder actually what this get_unaligned is doing here? You cast the const void into const int16_t buffer. Is this really needed? Where is our input and output buffer coming from? Aren’t these aligned anyway? Meaning aren’t they allocated?
> >>
> >> We have this buffer from AudioFlinger so we don't actually know
> >> alignment or if this is pointer to beginning of some internal buffer
> >> (it's most probably both, but I don't think we should assume this).
> >
> > and audio system that does not give you a guarantee here on the alignment is utterly screwed up. Seriously, that is just bad for performance. Especially bad on ARM CPUs, so I doubt that they have not thought about this.
> 
> This is only to be on safe side and in terms of performance it does
> not really matter here since this code won't probably be used at all -

It was definitely not used. It crashes. I have modified code for SCO where
downmix was used.

Best regards 
Andrei Emeltchenko