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

[Stefano Sabatini]

On date Wednesday 2013-01-02 17:28:33 +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 |   78 ++++++++++++++++++++++++++++++++++++++----
>  libavfilter/formats.c       |   79 ++++++++++++++++++++++++++++++++++++++-----
>  libavfilter/formats.h       |   31 +++++++++++++++++
>  5 files changed, 174 insertions(+), 19 deletions(-)
> 
> 
> Updated according to Stefano's comments.
> The next patches are unchanged apart from a macro rename.
> 
> 
> diff --git a/libavfilter/audio.c b/libavfilter/audio.c
> index 72dcd14..c72979d 100644
> --- a/libavfilter/audio.c
> +++ b/libavfilter/audio.c
> @@ -44,7 +44,7 @@ AVFilterBufferRef *ff_default_get_audio_buffer(AVFilterLink *link, int perms,
>      AVFilterBufferRef *samplesref = NULL;
>      uint8_t **data;
>      int planar      = av_sample_fmt_is_planar(link->format);
> -    int nb_channels = av_get_channel_layout_nb_channels(link->channel_layout);
> +    int nb_channels = link->channels;
>      int planes      = planar ? nb_channels : 1;
>      int linesize;
>      int full_perms = AV_PERM_READ | AV_PERM_WRITE | AV_PERM_PRESERVE |
> diff --git a/libavfilter/avcodec.c b/libavfilter/avcodec.c
> index 0c1f02a..2343d19 100644
> --- a/libavfilter/avcodec.c
> +++ b/libavfilter/avcodec.c
> @@ -96,9 +96,6 @@ AVFilterBufferRef *avfilter_get_audio_buffer_ref_from_frame(const AVFrame *frame
>      int channels = av_frame_get_channels(frame);
>      int64_t layout = av_frame_get_channel_layout(frame);
>  
> -    if(av_frame_get_channels(frame) > 8) // libavfilter does not suport more than 8 channels FIXME, remove once libavfilter is fixed
> -        return NULL;
> -
>      if (layout && av_get_channel_layout_nb_channels(layout) != av_frame_get_channels(frame)) {
>          av_log(0, AV_LOG_ERROR, "Layout indicates a different number of channels than actually present\n");
>          return NULL;
> diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
> index 10f9b7e..cfee00d 100644
> --- a/libavfilter/avfiltergraph.c
> +++ b/libavfilter/avfiltergraph.c
> @@ -185,9 +185,24 @@ AVFilterContext *avfilter_graph_get_filter(AVFilterGraph *graph, char *name)
>      return NULL;
>  }
>  
> +static void sanitize_channel_layouts(void *log, AVFilterChannelLayouts *l)
> +{
> +    if (!l)
> +        return;
> +    if (l->nb_channel_layouts) {
> +        if (l->all_layouts || l->all_counts)
> +            av_log(log, AV_LOG_WARNING, "All layouts set on non-empty list\n");
> +        l->all_layouts = l->all_counts = 0;
> +    } else {
> +        if (l->all_counts && !l->all_layouts)
> +            av_log(log, AV_LOG_WARNING, "All counts without all layouts\n");
> +        l->all_layouts = 1;
> +    }
> +}
> +
>  static int filter_query_formats(AVFilterContext *ctx)
>  {
> -    int ret;
> +    int ret, i;
>      AVFilterFormats *formats;
>      AVFilterChannelLayouts *chlayouts;
>      AVFilterFormats *samplerates;
> @@ -201,6 +216,11 @@ static int filter_query_formats(AVFilterContext *ctx)
>          return ret;
>      }
>  
> +    for (i = 0; i < ctx->nb_inputs; i++)
> +        sanitize_channel_layouts(ctx, ctx->inputs[i]->out_channel_layouts);
> +    for (i = 0; i < ctx->nb_outputs; i++)
> +        sanitize_channel_layouts(ctx, ctx->outputs[i]->in_channel_layouts);
> +
>      formats = ff_all_formats(type);
>      if (!formats)
>          return AVERROR(ENOMEM);
> @@ -470,7 +490,7 @@ static int pick_format(AVFilterLink *link, AVFilterLink *ref)
>          link->in_samplerates->format_count = 1;
>          link->sample_rate = link->in_samplerates->formats[0];
>  
> -        if (!link->in_channel_layouts->nb_channel_layouts) {
> +        if (link->in_channel_layouts->all_layouts) {
>              av_log(link->src, AV_LOG_ERROR, "Cannot select channel layout for"
>                     "the link between filters %s and %s.\n", link->src->name,
>                     link->dst->name);

Again I think a check (!all && count == 0) would be safer/more
readable (see below).

> @@ -478,7 +498,10 @@ static int pick_format(AVFilterLink *link, AVFilterLink *ref)
>          }
>          link->in_channel_layouts->nb_channel_layouts = 1;
>          link->channel_layout = link->in_channel_layouts->channel_layouts[0];
> -        link->channels = av_get_channel_layout_nb_channels(link->channel_layout);

> +        if ((link->channels = FF_LAYOUT2CHANS(link->channel_layout)))
> +            link->channel_layout = 0;
> +        else
> +            link->channels = av_get_channel_layout_nb_channels(link->channel_layout);

Nit: longer but imo more readable:
if (UNKNOWN(link->channel_layout)) {
    link->channel_layout = 0;
    link->channels = FF_LAYOUT2CHANS(link->channel_layout);
} else {
    link->channels = av_get_channel_layout_nb_channels(link->channel_layout);
}

[...]

>  int ff_add_channel_layout(AVFilterChannelLayouts **l, uint64_t channel_layout)
>  {
>      ADD_FORMAT(l, channel_layout, uint64_t, channel_layouts, nb_channel_layouts);
> +    (*l)->all_layouts = (*l)->all_counts = 0;
> +    return 0;
>  }

I don't like this, to me all+1 = all. If I understand the logic
correctly, AVFilterFormats now adopts the convenction all = none, and
adopts one or the other interpretation according to the context (which
calls for bug and confused code). Since AVFilterChannelLayouts allows
to distinguish between all and none we should make the distinction
effective.

[...]

I'll review the rest of the patch later, sorry for the many
iterations.
-- 
FFmpeg = Fabulous and Forgiving Mournful Puristic Everlasting God