Re: [pulseaudio-discuss] [PATCH] bluetooth: Fix rendering a2dp data
Hi, On Thu, Dec 16, 2010 at 5:36 AM, caiiiyua yuanqing@tieto.com wrote: 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.lankho...@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.lankho...@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? Ive tried some approaches like if we are sleep for more then some msec increase the bitpool, but it sometimes cause too many/quickly changes and some headsets just crashes, so for now it just reduces to a limit of 32 which is considered mid quality by a2dp spec, I couldn't notice any difference but the audio definably stopped skipping. 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. Ive figure out what was the problem, for source we need to initialize the bitpool to the minimum, this will generate the possible frames per packet and thus the maximum possible block size. Off course this could be done differently with sbc_parse but I don't think it worth
Re: [pulseaudio-discuss] [PATCH] bluetooth: Fix rendering a2dp data
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.lankho...@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 ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] bluetooth: Fix rendering a2dp data
Hi, On Tue, Dec 14, 2010 at 11:10 AM, Maarten Lankhorst m.b.lankho...@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.lankho...@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 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. -- Luiz Augusto von Dentz Computer Engineer ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] bluetooth: Fix rendering a2dp data
Hi, On Sat, Dec 11, 2010 at 1:05 AM, Maarten Lankhorst m.b.lankho...@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. 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. 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. Regards, -- Luiz Augusto von Dentz Computer Engineer ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss