[FFmpeg-devel] [RFC] Shaping the AVTextFormat API Surface

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Stefano Sabatini
> Sent: Sonntag, 27. April 2025 12:42
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [RFC] Shaping the AVTextFormat API Surface
> 
> On date Friday 2025-04-25 13:16:59 +0000, softworkz . wrote:
> [...]
> > Tell me what I should check for and what not in those four groups of
> > functions and for those things which should be checked, tell me
> which
> > way (return error, return silently, allow segfault or use an
> assertion).
> >
> > Then I'll apply that to all those functions in a uniform and
> consistent
> > way without even arguing and the case is closed.
> >
> > I just don't want to leave it alone like now without clear patterns,
> > that's all 😊
> 
> I don't really have an answer.

...still by far the best one.


> Probably it's good to start from the
> docs, so that we have a definition of the semantics in advance, for
> example stating that a pointer should not be NULL and so on so that at
> least we know what is to be considered undefined behavior. As noted by
> Nicolas, the pattern is dependant on the function behavior and on
> practical ergonomy considerations.
> 
> It also would be nice to have a good set of guidelines.

Exactly. That's one of the things I would like to work out here.


[..]

> This might fail in several ways: bikeshed might be NULL or invalid
> (e.g. a pointer to an unrelated structure), level might be invalid
> (e.g. negative or >MAX_SLICE_LEVEL) or the bikeshed might contain
> already too many slices.
> 
> The level might be checked by the programmer, so we might decide to
> have an assert. About the count check it is validated from within the
> function (since we need to access the bikeshed context) so we want to
> provide feedback and fail.
> 
> For both of these two examples, doing nothing does not seem a good
> idea. That's probably only good if we want to enable idem-potency or
> when one of the parameter can be interpreted as a "none" argument.
> 
> For example:
>    if (color == NULL) {
>        return 0;
>    }
> 
> In this case we should specify the behavior in the documentation,
> since that defines what is the undefined behavior and the input
> expectactions.

This all makes sense and the practical part is now to apply that kind
of considerations to the individual APIs we have.

Probably it's best when I start by making a suggestion as a starting
point, then we can refine it from there:


1. AVTextFormatter Implementations
==================================

print_section_header(AVTextFormatContext *tctx, const void *data);
print_section_footer(AVTextFormatContext *tctx);
print_integer(AVTextFormatContext *tctx, const char * key, int64_t);
print_string(AVTextFormatContext *tctx, const char *key, const char *value);

Rules

- assert tctx and key
- data and value can be null



2. AVTextWriter Implementations
===============================

writer_w8(AVTextWriterContext *wctx, int b);
writer_put_str(AVTextWriterContext *wctx, const char *str);
writer_vprintf(AVTextWriterContext *wctx, const char *fmt, va_list vl);


Rules

- assert wctx
- str, fmt, vl - ?




3. TextFormat API
=================


avtext_print_section_header(*tctx, const void *data, int section_id)
avtext_print_section_footer(*tctx)
avtext_print_integer(*tctx, const char *key, int64_t val)
avtext_print_integer_flags(*tctx, const char *key, int64_t val, int flags)
avtext_print_unit_int(*tctx, const char *key, int value, const char *unit)
avtext_print_rational(*tctx, const char *key, AVRational q, char sep)
avtext_print_time(*tctx, const char *key, int64_t ts, const AVRational *time_base, int is_duration)
avtext_print_ts(*tctx, const char *key, int64_t ts, int is_duration)
avtext_print_string(*tctx, const char *key, const char *val, int flags)
avtext_print_data(*tctx, const char *key, const uint8_t *data, int size)
avtext_print_data_hash(*tctx, const char *key, const uint8_t *data, int size)
avtext_print_integers(*tctx, const char *key, uint8_t *data, int size,
                      const char *format, int columns, int bytes, int offset_add)


Rules

- assert tctx and key
- how about uint8_t *data, unit and val in ..print_string?



4. TextWriter API
=================

avtextwriter_context_open(AVTextWriterContext **pwctx, const AVTextWriter *writer)
avtextwriter_context_close(AVTextWriterContext **pwctx)
avtextwriter_create_stdout(AVTextWriterContext **pwctx)
avtextwriter_create_avio(AVTextWriterContext **pwctx, AVIOContext *avio_ctx, int close_on_uninit)
avtextwriter_create_file(AVTextWriterContext **pwctx, const char *output_filename)
avtextwriter_create_buffer(AVTextWriterContext **pwctx, AVBPrint *buffer)


Rules

- **pwctx: leave unchecked
- writer: return AVERROR(EINVAL)
- avio_ctx: assert
- output_filename: log error and return EINVAL
- buffer: assert ?


5. General
==========

Assertions

Which assert - av_assert0() ?


Public/Private


Looking at AVTextFormatContext - should we start thinking about 
which members we would (at least logically) consider public and
which as non-public?


Thanks,
sw