From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <ffmpeg-devel-bounces@ffmpeg.org>
Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100])
by master.gitmailbox.com (Postfix) with ESMTPS id D03C54C56F
for <ffmpegdev@gitmailbox.com>; Thu, 8 May 2025 21:38:51 +0000 (UTC)
Received: from [127.0.1.1] (localhost [127.0.0.1])
by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id D1F4568C155;
Fri, 9 May 2025 00:38:46 +0300 (EEST)
Received: from mail-wm1-f53.google.com (mail-wm1-f53.google.com
[209.85.128.53])
by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 3ED1F68BA2A
for <ffmpeg-devel@ffmpeg.org>; Fri, 9 May 2025 00:38:41 +0300 (EEST)
Received: by mail-wm1-f53.google.com with SMTP id
5b1f17b1804b1-441ab63a415so14790385e9.3
for <ffmpeg-devel@ffmpeg.org>; Thu, 08 May 2025 14:38:41 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=gmail.com; s=20230601; t=1746740320; x=1747345120; darn=ffmpeg.org;
h=user-agent:in-reply-to:content-disposition:mime-version:references
:mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc
:subject:date:message-id:reply-to;
bh=I0uDV6TStR+xiLe/WVcv/yrau26SaIgoOzVJrRdY2nU=;
b=IKAAD5f8AaNQ1gUwz8/LlbV7yXV7b3UbIwvEQKNB9fU1QKoFPUHuHVXzyqVktRqvkw
fPzahxTRg1kMtkxt7c3gNpF7sXaWhY9u1DyuvGEltMKORR5mfC9ymK0JY1+Sq0KyEEc5
o8eZwRhFOM6fiZU/m0XfazWUZ9ViPNPkxBNbS2cAjQP46r+XleiEmLU+fXroanfXhRVf
aizZfYVtKaAiFkdefJbxlwXpJbcoUQ7PWFcWYYosPohMlMBOgiglWhl0Ueq+Pv/dJr/R
CKiFDmZJV+Rq42dKeVZaOy78dg5wSFAYIyQ7YYIm+oefkjifzZQ0qLWQX7zOQinG6EVR
siyg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20230601; t=1746740320; x=1747345120;
h=user-agent:in-reply-to:content-disposition:mime-version:references
:mail-followup-to:message-id:subject:cc:to:from:date
:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;
bh=I0uDV6TStR+xiLe/WVcv/yrau26SaIgoOzVJrRdY2nU=;
b=dlm2u7sp3h+IpkF0Bg2+2vpRHapagogNMK3P6nVjMn6r9c6QTxj/yj89GOn8Q7yd71
Z7aPWIWoUiBmYKe1c+rnKfdMGwidPwtJOHIaGCFNA3f21Cq/U819ya7TH2g6tyT+2Xvc
BH1W9NaLJ1I7/B/UhTh9yCnasGKfq3CyjDubnlfrbhMdVTIwfKuN3vDF66IY4D/n0ZC7
7fSXz16g3+dlJqsplXtn9EKgWr8V65XFqsLJVvHitOmjQi5/6fIalmOuUrqjAK/ydFYv
/DCiNe1xFYHkBhPKhW5h0HZejeAJPNu0j6C+JcHlPzEUBf5QoxjDMTrrj8HkrwZI75OQ
qlUg==
X-Gm-Message-State: AOJu0Yw7zYCgSdzHB8rrAanao10jBfTCWAiPLULG6MJFf88anBppI/Mo
jASk7Dy7GcMJEphOIYKn4SPYwLMJpWrEGI3BGqAvDiP+c7zqgT8dXoF3bw==
X-Gm-Gg: ASbGnctLAdCoTjHfa36P6RqUcdJoaCq8W2AYlmsrBtUjtbtIgxdTxjN1YZHKzLRZot6
745w6F5K239VjAHLF0lMlICVE5eb+4oC+Atv9TlE+qbKujOI24i3VFga1IGkTPhLjIt6mN3nd+X
I+tP+Y4iQYEmwgHSvgRDr6XToWmlrSvejJFdR2+zgCtw8eAqisHLPN4E2rhyIhncxgBeoDHEpys
YVVUCR+PhS+xT/oASQ3mEw6b0yA7/wKLjj3MX7amrOGKcboeurWUz/jCGmi4G2VNmJ5EZ9ckS4E
eVvRS7/AzpZfaXKvMNVAvL4T09VttXbbUcgE3OOZ3AaMrxPKynACNKwfr6O1CpQtE4zOqMdWN9v
rAtju
X-Google-Smtp-Source: AGHT+IHLj1aNa7c2RisVWzJuAgb45grevbCi+hcgHUUWIyWKBv8Q8nzOjQbgnUjWJsfOW2aYQk+bYw==
X-Received: by 2002:a05:600c:8285:b0:43d:49eb:9675 with SMTP id
5b1f17b1804b1-442d6dd8630mr6353875e9.22.1746740319886;
Thu, 08 May 2025 14:38:39 -0700 (PDT)
Received: from mariano (dynamic-adsl-84-220-189-10.clienti.tiscali.it.
[84.220.189.10]) by smtp.gmail.com with ESMTPSA id
5b1f17b1804b1-442d687ad31sm7515575e9.34.2025.05.08.14.38.39
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Thu, 08 May 2025 14:38:39 -0700 (PDT)
Received: by mariano (Postfix, from userid 1000)
id 2F293BFCE8; Thu, 8 May 2025 23:38:38 +0200 (CEST)
Date: Thu, 8 May 2025 23:38:38 +0200
From: Stefano Sabatini <stefasab@gmail.com>
To: "softworkz ." <softworkz@hotmail.com>
Message-ID: <aB0kXscb6RbcloYb@mariano>
Mail-Followup-To: "softworkz ." <softworkz-at-hotmail.com@ffmpeg.org>,
FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
References: <pull.66.v9.ffstaging.FFmpeg.1746260565.ffmpegagent@gmail.com>
<pull.66.v10.ffstaging.FFmpeg.1746327446.ffmpegagent@gmail.com>
<3aa16bc39f30554808c946673dd3862380314788.1746327446.git.ffmpegagent@gmail.com>
<aBv1V3pmE0iRt7Lq@mariano>
<DM8P223MB0365C851CD4090830CB64D7FBA8BA@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <DM8P223MB0365C851CD4090830CB64D7FBA8BA@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM>
User-Agent: Mutt/2.1.4 (2021-12-11)
Subject: Re: [FFmpeg-devel] [PATCH v10 06/15] fftools/textformat: Introduce
AVTextFormatOptions for avtext_context_open()
X-BeenThere: ffmpeg-devel@ffmpeg.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: FFmpeg development discussions and patches <ffmpeg-devel.ffmpeg.org>
List-Unsubscribe: <https://ffmpeg.org/mailman/options/ffmpeg-devel>,
<mailto:ffmpeg-devel-request@ffmpeg.org?subject=unsubscribe>
List-Archive: <https://ffmpeg.org/pipermail/ffmpeg-devel>
List-Post: <mailto:ffmpeg-devel@ffmpeg.org>
List-Help: <mailto:ffmpeg-devel-request@ffmpeg.org?subject=help>
List-Subscribe: <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>,
<mailto:ffmpeg-devel-request@ffmpeg.org?subject=subscribe>
Reply-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: ffmpeg-devel-bounces@ffmpeg.org
Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org>
Archived-At: <https://master.gitmailbox.com/ffmpegdev/aB0kXscb6RbcloYb@mariano/>
List-Archive: <https://master.gitmailbox.com/ffmpegdev/>
List-Post: <mailto:ffmpegdev@gitmailbox.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".