| 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();
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(+)