[FFmpeg-devel] lavu: header and documentation for AVWriter

The actual implementation, tests and uses in the rest of
FFmpeg code will be committed separately once the API is
settled.

Signed-off-by: Nicolas George <george@nsup.org>
---
 doc/avwriter_intro.md | 109 ++++++++++
 libavutil/writer.h    | 484 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 593 insertions(+)
 create mode 100644 doc/avwriter_intro.md
 create mode 100644 libavutil/writer.h


As suggested by JB, here is the header and documentation for AVWriter,
to discuss the principle before investing time in polishing the
implementation. I expect to discuss the API and if no blockign
objections are raised push it then spend time on the implementation.

This API is a nicer and much more powerful version of BPrint. It can be
used to simplify existing code where BPrint could help, plus places
where BPrint could not help.

It also is the prerequisite for more ambitious projects, expecially
universal serialization of FFmpeg objects (side data and such) into
standardized formats.

The implementation is in most part done, and I am sure I can deliver.
What remains is the boring part of integrating the tests in FATE,
polishing various parts, updating the parts where I changed my mind
midway, etc.

Note: FF_NEW_SZ is a macro defined elsewhere; it is really part of the
implementation more than the API.

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Wednesday, August 24, 2022 5:18 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] lavu: header and documentation for
> AVWriter
> 
> The actual implementation, tests and uses in the rest of
> FFmpeg code will be committed separately once the API is
> settled.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  doc/avwriter_intro.md | 109 ++++++++++
>  libavutil/writer.h    | 484
> ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 593 insertions(+)
>  create mode 100644 doc/avwriter_intro.md
>  create mode 100644 libavutil/writer.h
> 
> 
> As suggested by JB, here is the header and documentation for
> AVWriter,
> to discuss the principle before investing time in polishing the
> implementation. I expect to discuss the API and if no blockign
> objections are raised push it then spend time on the implementation.
> 
> This API is a nicer and much more powerful version of BPrint. It can
> be
> used to simplify existing code where BPrint could help, plus places
> where BPrint could not help.
> 
> It also is the prerequisite for more ambitious projects, expecially
> universal serialization of FFmpeg objects (side data and such) into
> standardized formats.
> 
> The implementation is in most part done, and I am sure I can deliver.
> What remains is the boring part of integrating the tests in FATE,
> polishing various parts, updating the parts where I changed my mind
> midway, etc.
> 
> Note: FF_NEW_SZ is a macro defined elsewhere; it is really part of
> the
> implementation more than the API.
> 
> 
> diff --git a/doc/avwriter_intro.md b/doc/avwriter_intro.md
> new file mode 100644
> index 0000000000..4fd8a5a4ad
> --- /dev/null
> +++ b/doc/avwriter_intro.md
> @@ -0,0 +1,109 @@
> +# Quick start guide for AVWriter
> +
> +AVWriter is an API to unify functions returning strings (or any
> arbitrary
> +binary buffer) and to make building strings from parts easier. Here
> is a
> +quick introduction with pairs of “what I would do without AVWriter”
> /
> +“how to do it with AVWriter” of example code.
> +
> +## I want a `char*` buffer, the function wants an AVWriter
> +
> +Old-style code:
> +
> +```
> +        char *buf;
> +        ret = av_something_to_string(&buf, something);
> +        if (ret < 0)
> +            die("Failed");
> +        use_string(buf);
> +        av_freep(&buf);
> +```
> +
> +Equivalent code with AVWriter:
> +
> +```
> +        AVWriter wr = av_dynbuf_writer();
> +        av_something_write(wr, something, 0);
> +        if (av_writer_get_error(wr, 0) < 0)
> +            die("Failed");

Will it be possible to do:

av_something_write(wr, something1, 0);
av_something_write(wr, something2, 0);
av_something_write(wr, something3, 0);
if (av_writer_get_error(wr, 0) < 0)
    die("Failed");

or would av_writer_get_error() need to be called after each av_something_write()?

Thanks,
softworkz

Nicolas George:
> The actual implementation, tests and uses in the rest of
> FFmpeg code will be committed separately once the API is
> settled.
> 

I think you misunderstood JB: He did not say that the headers are pushed
without the implementation, he just said that the headers should be
discussed and approved before you code the actual implementation:

> 
> You provide a full header and documentation, and then get it discussed.
> When there is a consensus for approval of the headers, then, you can code the core of it that matches the headers.
> That avoids the "I spent time for nothing" issue.

> diff --git a/libavutil/writer.h b/libavutil/writer.h
> new file mode 100644
> index 0000000000..55e2cf3ea6
> --- /dev/null
> +++ b/libavutil/writer.h
> @@ -0,0 +1,484 @@
> +/*
> + * Copyright (c) 2022 The FFmpeg project
> + *
> + * 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
> + */
> +
> +#ifndef AVUTIL_WRITER_H
> +#define AVUTIL_WRITER_H
> +
> +#include <stdio.h>
> +#include <stddef.h>
> +#include <stdarg.h>
> +
> +#include "extendable.h"

Where is this header?

> +#include "bprint.h"
> +
> +/**
> + * @defgroup av_writer AVWriter
> + *
> + * Object-oriented API to write strings and binary data.
> + *
> + * @{
> + */
> +
> +typedef struct AVWriterMethods AVWriterMethods;
> +
> +/**
> + * Opaque object to write strings and binary data.
> + *
> + * AVWriter is meant to allow to build and return strings (and blocks of
> + * binary data) efficiently between functions.
> + * For example, a function that serialize something into a string will
> + * actually write into an AVWriter, and the caller will choose the way the
> + * data will be stored or processed on the fly.
> + *
> + * For a quick introduction on how to use AVWriter in simple cases, see
> + * doc/avwriter_intro.md.
> + *
> + * There are various pre-defined types of AVWriter, see below:
> + *
> + * - av_dynbuf_writer() writes the data into a buffer that is grown with
> + *   av_realloc() if it does not fit in the initial buffer; it is the
> + *   recommended choice.
> + *
> + * - av_buf_writer() writes the data into a pre-existing finite buffer.
> + *
> + * - av_stdio_writer() writes the data into a FILE*.
> + *
> + * - av_log_writer() writes the data to av_log().
> + *
> + * AVWriter objects are passed by value. It allows creating a writer on the
> + * fly, without extra variable or error checking. The structure can never
> + * change.
> + */
> +typedef struct AVWriter {
> +    const AVWriterMethods *methods;
> +    void *obj;
> +} AVWriter;
> +
> +/**
> + * Write a data buffer to an AVWriter.
> + */
> +void av_writer_write(AVWriter wr, const char *buf, size_t size);
> +
> +/**
> + * Write a 0-terminated string to an AVWriter.
> + */
> +void av_writer_print(AVWriter wr, const char *str);
> +
> +/**
> + * Write a formatted string to an AVWriter.
> + * Note: do not use libc-specific extensions to the format string.
> + */
> +void av_writer_printf(AVWriter wr, const char *fmt, ...);
> +
> +/**
> + * Write a formatted to string an AVWriter using a va_list.

s/to string/string to/

> + */
> +void av_writer_vprintf(AVWriter wr, const char *fmt, va_list va);

Andreas Rheinhardt (12022-08-24):
> Where is this header?

# Note: FF_NEW_SZ is a macro defined elsewhere; it is really part of the
# implementation more than the API.

This is all there is to it for this case. I can move it here for now.

Regards,

Nicolas George (12022-08-24):
> The actual implementation, tests and uses in the rest of
> FFmpeg code will be committed separately once the API is
> settled.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  doc/avwriter_intro.md | 109 ++++++++++
>  libavutil/writer.h    | 484 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 593 insertions(+)
>  create mode 100644 doc/avwriter_intro.md
>  create mode 100644 libavutil/writer.h
> 
> 
> As suggested by JB, here is the header and documentation for AVWriter,
> to discuss the principle before investing time in polishing the
> implementation. I expect to discuss the API and if no blockign
> objections are raised push it then spend time on the implementation.
> 
> This API is a nicer and much more powerful version of BPrint. It can be
> used to simplify existing code where BPrint could help, plus places
> where BPrint could not help.
> 
> It also is the prerequisite for more ambitious projects, expecially
> universal serialization of FFmpeg objects (side data and such) into
> standardized formats.
> 
> The implementation is in most part done, and I am sure I can deliver.
> What remains is the boring part of integrating the tests in FATE,
> polishing various parts, updating the parts where I changed my mind
> midway, etc.
> 
> Note: FF_NEW_SZ is a macro defined elsewhere; it is really part of the
> implementation more than the API.
> 
> 

Ping.

Regards,

On 8/24/22 11:18, Nicolas George wrote:
> +```
> +        AVWriter wr = av_dynbuf_writer();
> +        av_something_write(wr, something, 0);
> +        if (av_writer_get_error(wr, 0) < 0)
> +            die("Failed");
> +        use_string(av_dynbuf_writer_get_data(wr, NULL));
> +        av_dynbuf_writer_finalize(wr, NULL, NULL);
> +```

Is there a reason this is AVWriter wr = foo() and not AVWriter *wr = 
foo()? Most other APIs return pointers to structs, rather than structs 
themselves (see: av_packet_alloc). Using a pointer would prevent us from 
having sizeof(AVWriter) as part of the ABI, as was done with AVPacket.

- Leo Izen (thebombzen)

Leo Izen (12022-08-30):
> Is there a reason this is AVWriter wr = foo() and not AVWriter *wr = foo()?
> Most other APIs return pointers to structs, rather than structs themselves
> (see: av_packet_alloc). Using a pointer would prevent us from having
> sizeof(AVWriter) as part of the ABI, as was done with AVPacket.

Yes: to return a pointer, you need somewhere to store the structure. One
of the point of AVWriter is that you can store it on the stack to avoid
dynamic allocations when the string is short enough.

Note that AVWriter is exactly two pointers. It will always be two
pointers, and all the objects that I intend to introduce later will
always be two pointers: one const pointer for the methods, one pointer
for the object itself.

This design is essential to the features I promised for AVWriter and for
later.

Regards,

On 8/30/22 15:37, Nicolas George wrote:
> Leo Izen (12022-08-30):
>> Is there a reason this is AVWriter wr = foo() and not AVWriter *wr = foo()?
>> Most other APIs return pointers to structs, rather than structs themselves
>> (see: av_packet_alloc). Using a pointer would prevent us from having
>> sizeof(AVWriter) as part of the ABI, as was done with AVPacket.
> 
> Yes: to return a pointer, you need somewhere to store the structure. One
> of the point of AVWriter is that you can store it on the stack to avoid
> dynamic allocations when the string is short enough.
> 
> Note that AVWriter is exactly two pointers. It will always be two
> pointers, and all the objects that I intend to introduce later will
> always be two pointers: one const pointer for the methods, one pointer
> for the object itself.
> 
> This design is essential to the features I promised for AVWriter and for
> later.
> 

I don't see how you are planning on avoiding dynamic allocations by 
stack-allocating AVWriter when AVWriter itself only contains two pointers.

I also don't see how this design is essential to the features you 
promised in a way that can't be done by just not making sizeof(AVWriter) 
part of the ABI and returning pointers to a struct.

- Leo Izen (thebombzen)

Leo Izen:
> 
> On 8/30/22 15:37, Nicolas George wrote:
>> Leo Izen (12022-08-30):
>>> Is there a reason this is AVWriter wr = foo() and not AVWriter *wr =
>>> foo()?
>>> Most other APIs return pointers to structs, rather than structs
>>> themselves
>>> (see: av_packet_alloc). Using a pointer would prevent us from having
>>> sizeof(AVWriter) as part of the ABI, as was done with AVPacket.
>>
>> Yes: to return a pointer, you need somewhere to store the structure. One
>> of the point of AVWriter is that you can store it on the stack to avoid
>> dynamic allocations when the string is short enough.
>>
>> Note that AVWriter is exactly two pointers. It will always be two
>> pointers, and all the objects that I intend to introduce later will
>> always be two pointers: one const pointer for the methods, one pointer
>> for the object itself.
>>
>> This design is essential to the features I promised for AVWriter and for
>> later.
>>
> 
> I don't see how you are planning on avoiding dynamic allocations by
> stack-allocating AVWriter when AVWriter itself only contains two pointers.
> 
> I also don't see how this design is essential to the features you
> promised in a way that can't be done by just not making sizeof(AVWriter)
> part of the ABI and returning pointers to a struct.
> 

He is not only stack-allocating AVWriter, he also intends to
stack-allocate the actual writers like AVDynbufWriter, AVBufWriter
(AVWriter is just a wrapper around the underlying writers). This means
that no allocations need to be performed for AVBufWriter at all (and due
to the inherent small-string optimization of an AVBPrint, it can also be
avoided for AVDynbufWriter in lots of cases).
If you return pointers to an AVWriter struct, you need to allocate this
struct somewhere, which means that your init/av_dynbuf_writer_wrap has
an allocation that can fail and therefore needs to be checked; and the
struct needs to be freed lateron.

- Andreas

Andreas Rheinhardt (12022-08-31):
> He is not only stack-allocating AVWriter, he also intends to
> stack-allocate the actual writers like AVDynbufWriter, AVBufWriter
> (AVWriter is just a wrapper around the underlying writers). This means
> that no allocations need to be performed for AVBufWriter at all (and due
> to the inherent small-string optimization of an AVBPrint, it can also be
> avoided for AVDynbufWriter in lots of cases).
> If you return pointers to an AVWriter struct, you need to allocate this
> struct somewhere, which means that your init/av_dynbuf_writer_wrap has
> an allocation that can fail and therefore needs to be checked; and the
> struct needs to be freed lateron.

Thanks. I could have been more precise.

AVWriter itself uses a new kind of mini decentralized object system,
where all objects are a pair of pointers, first a const pointer to the
structure of primitive methods and second the structure itself, and are
passed by value. It does not require any future extension, it just works
that way.

This scheme allows to add methods on an existing structure without
altering it. So, any kind of side data, for example, can become an
object with serialize methods, or an object with ref/unref methods, etc.

The various implementations of AVWriter are also designed to be
allocated on the stack by the caller. This time, to avoid being
encumbered by sizeof(struct) for compatibility, I use a different trick:
one of the first field of the structure is its own size, set by whoever
allocated it.

Currently, the code only checks that the size is at least the original
size. If we later extend the structure, the code will have to check if
the fields it tries to access are below the reported size, and if they
are not it will have to manage without them, use a fallback value.

For writers, need for extending the structure is very unlikely. But for
the methods structures, it can happen. Methods structures defined by
earlier applications will just behave as if these methods are
unimplemented.

The ability to allocate on the stack is essential to AVWriter, since it
needs to be lightweight enough that we never have to hesitate to use it.
Of course, it can always be allocated dynamically. I wonder if I should
include functions to allocate dynamically and init av_dynbuf_writer() at
least.

Regards,

On date Wednesday 2022-08-24 17:18:28 +0200, Nicolas George wrote:
> The actual implementation, tests and uses in the rest of
> FFmpeg code will be committed separately once the API is
> settled.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  doc/avwriter_intro.md | 109 ++++++++++
>  libavutil/writer.h    | 484 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 593 insertions(+)
>  create mode 100644 doc/avwriter_intro.md
>  create mode 100644 libavutil/writer.h
> 
[...] 
> diff --git a/libavutil/writer.h b/libavutil/writer.h
> new file mode 100644
> index 0000000000..55e2cf3ea6
> --- /dev/null
> +++ b/libavutil/writer.h
> @@ -0,0 +1,484 @@
> +/*
> + * Copyright (c) 2022 The FFmpeg project
> + *
> + * 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
> + */
> +
> +#ifndef AVUTIL_WRITER_H
> +#define AVUTIL_WRITER_H
> +
> +#include <stdio.h>
> +#include <stddef.h>
> +#include <stdarg.h>
> +
> +#include "extendable.h"
> +#include "bprint.h"
> +
> +/**
> + * @defgroup av_writer AVWriter
> + *
> + * Object-oriented API to write strings and binary data.
> + *
> + * @{
> + */
> +
> +typedef struct AVWriterMethods AVWriterMethods;
> +
> +/**
> + * Opaque object to write strings and binary data.
> + *
> + * AVWriter is meant to allow to build and return strings (and blocks of
> + * binary data) efficiently between functions.

> + * For example, a function that serialize something into a string will

serializeS

> + * actually write into an AVWriter, and the caller will choose the way the
> + * data will be stored or processed on the fly.
> + *
> + * For a quick introduction on how to use AVWriter in simple cases, see
> + * doc/avwriter_intro.md.
> + *
> + * There are various pre-defined types of AVWriter, see below:
> + *
> + * - av_dynbuf_writer() writes the data into a buffer that is grown with
> + *   av_realloc() if it does not fit in the initial buffer; it is the
> + *   recommended choice.
> + *
> + * - av_buf_writer() writes the data into a pre-existing finite buffer.
> + *
> + * - av_stdio_writer() writes the data into a FILE*.
> + *
> + * - av_log_writer() writes the data to av_log().
> + *
> + * AVWriter objects are passed by value. It allows creating a writer on the
> + * fly, without extra variable or error checking. The structure can never
> + * change.
> + */
> +typedef struct AVWriter {
> +    const AVWriterMethods *methods;
> +    void *obj;
> +} AVWriter;
> +
> +/**
> + * Write a data buffer to an AVWriter.
> + */
> +void av_writer_write(AVWriter wr, const char *buf, size_t size);
> +
> +/**
> + * Write a 0-terminated string to an AVWriter.
> + */
> +void av_writer_print(AVWriter wr, const char *str);
> +
> +/**
> + * Write a formatted string to an AVWriter.
> + * Note: do not use libc-specific extensions to the format string.
> + */
> +void av_writer_printf(AVWriter wr, const char *fmt, ...);
> +
> +/**
> + * Write a formatted to string an AVWriter using a va_list.
> + */
> +void av_writer_vprintf(AVWriter wr, const char *fmt, va_list va);
> +
> +/**
> + * Write a sequence of identical chars to an AVWriter.
> + */
> +void av_writer_add_chars(AVWriter wr, char c, size_t n);
> +
> +/**
> + * Flush data that may be buffered in the writer.
> + */
> +void av_writer_flush(AVWriter wr);
> +
> +/**
> + * Return the error status of the writer, an AVERROR code.
> + *
> + * If self_only is non-zero, return errors only on this writer and not on
> + * possible other writers that it got from the caller. If in doubt, use 0.
> + */
> +int av_writer_get_error(AVWriter wr, int self_only);
> +
> +/**
> + * Print a sane value for invalid input.
> + *

> + * This is a flag for all av_something_write() functions: when presented
> + * with an invalid "something", if the flag is not present they should leave

This "something" is vague and can be confused with the "something" in
"av_something_write", maybe something as:
when presented with invalid arguments, if the flag is not set the
called method should leave...

> + * the writer unchanged; if the flag is present they should print a sane
> + * fallback string.
> + */
> +#define AV_WRITER_FALLBACK 0x10000
> +
> +/***************************************************************************/
> +
> +/**
> + * @defgroup av_dynbuf_writer AVDynbufWriter
> + *
> + * An implementation of AVWriter that writes to a dynamic buffer.
> + *
> + * The buffer is kept 0-terminated.
> + *

> + * The buffer is initially kept in the variable itself, it switches to heap

what variable? => in the AVWriter data?

> + * allocation if it grows too large. In that case, av_writer_get_error() can
> + * return AVERROR(ENOMEM) if the allocation fails.
> + *
> + * @{
> + */
> +
> +/**
> + * An AVWriter object for pre-allocated memory buffers.
> + *
> + * Can be allocated on the stack.
> + *
> + * Should be inited with one of the utility functions.
> + */
> +typedef struct AVDynbufWriter {
> +    size_t self_size; /**< Size of the structure itself */
> +    unsigned flags;
> +    AVBPrint bp;
> +} AVDynbufWriter;
> +
> +const AVWriterMethods *av_dynbuf_writer_get_methods(void);
> +int av_dynbuf_writer_check(AVWriter wr);
> +
> +/**
> + * Initialize an AVDynbufWriter.
> + *
> + * @return  dwr itself
> + * dwr->self_size must be set.
> + */
> +AVDynbufWriter *av_dynbuf_writer_init(AVDynbufWriter *dwr);
> +
> +/**
> + * Create an AVWriter from an AVDynbufWriter structure.
> + */
> +AVWriter av_dynbuf_writer_wrap(AVDynbufWriter *dwr);
> +
> +/**
> + * Create an AVWriter to a dynamic buffer.
> + *
> + * Note: as it relies on a compound statement, the AVDynbufWriter object has
> + * a scope limited to the block where this macro is called.
> + */
> +#define av_dynbuf_writer() \
> +    av_dynbuf_writer_wrap(av_dynbuf_writer_init(&FF_NEW_SZ(AVDynbufWriter)))
> +
> +/**
> + * Get a pointer to the buffer data of an AVWriter to a dynamic buffer.
> + *
> + * If not null, size will be set to the size of the data in the buffer.
> + *
> + * Undefined behavior if called with another type of AVWriter.
> + * Undefined behavior if called without checking for error first.
> + */
> +char *av_dynbuf_writer_get_data(AVWriter wr, size_t *size);
> +
> +/**
> + * Get a pointer to the buffer data of an AVWriter to a dynamic buffer
> + * (unsafe version).
> + *
> + * If not null, size will be set to the size of the data in the buffer.
> + *
> + * If the writer is in error, the data may be truncated.
> + *
> + * Undefined behavior if called with another type of AVWriter.
> + */
> +char *av_dynbuf_writer_get_data_unsafe(AVWriter wr, size_t *size);
> +
> +/**
> + * Finalize an AVWriter to a dynamic buffer.
> + *
> + * @arg[out] buf   if not NULL, used to return the buffer contents
> + * @arg[out] size  if not NULL, used to return the size of the data
> + * @return  0 for success or error code (probably AVERROR(ENOMEM))
> + *
> + * In case of error, the buffer will not be duplicated but still freed.
> + * Same if the writer is already in error.
> + *
> + * Undefined behavior if called with another type of AVWriter.
> + */
> +int av_dynbuf_writer_finalize(AVWriter wr, char **buf, size_t *size);
> +
> +/**
> + * Finalize an AVWriter to a dynamic buffer (unsafe version).
> + *
> + * @arg[out] buf   if not NULL, used to return the buffer contents
> + * @arg[out] size  if not NULL, used to return the size of the data
> + * @return  0 for success or error code (probably AVERROR(ENOMEM))
> + *
> + * If the writer is in error, the returned data may be truncated.
> + *
> + * In case of error, the buffer will not be duplicated but still freed.
> + *
> + * Undefined behavior if called with another type of AVWriter.
> + */
> +int av_dynbuf_writer_finalize_unsafe(AVWriter wr, char **buf, size_t *size);
> +
> +/**
> + * Allocate chars in the buffer for external use.
> + *
> + * Undefined behavior if called with another type of AVWriter.
> + */
> +char *av_dynbuf_writer_get_buffer(AVWriter wr, size_t size, size_t *rsize);
> +
> +/**
> + * Advance the position in the buffer.
> + *

> + * size must be <= *rsize on a previous call to
> + * av_dynbuf_writer_get_buffer().

what is *rsize?

> + *
> + * Undefined behavior if called with another type of AVWriter.

> + * Undefined behavior is called not directly after

is called => if called

> + * av_dynbuf_writer_get_buffer().
> + */
> +void av_dynbuf_writer_advance_buffer(AVWriter wr, size_t size);
> +
> +/**
> + * @}
> + */
> +
> +/***************************************************************************/
> +
> +/**
> + * @defgroup av_buf_writer AVBufWriter
> + *
> + * An implementtion of AVWriter that writes into an already-allocated buffer
> + * of memory.
> + *
> + * The buffer is kept 0-terminated. If the buffer is too small, the data is
> + * discarded, but the total size is computed.
> + *
> + * @{
> + */
> +
> +/**
> + * An AVWriter object for pre-allocated memory buffers.
> + *
> + * Can be allocated on the stack.
> + *
> + * Should be inited with one of the utility functions.
> + */
> +typedef struct AVBufWriter {
> +    size_t self_size; /**< Size of the structure itself */
> +    char *buf; /**< Memory buffer. Must not be NULL nor empty. */
> +    size_t size; /**< Size of the memory buffer. Must not be 0. */
> +    size_t pos; /**< Position in the memory buffer. */
> +} AVBufWriter;
> +
> +const AVWriterMethods *av_buf_writer_get_methods(void);
> +int av_buf_writer_check(AVWriter wr);
> +
> +/**
> + * Initialize an AVBufWriter to an already-allocated memory buffer.
> + *
> + * @return  bwr itself
> + * bwr->self_size must be set.
> + */
> +AVBufWriter *av_buf_writer_init(AVBufWriter *bwr, char *buf, size_t size);
> +
> +/**
> + * Create an AVWriter from an AVBufWriter structure.
> + */
> +AVWriter av_buf_writer_wrap(AVBufWriter *bwr);
> +
> +/**
> + * Create an AVWriter to a buffer.
> + *
> + * Note: as it relies on a compound statement, the AVBufWriter object has
> + * a scope limited to the block where this macro is called.
> + */
> +#define av_buf_writer(buf, size) \
> +    av_buf_writer_wrap(av_buf_writer_init(&FF_NEW_SZ(AVBufWriter), (buf), (size)))
> +
> +/**
> + * Create an AVWriter to a char[] or equivalent.
> + *
> + * Note: as it relies on a compound statement, the AVBufWriter object has
> + * a scope limited to the block where this macro is called.
> + */

> +#define av_buf_writer_array(array) \
> +    av_buf_writer(array, sizeof (array))

av_buf_writer_from_array?

> +
> +/**
> + * @}
> + */
> +
> +/***************************************************************************/
> +
> +/**
> + * @defgroup av_stdio_writer AVStdioWriter
> + *
> + * An implementation of AVWriter that writes to stdio.
> + *
> + * @{
> + */
> +
> +const AVWriterMethods *av_stdio_writer_get_methods(void);
> +int av_stdio_writer_check(AVWriter wr);
> +
> +/**
> + * Create an AVWriter that goes to a stdio FILE.
> + */

> +AVWriter av_stdio_writer(FILE *out);

av_file_writer() ?

> +
> +/**
> + * @}
> + */
> +
> +/***************************************************************************/
> +
> +/**
> + * @defgroup av_log_writer AVLogWriter
> + *
> + * An implementation of AVWriter that writes to av_log().
> + *
> + * @{
> + */
> +
> +const AVWriterMethods *av_log_writer_get_methods(void);
> +int av_log_writer_check(AVWriter wr);
> +
> +/**
> + * Create an AVWriter that goes to av_log(obj).
> + */
> +AVWriter av_log_writer(void *obj);
> +
> +/**
> + * Log to a writer.
> + * If wr does not go to av_log(), level is ignored.
> + */
> +void av_log_writer_log(AVWriter wr, int level, const char *fmt, ...);
> +
> +/**
> + * @}
> + */
> +
> +/***************************************************************************/
> +
> +/**
> + * @defgroup av_writer_methods AVWriterMethods
> + *
> + * Structures and utility needed to define new types of AVWriter.
> + *
> + * @{
> + */
> +
> +/**
> + * Set of methods for implementing an AVWriter.
> + *
> + * Applications that want to implement other kinds of AVWriter
> + * need to create a structure with some of these methods implemented.
> + *
> + * Every type of AVWriter is supposed to have a pair of functions:
> + * av_something_writer_get_methods() returns the methods for that type.
> + * av_something_writer_checks() returns 1 if the writer is of that type.
> + *
> + * A macro to define the structure and functions is provided below.
> + *
> + * When a type of AVWriter defines specific functions, it is usually the
> + * responsibility of the caller to ensure that they are called only with a
> + * compatible AVWriter; otherwise the behavior is undefined.
> + */
> +struct AVWriterMethods {
> +
> +    /**
> +     * Size of the structure itself.
> +     * Must normally be set to sizeof(AVWriterMethods).
> +     */
> +    size_t self_size;
> +
> +    /**
> +     * Name of the object type.
> +     */
> +    const char *name;
> +

> +    /**
> +     * Warn that an operation was impossible even with fallbacks.
> +     */
> +    void (*impossible)(AVWriter wr, const char *message);

I'd use a verb for consistency/clarify.
notify_impossibility or notify_impossible_op?

> +
> +    /**
> +     * Notify that size chars have been discarded
> +     */
> +    void (*notify_discard)(AVWriter wr, size_t size);
> +
> +    /**
> +     * Get the error status of the writer
> +     * If self_only is non-zero, return errors only on this writer and not on
> +     * possible other writers that it got from the caller. Errors from
> +     * writers created internally should always be checked.
> +     */
> +    int (*get_error)(AVWriter wr, int self_only);
> +
> +    /**
> +     * Write chars directly.
> +     */
> +    void (*write)(AVWriter wr, const char *buf, size_t size);
> +
> +    /**
> +     * Write a formatted string.
> +     */
> +    void (*vprintf)(AVWriter wr, const char *fmt, va_list ap);
> +
> +    /**
> +     * Get a buffer to write data.

> +     * If *size is returned < min_size, data may get discarded.

If the returned *size is < min_size... ?

> +     * A writer that implements this method must implement advance_buffer too.
> +     * If vprintf is not implemented, av_write_printf() will use
> +     * get_buffer() and vsnprintf(): it will need an extra char in the
> +     * buffer for the 0 that vsnprintf() adds.
> +     */
> +    char *(*get_buffer)(AVWriter wr, size_t min_size, size_t *size);
> +
> +    /**
> +     * Acknowledge chars written in a buffer obtained with get_buffer().

> +     * size is guaranteed <= the size value returned by get_buffer().

size is or should be guaranteed?

> +     */
> +    void (*advance_buffer)(AVWriter wr, size_t size);
> +
[...]

Stefano Sabatini (12022-09-01):
> serializeS

Locally fixed.

> This "something" is vague and can be confused with the "something" in
> "av_something_write", maybe something as:
> when presented with invalid arguments, if the flag is not set the
> called method should leave...

No, the first interpretation was exactly what it meant, it is the same
"something". For exemple, with "something" = "sample format",
av_get_sample_fmt_name(42) returns NULL, av_sample_fmt_write(wr, 42, 0)
writes nothing but av_sample_fmt_write(wr, 42, AV_WRITER_FALLBACK)
writes "invalid".

> what variable? => in the AVWriter data?

Locally changed to:

+ * The buffer is initially kept in the AVWriter data itself, it switches to
+ * heap allocation if it grows too large. In that case,
+ * av_writer_get_error() can return AVERROR(ENOMEM) if the allocation fails.

> > +char *av_dynbuf_writer_get_buffer(AVWriter wr, size_t size, size_t *rsize);

> > + * size must be <= *rsize on a previous call to
> > + * av_dynbuf_writer_get_buffer().
> what is *rsize?

One of the arguments to av_dynbuf_writer_get_buffer(), explained just
above.

> is called => if called

Fixed.

> > +#define av_buf_writer_array(array) \
> > +    av_buf_writer(array, sizeof (array))
> av_buf_writer_from_array?

I would rather keep the name concise, but I will change if other agree
your version is better or if you insist.

> > +AVWriter av_stdio_writer(FILE *out);
> av_file_writer() ?

That would be ambiguous, it would suggest writing to an actual file.

> I'd use a verb for consistency/clarify.
> notify_impossibility or notify_impossible_op?

Locally changed to:

-    void (*impossible)(AVWriter wr, const char *message);
+    void (*notify_impossible)(AVWriter wr, const char *message);

> > +     * If *size is returned < min_size, data may get discarded.
> If the returned *size is < min_size... ?

Locally changed to:

-     * If *size is returned < min_size, data may get discarded.
+     * If the returned *size is < min_size, data may get discarded.

> > +     * size is guaranteed <= the size value returned by get_buffer().
> size is or should be guaranteed?

Is. The documentation is written for somebody who implements the
methods, and it tells them they do not have to bother checking size
here (although an assert would not be bad in this kind of situation). I
added a paragraph in the introduction:

  * Applications that want to implement other kinds of AVWriter
  * need to create a structure with some of these methods implemented.
  *
+ * These methods should only be called directly by the implementation of
+ * AVWriter. Their documentation is written from the point of view of
+ * implementing them.
+ *
  * Every type of AVWriter is supposed to have a pair of functions:
  * av_something_writer_get_methods() returns the methods for that type.
  * av_something_writer_checks() returns 1 if the writer is of that type.

Thanks for the review. I do not think it is necessary to send an updated
patch for these changes, is it?

Regards,

On date Thursday 2022-09-01 15:01:27 +0200, Nicolas George wrote:
[...] 
> Thanks for the review. I do not think it is necessary to send an updated
> patch for these changes, is it?

Not needed, thanks.

As I already said to you in private, I do not think the motivation and
use cases for this have been sufficiently established.

You claim this will bring massive advantages all over the place. You
should support these claims with some actual patches that demonstrate
these advantages on some real code.

Anton Khirnov (12022-09-02):
> As I already said to you in private, I do not think the motivation and
> use cases for this have been sufficiently established.
> 
> You claim this will bring massive advantages all over the place. You
> should support these claims with some actual patches that demonstrate
> these advantages on some real code.

I have not claimed “massive advantages all over the place”, I have
claimed minor advantages all over the places and massive advantages for
future API.

With that correction stated, I have already answered you that your
inability to see the benefits is only proof that your work on FFmpeg is
on areas that rarely need strings. In fact, if I remember correctly, the
last time it happened you used the worst possible implementation and we
had to be multiple persons insisting to get it fixed.

Therefore, I consider your objection to be orders of magnitude less
important than the positive feedback I finally had got from people who
do use strings routinely, who in particular have used BPrint, are
familiar with the benefits it brought but also the limitations I want to
fix.

If there are no more remarks in the next few days I will consider this
approved and start polishing the implementations.

On Wed, 7 Sep 2022, at 15:30, Nicolas George wrote:
> With that correction stated, I have already answered you that your
> inability to see the benefits is only proof that your work on FFmpeg is
[...]
> last time it happened you used the worst possible implementation and we

Please stop attacking people directly, like this. This is absolutely not acceptable.

> Therefore, I consider your objection to be orders of magnitude less
> important than the positive feedback I finally had got from people who

Open source communities are based on consensus. You cannot just discard an opinion
as less important than other just because YOU consider it's not important.

Especially about  someone who is in the top 5 contributor of the project, in the
last few years (with Michael, James, Andreas and Paul of course).

jb

Jean-Baptiste Kempf (12022-09-07):
> Open source communities are based on consensus. You cannot just discard an opinion
> as less important than other just because YOU consider it's not important.
> 
> Especially about  someone who is in the top 5 contributor of the project, in the
> last few years (with Michael, James, Andreas and Paul of course).

I am not the one who has been ignoring the opinion of peers in this
story.

Quoting Nicolas George (2022-09-07 15:30:09)
> Anton Khirnov (12022-09-02):
> > As I already said to you in private, I do not think the motivation and
> > use cases for this have been sufficiently established.
> > 
> > You claim this will bring massive advantages all over the place. You
> > should support these claims with some actual patches that demonstrate
> > these advantages on some real code.
> 
> I have not claimed “massive advantages all over the place”, I have
> claimed minor advantages all over the places and massive advantages for
> future API.
> 
> With that correction stated, I have already answered you that your
> inability to see the benefits is only proof that your work on FFmpeg is
> on areas that rarely need strings. In fact, if I remember correctly, the
> last time it happened you used the worst possible implementation and we
> had to be multiple persons insisting to get it fixed.
> 
> Therefore, I consider your objection to be orders of magnitude less
> important than the positive feedback I finally had got from people who
> do use strings routinely, who in particular have used BPrint, are
> familiar with the benefits it brought but also the limitations I want to
> fix.

If ad hominem is the best argument you have then this clearly needs a
lot more work.

On Wed, Sep 07, 2022 at 03:30:09PM +0200, Nicolas George wrote:
> Anton Khirnov (12022-09-02):
> > As I already said to you in private, I do not think the motivation and
> > use cases for this have been sufficiently established.
> > 
> > You claim this will bring massive advantages all over the place. You
> > should support these claims with some actual patches that demonstrate
> > these advantages on some real code.
> 
> I have not claimed “massive advantages all over the place”, I have
> claimed minor advantages all over the places and massive advantages for
> future API.
> 
> With that correction stated, I have already answered you that your
> inability to see the benefits is only proof that your work on FFmpeg is
> on areas that rarely need strings. In fact, if I remember correctly, the
> last time it happened you used the worst possible implementation and we
> had to be multiple persons insisting to get it fixed.
> 
> Therefore, I consider your objection to be orders of magnitude less
> important than the positive feedback I finally had got from people who
> do use strings routinely, who in particular have used BPrint, are
> familiar with the benefits it brought but also the limitations I want to
> fix.
> 
> If there are no more remarks in the next few days I will consider this
> approved and start polishing the implementations.

My oppinion about AVWriter was asked for, iam not sure my oppinion is
worth that much but ok.

1. I think the escalating tone of the thread is bad for peoples blood
pressure and as well for the eventual technical outcome as it pushed
decissions and support in an emotional direction not a direction fouded
in facts and solid choice.

2. If AVWriter was called bprint 2.0 you might have avoided this whole
opposition. And it really is IMHO a next generation bprint API.

3. You are the author of bprint and this is a from my point of view a
2nd generation bprint. I see nothing controversal on this from a technical
point of view. Its simply a step forward initiated by the original author
of the old code
I think we should move to this bprint 2.0 unless someone has a solid
argument why bprint 2.0 is worse than bprint 1.0. In which case maybe
that issue can then be fixed

thx


[...]

Hi.

I finally have some tome to go at this again.

Totalizing this thread, the previous discussions and the few private
mails I received, I conclude there is significant more support than
opposition to AVWriter, so I am moving forward with polishing and
pushing the implementation.

(To avoid antagonizing people, and because I knew a new job would mean
little time to work on it soon, I refrained from pushing the header, but
consider it done anyway: the fact that the implementation of AVWriter is
not present in FFmpeg is now a bug that needs to be fixed.)

Regards,

On 4/25/2023 2:11 PM, Nicolas George wrote:
> Hi.
> 
> I finally have some tome to go at this again.
> 
> Totalizing this thread, the previous discussions and the few private
> mails I received, I conclude there is significant more support than
> opposition to AVWriter, so I am moving forward with polishing and
> pushing the implementation.
> 
> (To avoid antagonizing people, and because I knew a new job would mean
> little time to work on it soon, I refrained from pushing the header, but
> consider it done anyway: the fact that the implementation of AVWriter is
> not present in FFmpeg is now a bug that needs to be fixed.)
> 
> Regards,

I support the addition of a new better API as a replacement for 
AVBprint, as long as you take into account the suggestions made by 
people and their requests for exemplifications of its usage and 
usefulness. This also means potential changes to the header.
And please, do not ignore or dismiss any of them. No one is out to block 
your work for the sake of it.

Quoting Nicolas George (2023-04-25 19:11:44)
> Hi.
> 
> I finally have some tome to go at this again.
> 
> Totalizing this thread, the previous discussions and the few private
> mails I received, I conclude there is significant more support than
> opposition to AVWriter, so I am moving forward with polishing and
> pushing the implementation.

This project is developed in the open, private emails, rumors, and
hearsay do not count as arguments.

This code is unnecessary bloat that adds nothing of value to the
project.

Anton Khirnov (12023-04-26):
> This project is developed in the open, private emails, rumors, and
> hearsay do not count as arguments.
> 
> This code is unnecessary bloat that adds nothing of value to the
> project.

Your negative opinion has already been taken into consideration, it is
the main reason I write “significant more support than opposition” and
not “no opposition at all”.

James Almer (12023-04-25):
> I support the addition of a new better API as a replacement for AVBprint, as
> long as you take into account the suggestions made by people and their
> requests for exemplifications of its usage and usefulness. This also means
> potential changes to the header.

Of course. If you notice remarks that I neglected to take into
consideration, please remind me of them.

> And please, do not ignore or dismiss any of them. No one is out to block
> your work for the sake of it.

By the way, thanks for intervening on my behalf on Monday.

Regards,