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

[Stefano Sabatini <stefasab@gmail.com>]

On date Thursday 2025-05-08 00:25:28 +0000, softworkz . wrote:
> > -----Original Message-----
> > From: Stefano Sabatini <stefasab@gmail.com>
> > Sent: Donnerstag, 8. Mai 2025 02:06
> > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Cc: softworkz <softworkz@hotmail.com>
> > Subject: Re: [FFmpeg-devel] [PATCH v10 06/15] fftools/textformat: Introduce
> > AVTextFormatOptions for avtext_context_open()
> > 
> > On date Sunday 2025-05-04 02:57:17 +0000, softworkz wrote:
> > > From: softworkz <softworkz@hotmail.com>
> > >
> > > This allows future addition of options without
> > > changes to the signature of avtext_context_open().
> > >
> > > Reviewed-by: Stefano Sabatini <stefasab@gmail.com>
> > > Signed-off-by: softworkz <softworkz@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?
> 
> Believe it or not, I actually had flags initially, but then I figured 
> how ugly this code:
> 
> > > +    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,
> > > +    };
> > > +
> 

> ..would look like when assembling flags from the settings values and then
> I changed to this way, which looks much cleaner and nicer.
> This also allows adding non-bool options in the future without changing the
> signature of avtextformat_open().

My concern was mostly about this not being very FFmpeg-ish as this is
a pattern not very much applied in FFmpeg (probably the only
instance?). A more FFmpeg-ish solution would be to have a context with
options. Anyway feel free to consider this an idea for a further
improvement rather than a blocker.

> > Also if we go with this we should use bool since it's used already in
> > other places.
> 
> Is this allowed in FFmpeg? I thought I had it some years ago in a patch
> and it was rejected (just vague memory, though).

Right, I probably was mixing different things in my mind.
_______________________________________________
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".