[FFmpeg-devel] [PATCH 1/3] lavu: add helper functions for integer lists.

[Stefano Sabatini]

On date Wednesday 2013-04-10 11:05:23 +0200, Nicolas George encoded:
> Add av_int_list_length() to compute a list length.
> Add av_opt_set_int_list() to set a binary option.
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  doc/APIchanges      |    4 ++++
>  libavutil/avutil.h  |   19 +++++++++++++++++++
>  libavutil/opt.h     |   12 ++++++++++++
>  libavutil/utils.c   |   19 +++++++++++++++++++
>  libavutil/version.h |    2 +-
>  5 files changed, 55 insertions(+), 1 deletion(-)
> 
> 
> Rebased on top of current head.
> 
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 01f7825..88c4c4a 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil:     2012-10-22
>  
>  API changes, most recent first:
>  
> +2013-04-10 - xxxxxxx - lavu 25.26.100 - avutil.h,opt.h
> +  Add av_int_list_length()
> +  and av_opt_set_int_list().

Nit+++: no need to put them on separate lines.

> +
>  2013-03-30 - xxxxxxx - lavu 52.24.100 - samplefmt.h
>    Add av_samples_alloc_array_and_samples().
>  
> diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> index 78deff1..54bbd9d 100644
> --- a/libavutil/avutil.h
> +++ b/libavutil/avutil.h
> @@ -253,6 +253,25 @@ static inline void *av_x_if_null(const void *p, const void *x)
>  }
>  
>  /**
> + * Compute the length of an integer list.
> + * @param elsize  size of the list elements (1, 2, 4 or 8)

Nit: empty line between brief and first @param is customary and helps
readability.

Docs for elsize are a bit ambiguous, and the list is confusing (only
those sizes are accepted, also "size of the list elements" could be
interpreted as "size of (the list elements)" rather than "size of each
list element".

I suggest:
size in bytes of each list element

and then specify if it only accepts a limited number of sizes (I still
didn't read the code though, so I may be wrong)

> + * @param term    list terminator (usually 0 or -1)
> + * @param list    pointer to the list
> + * @return  length of the list, in elements, not counting the terminator
> + */
> +unsigned av_int_list_length_for_size(unsigned elsize,
> +                                     const void *list, uint64_t term);

Maybe av_int_list_get_length_for_size(), but feel free to discard this
comment if you prefer the short form.

> +
> +/**
> + * Compute the length of an integer list.
> + * @param term  list terminator (usually 0 or -1)

Nit: empty line before @param, here and below

> + * @param list  pointer to the list
> + * @return  length of the list, in elements, not counting the terminator
> + */
> +#define av_int_list_length(list, term) \
> +    av_int_list_length_for_size(sizeof(*list), list, term)
> +
> +/**
>   * @}
>   * @}
>   */
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index 30f729e..6803a74 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -656,6 +656,18 @@ int av_opt_set_image_size(void *obj, const char *name, int w, int h, int search_
>  int av_opt_set_pixel_fmt (void *obj, const char *name, enum AVPixelFormat fmt, int search_flags);
>  int av_opt_set_sample_fmt(void *obj, const char *name, enum AVSampleFormat fmt, int search_flags);
>  int av_opt_set_video_rate(void *obj, const char *name, AVRational val, int search_flags);
> +
> +/**
> + * Set a binary option to an integer list.
> + * @param obj    object to set the option

 * @param obj       an AVClass object to set options on

> + * @param name   name of the option (must be binary)

name of the binary option, or drop the (...) which may be read as "the
name must be binary"

> + * @param val    pointer to an integer list with the correct type

what is the correct type?

> + * @param term   list terminator (usually 0 or -1)
> + * @param flags  search flags
> + */
> +#define av_opt_set_int_list(obj, name, val, term, flags) \
> +    av_opt_set_bin(obj, name, (const uint8_t *)val, \
> +                   av_int_list_length(val, term) * sizeof(*val), flags)

Don't you need to count the terminator as well?

>  /**
>   * @}
>   */
> diff --git a/libavutil/utils.c b/libavutil/utils.c
> index fbfbc49..291c736 100644
> --- a/libavutil/utils.c
> +++ b/libavutil/utils.c
> @@ -79,3 +79,22 @@ char av_get_picture_type_char(enum AVPictureType pict_type)
>      default:                 return '?';
>      }
>  }
> +
> +unsigned av_int_list_length_for_size(unsigned elsize,
> +                                     const void *list, uint64_t term)
> +{
> +    unsigned i;
> +
> +    if (!list)
> +        return 0;
> +#define LIST_LENGTH(type) \
> +    { type t = term, *l = list; for (i = 0; l[i] != t; i++); }
> +    switch (elsize) {
> +    case 1: LIST_LENGTH(uint8_t);  break;
> +    case 2: LIST_LENGTH(uint16_t); break;
> +    case 4: LIST_LENGTH(uint32_t); break;
> +    case 8: LIST_LENGTH(uint64_t); break;
> +    default: av_assert0(!"valid element size");
> +    }
> +    return i;
> +}
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 7d1ab9c..6531397 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -75,7 +75,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  52
> -#define LIBAVUTIL_VERSION_MINOR  25
> +#define LIBAVUTIL_VERSION_MINOR  26
>  #define LIBAVUTIL_VERSION_MICRO 100

Optionally, you may add an example in opt-test.

Nice idea.
-- 
FFmpeg = Friendly and Foolish Most Peaceless Elitist Gorilla