| Message ID | 20251121113411.111096-1-isaac.scott@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Isaac, Thank you for the patch. Quoting Isaac Scott (2025-11-21 12:34:11) > 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: > > v2 -> v3: > - Moved parsing back to createAlgorithm(). > - Added documentation to module.cpp. > - Ensured the 'enabled' flag is part of the algoData for an > algorithm to help avoid ambiguity. > > v1 -> v2: > - Moved parsing to createAlgorithms() instead of createAlgorithm() > - Changed "disabled" flag to "enabled: [boolean]" > --- > src/ipa/libipa/module.cpp | 8 ++++++++ > src/ipa/libipa/module.h | 11 +++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp > index 64ca91419..f142c5257 100644 > --- a/src/ipa/libipa/module.cpp > +++ b/src/ipa/libipa/module.cpp > @@ -94,6 +94,14 @@ namespace ipa { > * algorithms. The configuration data is expected to be correct, any error > * causes the function to fail and return immediately. > * > + * Algorithms can optionally be disabled via the tuning file of the camera module > + * as shown here, with AGC being used as an example: > + * > + * - Agc: > + * enabled: false > + * > + * If this is the case, the algorithm will not be instantiated. > + * > * \return 0 on success, or a negative error code on failure > */ > > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > index 116cb8968..0b3b84219 100644 > --- a/src/ipa/libipa/module.h > +++ b/src/ipa/libipa/module.h > @@ -82,6 +82,17 @@ private: > return -EINVAL; > } > > + /* > + * Optionally, algorithms can be disabled via the tuning file by including > + * enabled: false as a parameter within the algorithm tuning data. > + * This is not an error, so we return 0. > + */ > + if (!algoData["enabled"].get<bool>(true)) { > + LOG(IPAModuleAlgo, Debug) I think this could even be Info level? > + << "Algorithm '" << name << "' disabled via tuning file"; > + return 0; > + } > + Unfortunately this doesn't solve my main use case :-). I often switch between different libcamera versions that support different algorithms. So it would be great if IPA loading would continue on algorithms that failed to load but are marked as disabled. Something like std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name); bool enabled = algoData["enabled"].get<bool>(true); if (!algo) { LOG(IPAModuleAlgo, Error) << "Algorithm '" << name << "' not found"; if (enabled) return -EINVAL; } if (!enabled) { LOG(IPAModuleAlgo, Debug) << "Algorithm '" << name << "' disabled via tuning file"; return 0; } What do you think? Best regards, Stefan > int ret = algo->init(context, algoData); > if (ret) { > LOG(IPAModuleAlgo, Error) > -- > 2.43.0 >
2025. 11. 21. 12:54 keltezéssel, Stefan Klug írta: > Hi Isaac, > > Thank you for the patch. > > Quoting Isaac Scott (2025-11-21 12:34:11) >> 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: >> >> v2 -> v3: >> - Moved parsing back to createAlgorithm(). >> - Added documentation to module.cpp. >> - Ensured the 'enabled' flag is part of the algoData for an >> algorithm to help avoid ambiguity. >> >> v1 -> v2: >> - Moved parsing to createAlgorithms() instead of createAlgorithm() >> - Changed "disabled" flag to "enabled: [boolean]" >> --- >> src/ipa/libipa/module.cpp | 8 ++++++++ >> src/ipa/libipa/module.h | 11 +++++++++++ >> 2 files changed, 19 insertions(+) >> >> diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp >> index 64ca91419..f142c5257 100644 >> --- a/src/ipa/libipa/module.cpp >> +++ b/src/ipa/libipa/module.cpp >> @@ -94,6 +94,14 @@ namespace ipa { >> * algorithms. The configuration data is expected to be correct, any error >> * causes the function to fail and return immediately. >> * >> + * Algorithms can optionally be disabled via the tuning file of the camera module >> + * as shown here, with AGC being used as an example: >> + * >> + * - Agc: >> + * enabled: false >> + * >> + * If this is the case, the algorithm will not be instantiated. >> + * >> * \return 0 on success, or a negative error code on failure >> */ >> >> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h >> index 116cb8968..0b3b84219 100644 >> --- a/src/ipa/libipa/module.h >> +++ b/src/ipa/libipa/module.h >> @@ -82,6 +82,17 @@ private: >> return -EINVAL; >> } >> >> + /* >> + * Optionally, algorithms can be disabled via the tuning file by including >> + * enabled: false as a parameter within the algorithm tuning data. >> + * This is not an error, so we return 0. >> + */ >> + if (!algoData["enabled"].get<bool>(true)) { >> + LOG(IPAModuleAlgo, Debug) > > I think this could even be Info level? > >> + << "Algorithm '" << name << "' disabled via tuning file"; >> + return 0; >> + } >> + > > Unfortunately this doesn't solve my main use case :-). I often switch > between different libcamera versions that support different algorithms. > So it would be great if IPA loading would continue on algorithms that > failed to load but are marked as disabled. Something like > > std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name); > bool enabled = algoData["enabled"].get<bool>(true); > if (!algo) { > LOG(IPAModuleAlgo, Error) > << "Algorithm '" << name << "' not found"; > if (enabled) > return -EINVAL; > } > > if (!enabled) { > LOG(IPAModuleAlgo, Debug) > << "Algorithm '" << name << "' disabled via tuning file"; > return 0; > } > > What do you think? Can't we add something like `state: enabled (default) / disabled / optional` ? Would this "optional" solve your use case? > > Best regards, > Stefan > >> int ret = algo->init(context, algoData); >> if (ret) { >> LOG(IPAModuleAlgo, Error) >> -- >> 2.43.0 >>
Hi All, Quoting Barnabás Pőcze (2025-11-21 12:39:33) > 2025. 11. 21. 12:54 keltezéssel, Stefan Klug írta: > > Hi Isaac, > > > > Thank you for the patch. > > > > Quoting Isaac Scott (2025-11-21 12:34:11) > >> 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: > >> > >> v2 -> v3: > >> - Moved parsing back to createAlgorithm(). > >> - Added documentation to module.cpp. > >> - Ensured the 'enabled' flag is part of the algoData for an > >> algorithm to help avoid ambiguity. > >> > >> v1 -> v2: > >> - Moved parsing to createAlgorithms() instead of createAlgorithm() > >> - Changed "disabled" flag to "enabled: [boolean]" > >> --- > >> src/ipa/libipa/module.cpp | 8 ++++++++ > >> src/ipa/libipa/module.h | 11 +++++++++++ > >> 2 files changed, 19 insertions(+) > >> > >> diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp > >> index 64ca91419..f142c5257 100644 > >> --- a/src/ipa/libipa/module.cpp > >> +++ b/src/ipa/libipa/module.cpp > >> @@ -94,6 +94,14 @@ namespace ipa { > >> * algorithms. The configuration data is expected to be correct, any error > >> * causes the function to fail and return immediately. > >> * > >> + * Algorithms can optionally be disabled via the tuning file of the camera module > >> + * as shown here, with AGC being used as an example: > >> + * > >> + * - Agc: > >> + * enabled: false > >> + * > >> + * If this is the case, the algorithm will not be instantiated. > >> + * > >> * \return 0 on success, or a negative error code on failure > >> */ > >> > >> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > >> index 116cb8968..0b3b84219 100644 > >> --- a/src/ipa/libipa/module.h > >> +++ b/src/ipa/libipa/module.h > >> @@ -82,6 +82,17 @@ private: > >> return -EINVAL; > >> } > >> > >> + /* > >> + * Optionally, algorithms can be disabled via the tuning file by including > >> + * enabled: false as a parameter within the algorithm tuning data. > >> + * This is not an error, so we return 0. > >> + */ > >> + if (!algoData["enabled"].get<bool>(true)) { > >> + LOG(IPAModuleAlgo, Debug) > > > > I think this could even be Info level? > > > >> + << "Algorithm '" << name << "' disabled via tuning file"; > >> + return 0; > >> + } > >> + > > > > Unfortunately this doesn't solve my main use case :-). I often switch > > between different libcamera versions that support different algorithms. > > So it would be great if IPA loading would continue on algorithms that > > failed to load but are marked as disabled. Something like > > > > std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name); > > bool enabled = algoData["enabled"].get<bool>(true); > > if (!algo) { > > LOG(IPAModuleAlgo, Error) > > << "Algorithm '" << name << "' not found"; > > if (enabled) > > return -EINVAL; > > } > > > > if (!enabled) { > > LOG(IPAModuleAlgo, Debug) > > << "Algorithm '" << name << "' disabled via tuning file"; > > return 0; > > } > > > > What do you think? > > Can't we add something like `state: enabled (default) / disabled / optional` ? > Would this "optional" solve your use case? > I think just moving it to before the first createAlgorithm() instead of afterwards would probably also solve it? Best wishes, Isaac > > > > > Best regards, > > Stefan > > > >> int ret = algo->init(context, algoData); > >> if (ret) { > >> LOG(IPAModuleAlgo, Error) > >> -- > >> 2.43.0 > >> >
Quoting Isaac Scott (2025-11-21 12:44:52) > Hi All, > > Quoting Barnabás Pőcze (2025-11-21 12:39:33) > > 2025. 11. 21. 12:54 keltezéssel, Stefan Klug írta: > > > Hi Isaac, > > > > > > Thank you for the patch. > > > > > > Quoting Isaac Scott (2025-11-21 12:34:11) > > >> 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: > > >> > > >> v2 -> v3: > > >> - Moved parsing back to createAlgorithm(). > > >> - Added documentation to module.cpp. > > >> - Ensured the 'enabled' flag is part of the algoData for an > > >> algorithm to help avoid ambiguity. > > >> > > >> v1 -> v2: > > >> - Moved parsing to createAlgorithms() instead of createAlgorithm() > > >> - Changed "disabled" flag to "enabled: [boolean]" > > >> --- > > >> src/ipa/libipa/module.cpp | 8 ++++++++ > > >> src/ipa/libipa/module.h | 11 +++++++++++ > > >> 2 files changed, 19 insertions(+) > > >> > > >> diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp > > >> index 64ca91419..f142c5257 100644 > > >> --- a/src/ipa/libipa/module.cpp > > >> +++ b/src/ipa/libipa/module.cpp > > >> @@ -94,6 +94,14 @@ namespace ipa { > > >> * algorithms. The configuration data is expected to be correct, any error > > >> * causes the function to fail and return immediately. > > >> * > > >> + * Algorithms can optionally be disabled via the tuning file of the camera module > > >> + * as shown here, with AGC being used as an example: > > >> + * > > >> + * - Agc: > > >> + * enabled: false > > >> + * > > >> + * If this is the case, the algorithm will not be instantiated. > > >> + * > > >> * \return 0 on success, or a negative error code on failure > > >> */ > > >> > > >> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h > > >> index 116cb8968..0b3b84219 100644 > > >> --- a/src/ipa/libipa/module.h > > >> +++ b/src/ipa/libipa/module.h > > >> @@ -82,6 +82,17 @@ private: > > >> return -EINVAL; > > >> } > > >> > > >> + /* > > >> + * Optionally, algorithms can be disabled via the tuning file by including > > >> + * enabled: false as a parameter within the algorithm tuning data. > > >> + * This is not an error, so we return 0. > > >> + */ > > >> + if (!algoData["enabled"].get<bool>(true)) { > > >> + LOG(IPAModuleAlgo, Debug) > > > > > > I think this could even be Info level? > > > > > >> + << "Algorithm '" << name << "' disabled via tuning file"; > > >> + return 0; > > >> + } > > >> + > > > > > > Unfortunately this doesn't solve my main use case :-). I often switch > > > between different libcamera versions that support different algorithms. > > > So it would be great if IPA loading would continue on algorithms that > > > failed to load but are marked as disabled. Something like > > > > > > std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name); > > > bool enabled = algoData["enabled"].get<bool>(true); > > > if (!algo) { > > > LOG(IPAModuleAlgo, Error) > > > << "Algorithm '" << name << "' not found"; > > > if (enabled) > > > return -EINVAL; > > > } > > > > > > if (!enabled) { > > > LOG(IPAModuleAlgo, Debug) > > > << "Algorithm '" << name << "' disabled via tuning file"; > > > return 0; > > > } > > > > > > What do you think? > > > > Can't we add something like `state: enabled (default) / disabled / optional` ? > > Would this "optional" solve your use case? > > > > I think just moving it to before the first createAlgorithm() instead of > afterwards would probably also solve it? > Yes, I think this is best - we shouldn't /create/ an algorithm just to then identify that it's disabled and then destroy it. -- Kieran > Best wishes, > Isaac > > > > > > > > > Best regards, > > > Stefan > > > > > >> int ret = algo->init(context, algoData); > > >> if (ret) { > > >> LOG(IPAModuleAlgo, Error) > > >> -- > > >> 2.43.0 > > >> > >
diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp index 64ca91419..f142c5257 100644 --- a/src/ipa/libipa/module.cpp +++ b/src/ipa/libipa/module.cpp @@ -94,6 +94,14 @@ namespace ipa { * algorithms. The configuration data is expected to be correct, any error * causes the function to fail and return immediately. * + * Algorithms can optionally be disabled via the tuning file of the camera module + * as shown here, with AGC being used as an example: + * + * - Agc: + * enabled: false + * + * If this is the case, the algorithm will not be instantiated. + * * \return 0 on success, or a negative error code on failure */ diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h index 116cb8968..0b3b84219 100644 --- a/src/ipa/libipa/module.h +++ b/src/ipa/libipa/module.h @@ -82,6 +82,17 @@ private: return -EINVAL; } + /* + * Optionally, algorithms can be disabled via the tuning file by including + * enabled: false as a parameter within the algorithm tuning data. + * This is not an error, so we return 0. + */ + if (!algoData["enabled"].get<bool>(true)) { + LOG(IPAModuleAlgo, Debug) + << "Algorithm '" << name << "' disabled via tuning file"; + return 0; + } + int ret = algo->init(context, algoData); if (ret) { LOG(IPAModuleAlgo, Error)
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: v2 -> v3: - Moved parsing back to createAlgorithm(). - Added documentation to module.cpp. - Ensured the 'enabled' flag is part of the algoData for an algorithm to help avoid ambiguity. v1 -> v2: - Moved parsing to createAlgorithms() instead of createAlgorithm() - Changed "disabled" flag to "enabled: [boolean]" --- src/ipa/libipa/module.cpp | 8 ++++++++ src/ipa/libipa/module.h | 11 +++++++++++ 2 files changed, 19 insertions(+)