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

[Alexander Strasser]

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

[...]

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