On date Sunday 2025-04-27 17:54:21 +0000, softworkz . wrote:
>
>
> > -----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
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-requ...@ffmpeg.org with subject "unsubscribe".
