[pulseaudio-discuss] [PATCH] AC3 passthrough support

[Tanu Kaskinen]

On Thu, 2010-07-15 at 21:12 -0500, pl bossart wrote:
> Clean-up of my previous draft, can be applied to git master now.

Thank you for your continued work on this! Here's a review for the
patch.

> +    PA_SINK_PASSTHROUGH = 0x0100U
> +    /**< The latency can be adjusted dynamically depending on the
> +     * needs of the connected streams. \since 0.9.22 */
> +

Copy-paste mistake in the documentation.

> +    if (dest->flags&PA_SINK_PASSTHROUGH) {

Cosmetic: spaces missing around &.

> +        for (alt_i = pa_idxset_first(dest->inputs, &idx); alt_i; alt_i=pa_idxset_next(dest->inputs, &idx)) {

Cosmetic: spaces missing around =.

Also, PA_IDXSET_FOREACH can be used here. Or actually you don't need to
iterate through the set at all, you could just check the first input -
if that's a passthrough input, then it's the only input anyway, and if
it's not a passthrough input, then none of the inputs is a passthrough
input.

> +            if (alt_i->flags&PA_SINK_INPUT_PASSTHROUGH) {

Cosmetic: spaces missing around &.

> @@ -183,6 +216,8 @@ int pa_sink_input_new(
>      pa_return_val_if_fail(PA_SINK_IS_LINKED(pa_sink_get_state(data->sink)), -PA_ERR_BADSTATE);
>  
> +    pa_return_val_if_fail(check_passthrough_connection(data->flags,data->sink), -PA_ERR_INVALID);
> +
>      if (!data->sample_spec_is_set)
>          data->sample_spec = data->sink->sample_spec;

Cosmetic: space missing between the function arguments.

I wonder if we should return a more descriptive error in case the sink
is used by other streams. Returning just "invalid argument" doesn't
allow apps to give informative error messages. What do you think?

>  void pa_sink_input_set_volume(pa_sink_input *i, const pa_cvolume *volume, pa_bool_t save, pa_bool_t absolute) {
> +
> +        /* Do not allow for volume changes for non-audio types */
> +    if (i->flags & PA_SINK_INPUT_PASSTHROUGH)
> +        return;
> +

Cosmetic: bad indentation of the comment. (I guess this will be replaced
anyway when the volume handling is done properly, but it doesn't hurt to
have good formatting on temporary code too...)

> +    if(!check_passthrough_connection(i->flags,dest))

Cosmetic: space missing between the function arguments.

> +    if (s->flags&PA_SINK_PASSTHROUGH) {

Cosmetic: spaces missing around &.

> +            if (alt_i->flags&PA_SINK_INPUT_PASSTHROUGH) {

Cosmetic: spaces missing around &.

Regarding volume handling, I think there should be

    pa_assert(!((data->flags & PA_SINK_INPUT_PASSTHROUGH) && data->volume_is_set));

somewhere in pa_sink_input_new, and checks in protocol-native.c to
prevent that assertion from firing. That is, refuse to create
passthrough streams that also have volume specified by the client.

-- 
Tanu Kaskinen