| Message ID | 20251120134101.72892-1-isaac.scott@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Isaac Scott (2025-11-20 13:41:01) > It is beneficial to have the option during development to disable and > enable algorithms via the tuning file without having to delete their > entries. > > Add support for an optional "enabled" parameter to accomplish this. > > Usage example: > version: 1 > algorithms: > - Agc: > enabled: true > - Awb: > enabled: false > > This will enable AGC, and disable AWB. If the enabled flag is not > present, the algorithm will be enabled. > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > --- > > v1 of this patch can be found here: > > https://patchwork.libcamera.org/patch/23299/ > > Changelog: > > v1 -> v2: > - Moved parsing to createAlgorithms() instead of createAlgorithm() > - Changed "disabled" flag to "enabled: [boolean]" > --- > src/ipa/libipa/module.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > index 0fb51916f..f8c4bb664 100644 > --- a/src/ipa/libipa/module.h > +++ b/src/ipa/libipa/module.h > @@ -55,6 +55,15 @@ public: > return -EINVAL; > } > > + if (algo.contains("enabled")) { > + if (algo["enabled"].get<bool>() == false) { Can we simplify this to a single if ? I think something like this might work, could you test/check it please? if (algo["enabled"].get<bool>(true) == false) { LOG() << ...; continue; } -- Kieran > + LOG(IPAModuleAlgo, Debug) > + << "Algorithm " << i > + << " is disabled via tuning file"; > + continue; > + } > + } > + > int ret = createAlgorithm(context, algo); > if (ret) { > algorithms_.clear(); > -- > 2.43.0 >
Hi 2025. 11. 20. 14:41 keltezéssel, Isaac Scott írta: > It is beneficial to have the option during development to disable and > enable algorithms via the tuning file without having to delete their > entries. > > Add support for an optional "enabled" parameter to accomplish this. > > Usage example: > version: 1 > algorithms: > - Agc: > enabled: true > - Awb: > enabled: false > > This will enable AGC, and disable AWB. If the enabled flag is not > present, the algorithm will be enabled. > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > --- > > v1 of this patch can be found here: > > https://patchwork.libcamera.org/patch/23299/ > > Changelog: > > v1 -> v2: > - Moved parsing to createAlgorithms() instead of createAlgorithm() > - Changed "disabled" flag to "enabled: [boolean]" > --- > src/ipa/libipa/module.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > index 0fb51916f..f8c4bb664 100644 > --- a/src/ipa/libipa/module.h > +++ b/src/ipa/libipa/module.h > @@ -55,6 +55,15 @@ public: > return -EINVAL; > } > > + if (algo.contains("enabled")) { > + if (algo["enabled"].get<bool>() == false) { The operator[] returns a special empty object if the key is not present, so I think the above can be simplified as follows: if (!algo["enabled"].get(true)) { ... continue; } In any case I'm a bit concerned about the `createAlgorithm()` function: const auto &[name, algoData] = *data.asDict().begin(); this seems very fragile, e.g. - enabled: true Awb: wouldn't work. Regards, Barnabás Pőcze > + LOG(IPAModuleAlgo, Debug) > + << "Algorithm " << i > + << " is disabled via tuning file"; > + continue; > + } > + } > + > int ret = createAlgorithm(context, algo); > if (ret) { > algorithms_.clear();
Hi Isaac, On Thu, Nov 20, 2025 at 01:41:01PM +0000, Isaac Scott wrote: > It is beneficial to have the option during development to disable and > enable algorithms via the tuning file without having to delete their > entries. > > Add support for an optional "enabled" parameter to accomplish this. > > Usage example: > version: 1 > algorithms: > - Agc: > enabled: true > - Awb: > enabled: false > > This will enable AGC, and disable AWB. If the enabled flag is not > present, the algorithm will be enabled. > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > --- > > v1 of this patch can be found here: > > https://patchwork.libcamera.org/patch/23299/ > > Changelog: > > v1 -> v2: > - Moved parsing to createAlgorithms() instead of createAlgorithm() > - Changed "disabled" flag to "enabled: [boolean]" You seem to have missed the comment I left in the v1 review about documenting the property. > --- > src/ipa/libipa/module.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > index 0fb51916f..f8c4bb664 100644 > --- a/src/ipa/libipa/module.h > +++ b/src/ipa/libipa/module.h > @@ -55,6 +55,15 @@ public: > return -EINVAL; > } > > + if (algo.contains("enabled")) { > + if (algo["enabled"].get<bool>() == false) { > + LOG(IPAModuleAlgo, Debug) > + << "Algorithm " << i > + << " is disabled via tuning file"; > + continue; > + } > + } > + > int ret = createAlgorithm(context, algo); > if (ret) { > algorithms_.clear();
Hi Barnabás, Thanks for the review! Quoting Barnabás Pőcze (2025-11-20 14:04:51) > Hi > > 2025. 11. 20. 14:41 keltezéssel, Isaac Scott írta: > > It is beneficial to have the option during development to disable and > > enable algorithms via the tuning file without having to delete their > > entries. > > > > Add support for an optional "enabled" parameter to accomplish this. > > > > Usage example: > > version: 1 > > algorithms: > > - Agc: > > enabled: true > > - Awb: > > enabled: false > > > > This will enable AGC, and disable AWB. If the enabled flag is not > > present, the algorithm will be enabled. > > > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > > > --- > > > > v1 of this patch can be found here: > > > > https://patchwork.libcamera.org/patch/23299/ > > > > Changelog: > > > > v1 -> v2: > > - Moved parsing to createAlgorithms() instead of createAlgorithm() > > - Changed "disabled" flag to "enabled: [boolean]" > > --- > > src/ipa/libipa/module.h | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > > index 0fb51916f..f8c4bb664 100644 > > --- a/src/ipa/libipa/module.h > > +++ b/src/ipa/libipa/module.h > > @@ -55,6 +55,15 @@ public: > > return -EINVAL; > > } > > > > + if (algo.contains("enabled")) { > > + if (algo["enabled"].get<bool>() == false) { > > The operator[] returns a special empty object if the key is not present, > so I think the above can be simplified as follows: Ah, interesting, that makes it much cleaner! > > if (!algo["enabled"].get(true)) { > ... > continue; > } > > > In any case I'm a bit concerned about the `createAlgorithm()` function: > > const auto &[name, algoData] = *data.asDict().begin(); > > this seems very fragile, e.g. > > - enabled: true > Awb: > > wouldn't work. In that case, wouldn't that be undesirable anyway? Shouldn't algorithm names come first? Best wishes, Isaac > > > Regards, > Barnabás Pőcze > > > + LOG(IPAModuleAlgo, Debug) > > + << "Algorithm " << i > > + << " is disabled via tuning file"; > > + continue; > > + } > > + } > > + > > int ret = createAlgorithm(context, algo); > > if (ret) { > > algorithms_.clear(); >
Hi Laurent, Thank you for the review! Quoting Laurent Pinchart (2025-11-20 14:08:10) > Hi Isaac, > > On Thu, Nov 20, 2025 at 01:41:01PM +0000, Isaac Scott wrote: > > It is beneficial to have the option during development to disable and > > enable algorithms via the tuning file without having to delete their > > entries. > > > > Add support for an optional "enabled" parameter to accomplish this. > > > > Usage example: > > version: 1 > > algorithms: > > - Agc: > > enabled: true > > - Awb: > > enabled: false > > > > This will enable AGC, and disable AWB. If the enabled flag is not > > present, the algorithm will be enabled. > > > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > > > --- > > > > v1 of this patch can be found here: > > > > https://patchwork.libcamera.org/patch/23299/ > > > > Changelog: > > > > v1 -> v2: > > - Moved parsing to createAlgorithms() instead of createAlgorithm() > > - Changed "disabled" flag to "enabled: [boolean]" > > You seem to have missed the comment I left in the v1 review about > documenting the property. Ah - you're right, apologies! I'll make sure to add it in v3. Best wishes, Isaac > > > --- > > src/ipa/libipa/module.h | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > > index 0fb51916f..f8c4bb664 100644 > > --- a/src/ipa/libipa/module.h > > +++ b/src/ipa/libipa/module.h > > @@ -55,6 +55,15 @@ public: > > return -EINVAL; > > } > > > > + if (algo.contains("enabled")) { > > + if (algo["enabled"].get<bool>() == false) { > > + LOG(IPAModuleAlgo, Debug) > > + << "Algorithm " << i > > + << " is disabled via tuning file"; > > + continue; > > + } > > + } > > + > > int ret = createAlgorithm(context, algo); > > if (ret) { > > algorithms_.clear(); > > -- > Regards, > > Laurent Pinchart
2025. 11. 20. 15:18 keltezéssel, Isaac Scott írta: > Hi Barnabás, > > Thanks for the review! > > Quoting Barnabás Pőcze (2025-11-20 14:04:51) >> Hi >> >> 2025. 11. 20. 14:41 keltezéssel, Isaac Scott írta: >>> It is beneficial to have the option during development to disable and >>> enable algorithms via the tuning file without having to delete their >>> entries. >>> >>> Add support for an optional "enabled" parameter to accomplish this. >>> >>> Usage example: >>> version: 1 >>> algorithms: >>> - Agc: >>> enabled: true >>> - Awb: >>> enabled: false >>> >>> This will enable AGC, and disable AWB. If the enabled flag is not >>> present, the algorithm will be enabled. >>> >>> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> >>> >>> --- >>> >>> v1 of this patch can be found here: >>> >>> https://patchwork.libcamera.org/patch/23299/ >>> >>> Changelog: >>> >>> v1 -> v2: >>> - Moved parsing to createAlgorithms() instead of createAlgorithm() >>> - Changed "disabled" flag to "enabled: [boolean]" >>> --- >>> src/ipa/libipa/module.h | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h >>> index 0fb51916f..f8c4bb664 100644 >>> --- a/src/ipa/libipa/module.h >>> +++ b/src/ipa/libipa/module.h >>> @@ -55,6 +55,15 @@ public: >>> return -EINVAL; >>> } >>> >>> + if (algo.contains("enabled")) { >>> + if (algo["enabled"].get<bool>() == false) { >> >> The operator[] returns a special empty object if the key is not present, >> so I think the above can be simplified as follows: > > Ah, interesting, that makes it much cleaner! > >> >> if (!algo["enabled"].get(true)) { >> ... >> continue; >> } >> >> >> In any case I'm a bit concerned about the `createAlgorithm()` function: >> >> const auto &[name, algoData] = *data.asDict().begin(); >> >> this seems very fragile, e.g. >> >> - enabled: true >> Awb: >> >> wouldn't work. > > In that case, wouldn't that be undesirable anyway? Shouldn't algorithm > names come first? I'm not sure. I think generally the expectation is that a json/yaml dictionary is unordered, i.e. the order does not matter. > > Best wishes, > Isaac > >> >> >> Regards, >> Barnabás Pőcze >> >>> + LOG(IPAModuleAlgo, Debug) >>> + << "Algorithm " << i >>> + << " is disabled via tuning file"; >>> + continue; >>> + } >>> + } >>> + >>> int ret = createAlgorithm(context, algo); >>> if (ret) { >>> algorithms_.clear(); >>
Hi Kieran, Thanks for the review! Quoting Kieran Bingham (2025-11-20 14:03:10) > Quoting Isaac Scott (2025-11-20 13:41:01) > > It is beneficial to have the option during development to disable and > > enable algorithms via the tuning file without having to delete their > > entries. > > > > Add support for an optional "enabled" parameter to accomplish this. > > > > Usage example: > > version: 1 > > algorithms: > > - Agc: > > enabled: true > > - Awb: > > enabled: false > > > > This will enable AGC, and disable AWB. If the enabled flag is not > > present, the algorithm will be enabled. > > > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > > > --- > > > > v1 of this patch can be found here: > > > > https://patchwork.libcamera.org/patch/23299/ > > > > Changelog: > > > > v1 -> v2: > > - Moved parsing to createAlgorithms() instead of createAlgorithm() > > - Changed "disabled" flag to "enabled: [boolean]" > > --- > > src/ipa/libipa/module.h | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > > index 0fb51916f..f8c4bb664 100644 > > --- a/src/ipa/libipa/module.h > > +++ b/src/ipa/libipa/module.h > > @@ -55,6 +55,15 @@ public: > > return -EINVAL; > > } > > > > + if (algo.contains("enabled")) { > > + if (algo["enabled"].get<bool>() == false) { > > > Can we simplify this to a single if ? > > I think something like this might work, could you test/check it please? > > if (algo["enabled"].get<bool>(true) == false) { > LOG() << ...; > continue; > > } I'll give it a go - that would be much cleaner! Best wishes, Isaac > > -- > Kieran > > > + LOG(IPAModuleAlgo, Debug) > > + << "Algorithm " << i > > + << " is disabled via tuning file"; > > + continue; > > + } > > + } > > + > > int ret = createAlgorithm(context, algo); > > if (ret) { > > algorithms_.clear(); > > -- > > 2.43.0 > >
On Thu, Nov 20, 2025 at 02:03:10PM +0000, Kieran Bingham wrote: > Quoting Isaac Scott (2025-11-20 13:41:01) > > It is beneficial to have the option during development to disable and > > enable algorithms via the tuning file without having to delete their > > entries. > > > > Add support for an optional "enabled" parameter to accomplish this. > > > > Usage example: > > version: 1 > > algorithms: > > - Agc: > > enabled: true > > - Awb: > > enabled: false > > > > This will enable AGC, and disable AWB. If the enabled flag is not > > present, the algorithm will be enabled. > > > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > > > --- > > > > v1 of this patch can be found here: > > > > https://patchwork.libcamera.org/patch/23299/ > > > > Changelog: > > > > v1 -> v2: > > - Moved parsing to createAlgorithms() instead of createAlgorithm() > > - Changed "disabled" flag to "enabled: [boolean]" > > --- > > src/ipa/libipa/module.h | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > > index 0fb51916f..f8c4bb664 100644 > > --- a/src/ipa/libipa/module.h > > +++ b/src/ipa/libipa/module.h > > @@ -55,6 +55,15 @@ public: > > return -EINVAL; > > } > > > > + if (algo.contains("enabled")) { > > + if (algo["enabled"].get<bool>() == false) { > > > Can we simplify this to a single if ? > > I think something like this might work, could you test/check it please? > > if (algo["enabled"].get<bool>(true) == false) { if (!algo["enabled"].get<bool>(true)) { > LOG() << ...; > continue; > > } > > > + LOG(IPAModuleAlgo, Debug) > > + << "Algorithm " << i > > + << " is disabled via tuning file"; > > + continue; > > + } > > + } > > + > > int ret = createAlgorithm(context, algo); > > if (ret) { > > algorithms_.clear();
Hi, Quoting Barnabás Pőcze (2025-11-20 14:20:26) > 2025. 11. 20. 15:18 keltezéssel, Isaac Scott írta: > > Hi Barnabás, > > > > Thanks for the review! > > > > Quoting Barnabás Pőcze (2025-11-20 14:04:51) > >> Hi > >> > >> 2025. 11. 20. 14:41 keltezéssel, Isaac Scott írta: > >>> It is beneficial to have the option during development to disable and > >>> enable algorithms via the tuning file without having to delete their > >>> entries. > >>> > >>> Add support for an optional "enabled" parameter to accomplish this. > >>> > >>> Usage example: > >>> version: 1 > >>> algorithms: > >>> - Agc: > >>> enabled: true > >>> - Awb: > >>> enabled: false > >>> > >>> This will enable AGC, and disable AWB. If the enabled flag is not > >>> present, the algorithm will be enabled. > >>> > >>> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > >>> > >>> --- > >>> > >>> v1 of this patch can be found here: > >>> > >>> https://patchwork.libcamera.org/patch/23299/ > >>> > >>> Changelog: > >>> > >>> v1 -> v2: > >>> - Moved parsing to createAlgorithms() instead of createAlgorithm() > >>> - Changed "disabled" flag to "enabled: [boolean]" > >>> --- > >>> src/ipa/libipa/module.h | 9 +++++++++ > >>> 1 file changed, 9 insertions(+) > >>> > >>> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > >>> index 0fb51916f..f8c4bb664 100644 > >>> --- a/src/ipa/libipa/module.h > >>> +++ b/src/ipa/libipa/module.h > >>> @@ -55,6 +55,15 @@ public: > >>> return -EINVAL; > >>> } > >>> > >>> + if (algo.contains("enabled")) { > >>> + if (algo["enabled"].get<bool>() == false) { > >> > >> The operator[] returns a special empty object if the key is not present, > >> so I think the above can be simplified as follows: > > > > Ah, interesting, that makes it much cleaner! > > > >> > >> if (!algo["enabled"].get(true)) { > >> ... > >> continue; > >> } > >> > >> > >> In any case I'm a bit concerned about the `createAlgorithm()` function: > >> > >> const auto &[name, algoData] = *data.asDict().begin(); > >> > >> this seems very fragile, e.g. > >> > >> - enabled: true > >> Awb: > >> > >> wouldn't work. > > > > In that case, wouldn't that be undesirable anyway? Shouldn't algorithm > > names come first? > > I'm not sure. I think generally the expectation is that a json/yaml dictionary > is unordered, i.e. the order does not matter. > Hmmm ok, I'll see if theres an improvement I can make there for an additional patch. Thanks, Isaac > > > > > Best wishes, > > Isaac > > > >> > >> > >> Regards, > >> Barnabás Pőcze > >> > >>> + LOG(IPAModuleAlgo, Debug) > >>> + << "Algorithm " << i > >>> + << " is disabled via tuning file"; > >>> + continue; > >>> + } > >>> + } > >>> + > >>> int ret = createAlgorithm(context, algo); > >>> if (ret) { > >>> algorithms_.clear(); > >> >
On Thu, Nov 20, 2025 at 03:36:03PM +0000, Isaac Scott wrote: > Quoting Barnabás Pőcze (2025-11-20 14:20:26) > > 2025. 11. 20. 15:18 keltezéssel, Isaac Scott írta: > > > Quoting Barnabás Pőcze (2025-11-20 14:04:51) > > >> 2025. 11. 20. 14:41 keltezéssel, Isaac Scott írta: > > >>> It is beneficial to have the option during development to disable and > > >>> enable algorithms via the tuning file without having to delete their > > >>> entries. > > >>> > > >>> Add support for an optional "enabled" parameter to accomplish this. > > >>> > > >>> Usage example: > > >>> version: 1 > > >>> algorithms: > > >>> - Agc: > > >>> enabled: true > > >>> - Awb: > > >>> enabled: false > > >>> > > >>> This will enable AGC, and disable AWB. If the enabled flag is not > > >>> present, the algorithm will be enabled. > > >>> > > >>> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > >>> > > >>> --- > > >>> > > >>> v1 of this patch can be found here: > > >>> > > >>> https://patchwork.libcamera.org/patch/23299/ > > >>> > > >>> Changelog: > > >>> > > >>> v1 -> v2: > > >>> - Moved parsing to createAlgorithms() instead of createAlgorithm() > > >>> - Changed "disabled" flag to "enabled: [boolean]" > > >>> --- > > >>> src/ipa/libipa/module.h | 9 +++++++++ > > >>> 1 file changed, 9 insertions(+) > > >>> > > >>> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > > >>> index 0fb51916f..f8c4bb664 100644 > > >>> --- a/src/ipa/libipa/module.h > > >>> +++ b/src/ipa/libipa/module.h > > >>> @@ -55,6 +55,15 @@ public: > > >>> return -EINVAL; > > >>> } > > >>> > > >>> + if (algo.contains("enabled")) { > > >>> + if (algo["enabled"].get<bool>() == false) { > > >> > > >> The operator[] returns a special empty object if the key is not present, > > >> so I think the above can be simplified as follows: > > > > > > Ah, interesting, that makes it much cleaner! > > > > > >> > > >> if (!algo["enabled"].get(true)) { > > >> ... > > >> continue; > > >> } > > >> > > >> > > >> In any case I'm a bit concerned about the `createAlgorithm()` function: > > >> > > >> const auto &[name, algoData] = *data.asDict().begin(); > > >> > > >> this seems very fragile, e.g. > > >> > > >> - enabled: true > > >> Awb: > > >> > > >> wouldn't work. > > > > > > In that case, wouldn't that be undesirable anyway? Shouldn't algorithm > > > names come first? > > > > I'm not sure. I think generally the expectation is that a json/yaml dictionary > > is unordered, i.e. the order does not matter. > > Hmmm ok, I'll see if theres an improvement I can make there for an > additional patch. Oops, I haven't noticed that, my bad. The enabled property should be a child of the algorithm: - Awb: enabled: false > > >>> + LOG(IPAModuleAlgo, Debug) > > >>> + << "Algorithm " << i > > >>> + << " is disabled via tuning file"; > > >>> + continue; > > >>> + } > > >>> + } > > >>> + > > >>> int ret = createAlgorithm(context, algo); > > >>> if (ret) { > > >>> algorithms_.clear();
2025. 11. 20. 17:09 keltezéssel, Laurent Pinchart írta: > On Thu, Nov 20, 2025 at 03:36:03PM +0000, Isaac Scott wrote: >> Quoting Barnabás Pőcze (2025-11-20 14:20:26) >>> 2025. 11. 20. 15:18 keltezéssel, Isaac Scott írta: >>>> Quoting Barnabás Pőcze (2025-11-20 14:04:51) >>>>> 2025. 11. 20. 14:41 keltezéssel, Isaac Scott írta: >>>>>> It is beneficial to have the option during development to disable and >>>>>> enable algorithms via the tuning file without having to delete their >>>>>> entries. >>>>>> >>>>>> Add support for an optional "enabled" parameter to accomplish this. >>>>>> >>>>>> Usage example: >>>>>> version: 1 >>>>>> algorithms: >>>>>> - Agc: >>>>>> enabled: true >>>>>> - Awb: >>>>>> enabled: false >>>>>> >>>>>> This will enable AGC, and disable AWB. If the enabled flag is not >>>>>> present, the algorithm will be enabled. >>>>>> >>>>>> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> >>>>>> >>>>>> --- >>>>>> >>>>>> v1 of this patch can be found here: >>>>>> >>>>>> https://patchwork.libcamera.org/patch/23299/ >>>>>> >>>>>> Changelog: >>>>>> >>>>>> v1 -> v2: >>>>>> - Moved parsing to createAlgorithms() instead of createAlgorithm() >>>>>> - Changed "disabled" flag to "enabled: [boolean]" >>>>>> --- >>>>>> src/ipa/libipa/module.h | 9 +++++++++ >>>>>> 1 file changed, 9 insertions(+) >>>>>> >>>>>> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h >>>>>> index 0fb51916f..f8c4bb664 100644 >>>>>> --- a/src/ipa/libipa/module.h >>>>>> +++ b/src/ipa/libipa/module.h >>>>>> @@ -55,6 +55,15 @@ public: >>>>>> return -EINVAL; >>>>>> } >>>>>> >>>>>> + if (algo.contains("enabled")) { >>>>>> + if (algo["enabled"].get<bool>() == false) { >>>>> >>>>> The operator[] returns a special empty object if the key is not present, >>>>> so I think the above can be simplified as follows: >>>> >>>> Ah, interesting, that makes it much cleaner! >>>> >>>>> >>>>> if (!algo["enabled"].get(true)) { >>>>> ... >>>>> continue; >>>>> } >>>>> >>>>> >>>>> In any case I'm a bit concerned about the `createAlgorithm()` function: >>>>> >>>>> const auto &[name, algoData] = *data.asDict().begin(); >>>>> >>>>> this seems very fragile, e.g. >>>>> >>>>> - enabled: true >>>>> Awb: >>>>> >>>>> wouldn't work. >>>> >>>> In that case, wouldn't that be undesirable anyway? Shouldn't algorithm >>>> names come first? >>> >>> I'm not sure. I think generally the expectation is that a json/yaml dictionary >>> is unordered, i.e. the order does not matter. >> >> Hmmm ok, I'll see if theres an improvement I can make there for an >> additional patch. > > Oops, I haven't noticed that, my bad. The enabled property should be a > child of the algorithm: > > - Awb: > enabled: false > Hmm, to me the "obvious" choice is to have it "outside" as not to mix the two types of parameters. E.g. - name: Awb params: # params for Awb algorithm enabled: true # other "external" parameters for the algorithm loading code I've felt like the current design (array of dictionaries with a single key) is an mix of a "dictionary" and an "array" approach, and it seems to me that both would have advantages over the current format. But admittedly I have not investigated why this specific format was selected, so I'm probably missing something. >>>>>> + LOG(IPAModuleAlgo, Debug) >>>>>> + << "Algorithm " << i >>>>>> + << " is disabled via tuning file"; >>>>>> + continue; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> int ret = createAlgorithm(context, algo); >>>>>> if (ret) { >>>>>> algorithms_.clear(); >
On Thu, Nov 20, 2025 at 05:23:10PM +0100, Barnabás Pőcze wrote: > 2025. 11. 20. 17:09 keltezéssel, Laurent Pinchart írta: > > On Thu, Nov 20, 2025 at 03:36:03PM +0000, Isaac Scott wrote: > >> Quoting Barnabás Pőcze (2025-11-20 14:20:26) > >>> 2025. 11. 20. 15:18 keltezéssel, Isaac Scott írta: > >>>> Quoting Barnabás Pőcze (2025-11-20 14:04:51) > >>>>> 2025. 11. 20. 14:41 keltezéssel, Isaac Scott írta: > >>>>>> It is beneficial to have the option during development to disable and > >>>>>> enable algorithms via the tuning file without having to delete their > >>>>>> entries. > >>>>>> > >>>>>> Add support for an optional "enabled" parameter to accomplish this. > >>>>>> > >>>>>> Usage example: > >>>>>> version: 1 > >>>>>> algorithms: > >>>>>> - Agc: > >>>>>> enabled: true > >>>>>> - Awb: > >>>>>> enabled: false > >>>>>> > >>>>>> This will enable AGC, and disable AWB. If the enabled flag is not > >>>>>> present, the algorithm will be enabled. > >>>>>> > >>>>>> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > >>>>>> > >>>>>> --- > >>>>>> > >>>>>> v1 of this patch can be found here: > >>>>>> > >>>>>> https://patchwork.libcamera.org/patch/23299/ > >>>>>> > >>>>>> Changelog: > >>>>>> > >>>>>> v1 -> v2: > >>>>>> - Moved parsing to createAlgorithms() instead of createAlgorithm() > >>>>>> - Changed "disabled" flag to "enabled: [boolean]" > >>>>>> --- > >>>>>> src/ipa/libipa/module.h | 9 +++++++++ > >>>>>> 1 file changed, 9 insertions(+) > >>>>>> > >>>>>> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > >>>>>> index 0fb51916f..f8c4bb664 100644 > >>>>>> --- a/src/ipa/libipa/module.h > >>>>>> +++ b/src/ipa/libipa/module.h > >>>>>> @@ -55,6 +55,15 @@ public: > >>>>>> return -EINVAL; > >>>>>> } > >>>>>> > >>>>>> + if (algo.contains("enabled")) { > >>>>>> + if (algo["enabled"].get<bool>() == false) { > >>>>> > >>>>> The operator[] returns a special empty object if the key is not present, > >>>>> so I think the above can be simplified as follows: > >>>> > >>>> Ah, interesting, that makes it much cleaner! > >>>> > >>>>> > >>>>> if (!algo["enabled"].get(true)) { > >>>>> ... > >>>>> continue; > >>>>> } > >>>>> > >>>>> > >>>>> In any case I'm a bit concerned about the `createAlgorithm()` function: > >>>>> > >>>>> const auto &[name, algoData] = *data.asDict().begin(); > >>>>> > >>>>> this seems very fragile, e.g. > >>>>> > >>>>> - enabled: true > >>>>> Awb: > >>>>> > >>>>> wouldn't work. > >>>> > >>>> In that case, wouldn't that be undesirable anyway? Shouldn't algorithm > >>>> names come first? > >>> > >>> I'm not sure. I think generally the expectation is that a json/yaml dictionary > >>> is unordered, i.e. the order does not matter. > >> > >> Hmmm ok, I'll see if theres an improvement I can make there for an > >> additional patch. > > > > Oops, I haven't noticed that, my bad. The enabled property should be a > > child of the algorithm: > > > > - Awb: > > enabled: false > > > > Hmm, to me the "obvious" choice is to have it "outside" as not to mix the two > types of parameters. E.g. > > - name: Awb > params: > # params for Awb algorithm > enabled: true > # other "external" parameters for the algorithm loading code > > I've felt like the current design (array of dictionaries with a single key) is an mix of a > "dictionary" and an "array" approach, and it seems to me that both would have advantages over > the current format. But admittedly I have not investigated why this specific format was > selected, so I'm probably missing something. The sequence is required as mappings are unordered in yaml, as you've mentioned. Using the algorithm name as the key to the single-item mapping is a shortcut to avoid being too verbose. Other options could be possible. > >>>>>> + LOG(IPAModuleAlgo, Debug) > >>>>>> + << "Algorithm " << i > >>>>>> + << " is disabled via tuning file"; > >>>>>> + continue; > >>>>>> + } > >>>>>> + } > >>>>>> + > >>>>>> int ret = createAlgorithm(context, algo); > >>>>>> if (ret) { > >>>>>> algorithms_.clear();
2025. 11. 20. 17:33 keltezéssel, Laurent Pinchart írta: > On Thu, Nov 20, 2025 at 05:23:10PM +0100, Barnabás Pőcze wrote: >> 2025. 11. 20. 17:09 keltezéssel, Laurent Pinchart írta: >>> On Thu, Nov 20, 2025 at 03:36:03PM +0000, Isaac Scott wrote: >>>> Quoting Barnabás Pőcze (2025-11-20 14:20:26) >>>>> 2025. 11. 20. 15:18 keltezéssel, Isaac Scott írta: >>>>>> Quoting Barnabás Pőcze (2025-11-20 14:04:51) >>>>>>> 2025. 11. 20. 14:41 keltezéssel, Isaac Scott írta: >>>>>>>> It is beneficial to have the option during development to disable and >>>>>>>> enable algorithms via the tuning file without having to delete their >>>>>>>> entries. >>>>>>>> >>>>>>>> Add support for an optional "enabled" parameter to accomplish this. >>>>>>>> >>>>>>>> Usage example: >>>>>>>> version: 1 >>>>>>>> algorithms: >>>>>>>> - Agc: >>>>>>>> enabled: true >>>>>>>> - Awb: >>>>>>>> enabled: false >>>>>>>> >>>>>>>> This will enable AGC, and disable AWB. If the enabled flag is not >>>>>>>> present, the algorithm will be enabled. >>>>>>>> >>>>>>>> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> >>>>>>>> >>>>>>>> --- >>>>>>>> >>>>>>>> v1 of this patch can be found here: >>>>>>>> >>>>>>>> https://patchwork.libcamera.org/patch/23299/ >>>>>>>> >>>>>>>> Changelog: >>>>>>>> >>>>>>>> v1 -> v2: >>>>>>>> - Moved parsing to createAlgorithms() instead of createAlgorithm() >>>>>>>> - Changed "disabled" flag to "enabled: [boolean]" >>>>>>>> --- >>>>>>>> src/ipa/libipa/module.h | 9 +++++++++ >>>>>>>> 1 file changed, 9 insertions(+) >>>>>>>> >>>>>>>> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h >>>>>>>> index 0fb51916f..f8c4bb664 100644 >>>>>>>> --- a/src/ipa/libipa/module.h >>>>>>>> +++ b/src/ipa/libipa/module.h >>>>>>>> @@ -55,6 +55,15 @@ public: >>>>>>>> return -EINVAL; >>>>>>>> } >>>>>>>> >>>>>>>> + if (algo.contains("enabled")) { >>>>>>>> + if (algo["enabled"].get<bool>() == false) { >>>>>>> >>>>>>> The operator[] returns a special empty object if the key is not present, >>>>>>> so I think the above can be simplified as follows: >>>>>> >>>>>> Ah, interesting, that makes it much cleaner! >>>>>> >>>>>>> >>>>>>> if (!algo["enabled"].get(true)) { >>>>>>> ... >>>>>>> continue; >>>>>>> } >>>>>>> >>>>>>> >>>>>>> In any case I'm a bit concerned about the `createAlgorithm()` function: >>>>>>> >>>>>>> const auto &[name, algoData] = *data.asDict().begin(); >>>>>>> >>>>>>> this seems very fragile, e.g. >>>>>>> >>>>>>> - enabled: true >>>>>>> Awb: >>>>>>> >>>>>>> wouldn't work. >>>>>> >>>>>> In that case, wouldn't that be undesirable anyway? Shouldn't algorithm >>>>>> names come first? >>>>> >>>>> I'm not sure. I think generally the expectation is that a json/yaml dictionary >>>>> is unordered, i.e. the order does not matter. >>>> >>>> Hmmm ok, I'll see if theres an improvement I can make there for an >>>> additional patch. >>> >>> Oops, I haven't noticed that, my bad. The enabled property should be a >>> child of the algorithm: >>> >>> - Awb: >>> enabled: false >>> >> >> Hmm, to me the "obvious" choice is to have it "outside" as not to mix the two >> types of parameters. E.g. >> >> - name: Awb >> params: >> # params for Awb algorithm >> enabled: true >> # other "external" parameters for the algorithm loading code >> >> I've felt like the current design (array of dictionaries with a single key) is an mix of a >> "dictionary" and an "array" approach, and it seems to me that both would have advantages over >> the current format. But admittedly I have not investigated why this specific format was >> selected, so I'm probably missing something. > > The sequence is required as mappings are unordered in yaml, as you've > mentioned. Using the algorithm name as the key to the single-item > mapping is a shortcut to avoid being too verbose. Other options could be > possible. Yes, but the ordering is essentially determined by the IPA module in question, no? So regardless of the order in the tuning file, I think it is expected that things will work the same (and correctly), right? > >>>>>>>> + LOG(IPAModuleAlgo, Debug) >>>>>>>> + << "Algorithm " << i >>>>>>>> + << " is disabled via tuning file"; >>>>>>>> + continue; >>>>>>>> + } >>>>>>>> + } >>>>>>>> + >>>>>>>> int ret = createAlgorithm(context, algo); >>>>>>>> if (ret) { >>>>>>>> algorithms_.clear(); >
diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h index 0fb51916f..f8c4bb664 100644 --- a/src/ipa/libipa/module.h +++ b/src/ipa/libipa/module.h @@ -55,6 +55,15 @@ public: return -EINVAL; } + if (algo.contains("enabled")) { + if (algo["enabled"].get<bool>() == false) { + LOG(IPAModuleAlgo, Debug) + << "Algorithm " << i + << " is disabled via tuning file"; + continue; + } + } + int ret = createAlgorithm(context, algo); if (ret) { algorithms_.clear();
It is beneficial to have the option during development to disable and enable algorithms via the tuning file without having to delete their entries. Add support for an optional "enabled" parameter to accomplish this. Usage example: version: 1 algorithms: - Agc: enabled: true - Awb: enabled: false This will enable AGC, and disable AWB. If the enabled flag is not present, the algorithm will be enabled. Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> --- v1 of this patch can be found here: https://patchwork.libcamera.org/patch/23299/ Changelog: v1 -> v2: - Moved parsing to createAlgorithms() instead of createAlgorithm() - Changed "disabled" flag to "enabled: [boolean]" --- src/ipa/libipa/module.h | 9 +++++++++ 1 file changed, 9 insertions(+)