[FFmpeg-devel] [PATCH 2/8] lavu: new AVWriter API

[Rémi Denis-Courmont]

Le perjantaina 28. huhtikuuta 2023, 12.55.02 EEST Nicolas George a écrit :
> +/**************************************************************************
> * + * Generic API
> +
> ***************************************************************************
> / +
> +#define FIELDOK(st, f) ((char *)(&(st)->f + 1) <= (char *)(st) +
> (st)->self_size) +
> +#define methods_assert_abi(methods) av_assert0(FIELDOK(methods, flush))
> +
> +static void printf_unchecked(AVWriter wr, const char *fmt, ...)
> +{
> +    va_list va;
> +
> +    va_start(va, fmt);
> +    wr.methods->vprintf(wr, fmt, va);
> +    va_end(va);
> +}

Indirecting the printf function seems pretty pointless. The last thing you 
want are different implementations of printf() with different limitations. And 
it makes writing different backends needlessly complex, while you could just 
use vasprintf().

Typically, with this kind of abstraction, only the raw bytes writing is 
abstracted away. Examples include funopen() and fopencookie().

As for hypothetical use cases whence vasprintf() wouldb e "too slow", then 
should not use printf()-style or function pointers to begin with. Besides if 
you _really_ want to avoid the heap allocation, you can also use fopencookie() 
on systems that provide it or equivalent.

> +void av_writer_write(AVWriter wr, const char *data, size_t size)
> +{
> +    methods_assert_abi(wr.methods);
> +    if (wr.methods->write) {
> +        wr.methods->write(wr, data, size);
> +    } else if (wr.methods->get_buffer) {
> +        size_t buf_size;
> +        char *buf = wr.methods->get_buffer(wr, size, &buf_size);
> +        if (buf_size > 0)
> +            memcpy(buf, data, FFMIN(size, buf_size));
> +        write_or_discard(wr, buf_size, size);

That sounds like it belongs in whichever backend actually wants to heap-
allocate the output buffer, not the frontend.

> +    } else if (wr.methods->vprintf) {
> +        size_t i;
> +        for (i = 0; i < size; i++)
> +            printf_unchecked(wr, "%c", data[i]);

This is an abstraction inversion (and also highly inefficient) + what I noted 
above.

> +    } else {
> +        av_writer_impossible(wr, "av_writer_write()");
> +    }
> +}
> +
> +void av_writer_print(AVWriter wr, const char *str)
> +{

This is an analogue of puts/fputs, not "print".

> +    av_writer_write(wr, str, strlen(str));
> +}
> +
> +void av_writer_printf(AVWriter wr, const char *fmt, ...)
> +{
> +    va_list va;
> +
> +    va_start(va, fmt);
> +    av_writer_vprintf(wr, fmt, va);
> +    va_end(va);
> +}
> +
> +void av_writer_vprintf(AVWriter wr, const char *fmt, va_list va)
> +{
> +    methods_assert_abi(wr.methods);
> +    if (wr.methods->vprintf) {
> +        wr.methods->vprintf(wr, fmt, va);
> +    } else if (wr.methods->get_buffer) {
> +        size_t buf_size;
> +        char *buf = wr.methods->get_buffer(wr, 128, &buf_size);
> +        va_list va2;
> +        int ret;
> +        va_copy(va2, va);
> +        ret = vsnprintf(buf, buf_size, fmt, va2);
> +        va_end(va2);
> +        if (ret < 0)
> +            return;
> +        if ((size_t)ret + 1 > buf_size) {
> +            buf = wr.methods->get_buffer(wr, ret + 1, &buf_size);
> +            ret = vsnprintf(buf, buf_size, fmt, va);
> +            if (ret < 0)
> +                return;
> +        }
> +        write_or_discard(wr, buf_size, ret);
> +    } else if (wr.methods->write) {
> +        AVBPrint bp;
> +        av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
> +        av_vbprintf(&bp, fmt, va);
> +        if (av_bprint_is_complete(&bp)) {
> +            wr.methods->write(wr, bp.str, bp.len);
> +        } else {
> +            wr.methods->write(wr, bp.str, bp.size - 1);
> +            if (wr.methods->notify_discard)
> +                wr.methods->notify_discard(wr, bp.len - bp.size + 1);
> +        }
> +        av_bprint_finalize(&bp, NULL);
> +    } else {
> +        av_writer_impossible(wr, "av_writer_vprintf()");
> +    }
> +}

Same problems and overengineering as above.

> +
> +void av_writer_add_chars(AVWriter wr, char c, size_t n)
> +{
> +    char buf[512];
> +    size_t m;
> +
> +    m = FFMIN(n, sizeof(buf) - 1);
> +    memset(buf, c, m);
> +    while (n) {
> +        av_writer_write(wr, buf, m);
> +        n -= m;
> +        m = FFMIN(n, sizeof(buf) - 1);
> +    }
> +}
> +
> +void av_writer_flush(AVWriter wr)
> +{
> +    methods_assert_abi(wr.methods);
> +    if (wr.methods->flush)
> +        wr.methods->flush(wr);
> +}
> +
> +int av_writer_get_error(AVWriter wr, int self_only)
> +{
> +    methods_assert_abi(wr.methods);
> +    if (wr.methods->get_error)
> +        return wr.methods->get_error(wr, self_only);
> +    return 0;
> +}
> +

-- 
レミ・デニ-クールモン
http://www.remlab.net/