[pulseaudio-discuss] [PATCH 53/56] bluetooth: Process sink messages for BlueZ 5 cards

[Tanu Kaskinen]

On Fri, 2013-07-12 at 15:07 -0300, jprvita at gmail.com wrote:
> From: João Paulo Rechi Vita <jprvita at openbossa.org>
> 
> ---
>  src/modules/bluetooth/module-bluez5-device.c | 81 ++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index 9a5534c..6b852f8 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -714,6 +714,86 @@ static int add_source(struct userdata *u) {
>      return 0;
>  }
>  
> +/* Run from IO thread */
> +static int sink_process_msg(pa_msgobject *o, int code, void *data, int64_t offset, pa_memchunk *chunk) {
> +    struct userdata *u = PA_SINK(o)->userdata;
> +    bool failed = false;
> +    int r;
> +
> +    pa_assert(u->sink == PA_SINK(o));
> +    pa_assert(u->transport);
> +
> +    switch (code) {
> +
> +        case PA_SINK_MESSAGE_SET_STATE:
> +
> +            switch ((pa_sink_state_t) PA_PTR_TO_UINT(data)) {
> +
> +                case PA_SINK_SUSPENDED:
> +                    /* Ignore if transition is PA_SINK_INIT->PA_SINK_SUSPENDED */
> +                    if (!PA_SINK_IS_OPENED(u->sink->thread_info.state))
> +                        break;
> +
> +                    /* Stop the device if the source is suspended as well */
> +                    if (!u->source || u->source->state == PA_SOURCE_SUSPENDED)
> +                        /* We deliberately ignore whether stopping
> +                         * actually worked. Since the stream_fd is
> +                         * closed it doesn't really matter */
> +                        transport_release(u);
> +
> +                    break;
> +
> +                case PA_SINK_IDLE:
> +                case PA_SINK_RUNNING:
> +                    if (u->sink->thread_info.state != PA_SINK_SUSPENDED)
> +                        break;
> +
> +                    /* Resume the device if the source was suspended as well */
> +                    if (!u->source || !PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
> +                        if (transport_acquire(u, false) < 0)
> +                            failed = true;
> +                        else
> +                            setup_stream(u);
> +                    }
> +
> +                    break;
> +
> +                case PA_SINK_UNLINKED:
> +                case PA_SINK_INIT:
> +                case PA_SINK_INVALID_STATE:
> +                    break;
> +            }
> +
> +            break;
> +
> +        case PA_SINK_MESSAGE_GET_LATENCY:
> +
> +            if (u->read_smoother) {
> +                pa_usec_t wi, ri;
> +
> +                ri = pa_smoother_get(u->read_smoother, pa_rtclock_now());
> +                wi = pa_bytes_to_usec(u->write_index + u->write_block_size, &u->sample_spec);
> +
> +                *((pa_usec_t*) data) = wi > ri ? wi - ri : 0;

I can't wrap my head around this code, so I can't say whether this is
wrong or right.

> +            } else {
> +                pa_usec_t ri, wi;
> +
> +                ri = pa_rtclock_now() - u->started_at;
> +                wi = pa_bytes_to_usec(u->write_index, &u->sample_spec);
> +
> +                *((pa_usec_t*) data) = wi > ri ? wi - ri : 0;

I believe this calculation should always be simply wi - ri, possibly
with a negative result. The purpose of the calculation is to find out
how much wi is ahead of ri, that is, how much the write index is ahead
of the real time. Since writing happens only when the real time reaches
write_index, it is a regular occurrence that the real time is a bit
ahead of write_index, so negative results need to be allowed (after
adding FIXED_LATENCY_PLAYBACK_A2DP, the total latency of course can
never be negative).

> +            }
> +
> +            *((pa_usec_t*) data) += u->sink->thread_info.fixed_latency;

Using thread_info.fixed_latency seems wrong, because that value is the
maximum latency. It doesn't make sense to return the maximum latency and
then some. It would make more sense to add only
FIXED_LATENCY_PLAYBACK_A2DP here.

-- 
Tanu