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

Clément Bœsch ubitux at gmail.com
Sat Feb 18 18:08:27 CET 2012


On Sat, Feb 18, 2012 at 03:56:48PM +0100, Nicolas George wrote:
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  libavutil/avstring.c |  105 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  libavutil/avstring.h |   60 ++++++++++++++++++++++++++++
>  libavutil/common.h   |    7 +++
>  3 files changed, 172 insertions(+), 0 deletions(-)
> 
> 
> This new version addresses Michael's concern that the structure size being
> part of the ABI may cause future compatibility problems in a way that I am a
> bit proud of:
> 
> The structure is padded to a constant size (1k; arbitrary): the extra size 
> is used as an initial buffer, thus avoiding completely dynamic memory 
> allocation for small strings. If new fields are needed in the structure, 
> they eat some of that padding, which leaves us plenty of room to manoeuvre.
> 
> On the whole, this is a little bit hackish, but I believe it is worth it,
> because a lot of functions that build strings could benefit from a
> lightweight API like that: av_strerror, av_get_channel_layout_string, etc.
> 
> (Hum, then maybe a lightweight separate bprint.[ch] would be better than
> appending to avstring.)
> 

I agree with moving this to a dedicated place.

> Also, I may consider adding:
> 
> void av_bprint_init_buf(AVBPrint *buf, char *mem, unsigned mem_size);
> 
> to create an AVBPrint that writes directly in the provided buffer.
> 
> Please comment.
> 
> Regards,
> 
> -- 
>   Nicolas George
> 
> 
> 
> diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> index 76f6bb2..6cb3ddf 100644
> --- a/libavutil/avstring.c
> +++ b/libavutil/avstring.c
> @@ -210,6 +210,111 @@ int av_strncasecmp(const char *a, const char *b, size_t n)
>      return c1 - c2;
>  }
>  
> +#define av_bprint_room(buf) ((buf)->size - FFMIN((buf)->len, (buf)->size))
> +#define av_bprint_allocated(buf) ((buf)->str != (buf)->reserved_internal_buffer)

I think "av_bprint_is_allocated()" is better here; the current name may
suggest it returns the current internally allocated size.

> +
> +static int av_bprint_grow(AVBPrint *buf, unsigned new_size)
> +{
> +    char *old_str, *new_str;
> +
> +    if (!av_bprint_complete(buf))
> +        return -1;

AVERROR_BUG?

> +    if (!new_size)
> +        new_size = buf->size > buf->size_max / 2 ? buf->size_max :
> +                                                   buf->size * 2;
> +    old_str = av_bprint_allocated(buf) ? buf->str : NULL;
> +    new_str = av_realloc(old_str, new_size);
> +    if (!new_str)
> +        return -1;

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;
> +    buf->str      = buf->reserved_internal_buffer;
> +    buf->len      = 0;
> +    buf->size     = FFMIN(asize, size_max);
> +    buf->size_max = size_max;
> +    *buf->str = 0;
> +    if (size_init > buf->size)
> +        av_bprint_grow(buf, size_init);
> +}
> +
> +void av_bprintf(AVBPrint *buf, const char *fmt, ...)
> +{
> +    unsigned room;
> +    char *dst;
> +    va_list vl;
> +    int add;
> +
> +    while (1) {
> +        room = av_bprint_room(buf);
> +        dst = room ? buf->str + buf->len : NULL;
> +        va_start(vl, fmt);
> +        add = vsnprintf(dst, room, fmt, vl);
> +        va_end(vl);
> +        if (add <= 0)
> +            return;
> +        if (add < room || buf->size == buf->size_max)
> +            break;
> +        if (av_bprint_grow(buf, 0))
> +            break;
> +    }
> +    add = FFMIN(add, UINT_MAX - 5 - buf->len);
> +    buf->len += add;
> +}
> +
> +void av_bprint_chars(AVBPrint *buf, char c, unsigned n)
> +{
> +    unsigned room, real_n;
> +
> +    while (1) {
> +        room = av_bprint_room(buf);
> +        if (n < room || buf->size == buf->size_max)
> +            break;
> +        if (av_bprint_grow(buf, 0))
> +            break;
> +    }
> +    if (room) {
> +        real_n = FFMIN(n, room - 1);
> +        memset(buf->str + buf->len, c, real_n);
> +        buf->str[buf->len + real_n] = 0;
> +    }
> +    buf->len += FFMIN(n, UINT_MAX - 5 - buf->len);
> +}
> +
> +void av_bprint_finalize(AVBPrint *buf, char **ret_str)
> +{
> +    unsigned real_size = FFMIN(buf->len + 1, buf->size);
> +    char *str;
> +
> +    if (ret_str) {
> +        if (av_bprint_allocated(buf)) {
> +            str = av_realloc(buf->str, real_size);
> +            if (!str)
> +                str = buf->str;
> +            buf->str = NULL;
> +        } else {
> +            str = av_malloc(real_size);
> +            if (str)
> +                memcpy(str, buf->str, real_size);
> +        }
> +        *ret_str = str;
> +    } else {
> +        if (av_bprint_allocated(buf))
> +            av_freep(&buf->str);
> +    }
> +    buf->size = real_size;
> +}
> +
>  #ifdef TEST
>  

