[FFmpeg-devel] [PATCH] lavfi: support unknown channel layouts.

[Stefano Sabatini]

On date Friday 2013-01-04 16:33:03 +0100, Nicolas George encoded:
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  libavfilter/audio.c         |    2 +-
>  libavfilter/avcodec.c       |    3 --
>  libavfilter/avfiltergraph.c |   80 +++++++++++++++++++++++++++++++++++++++----
>  libavfilter/formats.c       |   80 ++++++++++++++++++++++++++++++++++++++-----
>  libavfilter/formats.h       |   31 +++++++++++++++++
>  5 files changed, 177 insertions(+), 19 deletions(-)
> 
> 
> Updated according to Stefano's comments.
[...] 
> @@ -663,7 +720,18 @@ static void swap_channel_layouts_on_filter(AVFilterContext *filter)
>              int out_channels      = av_get_channel_layout_nb_channels(out_chlayout);
>              int count_diff        = out_channels - in_channels;
>              int matched_channels, extra_channels;
> -            int score = 0;
> +            int score = 100000;
> +

> +            if (FF_LAYOUT2COUNT(in_chlayout) || FF_LAYOUT2COUNT(out_chlayout)) {
> +                if (FF_LAYOUT2COUNT(in_chlayout))
> +                    in_channels = FF_LAYOUT2COUNT(in_chlayout);
> +                if (FF_LAYOUT2COUNT(out_chlayout))
> +                    out_channels = FF_LAYOUT2COUNT(out_chlayout);
> +                score -= 10000 + FFABS(out_channels - in_channels) +
> +                         (in_channels > out_channels ? 10000 : 0);
> +                /* let the remaining computation run and get 0 */
> +                in_chlayout = out_chlayout = 0;
> +            }

I'm still confused by this. This is computing a score in case in or
out channel count is defined, then layouts are set to unknown. What is
this "run and get 0"? Also why to run the computation if you already
know the result? I can't propose a better wording since I don't know
what the comment is trying to address in the first place, I'm stating
that it is confusing since it is confusing at least *me*.

[...]
> @@ -69,15 +69,46 @@ struct AVFilterFormats {
>      struct AVFilterFormats ***refs; ///< references to this list
>  };
>  
> +/**
> + * A list of supported channel layouts.
> + *
> + * The list works the same as AVFilterFormats, except for the following
> + * differences:
> + * - A list with all_layouts = 1 means all channel layouts with a known
> + *   disposition; nb_channel_layouts must then be 0.
> + * - A list with all_counts = 1 means all channel counts, with a known or
> + *   unknown disposition ; nb_channel_layouts must then be 0 and all_layouts 1.
> + * - The list must not contain a layout with a known disposition and a
> + *   channel count with unknown disposition with the same number of channels
> + *   (e.g. AV_CH_LAYOUT_STEREO and FF_COUNT2LAYOUT(2).
> + */
>  typedef struct AVFilterChannelLayouts {
>      uint64_t *channel_layouts;  ///< list of channel layouts
>      int    nb_channel_layouts;  ///< number of channel layouts
> +    char all_layouts;           ///< accept any known channel layout
> +    char all_counts;            ///< accept any channel layout or count
>  
>      unsigned refcount;          ///< number of references to this list
>      struct AVFilterChannelLayouts ***refs; ///< references to this list
>  } AVFilterChannelLayouts;
>  

>  /**
> + * Encode a channel count as a channel layout.
> + * FF_COUNT2LAYOUT(c) means any channel layout with c channels, with a known
> + * or unknown disposition.
> + * The result is only valid inside AVFilterChannelLayouts and immediately
> + * related functions.
> + */
> +#define FF_COUNT2LAYOUT(c) (0x8000000000000000ULL | (c))
> +
> +/**
> + * Decode a channel count encoded as a channel layout.
> + * Return 0 if the channel layout was a real one.
> + */

> +#define FF_LAYOUT2COUNT(l) (((l) & 0x8000000000000000ULL) ? \
> +                           (int)((l) & 0x7FFFFFFF) : 0)

hopefully sizeof(int) == 4

[...]

I have no more comments on this patch. The logic is quite complex and
I'm a bit unconfortable with the heuristics choosen in
swap_channel_layouts, but I don't think this should block inclusion if
it doesn't break anything/passes FATE and does what it is meant to do.

Thanks for working on this.
-- 
FFmpeg = Fostering & Foolish Mortal Portable Energized Geisha