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

[Tanu Kaskinen]

On Wed, 2013-12-18 at 22:56 +0100, Peter Meerwald wrote:
> 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

OK, fair enough. I like to have an empty space before every if
statement, but it's true that it would be more consistent with existing
code to not have the empty line.

> > +    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

My principle with documenting error codes is that if I know that someone
wants to check for a certain error code (like I want to check for
PA_ERR_NOENTITY in this case), then I document that, otherwise I leave
error codes undocumented, because documenting them limits the freedom to
change the function implementation later (in this case it doesn't matter
so much, because it's not a public API, but I still like to follow the
same principle).

> > +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?

I prefer to check all pointers in fail sections. It brings some safety
against later changes the main function body. It's often very easy to
break the fail section if the pointers aren't checked, by reordering the
code a bit or by adding a new "goto fail" statement.

However, following this practice is only useful if the checked pointers
are also initialized to NULL at declaration time, otherwise the code
will break anyway due to accessing an uninitialized variable. It seems
that I don't initialize format to NULL (will fix).

> (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."

OK, that's definitely better, will fix.

> (5) there still is a FIXME in patch 17

That's intentional. The FIXME item is not a regression, the problem was
there already before. I didn't have the motivation to fix the issue, so
I added the FIXME note and also filed a bug about it.

> patches look fine to me (but I'm not too confident in my capacity to 
> review these)

Thanks for the review!

-- 
Tanu