[FFmpeg-devel] [RFC] Change av_get_channel_layout() syntax

[Michael Niedermayer]

On Fri, Oct 04, 2013 at 05:05:38PM +0200, Stefano Sabatini wrote:
> On date Friday 2013-10-04 12:29:48 +0200, Stefano Sabatini encoded:
> > Hi,
> > 
> > right now we set channel layouts in swr/resample specifying the
> > channel layout (as an integer code).
> > 
> > In order to switch to symbolic names, we need to switch to a new
> > option type for channel layout, while retaining compatibility.
> > 
> > To retain compatibility we need to make av_get_channel_layout() [1]
> > accept an integer as a channel layout mask, rather than as a number of
> > channels.
> > 
> > I propose to force the "number of channels" interpretation only when a
> > "c" is appended to the number (and maybe also accept "N
> > channels").
> > 
> > This will break syntax (alternatively: we break syntax for aresample,
> > or we break syntax at the next lavu major bump, but seems quite
> > overkill).
> > 
> > [1] http://ffmpeg.org/pipermail/ffmpeg-devel/2011-November/116452.html
> 
> Untested attempt in attachment - also delievering a notable example of
> backward-compatibility stunt.
> 
> ff_get_channel_layout() with compat=0 will be used in the opt
> handlers.
> -- 
> FFmpeg = Fundamentalist & Fanciful MultiPurpose Embarassing Geisha

>  channel_layout.c |   21 ++++++++++++++++++---
>  channel_layout.h |    9 +++++++--
>  internal.h       |    5 +++++
>  version.h        |    3 +++
>  4 files changed, 33 insertions(+), 5 deletions(-)
> 43b5009e71e91a5be2e253a05123d294f888ccfd  0004-lavu-channel_layout-change-av_get_channel_layout-beh.patch
> From 1743fb2c469f71fc2a37a50a15e0bbcd9174c441 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefasab at gmail.com>
> Date: Fri, 4 Oct 2013 15:20:50 +0200
> Subject: [PATCH] lavu/channel_layout: change av_get_channel_layout() behavior
>  at the next bump
> 
> This is done introducing a compatibility option. The new syntax is
> preferred since it allows backward syntax compatibility with libswr when
> switching to the new option handling code with
> AV_OPT_TYPE_CHANNEL_LAYOUT.
> 
> With the new parser the string:
> 1234
> 
> is interpreted as a channel layout mask, rather than as a number of
> channels, and thus it's compatible with the current way to set a channel
> layout as an integer (for the icl and ocl options) making use of integer
> option values.
> 
> ff_get_channel_layout() with compat=0 will be used in the
> AV_OPT_TYPE_CHANNEL handler code.
> ---
>  libavutil/channel_layout.c | 21 ++++++++++++++++++---
>  libavutil/channel_layout.h |  9 +++++++--
>  libavutil/internal.h       |  5 +++++
>  libavutil/version.h        |  3 +++
>  4 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
> index e582760..2e50ddc 100644
> --- a/libavutil/channel_layout.c
> +++ b/libavutil/channel_layout.c
> @@ -103,7 +103,8 @@ static const struct {
>      { "downmix",     2,  AV_CH_LAYOUT_STEREO_DOWNMIX, },
>  };
>  
> -static uint64_t get_channel_layout_single(const char *name, int name_len)
> +/* drop compat argument at the next bump */
> +static uint64_t get_channel_layout_single(const char *name, int name_len, int compat)
>  {
>      int i;
>      char *end;
> @@ -119,17 +120,24 @@ static uint64_t get_channel_layout_single(const char *name, int name_len)
>              strlen(channel_names[i].name) == name_len &&
>              !memcmp(channel_names[i].name, name, name_len))
>              return (int64_t)1 << i;
> +
> +    /* parse argument as a code or as a number of channels */
>      i = strtol(name, &end, 10);
> -    if (end - name == name_len ||
> +    if ((compat && end - name == name_len) ||
>          (end + 1 - name == name_len && *end  == 'c'))
>          return av_get_default_channel_layout(i);
>      layout = strtoll(name, &end, 0);
>      if (end - name == name_len)
>          return FFMAX(layout, 0);
> +
>      return 0;
>  }
>  
> +#if FF_API_GET_CHANNEL_LAYOUT_COMPAT
> +uint64_t ff_get_channel_layout(const char *name, int compat)
> +#else
>  uint64_t av_get_channel_layout(const char *name)
> +#endif
>  {
>      const char *n, *e;
>      const char *name_end = name + strlen(name);
> @@ -137,7 +145,7 @@ uint64_t av_get_channel_layout(const char *name)
>  
>      for (n = name; n < name_end; n = e + 1) {
>          for (e = n; e < name_end && *e != '+' && *e != '|'; e++);
> -        layout_single = get_channel_layout_single(n, e - n);
> +        layout_single = get_channel_layout_single(n, e - n, compat);
>          if (!layout_single)
>              return 0;
>          layout |= layout_single;
> @@ -145,6 +153,13 @@ uint64_t av_get_channel_layout(const char *name)
>      return layout;
>  }
>  
> +#if FF_API_GET_CHANNEL_LAYOUT_COMPAT
> +uint64_t av_get_channel_layout(const char *name)
> +{
> +    return ff_get_channel_layout(name, 1);
> +}
> +#endif
> +
>  void av_bprint_channel_layout(struct AVBPrint *bp,
>                                int nb_channels, uint64_t channel_layout)
>  {
> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
> index 2906098..0f3eb4e 100644
> --- a/libavutil/channel_layout.h
> +++ b/libavutil/channel_layout.h
> @@ -132,11 +132,16 @@ enum AVMatrixEncoding {
>   *   SL, SR, TC, TFL, TFC, TFR, TBL, TBC, TBR, DL, DR);
>   * - a number of channels, in decimal, optionally followed by 'c', yielding
>   *   the default channel layout for that number of channels (@see
> - *   av_get_default_channel_layout);
> + *   av_get_default_channel_layout());
> + *   Note: the use of the following character 'c' to specify a number
> + *   of channels will be required since the next major bump to make
> + *   the syntax not ambiguous.
>   * - a channel layout mask, in hexadecimal starting with "0x" (see the
>   *   AV_CH_* macros).

> + *   Note: since the next major bump a channel layout mask can also be
> + *   specified with a decimal number (iff not followed by "c").

maybe leave this "undefined" or deprecated or something
as its rather ugly but iam fine with it as is too

and rest LGTM

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131013/1d3109a3/attachment.asc>