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

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.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 doc/APIchanges        |  3 +++
 libavformat/avio.h    | 16 ++++++++++++++++
 libavformat/aviobuf.c | 17 +++++++++++++++++
 libavformat/version.h |  2 +-
 4 files changed, 37 insertions(+), 1 deletion(-)

Marton Balint (12019-08-05):
> 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.

I do not like this very much: the VA design disables type checking,
which makes it rather fragile, and the benefit is really minor.

Maybe add avio_print_string() to print a single string, and call it as
many times as necessary.

Regards,

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@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, \
> --

On Tue, 6 Aug 2019, Nicolas George wrote:

> Marton Balint (12019-08-05):
>> 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.
>
> I do not like this very much: the VA design disables type checking,
> which makes it rather fragile, and the benefit is really minor.

This is only a problem if avio_print_n_strings is called directly, but the 
user should always use avio_print. And avio_print macro is casting every 
parameter to const char *, so the compiler emits a warning if the user 
passes an argument which is not const char *.

>
> Maybe add avio_print_string() to print a single string, and call it as
> many times as necessary.

That reduces readability slightly, so I'd rather not do it.

Regards,
Marton

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

On 2019-08-07 21:28 +0200, Marton Balint wrote:
>
> 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.

True. I meant one could use avio_write like Paul demonstrated
or like Nicolas suggested introduce a simple wrapper function
avio_write_string .

But as I said right below, I'm not against this API.

> > 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@passwd.hu>
> > > ---
> > >  doc/APIchanges        |  3 +++
> > >  libavformat/avio.h    | 16 ++++++++++++++++
> > >  libavformat/aviobuf.c | 17 +++++++++++++++++
> > >  libavformat/version.h |  2 +-
> > >  4 files changed, 37 insertions(+), 1 deletion(-)

[...]

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

Sounds good.

Another thing is, that the convenient macro will probably only be
usable in C client code. That's of course expected, and bindings
could provide similar implementations inspired by your design of
avio_print, which might be easier anyway in many cases. Just saying
because I think we should consider those things when designing and
extending FFmpeg's APIs.


[...]
> > >
> > > +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.

I have seen it in your patch version 2. Looks fine.

No more comments from me.


Thanks,
  Alexander

On 05.08.2019, at 23:34, Marton Balint <cus@passwd.hu> 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.

Hm, is there a use-example patch I missed?
Also is the #define ugliness worth it compared to just requiring NULL termination?
You can use the sentinel function attribute to have the compiler check that users do not forget it.

On 11.08.2019, at 20:41, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:

> On 05.08.2019, at 23:34, Marton Balint <cus@passwd.hu> 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.
> 
> Hm, is there a use-example patch I missed?

Sorry, I see it now, and also the argument about the macro providing type checking.
Ignore me if you feel the code is fine as-is.

> Also is the #define ugliness worth it compared to just requiring NULL termination?
> You can use the sentinel function attribute to have the compiler check that users do not forget it.

On Sun, 11 Aug 2019, Reimar Döffinger wrote:

> On 11.08.2019, at 20:41, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
>
>> On 05.08.2019, at 23:34, Marton Balint <cus@passwd.hu> 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.
>> 
>> Hm, is there a use-example patch I missed?
>
> Sorry, I see it now, and also the argument about the macro providing type checking.
> Ignore me if you feel the code is fine as-is.
>
>> Also is the #define ugliness worth it compared to just requiring NULL termination?
>> You can use the sentinel function attribute to have the compiler check that users do not forget it.

There is yet another approach which does not count parameters but uses a 
NULL sentinel, and is more simple:

void avio_print_string_array(AVIOContext *s, const char *strings[])
{
     for(; strings[0]; strings++)
         avio_write(s, (const unsigned char *)strings[0], strlen(strings[0]));
}

#define avio_print(s, ...) \
     avio_print_string_array(s, (const char*[]){__VA_ARGS__, NULL})

This might also has the benefit that __VA_ARGS__ is referenced only once, 
because for the old version some compilers (msvc) was not optimizing away 
the additional local variable and their string references which were used 
in the sizeof magic.

I can change the patch to use this version if people prefer.

Regards,
Marton

On 11.08.2019, at 21:51, Marton Balint <cus@passwd.hu> wrote:

> 
> 
> On Sun, 11 Aug 2019, Reimar Döffinger wrote:
> 
>> On 11.08.2019, at 20:41, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
>> 
>>> On 05.08.2019, at 23:34, Marton Balint <cus@passwd.hu> 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.
>>> Hm, is there a use-example patch I missed?
>> 
>> Sorry, I see it now, and also the argument about the macro providing type checking.
>> Ignore me if you feel the code is fine as-is.
>> 
>>> Also is the #define ugliness worth it compared to just requiring NULL termination?
>>> You can use the sentinel function attribute to have the compiler check that users do not forget it.
> 
> There is yet another approach which does not count parameters but uses a NULL sentinel, and is more simple:
> 
> void avio_print_string_array(AVIOContext *s, const char *strings[])
> {
>    for(; strings[0]; strings++)
>        avio_write(s, (const unsigned char *)strings[0], strlen(strings[0]));
> }

Nitpick: I find using [0] confusing when used on a loop variable and would consider *strings better

> #define avio_print(s, ...) \
>    avio_print_string_array(s, (const char*[]){__VA_ARGS__, NULL})
> 
> This might also has the benefit that __VA_ARGS__ is referenced only once, because for the old version some compilers (msvc) was not optimizing away the additional local variable and their string references which were used in the sizeof magic.
> 
> I can change the patch to use this version if people prefer.

It also avoids potential multiple-evaluation issues, so it does seem more robust to me.
But I admit my macro-foo is rather weak due to too much C++ use in the last years...