[FFmpeg-devel] [libav-devel] [PATCH] Change behviour of empty AVFilterFormats lists

[Stefano Sabatini]

On date Tuesday 2011-06-07 17:51:27 +0300, Mina Nagy Zaki encoded:
> This makes an empty list mean 'all formats supported'. This will simplify some 
> things for the format negotiation later on. Before the patch, returning an 
> empty list in query_formats() would simply mean no format supported, which is 
> non-sensical. This way a lot of unnecessary merging of 'all formats' lists is 
> avoided. 
> 
> 
> -- 
> Mina

> From 46347d6ded4272430a386696065af5a6f9993abb Mon Sep 17 00:00:00 2001
> From: Mina Nagy Zaki <mnzaki at gmail.com>
> Date: Tue, 7 Jun 2011 17:42:32 +0300
> Subject: [PATCH 1/2] lavfi: Make avfilter_make_format_list handle NULL lists.
> 
> ---
>  libavfilter/avfilter.h |    3 ++-
>  libavfilter/formats.c  |   13 ++++++++-----
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> index 9b1ea2d..0375a38 100644
> --- a/libavfilter/avfilter.h
> +++ b/libavfilter/avfilter.h
> @@ -233,7 +233,8 @@ typedef struct AVFilterFormats {
>   * Create a list of supported formats. This is intended for use in
>   * AVFilter->query_formats().
>   *
> - * @param fmts list of media formats, terminated by -1
> + * @param fmts list of media formats, terminated by -1. If NULL an
> + *        empty list is created.
>   * @return the format list, with no existing references
>   */
>  AVFilterFormats *avfilter_make_format_list(const int *fmts);
> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> index 101ef09..ec7fca3 100644
> --- a/libavfilter/formats.c
> +++ b/libavfilter/formats.c
> @@ -73,15 +73,18 @@ AVFilterFormats *avfilter_merge_formats(AVFilterFormats *a, AVFilterFormats *b)
>  AVFilterFormats *avfilter_make_format_list(const int *fmts)
>  {
>      AVFilterFormats *formats;
> -    int count;
> +    int count = 0;
>  
> -    for (count = 0; fmts[count] != -1; count++)
> -        ;
> +    if (fmts)
> +        for (count = 0; fmts[count] != -1; count++)
> +            ;
>  
>      formats               = av_mallocz(sizeof(AVFilterFormats));
> -    formats->formats      = av_malloc(sizeof(*formats->formats) * count);
>      formats->format_count = count;
> -    memcpy(formats->formats, fmts, sizeof(*formats->formats) * count);
> +    if (count) {
> +        formats->formats  = av_malloc(sizeof(*formats->formats) * count);
> +        memcpy(formats->formats, fmts, sizeof(*formats->formats) * count);
> +    }
>  
>      return formats;
>  }
> -- 
> 1.7.4.4

Makes sense, I'll apply it tomorrow if I see no objections.

> From 51d8dea811f2a267050d0ffa9e6521b642da363e Mon Sep 17 00:00:00 2001
> From: Mina Nagy Zaki <mnzaki at gmail.com>
> Date: Tue, 7 Jun 2011 17:43:30 +0300
> Subject: [PATCH 2/2] lavfi: Make an empty AVFilterFormats mean 'all formats'
>  supported.
> 
> This makes avfilter_all_formats unnecessary, and so it was removed.

I have to confess that I don't like this change. Indeed to me "empty
formats list" has a different meaning than "list containing all
supported formats", and at least for me it's making harder to
understand the code.

Also there are some cases (e.g. the noformat filter), where it makes
sense to actually return an empty list which means "an empty list".

So I'd prefer to see the following patches before to judge.

[...]
-- 
FFmpeg = Foolish Foolish Mere Prodigious Enchanting Gorilla