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
