On Fri, 2012-02-03 at 13:10 -0500, Wang Xingchao wrote:
> for pcm sink which doesnot supports requested formats,there should be
> proper return value.
Sorry for the delay. Some comments after a quick glance (I haven't done
a thorough review yet):
IMO pa_sink_input_new_data_set_sink() should return an int instead of
pa_bool_t. I believe this code was written before we had the discussion
about failure reporting style. IIRC, also Arun (who wrote this code)
agreed in the end that errors should be reported with negative integers,
not with booleans. So, I'd like to have two patches: first one that
changes the return type and second that adds checks for the return
value.
Please configure your editor to replace tabs with spaces.
It would be nice to print an error message instead of failing silently
when pa_sink_input_new_data_set_sink() fails.
Arun, it seems that if the sink input new data doesn't contain
req_formats, pa_sink_input_new_data_set_sink() will always succeed. Is
that so because if there are no req_formats, the stream format will be
PCM, and we have no sinks that don't support PCM? I guess we might
someday want to have sinks that don't support PCM, so the return value
of pa_sink_input_new_data_set_sink() should be always checked (like is
done in this patch) to be future-proof?
-- Tanu
> Signed-off-by: Wang Xingchao <xingchao.wang at intel.com>
> ---
> src/modules/echo-cancel/module-echo-cancel.c | 4 +++-
> src/modules/module-ladspa-sink.c | 3 ++-
> src/modules/module-loopback.c | 3 ++-
> src/modules/module-remap-sink.c | 3 ++-
> src/modules/module-sine.c | 3 ++-
> src/modules/module-virtual-sink.c | 3 ++-
> src/modules/rtp/module-rtp-recv.c | 3 ++-
> src/pulsecore/protocol-esound.c | 6 ++++--
> src/pulsecore/protocol-native.c | 10 +++++++---
> src/pulsecore/protocol-simple.c | 3 ++-
> src/pulsecore/sink-input.c | 6 ++++--
> 11 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/src/modules/echo-cancel/module-echo-cancel.c b/src/modules/echo-cancel/module-echo-cancel.c
> index d977ef9..8203fa0 100644
> --- a/src/modules/echo-cancel/module-echo-cancel.c
> +++ b/src/modules/echo-cancel/module-echo-cancel.c
> @@ -1852,7 +1852,9 @@ int pa__init(pa_module*m) {
> pa_sink_input_new_data_init(&sink_input_data);
> sink_input_data.driver = __FILE__;
> sink_input_data.module = m;
> - pa_sink_input_new_data_set_sink(&sink_input_data, sink_master, FALSE);
> + if (!pa_sink_input_new_data_set_sink(&sink_input_data, sink_master, FALSE))
> + goto fail;
> +
> sink_input_data.origin_sink = u->sink;
> pa_proplist_sets(sink_input_data.proplist, PA_PROP_MEDIA_NAME, "Echo-Cancel Sink Stream");
> pa_proplist_sets(sink_input_data.proplist, PA_PROP_MEDIA_ROLE, "filter");
> diff --git a/src/modules/module-ladspa-sink.c b/src/modules/module-ladspa-sink.c
> index be05715..be9d9ae 100644
> --- a/src/modules/module-ladspa-sink.c
> +++ b/src/modules/module-ladspa-sink.c
> @@ -906,7 +906,8 @@ int pa__init(pa_module*m) {
> pa_sink_input_new_data_init(&sink_input_data);
> sink_input_data.driver = __FILE__;
> sink_input_data.module = m;
> - pa_sink_input_new_data_set_sink(&sink_input_data, master, FALSE);
> + if (!pa_sink_input_new_data_set_sink(&sink_input_data, master, FALSE))
> + goto fail;
> sink_input_data.origin_sink = u->sink;
> pa_proplist_sets(sink_input_data.proplist, PA_PROP_MEDIA_NAME, "LADSPA Stream");
> pa_proplist_sets(sink_input_data.proplist, PA_PROP_MEDIA_ROLE, "filter");
> diff --git a/src/modules/module-loopback.c b/src/modules/module-loopback.c
> index 1f2ef91..20ce07f 100644
> --- a/src/modules/module-loopback.c
> +++ b/src/modules/module-loopback.c
> @@ -701,7 +701,8 @@ int pa__init(pa_module *m) {
> pa_sink_input_new_data_init(&sink_input_data);
> sink_input_data.driver = __FILE__;
> sink_input_data.module = m;
> - pa_sink_input_new_data_set_sink(&sink_input_data, sink, FALSE);
> + if (!pa_sink_input_new_data_set_sink(&sink_input_data, sink, FALSE))
> + goto fail;
>
> if (pa_modargs_get_proplist(ma, "sink_input_properties", sink_input_data.proplist, PA_UPDATE_REPLACE) < 0) {
> pa_log("Failed to parse the sink_input_properties value.");
> diff --git a/src/modules/module-remap-sink.c b/src/modules/module-remap-sink.c
> index 2822a7f..73c74ec 100644
> --- a/src/modules/module-remap-sink.c
> +++ b/src/modules/module-remap-sink.c
> @@ -416,7 +416,8 @@ int pa__init(pa_module*m) {
> pa_sink_input_new_data_init(&sink_input_data);
> sink_input_data.driver = __FILE__;
> sink_input_data.module = m;
> - pa_sink_input_new_data_set_sink(&sink_input_data, master, FALSE);
> + if (!pa_sink_input_new_data_set_sink(&sink_input_data, master, FALSE))
> + goto fail;
> sink_input_data.origin_sink = u->sink;
> pa_proplist_sets(sink_input_data.proplist, PA_PROP_MEDIA_NAME, "Remapped Stream");
> pa_proplist_sets(sink_input_data.proplist, PA_PROP_MEDIA_ROLE, "filter");
> diff --git a/src/modules/module-sine.c b/src/modules/module-sine.c
> index c6d7303..920afa1 100644
> --- a/src/modules/module-sine.c
> +++ b/src/modules/module-sine.c
> @@ -155,7 +155,8 @@ int pa__init(pa_module*m) {
> pa_sink_input_new_data_init(&data);
> data.driver = __FILE__;
> data.module = m;
> - pa_sink_input_new_data_set_sink(&data, sink, FALSE);
> + if (!pa_sink_input_new_data_set_sink(&data, sink, FALSE))
> + goto fail;
> pa_proplist_setf(data.proplist, PA_PROP_MEDIA_NAME, "%u Hz Sine", frequency);
> pa_proplist_sets(data.proplist, PA_PROP_MEDIA_ROLE, "abstract");
> pa_proplist_setf(data.proplist, "sine.hz", "%u", frequency);
> diff --git a/src/modules/module-virtual-sink.c b/src/modules/module-virtual-sink.c
> index e7c476e..c9a8cd0 100644
> --- a/src/modules/module-virtual-sink.c
> +++ b/src/modules/module-virtual-sink.c
> @@ -582,7 +582,8 @@ int pa__init(pa_module*m) {
> pa_sink_input_new_data_init(&sink_input_data);
> sink_input_data.driver = __FILE__;
> sink_input_data.module = m;
> - pa_sink_input_new_data_set_sink(&sink_input_data, master, FALSE);
> + if (!pa_sink_input_new_data_set_sink(&sink_input_data, master, FALSE))
> + goto fail;
> sink_input_data.origin_sink = u->sink;
> pa_proplist_setf(sink_input_data.proplist, PA_PROP_MEDIA_NAME, "Virtual Sink Stream from %s", pa_proplist_gets(u->sink->proplist, PA_PROP_DEVICE_DESCRIPTION));
> pa_proplist_sets(sink_input_data.proplist, PA_PROP_MEDIA_ROLE, "filter");
> diff --git a/src/modules/rtp/module-rtp-recv.c b/src/modules/rtp/module-rtp-recv.c
> index 412f4c3..bb7b8db 100644
> --- a/src/modules/rtp/module-rtp-recv.c
> +++ b/src/modules/rtp/module-rtp-recv.c
> @@ -517,7 +517,8 @@ static struct session *session_new(struct userdata *u, const pa_sdp_info *sdp_in
> goto fail;
>
> pa_sink_input_new_data_init(&data);
> - pa_sink_input_new_data_set_sink(&data, sink, FALSE);
> + if (!pa_sink_input_new_data_set_sink(&data, sink, FALSE))
> + goto fail;
> data.driver = __FILE__;
> pa_proplist_sets(data.proplist, PA_PROP_MEDIA_ROLE, "stream");
> pa_proplist_setf(data.proplist, PA_PROP_MEDIA_NAME,
> diff --git a/src/pulsecore/protocol-esound.c b/src/pulsecore/protocol-esound.c
> index 4e1c56c..8d06c93 100644
> --- a/src/pulsecore/protocol-esound.c
> +++ b/src/pulsecore/protocol-esound.c
> @@ -424,8 +424,10 @@ static int esd_proto_stream_play(connection *c, esd_proto_t request, const void
> sdata.driver = __FILE__;
> sdata.module = c->options->module;
> sdata.client = c->client;
> - if (sink)
> - pa_sink_input_new_data_set_sink(&sdata, sink, FALSE);
> + if (sink) {
> + if (!pa_sink_input_new_data_set_sink(&sdata, sink, FALSE))
> + return -PA_ERR_NOTSUPPORTED;
> + }
> pa_sink_input_new_data_set_sample_spec(&sdata, &ss);
>
> pa_sink_input_new(&c->sink_input, c->protocol->core, &sdata);
> diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c
> index cbd5fef..fc1a9aa 100644
> --- a/src/pulsecore/protocol-native.c
> +++ b/src/pulsecore/protocol-native.c
> @@ -1121,14 +1121,18 @@ static playback_stream* playback_stream_new(
> data.driver = __FILE__;
> data.module = c->options->module;
> data.client = c->client;
> - if (sink)
> - pa_sink_input_new_data_set_sink(&data, sink, TRUE);
> + if (sink) {
> + if (!pa_sink_input_new_data_set_sink(&data, sink, TRUE))
> + return -PA_ERR_NOTSUPPORTED;
> + }
> +
> if (pa_sample_spec_valid(ss))
> pa_sink_input_new_data_set_sample_spec(&data, ss);
> if (pa_channel_map_valid(map))
> pa_sink_input_new_data_set_channel_map(&data, map);
> if (formats) {
> - pa_sink_input_new_data_set_formats(&data, formats);
> + if (!pa_sink_input_new_data_set_formats(&data, formats))
> + return -PA_ERR_NOTSUPPORTED;
> /* Ownership transferred to new_data, so we don't free it ourselves */
> formats = NULL;
> }
> diff --git a/src/pulsecore/protocol-simple.c b/src/pulsecore/protocol-simple.c
> index 8d8f5b8..b62b216 100644
> --- a/src/pulsecore/protocol-simple.c
> +++ b/src/pulsecore/protocol-simple.c
> @@ -536,7 +536,8 @@ void pa_simple_protocol_connect(pa_simple_protocol *p, pa_iochannel *io, pa_simp
> data.driver = __FILE__;
> data.module = o->module;
> data.client = c->client;
> - pa_sink_input_new_data_set_sink(&data, sink, FALSE);
> + if (!pa_sink_input_new_data_set_sink(&data, sink, FALSE))
> + goto fail;
> pa_proplist_update(data.proplist, PA_UPDATE_MERGE, c->client->proplist);
> pa_sink_input_new_data_set_sample_spec(&data, &o->sample_spec);
>
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index 93caa8f..035e9e2 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -267,7 +267,8 @@ int pa_sink_input_new(
> pa_format_info *f = pa_format_info_from_sample_spec(&data->sample_spec,
> data->channel_map_is_set ? &data->channel_map : NULL);
> pa_idxset_put(tmp, f, NULL);
> - pa_sink_input_new_data_set_formats(data, tmp);
> + if (!pa_sink_input_new_data_set_formats(data, tmp))
> + return -PA_ERR_NOTSUPPORTED;
> }
>
> if ((r = pa_hook_fire(&core->hooks[PA_CORE_HOOK_SINK_INPUT_NEW], data)) < 0)
> @@ -278,7 +279,8 @@ int pa_sink_input_new(
> if (!data->sink) {
> pa_sink *sink = pa_namereg_get(core, NULL, PA_NAMEREG_SINK);
> pa_return_val_if_fail(sink, -PA_ERR_NOENTITY);
> - pa_sink_input_new_data_set_sink(data, sink, FALSE);
> + if (!pa_sink_input_new_data_set_sink(data, sink, FALSE))
> + return -PA_ERR_NOTSUPPORTED;
> }
> /* Routing's done, we have a sink. Now let's fix the format and set up the
> * sample spec */
