On Tue, 2013-03-12 at 10:32 +0100, Mikel Astiz wrote:
> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
>
> Dynamically change the size of the SCO packets according to the size of
> the received ones.
> ---
> This patch has been hanging around for some time since our last
> discussion [1].
>
> I haven't been able to clarify why exactly this MTU change is needed,
> but it can be experimentally observed that several Bluetooth chips
> from different manufacturers behave similarly: when two SCO streams
> are set up, the size of the packets doubles.
>
> My best guess is that it's influenced by the constraints in the USB
> transport layer, but I wasn't able to confirm this yet nor was I able
> to find such behavior in the Bluetooth spec.
>
> In any case, I believe the patch should do no harm.
>
> This v1 proposal rewrites the patch in a more conservative way. The
> MTU changes are accepted only if they match the known pattern of
> changing between 48 and 96 back and forth.
Is there no explicit message for reconfiguring the MTU? Is the MTU
reconfiguration really done so that PulseAudio just starts to receive
bigger or smaller packets than originally configured, at an arbitrary
time? If so, I think this
memchunk.memblock = pa_memblock_new(u->core->mempool, PA_MAX(96U, u->read_block_size));
should be changed to
memchunk.memblock = pa_memblock_new(u->core->mempool, MAX_SCO_READ_MTU);
in order to ensure that we are able to handle any MTU change. PA_MAX(96,
u->read_block_size) implies that the MTU can never change to a value
greater than 96, even though the initial MTU can be greater than 96. I
don't know enough to say that this couldn't be true, but it sounds very
unlikely to me.
We should then also ensure that u->read_link_mtu is never greater than
MAX_SCO_READ_MTU. MAX_SCO_READ_MTU should be chosen so that this would
never cause problems.
Another thing is the write MTU. What happens if we don't update it, so
that it doesn't match the read MTU? The reason why I ask is that
MediaTransport.Acquire() can return different read and write MTU, and we
don't currently fail if that happens. Is it guaranteed that with the SCO
profiles the MTUs are always symmetric? If so, we should fail if
MediaTransport.Acquire() returns asymmetric MTUs. If there's no such
guarantee, it doesn't seem like a good idea to copy the read MTU to the
write MTU when the read MTU changes.
> You can test this patch by connecting two headsets (or two phones or
> whatever else doing HFP/HSP). You probably also need to change the
> adapter's SCO MTU by doing:
>
> sudo ./hciconfig hci0 scomtu 96:8
>
> [1] http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/12982/focus=15363
>
> src/modules/bluetooth/module-bluetooth-device.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
> index c877df2..552db4b 100644
> --- a/src/modules/bluetooth/module-bluetooth-device.c
> +++ b/src/modules/bluetooth/module-bluetooth-device.c
> @@ -625,7 +625,8 @@ static int hsp_process_push(struct userdata *u) {
> pa_assert(u->source);
> pa_assert(u->read_smoother);
>
> - memchunk.memblock = pa_memblock_new(u->core->mempool, u->read_block_size);
> + /* Read minimum 96 bytes to be able to detect multi-SCO setup */
> + memchunk.memblock = pa_memblock_new(u->core->mempool, PA_MAX(96U, u->read_block_size));
> memchunk.index = memchunk.length = 0;
>
> for (;;) {
> @@ -681,6 +682,23 @@ static int hsp_process_push(struct userdata *u) {
> break;
> }
>
> + /* In order to support multi-SCO scenarios, adapt size of I/O blocks
> + * according to the size we just got, only considering the typical MTU
> + * changes observed in these cases */
> + if ((size_t) l != u->read_link_mtu && (l == 48U || l == 96U)) {
> + pa_log_info("SCO MTU change detected: %zu->%zd", u->write_block_size, l);
> +
> + u->read_link_mtu = (size_t) l;
> + u->write_link_mtu = (size_t) l;
> +
> + bt_transport_config_mtu(u);
> +
> + if (u->write_memchunk.memblock) {
> + pa_memblock_unref(u->write_memchunk.memblock);
> + pa_memchunk_reset(&u->write_memchunk);
> + }
This causes data to be dropped, which is pretty nasty. But on the other
hand, this can apparently only happen if we get a POLLOUT event for the
socket, but then it turns out that the socket isn't writable after all.
This should probably never happen, so this shouldn't be a big issue in
practice. I wonder if hsp_process_render() should fail hard if the
socket is not writable. It could then always reset the write_memchunk,
and we wouldn't need to care about it here.
--
Tanu
