[pulseaudio-discuss] [PATCH next v0 07/10] bluetooth: Abstract transport access types inside bluetooth-util

[Mikel Astiz]

Hi Tanu,

On Fri, Dec 14, 2012 at 10:15 AM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Wed, 2012-12-12 at 13:17 +0100, Mikel Astiz wrote:
>>  static bool bt_transport_is_acquired(struct userdata *u) {
>> -    if (u->accesstype == NULL) {
>> +    if (!u->transport_acquired) {
>>          pa_assert(u->stream_fd < 0);
>>          return FALSE;
>>      } else {
>
> Now that we have the transport_acquired variable in userdata, isn't
> bt_transport_is_acquired() pretty useless? It contains an assertion,
> which has some value, but I don't think it's valuable enough to justify
> the existence of this function.

I'll include this as a separate patch in v1 and you can decide if
you'd rather squash these two changes.

>
>> @@ -397,21 +394,7 @@ static int bt_transport_acquire(struct userdata *u, pa_bool_t start) {
>>
>>      pa_log_debug("Acquiring transport %s", u->transport->path);
>>
>> -    if (!start) {
>> -        /* FIXME: we are trying to acquire the transport only if the stream is
>> -           playing, without actually initiating the stream request from our side
>> -           (which is typically undesireable specially for hfgw use-cases.
>> -           However this approach is racy, since the stream could have been
>> -           suspended in the meantime, so we can't really guarantee that the
>> -           stream will not be requested until BlueZ's API supports this
>> -           atomically. */
>> -        if (u->device->profile_state[u->profile] < PA_BT_AUDIO_STATE_PLAYING) {
>> -            pa_log_info("Failed optional acquire of transport %s", u->transport->path);
>> -            return -1;
>> -        }
>> -    }
>> -
>> -    u->stream_fd = pa_bluetooth_transport_acquire(u->transport, accesstype, &u->read_link_mtu, &u->write_link_mtu);
>> +    u->stream_fd = pa_bluetooth_transport_acquire(u->transport, !start, &u->read_link_mtu, &u->write_link_mtu);
>
> The start parameter could be changed from "start" to "optional" to be
> consistent with pa_bluetooth_transport_acquire(). On the other hand,
> "start" describes better the behavior regarding whether setup_stream()
> will be called or not. On the third hand, does setup_stream() really
> belong in this function? Could it be moved out? In that case, could this
> whole function be removed in favor of using
> pa_bluetooth_transport_acquire() directly?

This is indeed quite tricky. The reason I was trying to leave this
unmodified is the second argument you mention: "start" describes
whether setup_stream() gets called. Another option would be to split
the parameter and have both "optional" and "start", but I'd rather
avoid this.

Regarding your third point, I agree that splitting the functions
acquire()/setup_stream() would be the cleanest solution, with the
drawback of adding duplicated code since most of the times both
functions need to be called.

So I would vote in favor of this last option. Any preference from your side?

In any case, I think this should also be a separate patch.

Cheers,
Mikel