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@xxxxxxxxx> 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 _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
