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

["softworkz ." <softworkz-at-hotmail.com@ffmpeg.org>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Stefano
> Sabatini
> Sent: Dienstag, 29. April 2025 00:27
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [RFC] Shaping the AVTextFormat API Surface
> 
> 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
> > >

[..]

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

This is handled by doing nothing in that case. 
The functions are all void. Changing that and checking the return 
values would probably blow-up the printing code in ffprobe.c to 200%
or 300% (when for all) or still a large amount when only for 
section header and footer.

"Do nothing" has always been a behavior for that case, but it wasn't
implemented consistently.

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

That's one of the big questions, whether this should be public.
I'd rather say no - not for consumption. It's rather public in the 
sense that a lib consumer could create their own implementations
and supply it to av_textformat_open(), then use the other textformat
apis to write output - but not by calling the implementation functions
of AVTextWriter directly.



> >
> > Rules
> >
> > - assert wctx
> > - str, fmt, vl - ?
> 
> Can the operation fail? Should we return an error code?

Our implementation functions return void and so do most of
the upstream functions that the writers are calling.

Same as above, probably not economic to add checks for all invocations.


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

Yea, I'll try.


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

That's not my addition. It is used in ffprobe.c for DisplayMatrix.


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

Ok

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

Both is happening already.


> > - buffer: assert ?
> 
> unless it makes sense to support an empty buffer?

I believe it does not.

> 
> >
> > 5. General
> > ==========
> >
> > Assertions
> >
> 
> > Which assert - av_assert0() ?
> 
> they are once-checks, therefore no performance critical, so yes

Alright, thanks.


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

I meant that the final result would be using nested structs like it's
done in several places meanwhile.
But for now, I just meant to do something like adding a comment line
saying something like "members below this line are not part of the
API.


Thanks again,
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-request@ffmpeg.org with subject "unsubscribe".