[pulseaudio-discuss] [PATCH] bluetooth: Fix rendering a2dp data

[Maarten Lankhorst]

  Hi Luiz,

Op 13-12-10 09:55, Luiz Augusto von Dentz schreef:
> Hi,
>
> On Sat, Dec 11, 2010 at 1:05 AM, Maarten Lankhorst
> <m.b.lankhorst at gmail.com>  wrote:
>> makes my android phone slightly happier
>> ---
>>   src/modules/bluetooth/module-bluetooth-device.c |   11 ++++++++---
>>   1 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
>> index 6d31c1e..8664001 100644
>> --- a/src/modules/bluetooth/module-bluetooth-device.c
>> +++ b/src/modules/bluetooth/module-bluetooth-device.c
>> @@ -1387,7 +1387,7 @@ static int a2dp_process_push(struct userdata *u) {
>>      pa_assert(u->source);
>>      pa_assert(u->read_smoother);
>>
>> -    memchunk.memblock = pa_memblock_new(u->core->mempool, u->block_size);
>> +    memchunk.memblock = pa_memblock_new(u->core->mempool, u->block_size * 2);
>>      memchunk.index = memchunk.length = 0;
> Im not sure how this would help, we decode sbc frame by frame so
> having twice as much memory doesn't really make any sense, there could
> be that we should decode the entire buffer read, but that would take
> much more memory. What could be the real problem is that the source is
> changing the bitpool but that shouldn't be a problem since the
> block_size is supposed to be calculated from maximum bitpool range so
> the buffer will always be big enough.
Well it seems the block_size is not fixed on my phone, sometimes it is 1 
sample more, sometimes 2, the net effect is 1 or 2 frames stay undecoded 
leading to massive underruns.

I don't think u->block_size has to be fixed per se, a2dp->code_size 
seems to be though.
>>      for (;;) {
>> @@ -1442,7 +1442,8 @@ static int a2dp_process_push(struct userdata *u) {
>>          to_decode = l - sizeof(*header) - sizeof(*payload);
>>
>>          d = pa_memblock_acquire(memchunk.memblock);
>> -        to_write = memchunk.length = pa_memblock_get_length(memchunk.memblock);
>> +        to_write = pa_memblock_get_length(memchunk.memblock);
>> +        memchunk.length = 0;
>>
>>          while (PA_LIKELY(to_decode>  0&&  to_write>  0)) {
>>              size_t written;
>> @@ -1464,7 +1465,7 @@ static int a2dp_process_push(struct userdata *u) {
>>   /*             pa_log_debug("SBC: frame_length: %lu; codesize: %lu", (unsigned long) a2dp->frame_length, (unsigned long) a2dp->codesize); */
>>
>>              pa_assert_fp((size_t) decoded<= to_decode);
>> -            pa_assert_fp((size_t) decoded == a2dp->frame_length);
>> +            pa_assert_fp((size_t) decoded<= a2dp->frame_length);
> This is the real problem, either we do this if the bitpool is changing
> or we have to call sbc_reinit in each frame.
Well, a2dp->frame_length was calculated based on the maximum bitpool 
size. The frames decoded have a smaller bitpool size. a2dp->codesize is 
still the same though, so I don't think this change matters much.
>>              pa_assert_fp((size_t) written<= to_write);
>>              pa_assert_fp((size_t) written == a2dp->codesize);
>> @@ -1474,10 +1475,14 @@ static int a2dp_process_push(struct userdata *u) {
>>
>>              d = (uint8_t*) d + written;
>>              to_write -= written;
>> +            memchunk.length += written;
>>
>>              frame_count++;
>>          }
>>
>> +        if (to_decode)
>> +            pa_log_error("SBC: %lu bytes not decoded\n", to_decode);
>> +
>>          pa_memblock_release(memchunk.memblock);
> Actually I think we should be solving this in libsbc, both sbc_encode
> and sbc_decode should be checking if the bitpool has changed and
> reinit automatically, otherwise we gonna get performance penalty doing
> sbc_reinit for every frame.
I don't think the bitpool changed, the code just assumes the maximum 
bitpool size, while my phone sends data at a lower one.


Of course I still don't know how to handle packet loss correctly, which 
is easy to create by moving phone out of the range of my bluetooth device.

Cheers,
Maarten