[FFmpeg-devel] [PATCH v3 1/7] fftools/textformat: Extract and generalize textformat api from ffprobe.c

[Soft Works]

> -----Original Message-----
> From: Stefano Sabatini <stefasab at gmail.com>
> Sent: Samstag, 8. März 2025 15:37
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Cc: Soft Works <softworkz-at-hotmail.com at ffmpeg.org>; softworkz
> <softworkz at hotmail.com>; Andreas Rheinhardt
> <andreas.rheinhardt at outlook.com>
> Subject: Re: [FFmpeg-devel] [PATCH v3 1/7] fftools/textformat: Extract
> and generalize textformat api from ffprobe.c
> 
> On date Saturday 2025-03-01 10:01:58 +0000, softworkz wrote:
> [...]
> 
> > +int avtext_context_open(AVTextFormatContext **ptctx, const
> AVTextFormatter *formatter, AVTextWriterContext *writer, const char
> *args,
> > +                        const struct AVTextFormatSection *sections,
> int nb_sections,
> > +                        int show_value_unit,
> > +                        int use_value_prefix,
> > +                        int use_byte_value_binary_prefix,
> > +                        int use_value_sexagesimal_format,
> > +                        int show_optional_fields,
> > +                        char *show_data_hash);
> 
> writer -> writer_ctx?
> 
> I'm fine with changing this later to avoid massive rebase edits.
> 
> Also I notice there is some of the usual inconsistencies here:
> av_X_Y against avXY and avX_Y that we have in the rest of the code.
> 
> Maybe let's stick to avX_Y or to av_X_Y.
> 
> Also this might be:
> av_text_format_open(...)
> av_text_format_close(...)
> av_text_format_print_X(...)
> 
> Or to simplify we can just call the structure AVTextContext (I see
> text as an evolution of a string, meant for structured formatted data,
> which is implied by the fact that we need a formatter) and simplify
> related functions naming to:
> av_text_open(...)
> av_text_close(...)
> av_text_print_X(...)
> 
> av_text_formatter_...
> av_text_writer_open...
> av_text_writer_close...
> 
> In fact I don't think there is much gain in keeping "context" in the
> name of the functions.
> 
> What do you think?
> 
> Again, since this is not public API (yet?) this should not be
> considered a blocker (also I've been out of touch with FFmpeg and I
> might be not aware of API conventions evolution).


Hi Stefano,

regarding the API naming I thought that maybe it's better to get it straight right away. To make it fully consistent as mentioned, we should look at all "public" API names. Here's a complete list:


# "avtexformat.h"

## Defines

AV_TEXTFORMAT_FLAG_SUPPORTS_OPTIONAL_FIELDS
                                        => AV_TEXTFORMAT_FLAG_SUPPORTS_OPTIONAL_FIELDS
AV_TEXTFORMAT_FLAG_SUPPORTS_MIXED_ARRAY_CONTENT
                                        => AV_TEXTFORMAT_FLAG_SUPPORTS_MIXED_ARRAY_CONTENT

AV_TEXTFORMAT_PRINT_STRING_OPTIONAL     => AV_TEXTFORMAT_PRINT_STRING_OPTIONAL
AV_TEXTFORMAT_PRINT_STRING_VALIDATE     => AV_TEXTFORMAT_PRINT_STRING_VALIDATE

## Enum

StringValidation                        => StringValidation
AV_TEXTFORMAT_STRING_VALIDATION_FAIL    => AV_TEXTFORMAT_STRING_VALIDATION_FAIL
AV_TEXTFORMAT_STRING_VALIDATION_REPLACE => AV_TEXTFORMAT_STRING_VALIDATION_REPLACE
AV_TEXTFORMAT_STRING_VALIDATION_IGNORE  => AV_TEXTFORMAT_STRING_VALIDATION_IGNORE
AV_TEXTFORMAT_STRING_VALIDATION_NB      => AV_TEXTFORMAT_STRING_VALIDATION_NB

## Structs

AVTextFormatSection                     => AVTextFormatSection

AVTextFormatter                         => AVTextFormatter
AVTextFormatContext                     => AVTextFormatContext

## Functions

avtext_context_open()                   => avtext_context_open()
avtext_context_close()                  => avtext_context_close()

avtext_print_section_header()           => avtext_print_section_header()
avtext_print_section_footer()           => avtext_print_section_footer()
avtext_print_integer()                  => avtext_print_integer()
avtext_print_string()                   => avtext_print_string()
avtext_print_unit_int()                 => avtext_print_unit_int()
avtext_print_rational()                 => avtext_print_rational()
avtext_print_time()                     => avtext_print_time()
avtext_print_ts()                       => avtext_print_ts()
avtext_print_data()                     => avtext_print_data()
avtext_print_data_hash()                => avtext_print_data_hash()
avtext_print_integers()                 => avtext_print_integers()
avtext_get_formatter_by_name()          => avtext_get_formatter_by_name()


# "avtextwriters.h"

## Structs

AVTextWriter                            => AVTextWriter
AVTextWriterContext                     => AVTextWriterContext

## Functions

avtextwriter_context_open()             => avtextwriter_context_open()
avtextwriter_context_close()            => avtextwriter_context_close()

avtextwriter_create_stdout()            => avtextwriter_create_stdout()
avtextwriter_create_avio()              => avtextwriter_create_avio()
avtextwriter_create_file()              => avtextwriter_create_file()
avtextwriter_create_buffer()            => avtextwriter_create_buffer()



If you could edit the right sides in the way you think it should be, I'll make the changes accordingly.

Thanks,
sw