[FFmpeg-devel] [PATCH 2/3] audioconvert: add av_get_standard_channel_layout().

[Stefano Sabatini]

On date Monday 2012-07-30 00:40:08 +0200, Nicolas George encoded:
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  doc/APIchanges           |    3 +++
>  libavutil/audioconvert.c |   29 +++++++++++++++++++++++++++++
>  libavutil/audioconvert.h |   16 ++++++++++++++++
>  libavutil/version.h      |    2 +-
>  4 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 9203cee..a3c9203 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2011-04-18
>  
>  API changes, most recent first:
>  
> +2012-07-30 - xxxxxxx - lavu 51.66.100
> +  Add av_get_standard_channel_layout() function.
> +
>  2012-07-20 - xxxxxxx - lavc 54.43.100
>    Add decode_error_flags field to AVFrame.
>  
> diff --git a/libavutil/audioconvert.c b/libavutil/audioconvert.c
> index 7010144..4937a60 100644
> --- a/libavutil/audioconvert.c
> +++ b/libavutil/audioconvert.c
> @@ -232,3 +232,32 @@ uint64_t av_channel_layout_extract_channel(uint64_t channel_layout, int index)
>      }
>      return 0;
>  }
> +
> +int av_get_standard_channel_layout(unsigned index, uint64_t *layout,
> +                                   const char **short_name,
> +                                   const char **long_name)
> +{
> +    uint64_t l = 0;
> +    const char *sn = NULL, *ln = NULL;
> +    int ret = 0;
> +
> +    if (index < 64) {
> +        l = (uint64_t)1 << index;
> +        if (index < FF_ARRAY_ELEMS(channel_names)) {
> +            sn = channel_names[index].short_name;
> +            ln = channel_names[index]. long_name;
> +        }
> +    } else {
> +        index -= 64;
> +        if (index < FF_ARRAY_ELEMS(channel_layout_map)) {
> +            l  = channel_layout_map[index].layout;
> +            sn = channel_layout_map[index].name;
> +        } else {

> +            ret = AVERROR_EOF;

EINVAL is an alternative, but may be awkward for a different reason.

> +        }
> +    }
> +    if (layout)     *layout     = l;
> +    if (short_name) *short_name = sn;
> +    if (long_name)  * long_name = ln;

nit: I would prefer to directly set the values in the above code and
avoid the slightly obfuscating temporaries l/sn/ln

> +    return ret;
> +}
> diff --git a/libavutil/audioconvert.h b/libavutil/audioconvert.h
> index 25e9cf0..a6d5fd6 100644
> --- a/libavutil/audioconvert.h
> +++ b/libavutil/audioconvert.h
> @@ -181,6 +181,22 @@ uint64_t av_channel_layout_extract_channel(uint64_t channel_layout, int index);
>  const char *av_get_channel_name(uint64_t channel);
>  
>  /**
> + * Get the value, name and description of standard channel layouts.
> + *
> + * @param[in]  index       index in the list; indices between 0 and 63 are
                                           ^^^^
in which list?

> + *                         individual channels

I suggest: "index in an internal list containing common channel
layouts information"

> + * @param[out] layout      channel layout mask
> + * @param[out] short_name  short name of the layout, NULL for unaffected
> + *                         individual channels
> + * @param[out] long_name   long name of the layout, or NULL

> + * @return  0  if the layout exists (in the broad sense),
                                        ^^^^^^^^^^^^^^^^^^

Please explicate what this means.

> + *          <0 if index is beyond the limits
> + */
> +int av_get_standard_channel_layout(unsigned index, uint64_t *layout,
> +                                   const char **short_name,
> +                                   const char **long_name);

As an alternative we may consider having two separate functions:

int av_get_standard_channel_layout(unsigned index, uint64_t *layout, const char **name);
int av_get_standard_channel(unsigned index, const char **short_name, const char **long_name);

This sounds less confusing to the user, so I tend to favor this second
option.
-- 
FFmpeg = Faithful and Fierce Monstrous Programmable Egregious Glue