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

[caiiiyua]

  Hi:-)

On 12/15/2010 05:33 AM, Luiz Augusto von Dentz wrote:
> Hi,
>
> On Tue, Dec 14, 2010 at 11:10 AM, Maarten Lankhorst
> <m.b.lankhorst at gmail.com>  wrote:
>>   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.
> Again if we want to be able to support devices that changes bitpool we
> have to recalculate the block_size on every frame (if bitpool has
> really changed), doubling the buffer is not the right fix, sorry.
>
>>>>      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.
> It was you that suggested this changes, so when you say it doesn't
> matter it makes me wonder why this is part of the patch then, Btw,
> changing bitpool changes the frame length and it actually does assert,
> I tried it myself with this patch to source:
>
> http://gitorious.org/pulseaudio/mainline/commit/9b938f8f8f40772e626b3e71fa4d7b7cbd5de5e7
>
   That's really good to change "bitpool" dynamically.
   As in your patch source,I saw a function called 
"a2dp_reduce_bitpool",in some cases,we need bitpool to be increased,
   shall we think about this situation?
>>>>              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.
> Well what other bitpool would you use instead? If we don't have any
> frame to parse obviously we need to start with the biggest possible
> one, it the only possible way to avoid decode errors due to too small
> buffer to decode.
IMO,as a A2DP sink,we should get some information from source before 
decoding,thus,we can decide the size of framelen and blocksize .I am 
still a Pulseaudio newbie,so I don't complete realise how a2dp works for 
pulseaudio,could you teach me how this "blocksize" be fixed when we 
start to
decode the sound.