[pulseaudio-discuss] [PATCH] pacat: paplay stream format extention

[Tanu Kaskinen]

On Fri, 2012-05-11 at 14:43 +0800, Wang Xingchao wrote:
> paplay with "--passthrough" only indicate the stream with PASSTHROUGH flag,
> but there's no exact format infomation. the patch makes stream easy to notify
> pacore its type in passthrough mode.
> 
> Signed-off-by: Wang Xingchao <xingchao.wang at intel.com>
> ---
>  src/map-file       |    2 ++
>  src/pulse/stream.c |    2 +-
>  src/pulse/stream.h |    9 +++++++++
>  src/utils/pacat.c  |   35 ++++++++++++++++++++++++++++++-----
>  4 files changed, 42 insertions(+), 6 deletions(-)

Thanks for the patch, and sorry for the ridiculously long delay with
review... Comments below.

> 
> diff --git a/src/map-file b/src/map-file
> index 69cf25b..867a8c5 100644
> --- a/src/map-file
> +++ b/src/map-file
> @@ -295,6 +295,8 @@ pa_stream_is_suspended;
>  pa_stream_new;
>  pa_stream_new_extended;
>  pa_stream_new_with_proplist;
> +pa_stream_new_with_proplist_internal;

Isn't it rather obvious that a function with name
"pa_stream_new_with_proplist_internal" doesn't belong in the public API?

> +pa_encoding_from_string;

The entries in map-file should be in alphabetical order. Also, I think
it would be better to have this change in a separate patch.

>  pa_stream_peek;
>  pa_stream_prebuf;
>  pa_stream_proplist_remove;
> diff --git a/src/pulse/stream.c b/src/pulse/stream.c
> index 39338c1..77e5ec4 100644
> --- a/src/pulse/stream.c
> +++ b/src/pulse/stream.c
> @@ -80,7 +80,7 @@ static void reset_callbacks(pa_stream *s) {
>      s->buffer_attr_userdata = NULL;
>  }
>  
> -static pa_stream *pa_stream_new_with_proplist_internal(
> +pa_stream *pa_stream_new_with_proplist_internal(
>          pa_context *c,
>          const char *name,
>          const pa_sample_spec *ss,
> diff --git a/src/pulse/stream.h b/src/pulse/stream.h
> index b4464fa..ba7b1ff 100644
> --- a/src/pulse/stream.h
> +++ b/src/pulse/stream.h
> @@ -357,6 +357,15 @@ pa_stream* pa_stream_new_with_proplist(
>          const pa_channel_map *map         /**< The desired channel map, or NULL for default */,
>          pa_proplist *p                    /**< The initial property list */);
>  
> +pa_stream *pa_stream_new_with_proplist_internal(
> +        pa_context *c,
> +        const char *name,
> +        const pa_sample_spec *ss,
> +        const pa_channel_map *map,
> +        pa_format_info * const *formats,
> +        unsigned int n_formats,
> +        pa_proplist *p);
> +
>  /** Create a new, unconnected stream with the specified name, the set of formats
>   * this client can provide, and an initial list of properties. While
>   * connecting, the server will select the most appropriate format which the
> diff --git a/src/utils/pacat.c b/src/utils/pacat.c
> index ec360f7..bd984af 100644
> --- a/src/utils/pacat.c
> +++ b/src/utils/pacat.c
> @@ -39,6 +39,8 @@
>  
>  #include <pulse/pulseaudio.h>
>  #include <pulse/rtclock.h>
> +#include <pulse/format.h>
> +#include <pulse/stream.h>
>  
>  #include <pulsecore/core-util.h>
>  #include <pulsecore/i18n.h>
> @@ -76,6 +78,8 @@ static pa_sample_spec sample_spec = {
>      .channels = 2
>  };
>  static pa_bool_t sample_spec_set = FALSE;
> +static pa_bool_t passthrough_set = FALSE;
> +static pa_encoding_t passthrough_format;
>  
>  static pa_channel_map channel_map;
>  static pa_bool_t channel_map_set = FALSE;
> @@ -443,17 +447,30 @@ static void context_state_callback(pa_context *c, void *userdata) {
>  
>          case PA_CONTEXT_READY: {
>              pa_buffer_attr buffer_attr;
> +            pa_format_info *formats[1];

No need to use an array if it contains only one element.

>  
> -            pa_assert(c);
> +	    if (passthrough_set == TRUE) {

No tabs, please. This is not the only place where you use tabs for
indentation; please remove all tabs.

> +                formats[0] = pa_format_info_new();
> +                formats[0]->encoding = passthrough_format;
> +	    }
> +
> +	    pa_assert(c);
>              pa_assert(!stream);
>  
>              if (verbose)
>                  pa_log(_("Connection established.%s"), CLEAR_LINE);
>  
> -            if (!(stream = pa_stream_new_with_proplist(c, NULL, &sample_spec, &channel_map, proplist))) {
> +	    if (passthrough_set == TRUE) {
> +		    if (!(stream = pa_stream_new_with_proplist_internal(c, NULL, NULL, NULL, formats, 1, proplist))) {

Instead of pa_stream_new_with_proplist_internal(), you want to use
pa_stream_new_extended().

> +	                pa_log(_("pa_stream_new_new_with_proplist_internal() failed: %s"), pa_strerror(pa_context_errno(c)));
> +		        goto fail;
> +		    }
> +	    }else {

Space missing before "else".

> +		    if (!(stream = pa_stream_new_with_proplist(c, NULL, &sample_spec, &channel_map, proplist))) {
>                  pa_log(_("pa_stream_new() failed: %s"), pa_strerror(pa_context_errno(c)));
>                  goto fail;
> -            }
> +		    }
> +	    }
>  
>              pa_stream_set_state_callback(stream, stream_state_callback, NULL);
>              pa_stream_set_write_callback(stream, stream_write_callback, NULL);
> @@ -682,7 +699,7 @@ static void help(const char *argv0) {
>               "      --process-time-msec=MSEC          Request the specified process time per request in msec.\n"
>               "      --property=PROPERTY=VALUE         Set the specified property to the specified value.\n"
>               "      --raw                             Record/play raw PCM data.\n"
> -             "      --passthrough                     passthrough data \n"
> +             "      --passthrough[=DATAFORMAT]        passthrough data \n"

The man page needs to be updated too.

Also, I'm not sure it's a good idea to give the encoding as a parameter
to --passthrough. I think it's somewhat possible that some day we want
to be able to specify the encoding even when not using the passthrough
mode. Therefore, I'd add a new option: --encoding=ENCODING.

-- 
Tanu