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

[Mikel Astiz]

Hi Tanu,

On Tue, Mar 12, 2013 at 1:42 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Tue, 2013-03-12 at 12:58 +0100, Mikel Astiz wrote:
>> 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.
>
> I guess it's not documented anywhere that the MTU may only change when
> additional SCO connections are established (or torn down)? From this
> point of view the reconfiguration time is unspecified, so I think the
> correct way to handle it is to prepare to reconfigure at any time.

Agreed.

>> >
>> >     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.
>
> If the MTU is initially set to 128, it sounds logical that it may be
> reconfigured to 128 too. This wouldn't work with the proposed code: 128
> -> 96 -> 128.

The proposed code specifically handles only two possible transitions:
either 48->96 or 96->48.

If the MTU is set to something else other than 48 or 96, the code
won't change the previous behavior. That's what I meant when I said it
was 'conservative', since it handles only the cases that have been
experimentally observed.

We can try to generalize this for any random MTU value, as I was
trying to do in my first proposal, but I see very little benefit
unless someone proves that similar policies are useful in other
working conditions (e.g. using a non-USB Bluetooth adapter). In this
case I would agree that something like MAX_SCO_READ_MTU would be
required.

>> > 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.
>
> So what do you think? I don't want to rely on undocumented behavior. If
> BlueZ guarantees that the MTUs will always be symmetric (also in the
> future), it's fine to update the write MTU in PulseAudio when the read
> MTU changes. But if there's no such guarantee, I wouldn't change the
> write MTU, unless there's a really good reason to do so. So, what
> horrible things happen if we don't update the write MTU?

If you don't update the MTU as proposed by this patch, the Bluetooth
adapter typically powers down.

Concerning the 'undocumented behavior', you're completely right,
starting from the fact that I found no reference in the spec about all
this.

Regarding what BlueZ's API guarantees, we shouldn't assume the MTUs
are symmetric. If you're concerned about this, I could extend the
patch in such a way that the policy is triggered if and only if this
condition holds true (input MTU == output MTU).

>
>> >
>> >> 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.
>
> If we can remove write_memchunk from userdata, I think that's worth
> doing regardless of the number of users for any particular feature.

Sure. I will send patches for this.

Cheers,
Mikel