[FFmpeg-devel] [PATCH 1/3] avformat/avio: add avio_print_n_strings and avio_print

[Alexander Strasser]

Hi Marton!

Not really sure if we need the API, but it definitely looks
handy. Just not sure how often it will used and really result
in clearer or shorter code.

Not opposed to it though; no strong opinion on this one.

Some comments follow inline. No in depth review, just what
came to my mind when reading your patch quickly.


On 2019-08-05 23:34 +0200, Marton Balint wrote:
> These functions can be used to print a variable number of strings consecutively
> to the IO context. Unlike av_bprintf, no temporery buffer is necessary.

Small typo here: temporary

> Signed-off-by: Marton Balint <cus at passwd.hu>
> ---
>  doc/APIchanges        |  3 +++
>  libavformat/avio.h    | 16 ++++++++++++++++
>  libavformat/aviobuf.c | 17 +++++++++++++++++
>  libavformat/version.h |  2 +-
>  4 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 6603a8229e..0b19fb067d 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>
>  API changes, most recent first:
>
> +2019-08-xx - xxxxxxxxxx - lavf 58.31.100 - avio.h
> +  Add avio_print_n_strings and avio_print.
> +
>  2019-07-27 - xxxxxxxxxx - lavu 56.33.100 - tx.h
>    Add AV_TX_DOUBLE_FFT and AV_TX_DOUBLE_MDCT
>
> diff --git a/libavformat/avio.h b/libavformat/avio.h
> index dcb8dcdf93..ca08907917 100644
> --- a/libavformat/avio.h
> +++ b/libavformat/avio.h
> @@ -574,6 +574,22 @@ int avio_feof(AVIOContext *s);
>  /** @warning Writes up to 4 KiB per call */
>  int avio_printf(AVIOContext *s, const char *fmt, ...) av_printf_format(2, 3);
>
> +/**
> + * Write nb_strings number of strings (const char *) to the context.
> + * @return the total number of bytes written
> + */
> +int avio_print_n_strings(AVIOContext *s, int nb_strings, ...);
> +
> +/**
> + * Write strings (const char *) to the context.
> + * This is a macro around avio_print_n_strings but the number of strings to be
> + * written is determined automatically at compile time based on the number of
> + * parameters passed to this macro. For simple string concatenations this
> + * function is more performant than using avio_printf.
> + */
> +#define avio_print(s, ...) \
> +    avio_print_n_strings(s, sizeof((const char*[]){__VA_ARGS__}) / sizeof(const char*), __VA_ARGS__)

If we don't have this pattern in the code base already, it would
be better to compile and test on bunch of different compilers.


> +
>  /**
>   * Force flushing of buffered data.
>   *
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 2d011027c9..c37b056b64 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -1225,6 +1225,23 @@ int avio_printf(AVIOContext *s, const char *fmt, ...)
>      return ret;
>  }
>
> +int avio_print_n_strings(AVIOContext *s, int nb_strings, ...)
> +{
> +    va_list ap;
> +    int ret = 0;
> +
> +    va_start(ap, nb_strings);
> +    for (int i = 0; i < nb_strings; i++) {
> +        const char *str = va_arg(ap, const char *);
> +        int len = strlen(str);
> +        avio_write(s, (const unsigned char *)str, len);
> +        ret += len;

I first wanted to say you should compute with the count returned
by avio_write; then after diving into libavformat/avio_buf and
reading a cascade of void functions and seeing signalling via
field error in the context which also is sometimes (mis)used
to store a length(?), I withdraw that comment.

Anyway you might consider using void for this function too,
otherwise I guess you should figure out how to do the error
checking inside of this function because if an error occurs
that call might have been partial and the following writes will
effectively not occur. So I'm feeling rather uncomfortable
with returning a count here. Maybe my stance is to narrow.


  Alexander

> +    }
> +    va_end(ap);
> +
> +    return ret;
> +}
> +
>  int avio_pause(AVIOContext *s, int pause)
>  {
>      if (!s->read_pause)
> diff --git a/libavformat/version.h b/libavformat/version.h
> index 45efaff9b9..feceaedc08 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -32,7 +32,7 @@
>  // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
>  // Also please add any ticket numbers that you believe might be affected here
>  #define LIBAVFORMAT_VERSION_MAJOR  58
> -#define LIBAVFORMAT_VERSION_MINOR  30
> +#define LIBAVFORMAT_VERSION_MINOR  31
>  #define LIBAVFORMAT_VERSION_MICRO 100
>
>  #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
> --