[pulseaudio-discuss] [PATCH 3/6] stream: Implement a pa_stream_get_volume() API

[Tanu Kaskinen]

On Tue, 2015-12-29 at 09:03 +0530, arun at accosted.net wrote:
> +void pa_command_stream_volume_changed(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
> +    pa_context *c = userdata;
> +    pa_stream *s;
> +    uint32_t index;
> +    pa_cvolume volume;
> +
> +    pa_assert(pd);
> +    pa_assert(command == PA_COMMAND_PLAYBACK_STREAM_VOLUME_CHANGED || command == PA_COMMAND_RECORD_STREAM_VOLUME_CHANGED);
> +    pa_assert(t);
> +    pa_assert(c);
> +    pa_assert(PA_REFCNT_VALUE(c) >= 1);
> +
> +    pa_context_ref(c);
> +
> +    if (c->version < 31) {
> +        pa_context_fail(c, PA_ERR_PROTOCOL);
> +        goto finish;
> +    }
> +
> +    if (pa_tagstruct_getu32(t, &index) < 0 ||
> +        pa_tagstruct_get_cvolume(t, &volume) < 0 ||
> +        !pa_tagstruct_eof(t)) {
> +        pa_context_fail(c, PA_ERR_PROTOCOL);
> +        goto finish;
> +    }
> +
> +    if (!(s = pa_hashmap_get(command == PA_COMMAND_PLAYBACK_STREAM_VOLUME_CHANGED ? c->playback_streams : c->record_streams,
> +                    PA_UINT32_TO_PTR(index))))
> +        goto finish;
> +
> +    s->volume = volume;

We should invoke protocol error if s is an upload stream (upload
streams are stored in c->playback_streams).

> +
> +finish:
> +    pa_context_unref(c);
> +}
> +
>  static void invalidate_indexes(pa_stream *s, bool r, bool w) {
>      pa_assert(s);
>      pa_assert(PA_REFCNT_VALUE(s) >= 1);
> @@ -1154,6 +1190,9 @@ void pa_create_stream_callback(pa_pdispatch *pd, uint32_t command, uint32_t tag,
>          }
>      }
>  
> +    if (s->context->version >= 31)
> +        pa_tagstruct_get_cvolume(t, &s->volume);

pa_tagstruct_get_cvolume() return value should be checked.

Also, I think upload streams shouldn't have volume associated with
them.

Also, s->volume_set needs to be set.

> +
>      if (!pa_tagstruct_eof(t)) {
>          pa_context_fail(s->context, PA_ERR_PROTOCOL);
>          goto finish;
> @@ -2968,3 +3007,19 @@ int pa_stream_set_volume(pa_stream *s, pa_cvolume *v, pa_stream_success_cb_t cb,
>  
>      return 0;
>  }
> +
> +int pa_stream_get_volume(pa_stream *s, pa_cvolume *v) {
> +    pa_assert(s);
> +    pa_assert(v);
> +    pa_assert(PA_REFCNT_VALUE(s) >= 1);
> +
> +    PA_CHECK_VALIDITY(s->context, !pa_detect_fork(), PA_ERR_FORKED);
> +    PA_CHECK_VALIDITY(s->context, s->state != PA_STREAM_FAILED && s->state != PA_STREAM_TERMINATED, PA_ERR_BADSTATE);
> +    PA_CHECK_VALIDITY(s->context, s->state == PA_STREAM_READY || s->volume_set, PA_ERR_NODATA);

Shouldn't it be enough to check only s->volume_set? If the stream is
ready, then the volume should be always set.

Also, I think this should be the last check. If multiple error
conditions exist, PA_ERR_NODATA should have the lowest priority.

> +    PA_CHECK_VALIDITY(s->context, s->direction != PA_STREAM_UPLOAD, PA_ERR_INVALID); /* TODO: do we want to support this? */

No, we don't.

> +    PA_CHECK_VALIDITY(s->context, s->context->version >= 31, PA_ERR_NOTSUPPORTED);
> +
> +    *v = s->volume;
> +
> +    return 0;
> +}
> diff --git a/src/pulse/stream.h b/src/pulse/stream.h
> index cc74629..b4e6ff9 100644
> --- a/src/pulse/stream.h
> +++ b/src/pulse/stream.h
> @@ -828,6 +828,13 @@ uint32_t pa_stream_get_monitor_stream(pa_stream *s);
>   * \since 9.0 */
>  int pa_stream_set_volume(pa_stream *s, pa_cvolume *v, pa_stream_success_cb_t cb, void *userdata);
>  
> +/** Get the volume on the given stream.
> + *
> + * Returns 0 on success, negative error value on failure.
> + *
> + * \since 9.0 */
> +int pa_stream_get_volume(pa_stream *s, pa_cvolume *v);

It should be documented that this is only supported starting from
protocol version 31.

> +
>  PA_C_DECL_END
>  
>  #endif
> diff --git a/src/pulsecore/native-common.h b/src/pulsecore/native-common.h
> index dc62895..73aded1 100644
> --- a/src/pulsecore/native-common.h
> +++ b/src/pulsecore/native-common.h
> @@ -179,6 +179,11 @@ enum {
>      PA_COMMAND_ENABLE_SRBCHANNEL,
>      PA_COMMAND_DISABLE_SRBCHANNEL,
>  
> +    /* Supported since protocol v31 (8.0) */

Should be 9.0.

> +    /* SERVER->CLIENT */
> +    PA_COMMAND_PLAYBACK_STREAM_VOLUME_CHANGED,
> +    PA_COMMAND_RECORD_STREAM_VOLUME_CHANGED,


-- 
Tanu