[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"); + use_string(av_dynbuf_writer_get_data(wr, NULL)); + av_dynbuf_writer_finalize(wr, NULL, NULL); +``` + +If the string is small enough, no dynamic memory allocation happens. + +The NULL to `av_dynbuf_writer_get_data()` can be used to retrieve the size +of the data in the buffer. + +Calling `av_writer_get_error()` is mandatory. + +## I want a *persistent* `char*` buffer, the function wants an AVWriter + +Old-style code: + +``` + char *buf; + ret = av_something_to_string(&buf, something); + if (ret < 0) + die("Failed"); + ctx->string = buf; +``` + +Equivalent code with AVWriter: + +``` + AVWriter wr = av_dynbuf_writer(); + av_something_write(wr, something, 0); + ret = av_dynbuf_writer_finalize(wr, &ctx->string, NULL); + if (ret < 0) + die("Failed"); +``` + +## I have a `char[]` buffer, the function wants an AVWriter + +Old-style code: + +``` + char buf[BUF_SIZE]; + av_something_to_string(buf, sizeof(buf), something); + use_string(buf); +``` + +Equivalent code with AVWriter: + +``` + char buf[BUF_SIZE]; + av_something_write(av_buf_writer(buf, sizeof(buf)), something, 0); + use_string(buf); +``` + +## I need to build a string from parts + +Old-style code: + +``` + char buf[1024]; + int pos = 0; + pos += snprintf(buf + pos, sizeof(buf) - pos, + "Stream %d: ", i); + av_get_channel_layout_string(buf + pos, sizeof(buf) - pos, + nb, layout); + pos += strlen(buf + pos); + pos += snprintf(buf + pos, sizeof(buf) - pos, + ", %s", av_get_sample_fmt_name(fmt)); +``` + +Note: this code could overflow the buffer. + +Equivalent code with AVWriter: + +``` + AVWriter wr = av_dynbuf_writer(); + av_writer_printf(wr, "Stream %d: ", i); + av_channel_layout_write(wr, nb, layout, 0); + av_writer_printf(wr, ", %s", av_get_sample_fmt_name(fmt)); +``` + +See the first example on how to access the resulting string. + +Note: this is very similar to using AVBPrint; from this side, AVWriter +replaces AVBPrint. 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 + * 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 + * 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 + * 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(). + * + * Undefined behavior if called with another type of AVWriter. + * Undefined behavior is called not directly after + * 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)) + +/** + * @} + */ + +/***************************************************************************/ + +/** + * @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); + +/** + * @} + */ + +/***************************************************************************/ + +/** + * @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); + + /** + * 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. + * 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(). + */ + void (*advance_buffer)(AVWriter wr, size_t size); + + /** + * Flush immediately data that may be buffered in the writer. + */ + void (*flush)(AVWriter wr); + +}; + +/** + * Convenience macro for the boilerplate necessary to define a writer class. + * + * @arg qual type qualifier for for the symbols, for example "static" + * @arg type class name for the type of writer, + * for example "AVBufWriter" or "MyWriter" + * @arg prefix prefix for the symbols, + * for example "av_buf_writer" or "my_writer" + */ +#define AV_WRITER_DEFINE_METHODS(qual, type, prefix) \ +static const AVWriterMethods prefix##_methods; \ +qual const AVWriterMethods *prefix##_get_methods(void) { \ + return &prefix##_methods; \ +} \ +qual int prefix##_check(AVWriter wr) { \ + return wr.methods == prefix##_get_methods(); \ +} \ +static const AVWriterMethods prefix##_methods = + +/** + * @} + */ + +/** + * @} + */ + +#endif /* AVUTIL_WRITER_H */ -- 2.35.1
-----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
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, -- Nicolas George
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, -- Nicolas George
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, -- Nicolas George
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, -- Nicolas George
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
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. -- Nicolas George
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 - President +33 672 704 734
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. -- Nicolas George
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. -- Anton Khirnov
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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Nations do behave wisely once they have exhausted all other alternatives. -- Abba Eban
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, -- Nicolas George
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.
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, -- Nicolas George
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
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”. -- Nicolas George
participants (9)
-
Andreas Rheinhardt -
Anton Khirnov -
James Almer -
Jean-Baptiste Kempf -
Leo Izen -
Michael Niedermayer -
Nicolas George -
Soft Works -
Stefano Sabatini