[FFmpeg-devel] AVWriter API

[Anton Khirnov]

Replying to this thread, since it contains the motivation.

Quoting Nicolas George (2019-12-08 15:25:43)
> Hi.
> 
> [ TL;DR: a lightweight oo API for functions that need to return a
> string, designed to be as convenient as possible. ]

TL;DR: sorry, I do not like this. It seems to me you are adding a
substantial amount of new complexity in an awkward place, that ends up
being worse than the problem it's supposed to address.

> 
> Yesterday, in my comments about the new channel layout API:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/254020.html
> I evoked the following issue:
> 
> We have many functions that return strings to the caller:
> 
> char *av_get_sample_fmt_string(char *buf, int buf_size, enum AVSampleFormat sample_fmt);
> const char *av_get_media_type_string(enum AVMediaType media_type);
> char *av_base64_encode(char *out, int out_size, const uint8_t *in, int in_size);
> int av_dict_get_string(const AVDictionary *m, char **buffer,
>                        const char key_val_sep, const char pairs_sep);
> int av_strerror(int errnum, char *errbuf, size_t errbuf_size);
> void av_hash_final_hex(struct AVHashContext *ctx, uint8_t *dst, int size);
> const char *av_get_known_color_name(int color_idx, const uint8_t **rgb);
> const char *av_color_range_name(enum AVColorRange range);
> char *av_timecode_make_mpeg_tc_string(char *buf, uint32_t tc25bit);
> static inline char *av_ts_make_string(char *buf, int64_t ts)
> void avcodec_string(char *buf, int buf_size, AVCodecContext *enc, int encode);
> 
> The simplest of them are "enum → static string" functions, but what do
> they do with an invalid value: return NULL or a meaningful string like
> "unknown"?
> 
> A lot of them demand a pair of "char *buf, size_t buf_size" arguments to
> return the string. What is a reasonable size for the buffer? How do we
> handle the cases where the buffer is too small?
> 
> When the string is likely to be long, they dynamically allocate the
> buffer.
> 
> This is an inconsistent mess.

You assume every function that produces a string for the caller is
supposed to be consistent. I do not agree with that - the functions
serve different purposes and different patterns may be appropriate for
each of those. A static string, an informative message and serialized
structured data are all different things.

And most importantly - strings are simple. Every C programmer (worth the
name) understands strings. But now you'd be forcing every API user to
understand and remember this new API.

> And unsatisfactory on top of it: most of
> the time, we will just copy the buffer to another buffer, write it to a
> file or dump it to a log. The string function may need a big buffer to
> build the string while it is at no point necessary to have it in full.
> 
> I have been wanting to find a clean solution for this issue for a long
> time. And now I have. (The AVBPrint was a preliminary attempt at that.)
> 
> But before polishing it, I wanted to know the opinions of my fellow
> developers on this issue. The design is a bit unconventional (which is
> what enables it to be convenient), I suspect some people may object
> because of that.
> 
> I have attached the header file so people can have a look at it.
> 
> The idea is to implement the bare minimum of a very simple object
> system. 
> 
> To return a string (or a binary blob, that changes nothing), the
> functions take an AVWriter object and just write to it using a
> fprintf()-like function or other useful functions.
> 
> The AVWriter object is abstract and can actually write in several
> different ways. av_buf_writer_array(buf) will just wrap "char buf[42]"
> and write in it. av_dynbuf_writer() prepares a dynamic buffer.
> av_log_writer(obj) writes to av_log().
> 
> User application can implement their own AVWriter types: write directly
> in a GUI text buffer or to syslog.
> 
> I wanted to avoid as much as possible adding new necessary error checks.
> I think I have succeeded.
> 
>         av_foobar_to_string(buf, sizeof(buf), foobar);
> 
> becomes
> 
>         av_foobar_write(av_buf_writer_array(buf), foobar);
> 
> which has a little more letters but is actually simpler.

Error checks are not an evil to be avoided. On the contrary, where error
checks are necessary (like in memory allocation), we want to shove them
in the caller's face as strongly as we can.

And if the buffer was static and not large enough, again we want to
shout very loudly that the result may be invalid. Especially if the
result is structured and is supposed to be interpreted somehow.

-- 
Anton Khirnov