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
