[FFmpeg-devel] [PATCH] lavu: add av_bprintf and related.

[Stefano Sabatini]

On date Tuesday 2012-03-13 13:04:09 +0100, Nicolas George encoded:
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  doc/APIchanges     |    3 +
>  libavutil/Makefile |    6 +-
>  libavutil/avutil.h |    2 +-
>  libavutil/bprint.c |  206 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libavutil/bprint.h |  136 ++++++++++++++++++++++++++++++++++
>  5 files changed, 350 insertions(+), 3 deletions(-)
>  create mode 100644 libavutil/bprint.c
>  create mode 100644 libavutil/bprint.h
> 
> 
> Changes since last version:
> 
> Name change inspired by Stefano's remark: add -> extra_len.
> 
> Renamed av_bprint_grow into av_bprint_alloc; slightly changed its argument
> to make it more useful and efficient.
> 
> New av_bprint_grow to handle buf->len update and NUL-termination.
> 
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index cd93495..1dd0ee5 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -13,6 +13,9 @@ libavutil:   2011-04-18
>  
>  API changes, most recent first:
>  
> +2012-03-13 - xxxxxxx - lavu 51.43.100
> +  Add bprint.h for bprint API.
> +
>  2012-02-21 - xxxxxxx - lavc 54.4.100
>    Add av_get_pcm_codec() function.
>  
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 77d23d4..b02a1e3 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -10,6 +10,7 @@ HEADERS = adler32.h                                                     \
>            avstring.h                                                    \
>            avutil.h                                                      \
>            base64.h                                                      \
> +          bprint.h                                                      \
>            bswap.h                                                       \
>            common.h                                                      \
>            cpu.h                                                         \
> @@ -47,6 +48,7 @@ OBJS = adler32.o                                                        \
>         audioconvert.o                                                   \
>         avstring.o                                                       \
>         base64.o                                                         \
> +       bprint.o                                                         \
>         cpu.o                                                            \
>         crc.o                                                            \
>         des.o                                                            \
> @@ -81,8 +83,8 @@ OBJS-$(ARCH_PPC) += ppc/cpu.o
>  OBJS-$(ARCH_X86) += x86/cpu.o
>  
>  
> -TESTPROGS = adler32 aes avstring base64 cpu crc des eval file fifo lfg lls \
> -            md5 opt pca parseutils random_seed rational sha tree
> +TESTPROGS = adler32 aes avstring base64 bprint cpu crc des eval file fifo \
> +            lfg lls md5 opt pca parseutils random_seed rational sha tree
>  TESTPROGS-$(HAVE_LZO1X_999_COMPRESS) += lzo
>  
>  TOOLS = ffeval
> diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> index 2ec2f2e..db3bc5c 100644
> --- a/libavutil/avutil.h
> +++ b/libavutil/avutil.h
> @@ -153,7 +153,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR 51
> -#define LIBAVUTIL_VERSION_MINOR 42
> +#define LIBAVUTIL_VERSION_MINOR 43
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> diff --git a/libavutil/bprint.c b/libavutil/bprint.c
> new file mode 100644
> index 0000000..08383b2
> --- /dev/null
> +++ b/libavutil/bprint.c
> @@ -0,0 +1,206 @@
> +/*
> + * Copyright (c) 2012 Nicolas George
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include "bprint.h"
> +#include "common.h"
> +#include "error.h"
> +#include "mem.h"
> +
> +#define av_bprint_room(buf) ((buf)->size - FFMIN((buf)->len, (buf)->size))
> +#define av_bprint_is_allocated(buf) ((buf)->str != (buf)->reserved_internal_buffer)
> +
> +static int av_bprint_alloc(AVBPrint *buf, unsigned room)

nit++: _alloc -> _realloc better expresses what the function does

> +{
> +    char *old_str, *new_str;
> +    unsigned min_size, new_size;
> +
> +    if (buf->size == buf->size_max)
> +        return AVERROR(EIO);
> +    if (!av_bprint_is_complete(buf))
> +        return AVERROR_INVALIDDATA; /* it is already truncated anyway */
> +    min_size = buf->len + 1 + FFMIN(UINT_MAX - buf->len - 1, room);
> +    new_size = buf->size > buf->size_max / 2 ? buf->size_max : buf->size * 2;
> +    if (new_size < min_size)
> +        new_size = FFMIN(buf->size_max, min_size);
> +    old_str = av_bprint_is_allocated(buf) ? buf->str : NULL;
> +    new_str = av_realloc(old_str, new_size);
> +    if (!new_str)
> +        return AVERROR(ENOMEM);
> +    if (!old_str)
> +        memcpy(new_str, buf->str, buf->len + 1);
> +    buf->str  = new_str;
> +    buf->size = new_size;
> +    return 0;
> +}
> +

> +static void av_bprint_grow(AVBPrint *buf, unsigned extra_len)
> +{
> +    /* arbitrary margin to avoid small overflows */
> +    extra_len = FFMIN(extra_len, UINT_MAX - 5 - buf->len);

OK, but this is setting an hard limit on the maximum length for the
stored string (UINT_MAX-5), mentioning it in the header may be
helpful, something like:

#define AV_BPRINT_MAX_STRING_LEN ...

[...]

Either way I'm fine with the patch.
-- 
FFmpeg = Formidable & Frightening Meaningless Puristic Embarassing Guru