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

[Marton Balint]

On Wed, 7 Aug 2019, Alexander Strasser wrote:

> 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.

It has better performance than using av_printf because it does not need a 
temporary buffer.

>
> 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

Fixed locally.

>
>> 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.

I tested a few compilers (Clang, GCC, MSVC 2015) on https://godbolt.org/ 
and it seems to work.

>
>
>> +
>>  /**
>>   * 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.

It returns int because avio_printf also returns int, but since no error 
(other than IO error) can happen, maybe it is better to return void the 
same way as avio_write functions do. For IO errors the user should always 
check the IO context error flag, that is by design as far as I know.

I'll change it to return void.

Thanks,
Marton