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

Stefano Sabatini stefasab at gmail.com
Sun Mar 11 16:38:48 CET 2012


On date Thursday 2012-03-08 15:02:10 +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 |  195 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libavutil/bprint.h |  125 +++++++++++++++++++++++++++++++++
>  5 files changed, 328 insertions(+), 3 deletions(-)
>  create mode 100644 libavutil/bprint.c
>  create mode 100644 libavutil/bprint.h
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index cd93495..fe7a73e 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -13,6 +13,9 @@ libavutil:   2011-04-18
>  
>  API changes, most recent first:
>  
> +2012-03-07 - 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 23049f6..54f420e 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                                                            \
> @@ -82,8 +84,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..72e8297
> --- /dev/null
> +++ b/libavutil/bprint.c
> @@ -0,0 +1,195 @@
> +/*
> + * 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 <stddef.h>
> +#include <stdio.h>
> +#include <string.h>

Nit+++: maybe you can remove some of these includes, not that I care much...

> +#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_grow(AVBPrint *buf, unsigned new_size)
> +{
> +    char *old_str, *new_str;
> +
> +    if (buf->size == buf->size_max)
> +        return AVERROR(EIO);
> +    if (!av_bprint_is_complete(buf))
> +        return AVERROR_INVALIDDATA; /* it is already truncated anyway */
> +    if (!new_size)
> +        new_size = buf->size > buf->size_max / 2 ? buf->size_max :
> +                                                   buf->size * 2;
> +    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;
> +}
> +
> +void av_bprint_init(AVBPrint *buf, unsigned size_init, unsigned size_max)
> +{
> +    unsigned asize = (char *)buf + sizeof(*buf) - buf->reserved_internal_buffer;
> +
> +    if (size_max == 1)
> +        size_max = asize;

Nit++: maybe asize -> a more descriptive name, such as available_size
or automatic_size or a feasible abbreviation.

[...]
> +int main(void)
> +{
> +    AVBPrint b;
> +
> +    av_bprint_init(&b, 0, -1);
> +    bprint_pascal(&b, 5);
> +    printf("Short text in unlimited buffer: %zu/%u\n", strlen(b.str), b.len);
> +    printf("%s\n", b.str);
> +    av_bprint_finalize(&b, NULL);
> +
> +    av_bprint_init(&b, 0, -1);
> +    bprint_pascal(&b, 25);
> +    printf("Long text in unlimited buffer: %zu/%u\n", strlen(b.str), b.len);
> +    av_bprint_finalize(&b, NULL);

You may add another example for max_size = 0:

   av_bprint_init(&b, 0, 0);
   bprint_pascal(&b, 25);
   printf("Long text in empty buffer: %zu/%u\n", strlen(b.str), b.len);
   av_bprint_finalize(&b, NULL);

> +
> +    av_bprint_init(&b, 0, 2048);
> +    bprint_pascal(&b, 25);
> +    printf("Long text in limited buffer: %zu/%u\n", strlen(b.str), b.len);
> +    av_bprint_finalize(&b, NULL);
> +
> +    av_bprint_init(&b, 0, 1);
> +    bprint_pascal(&b, 5);
> +    printf("Short text in automatic buffer: %zu/%u\n", strlen(b.str), b.len);
> +
> +    av_bprint_init(&b, 0, 1);
> +    bprint_pascal(&b, 25);
> +    printf("Long text in automatic buffer: %zu/%u\n", strlen(b.str), b.len);
> +    /* Note that the size of the automatic buffer is arch-dependant. */
> +
> +    return 0;
> +}
> +
> +#endif
> diff --git a/libavutil/bprint.h b/libavutil/bprint.h
> new file mode 100644
> index 0000000..eab98d4
> --- /dev/null
> +++ b/libavutil/bprint.h
[...]
> + * Append operations do not need to be tested for failure: if a memory
> + * allocation fails, data stop being appended to the buffer, but the length
> + * is still updated. This situation can be tested with
> + * av_bprint_is_complete().
> + *
> + * The size_max field determines several possible behaviours:
> + *
> + * size_max = -1 (= UINT_MAX) or any large value will let the buffer be
> + * reallocated as necessary, with an amortized linear cost.
> + *
> + * size_max = 0 prevents writing anything to the buffer: only the total
> + * length is computed. The write operations can then possibly be repeated in
> + * a buffer with exactly the necessary size
> + * (using size_init = size_max = len + 1).
> + *
> + * size_max = 1 is automatically replaced by the exact size available in the
> + * structure itself, thus ensuring no dynamic memory allocation. The
> + * internal buffer is large enough to hold a reasonable paragraph of text,
> + * such as the current paragraph.
> + */
> +typedef struct AVBPrint {
> +    FF_PAD_STRUCTURE(1024,
> +    char *str;         /** string so far; or NULL if size == 0 */
> +    unsigned len;      /** length so far */
> +    unsigned size;     /** allocated memory */
> +    unsigned size_max; /** maximum allocated memory */
> +    char reserved_internal_buffer[1];
> +    )
> +} AVBPrint;
> +
> +/**
> + * Init a print buffer.
> + *
> + * @param buf        buffer to init
> + * @param size_init  initial size (including the final 0)
> + * @param size_max   maximum size;
> + *                   0 means do not write anything, just count the length;
> + *                   1 is replaced by the maximum value for automatic storage
> + */
> +void av_bprint_init(AVBPrint *buf, unsigned size_init, unsigned size_max);

Nit: init_size and max_size sound more correct to my
non-native-English-speacker ears.

Elaborating more on what I told the other time, using symbolic
constants may help readability:

av_bprint_init(&b, 0, -1) -> av_bprint_init(&b, 0, AV_BPRINT_UNLIMITED_SIZE)
av_bprint_init(&b, 0,  1) -> av_bprint_init(&b, 0, AV_BPRINT_AUTOMATIC_SIZE)
av_bprint_init(&b, 0,  0) -> av_bprint_init(&b, 0, AV_BPRINT_NULL_SIZE)

so people won't have to check the docs.

I agree that using a distinct parameter for the "buffering/appending
mode" may increase API complexity too much, so I'm not sure that was a
good idea.

LGTM otherwise.
-- 
FFmpeg = Fundamentalist and Fierce Mystic Portable EnGine


More information about the ffmpeg-devel mailing list