Adding tests might be welcome; not only for testing, but as an API
demonstration purpose.

>  #undef printf
> diff --git a/libavutil/avstring.h b/libavutil/avstring.h
> index f73d6e7..258b394 100644
> --- a/libavutil/avstring.h
> +++ b/libavutil/avstring.h
> @@ -23,6 +23,7 @@
>  
>  #include <stddef.h>
>  #include "attributes.h"
> +#include "common.h"
>  
>  /**
>   * @addtogroup lavu_string
> @@ -203,6 +204,65 @@ int av_strcasecmp(const char *a, const char *b);
>  int av_strncasecmp(const char *a, const char *b, size_t n);
>  
>  /**
> + * Buffer to print data progressively
> + * The string buffer grows as necessary and is always 0-terminated.
> + * The length of the string can go beyond the allocated size: the buffer is
> + * then truncated, but the functions still keep account of the required
> + * size.
> + * Small buffers are kept in the structure itself, and thus require no
> + * memory allocation at all (unless the contents of the buffer is needed
> + * after the structure goes out of scope).
> + */
> +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);
> +
> +/**
> + * Append a formated string to a print buffer
> + */

What happens in case of error?

> +void av_bprintf(AVBPrint *buf, const char *fmt, ...) av_printf_format(2, 3);
> +
> +/**
> + * Append n times char c to a print buffer
> + */

ditto.

> +void av_bprint_chars(AVBPrint *buf, char c, unsigned n);
> +
> +/**
> + * Tests if the print buffer is complete
> + * It may have been truncated due to a memory allocation failure.
> + */
> +static inline int av_bprint_complete(AVBPrint *buf)
> +{
> +    return buf->len < buf->size;
> +}
> +

Any reason this function is inline while av_bprint_room() and
av_bprint_allocated() are (unexported) macros?

> +/**
> + * Finalize a print buffer
> + * The print buffer can no longer be used afterwards,
> + * but the len and size fields are still valid.
> + * @arg[out] ret_str  if not NULL, used to return a permanent copy of the
> + *                    buffer contents, or NULL if memory allocation fails;
> + *                    if NULL, the buffer is discarded and freed
> + */
> +void av_bprint_finalize(AVBPrint *buf, char **ret_str);
> +
> +/**
>   * @}
>   */
>  
> diff --git a/libavutil/common.h b/libavutil/common.h
> index 84290c6..d3308ef 100644
> --- a/libavutil/common.h
> +++ b/libavutil/common.h
> @@ -61,6 +61,13 @@
>  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
>  #define FFALIGN(x, a) (((x)+(a)-1)&~((a)-1))
>  
> +/**
> + * Define a structure with extra padding to a fixed size.
> + */

Maybe you should mention it is mainly for ABI compat purpose?

> +#define FF_PAD_STRUCTURE(size, ...) \
> +    __VA_ARGS__ \
> +    char reserved_padding[size - sizeof(struct { __VA_ARGS__ })];
> +
>  /* misc math functions */
>  extern const uint8_t ff_log2_tab[256];

I still like this very much, and will be happy to make use of it in a few
places.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120218/d4895b1d/attachment.asc>


More information about the ffmpeg-devel mailing list