Hello Tanu,
I had a look at the patches, I only have very minor comments
(1) as a matter of style, in most places in PA there is no newline between
an assignment and the conditional statement checking the assigned value
> + r = pa_format_info_get_prop_string(f, PA_PROP_FORMAT_CHANNEL_MAP, &map_str);
> +
I'd rather not put a newline here (and many other places)
> + if (r < 0)
(2) document both error codes?
> +/* Gets the channel map stored in the format info. Returns a negative error
> + * code on failure. If the channel map property is not set at all, returns
> + * -PA_ERR_NOENTITY. */
PA_ERR_INVALID would be the other possible error code
> +int pa_format_info_get_channel_map(pa_format_info *f, pa_channel_map *map);
(3) in pa_format_info_from_sample_spec2():
> +fail:
> + if (format)
> + pa_format_info_free(format);
format is guaranteed to be !NULL; shall it nevertheless be checked?
(4)
> +int pa_stream_set_volume_channel_map(pa_stream *s, const pa_channel_map *map) {
> + pa_assert(s);
> + pa_assert(map);
> + pa_assert(pa_channel_map_valid(map));
> +
> + if (s->state != PA_STREAM_UNCONNECTED) {
> + pa_log_debug("Stream state is not unconnected.");
'not unconnected' is kind of negative :)
how about "Stream state 'unconnected' required/expected."
(5) there still is a FIXME in patch 17
patches look fine to me (but I'm not too confident in my capacity to
review these)
regards, p.
> Changes in v2:
> - Support clients that want to set the stream volume while at the
> same leaving the stream channel map unspecified.
> - Support setting mono volume even if the stream has multiple
> channels (the volume is copied to all channels).
> - Constify some format info related function parameters.
> - More error logging.
> - Removed sample spec and channel map validity checking from
> pa_format_info_from_sample_spec2(). It doesn't really make sense
> to prepare everywhere for invalid sample specs and channel maps.
> Preparing for that should be a special occasion, such as
> receiving those structs directly from an untrusted source (i.e.
> clients).
> - Removed a volume validity check from create_stream() in
> pulse/stream.c.
>
> Some words about supporting clients that use pa_stream_new_extended()
> and want to set the volume while also leaving the stream channel map
> unspecified... The proposed solution is to add new function
> pa_stream_set_volume_channel_map(). Another possibility would have
> been to set the volume channel map as a format info property. I don't
> like that, because the volume itself is not specific to one format
> info, so why should the volume channel map be? Also, the volume has
> nothing to do with the stream format, so putting the channel map to
> a format info would mean basically abusing the extensible format info
> structure for passing arbitrary extra parameters for the stream.
>
> Tanu Kaskinen (18):
> Move pa_format_info_to_sample_spec_fake() to core-format
> core-format: Add pa_format_info_get_sample_format()
> core-format: Add pa_format_info_get_rate()
> core-format: Add pa_format_info_get_channels()
> core-format: Add pa_format_info_get_channel_map()
> format: Simplify pa_format_info_to_sample_spec()
> core-format: Add pa_format_info_to_sample_spec2()
> core-format: Add pa_format_info_from_sample_spec2()
> sink-input, source-output: Do routing related validity checks
> immediately after routing
> format, core-format: Constify some function parameters
> stream-util: Add pa_stream_get_volume_channel_map()
> stream: Remove a volume channel validity check
> sink-input, source-output: Interpret missing PCM parameters in format
> info as a request to decide those parameters at the server end
> stream: Add pa_stream_set_volume_channel_map()
> def, format: Document how to leave PCM parameters to be decided by the
> server
> stream: Improve pa_stream_connect_playback() documentation
> stream: Mention pa_stream_new_extended() in the high-level stream
> creation documentation
> format: Add some error logging
>
> src/Makefile.am | 2 +
> src/map-file | 1 +
> src/pulse/def.h | 38 ++++++-
> src/pulse/format.c | 116 ++++++++------------
> src/pulse/format.h | 74 ++++++++++---
> src/pulse/internal.h | 1 +
> src/pulse/stream.c | 29 ++++-
> src/pulse/stream.h | 53 ++++++++-
> src/pulsecore/core-format.c | 250 ++++++++++++++++++++++++++++++++++++++++++
> src/pulsecore/core-format.h | 81 ++++++++++++++
> src/pulsecore/sink-input.c | 104 ++++++++----------
> src/pulsecore/source-output.c | 99 +++++++----------
> src/pulsecore/stream-util.c | 87 +++++++++++++++
> src/pulsecore/stream-util.h | 50 +++++++++
> 14 files changed, 770 insertions(+), 215 deletions(-)
> create mode 100644 src/pulsecore/core-format.c
> create mode 100644 src/pulsecore/core-format.h
> create mode 100644 src/pulsecore/stream-util.c
> create mode 100644 src/pulsecore/stream-util.h
>
>
--
Peter Meerwald
+43-664-2444418 (mobile)
