[pulseaudio-discuss] [PATCH v5 0/7] New API for Bluetooth A2DP codecs

[Tanu Kaskinen]

On Wed, 2019-01-23 at 18:52 +0100, Pali Rohár wrote:
> On Wednesday 23 January 2019 19:16:16 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> > On Sat, Jan 19, 2019 at 7:19 PM Pali Rohár <pali.rohar at gmail.com> wrote:
> > > I think that this patch series is now ready for review and testing. All
> > > needed parts are finished.
> > > 
> > > The only problem is with asynchronous profile switching as function
> > > pa_card_set_profile() does not support it. All details are in patch
> > > "[PATCH v5 5/7] bluetooth: Modular API for A2DP codecs".
> > > 
> > > So can you look at this patch series and review it?
> > 
> > I tested this today, it seems to work fine with sbc but with
> > faststream there seems to be some problem somewhere as it does seems
> > to consume too much cpu time. Regarding set_profile that is something
> > I think we will need to address otherwise the UI is pretty much
> > useless to switch as it doesn't seems to react well when -EAGAIN is
> > returned, we could perhaps block on SetConfiguration since we do block
> > on Acquire anyway, or you have already tried that?
> 
> Yes, I tried it. Pulseaudio blocks communication and therefore it also
> does not receive following DBus requests from bluez and so
> SetConfiguration timeout.
> 
> In fact -EAGAIN is just dropped in pulseaudio bluetooth module and
> replaced by -1. I just wanted to "highlight" that part of code, so I
> used -EAGAIN, but it has same meaning as any other non-zero value.
> 
> I was thinking about it and we could block this on another layer.
> pa_card_set_profile() is called when application request to change
> profile via PA socket. So extend pa_card_set_profile() API to pass
> callback function which would be called after profile is changed
> asynchronously. And just block user request until callback is called.
> 
> Maybe some other people who understand pulseaudio better can help? How
> to implement asynchronous profile switching?

To me it seems that either pa_card_set_profile() should be made
asynchronous, which would be nice, because it would allow errors to be
reported properly, or the bluetooth code should just initiate the
profile change operation and pretend that the switching always
succeeds. In the latter option, in case of failure the bluetooth code
would switch to the off profile.

I'm fine with either option.

In case you wanted some more detailed thoughts about implementation,
here's what I'd do if I chose the "make pa_card_set_profile()
asynchronous" option:

pa_card_set_profile() should be renamed to pa_card_set_profile_begin().
It would take a user callback to be called when the operation finishes.

pa_card_set_profile_begin() should return an operation object similar
to pa_operation in the client API (but I believe pa_operation is not
suitable to be used in the core, because it's tied to pa_context, and I
also dislike its use of reference counting). The operation object would
store the callback and any other necessary information related to the
profile switch.

The operation object would be created by pa_card_set_profile_begin()
and owned by pa_card. The card backend can also save some backend
specific data in the operation object.

Once the profile switch completes, the card backend should set the
result information (success/failure, error codes, etc.) in the
operation object and call pa_card_set_profile_finish() with the
operation object as an argument. pa_card_set_profile_finish() would
then call the user callback and free the operation object.

The operation needs to be cancellable too. If
pa_card_set_profile_begin() is called while there's an older operation
still in progress, the old operation needs to be cancelled. The card
backend needs to set a callback in the operation object to get notified
about cancellation. The pa_card_set_profile_begin() user can probably
handle a cancellation in the same way it handles any kind of failure
(so it gets notified via the operation user callback).

When the card is destroyed, any ongoing asynchronus operation needs to
be cancelled.

It would probably be a good idea to define a generic asynchronous
operation object at some point (the name could be pa_async_operation),
but as long as pa_card_set_profile() remains the only user for such
functionality, we can begin by making the operation object specific to
the profile switch operation, which would keep things a bit less
complex.

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk