[pulseaudio-discuss] [PATCH v2 00/18] Format negotiation fixing

[Peter Meerwald]

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)