[pulseaudio-discuss] [PATCH v1] bluetooth: Dynamically change outgoing MTU

[Mikel Astiz]

Hi Tanu,

On Tue, Mar 12, 2013 at 12:12 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> 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

Yes, except the "arbitrary time" part, since the change is triggered
when the second SCO is stablished. We could try to consider this
transition, instead of testing the packet size, but this would be more
complex in my opinion.

>
>     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

I'm not following here. If the MTU is set to 128, the memory block
would be of size 128. PA_MAX just guarantees that it won't be smaller
than 96.

> 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.

The spec considers different MTU sizes for input and output, but in
practice BlueZ will return the same value for both.

Having said that, I admit the feature kind of collides with the
parameters received from BlueZ.

>
>> 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.

Indeed, that would be another option. I just avoided rewriting too
much code considering that the feature won't be widely used. So far
only a bunch of users have shown interest in having multiple SCOs,
AFAIK.

Cheers,
Mikel