> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of
> Stefano Sabatini
> Sent: Sonntag, 27. April 2025 12:42
> To: FFmpeg development discussions and patches <ffmpeg-
> de...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
