[FFmpeg-devel] [PATCH v10 06/15] fftools/textformat: Introduce AVTextFormatOptions for avtext_context_open()

[Stefano Sabatini]

On date Sunday 2025-05-04 02:57:17 +0000, softworkz wrote:
> From: softworkz <softworkz at hotmail.com>
> 
> This allows future addition of options without
> changes to the signature of avtext_context_open().
> 
> Reviewed-by: Stefano Sabatini <stefasab at gmail.com>
> Signed-off-by: softworkz <softworkz at hotmail.com>
> ---
>  fftools/ffprobe.c                 | 13 +++++++++----
>  fftools/textformat/avtextformat.c | 21 ++++++++-------------
>  fftools/textformat/avtextformat.h | 16 +++++++++-------
>  3 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> index f5c83925b9..1277b1e4f9 100644
> --- a/fftools/ffprobe.c
> +++ b/fftools/ffprobe.c
> @@ -3168,10 +3168,15 @@ int main(int argc, char **argv)
>      if (ret < 0)
>          goto end;
>  
> -    if ((ret = avtext_context_open(&tctx, f, wctx, f_args,
> -                           sections, FF_ARRAY_ELEMS(sections), show_value_unit,
> -                            use_value_prefix, use_byte_value_binary_prefix, use_value_sexagesimal_format,
> -                            show_optional_fields, show_data_hash)) >= 0) {

> +    AVTextFormatOptions tf_options = {
> +        .show_optional_fields = show_optional_fields,
> +        .show_value_unit = show_value_unit,
> +        .use_value_prefix = use_value_prefix,
> +        .use_byte_value_binary_prefix = use_byte_value_binary_prefix,
> +        .use_value_sexagesimal_format = use_value_sexagesimal_format,
> +    };
> +
> +    if ((ret = avtext_context_open(&tctx, f, wctx, f_args, sections, FF_ARRAY_ELEMS(sections), tf_options, show_data_hash)) >= 0) {
>          if (f == &avtextformatter_xml)
>              tctx->string_validation_utf8_flags |= AV_UTF8_FLAG_EXCLUDE_XML_INVALID_CONTROL_CODES;
>  
> diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
> index b2c3aa3fc7..91469ef576 100644
> --- a/fftools/textformat/avtextformat.c
> +++ b/fftools/textformat/avtextformat.c
> @@ -125,13 +125,7 @@ void avtext_context_close(AVTextFormatContext **ptctx)
>  
>  
>  int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *formatter, AVTextWriterContext *writer_context, const char *args,
> -                        const 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)
> +                        const AVTextFormatSection *sections, int nb_sections, AVTextFormatOptions options, char *show_data_hash)
>  {
>      AVTextFormatContext *tctx;
>      int i, ret = 0;
> @@ -154,11 +148,11 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
>          goto fail;
>      }
>  
> -    tctx->show_value_unit = show_value_unit;
> -    tctx->use_value_prefix = use_value_prefix;
> -    tctx->use_byte_value_binary_prefix = use_byte_value_binary_prefix;
> -    tctx->use_value_sexagesimal_format = use_value_sexagesimal_format;
> -    tctx->show_optional_fields = show_optional_fields;
> +    tctx->show_value_unit = options.show_value_unit;
> +    tctx->use_value_prefix = options.use_value_prefix;
> +    tctx->use_byte_value_binary_prefix = options.use_byte_value_binary_prefix;
> +    tctx->use_value_sexagesimal_format = options.use_value_sexagesimal_format;
> +    tctx->show_optional_fields = options.show_optional_fields;
>  
>      if (nb_sections > SECTION_MAX_NB_SECTIONS) {
>          av_log(tctx, AV_LOG_ERROR, "The number of section definitions (%d) is larger than the maximum allowed (%d)\n", nb_sections, SECTION_MAX_NB_SECTIONS);
> @@ -201,7 +195,7 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
>          av_dict_free(&opts);
>      }
>  
> -    if (show_data_hash)
> +    if (show_data_hash) {
>          if ((ret = av_hash_alloc(&tctx->hash, show_data_hash)) < 0) {
>              if (ret == AVERROR(EINVAL)) {
>                  const char *n;
> @@ -212,6 +206,7 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
>              }
>              goto fail;
>          }
> +    }
>  
>      /* validate replace string */
>      {
> diff --git a/fftools/textformat/avtextformat.h b/fftools/textformat/avtextformat.h
> index 8ff503401a..87f57d8c24 100644
> --- a/fftools/textformat/avtextformat.h
> +++ b/fftools/textformat/avtextformat.h
> @@ -117,17 +117,19 @@ struct AVTextFormatContext {
>      unsigned int string_validation_utf8_flags;
>  };
>  
> +typedef struct AVTextFormatOptions {
> +    int show_optional_fields;
> +    int show_value_unit;
> +    int use_value_prefix;
> +    int use_byte_value_binary_prefix;
> +    int use_value_sexagesimal_format;
> +} AVTextFormatOptions;

I'm not yet convinced this is needed - why not to use a flags field as
in most other places?

Also if we go with this we should use bool since it's used already in
other places.