Re: [FFmpeg-devel] [PATCH 2/7] lavu: new AVWriter API.

[Stefano Sabatini <stefasab@gmail.com>]

Preliminary quick review.

On date Wednesday 2021-04-21 14:27:01 +0200, Nicolas George wrote:
[...]
> --- /dev/null
> +++ b/libavutil/writer.c
> @@ -0,0 +1,443 @@
> +/*
> + * Copyright (c) 2021 Nicolas George
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "avassert.h"
> +#include "log.h"
> +#include "writer.h"
> +
> +/***************************************************************************
> + * Generic API
> + ***************************************************************************/
> +
> +#define FIELDOK(st, f) ((char *)(&(st)->f + 1) <= (char *)(st) + (st)->self_size)
> +

> +#define methods_assert_abi(methods) av_assert1(FIELDOK(methods, flush))

"methods" is somehow confusing since it can contain also other things
(name, size, etc.). What about using something as class instead? Or
"implementation" to mean that the functionality is embedded there.

> +
> +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);
> +}
> +
> +static void write_or_discard(AVWriter wr, size_t buf_size, size_t write_size)
> +{
> +    av_assert1(wr.methods->advance_buffer);
> +    wr.methods->advance_buffer(wr, FFMIN(buf_size, write_size));
> +    if (write_size > buf_size && wr.methods->notify_discard)
> +        wr.methods->notify_discard(wr, write_size - buf_size);
> +}
> +

> +static void av_writer_impossible(AVWriter wr, const char *message)

you can possibly discard the av_ and rename to something as
notify_unsupported_operation() ("impossible" is confusing in this
context).

[...]
> +/***************************************************************************
> + * AVBufWriter - write to pre-allocated memory
> + ***************************************************************************/
> +
> +#define buf_writer_assert_abi(bwr) av_assert1(FIELDOK(bwr, pos))
> +

> +static size_t buf_writer_room(AVBufWriter *bwr)

inline?

> +{
> +    return bwr->pos < bwr->size ? bwr->size - bwr->pos - 1 : 0;
> +}
> +
> +static void buf_writer_write(AVWriter wr, const char *data, size_t size)
> +{

> +    AVBufWriter *bwr = wr.obj;
> +
> +    av_assert1(av_buf_writer_check(wr));
> +    buf_writer_assert_abi(bwr);

maybe factorize with a macro?

> +    size = FFMIN(buf_writer_room(bwr), size);
> +    memcpy(bwr->buf + bwr->pos, data, size);
> +    bwr->pos += size;
> +    bwr->buf[bwr->pos] = 0;
> +}
> +
> +static void buf_writer_vprintf(AVWriter wr, const char *fmt, va_list va)
> +{
> +    AVBufWriter *bwr = wr.obj;
> +    int ret;
> +
> +    av_assert1(av_buf_writer_check(wr));
> +    buf_writer_assert_abi(bwr);
> +    ret = vsnprintf(bwr->buf + bwr->pos, buf_writer_room(bwr) + 1, fmt, va);
> +    if (ret > 0)
> +        bwr->pos += ret;
> +}
> +
> +static char *buf_writer_get_buffer(AVWriter wr, size_t min_size, size_t *size)
> +{
> +    AVBufWriter *bwr = wr.obj;
> +
> +    av_assert1(av_buf_writer_check(wr));
> +    buf_writer_assert_abi(bwr);
> +    *size = bwr->size - 1 - bwr->pos;
> +    return bwr->buf + bwr->pos;
> +}
> +

> +static void buf_writer_advance_buffer(AVWriter wr, size_t size)
> +{
> +    AVBufWriter *bwr = wr.obj;
> +
> +    av_assert1(av_buf_writer_check(wr));
> +    buf_writer_assert_abi(bwr);
> +    bwr->pos += size;

> +    bwr->buf[bwr->pos] = 0;

buffer overflow, is this set needed?

> +}
> +
> +AV_WRITER_DEFINE_METHODS(/*public*/, AVBufWriter, av_buf_writer) {
> +    .self_size        = sizeof(AVWriterMethods),
> +    .name             = "AVBufWriter",
> +    .write            = buf_writer_write,
> +    .vprintf          = buf_writer_vprintf,
> +    .get_buffer       = buf_writer_get_buffer,
> +    .advance_buffer   = buf_writer_advance_buffer,
> +};
> +
> +AVBufWriter *av_buf_writer_init(AVBufWriter *bwr, char *buf, size_t size)
> +{
> +    buf_writer_assert_abi(bwr);
> +    bwr->buf       = buf;
> +    bwr->size      = size;
> +    bwr->pos       = 0;
> +    buf[0] = 0;
> +    return bwr;
> +}
> +

> +AVWriter av_buf_writer_wrap(AVBufWriter *bwr)
> +{
> +    AVWriter r = { av_buf_writer_get_methods(), bwr };

nit+++: wr for consistency

[...]

> +AVWriter av_dynbuf_writer_wrap(AVDynbufWriter *dwr)
> +{
> +    AVWriter r = { av_dynbuf_writer_get_methods(), dwr };
> +    dynbuf_writer_assert_abi(dwr);
> +    return r;
> +}

this also seems a common pattern and could be factorized:

#define AV_WRITER_DEFINE_WRAP(type_, name_) \
AVWriter av_##name_##_writer_wrap(type_, *wrimpl) \
{ \ 
    AVWriter wr = { av_##name_##_writer_get_methods(), wrimpl }; \
    type_##_writer_assert_abi(wrimpl); \
    return wr; \
}

[...]
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".