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

[Stefano Sabatini <stefasab@gmail.com>]

On date Sunday 2025-04-27 17:54:21 +0000, softworkz . wrote:
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Stefano Sabatini
> > Sent: Sonntag, 27. April 2025 12:42
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@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

Also: should we return en error in case of invalid nesting level?
This is context dependent so maybe this should be a recoverable error
- my guess is yes although this means complicating usage.

> 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);

assuming this is directly used by a programmer, the variadic variant
might also make sense

> 
> 
> Rules
> 
> - assert wctx
> - str, fmt, vl - ?

Can the operation fail? Should we return an error code?
 
> 
> 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)

a single variant might do (as we have a single print_string)

> 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)

is this really needed? also this seems a complication as it implies
tabular format

> 
> 
> Rules
> 
> - assert tctx and key

> - how about uint8_t *data, unit and val in ..print_string?

what are the current use cases? Can we have empty data/unit/val? Do we
need to support null semantics? I seem to remember we do, let's check.

> 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

or better propagate the failure from open (see libavutil/open_file)

> - buffer: assert ?

unless it makes sense to support an empty buffer?

> 
> 5. General
> ==========
> 
> Assertions
> 

> Which assert - av_assert0() ?

they are once-checks, therefore no performance critical, so yes
  
> Public/Private
> 
> 
> Looking at AVTextFormatContext - should we start thinking about 
> which members we would (at least logically) consider public and
> which as non-public?

From what I know there are no public/non-public fields in FF structs,
but we can extend them with private data/class to be handled in
specialization code if needed.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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