[PATCH v1 04/11] libipa: awb: Add helper functions for AWB mode support

[Stefan Klug]

Hi Dan

Thank you for the review. 

On Fri, Jan 17, 2025 at 02:41:02PM +0000, Dan Scally wrote:
> Hi Stefan
> 
> 
> On 09/01/2025 11:53, Stefan Klug wrote:
> > The AWB modes are specified in the libcamera core controls. It is
> > therefore quite likely that every AWB algorithm will implement them. Add
> > helper functions for parsing and storing the configured modes and the
> > currently selected mode to the AwbAlgorithm base class.
> 
> 
> I think that this can just be squashed into the last patch.
> 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >   src/ipa/libipa/awb.cpp | 100 +++++++++++++++++++++++++++++++++++++++++
> >   src/ipa/libipa/awb.h   |  12 +++++
> >   2 files changed, 112 insertions(+)
> > 
> > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> > index 74e88d513b27..07502da66f73 100644
> > --- a/src/ipa/libipa/awb.cpp
> > +++ b/src/ipa/libipa/awb.cpp
> > @@ -9,6 +9,8 @@
> >   #include <libcamera/base/log.h>
> > +#include <libcamera/control_ids.h>
> > +
> >   /**
> >    * \file awb.h
> >    * \brief Base classes for AWB algorithms
> > @@ -132,6 +134,104 @@ namespace ipa {
> >    * \brief Controls info map for the controls provided by the algorithm
> >    */
> > +/**
> > + * \var AwbAlgorithm::modes_
> > + * \brief Map of all configured modes
> > + * \sa AwbAlgorithm::parseModeConfigs
> > + */
> > +
> > +/**
> > + * \class AwbAlgorithm::ModeConfig
> > + * \brief Holds the configuration of a single AWB mode
> > + *
> > + * Awb modes limit the regulation of the AWB algorithm to a specific range of
> > + * colour temperatures.
> > + */
> > +
> > +/**
> > + * \var AwbAlgorithm::ModeConfig::ctLo
> > + * \brief The lowest valid colour temperature of that mode
> > + */
> > +
> > +/**
> > + * \var AwbAlgorithm::ModeConfig::ctHi
> > + * \brief The highest valid colour temperature of that mode
> > + */
> > +
> > +/**
> > + * \brief Parse the mode configurations from the tuning data
> > + * \param[in] tuningData the YamlObject representing the tuning data
> > + *
> > + * Utility function to parse the tuning data for a AwbMode entry and read all
> > + * provided modes. It populetes AwbAlgorithm::controls_, AwbAlgorithm::modes_
> s/populetes/populates
> > + * and sets the current mode to AwbAuto.
> 
> What if AwbAuto isn't one of the modes defined? Ah, I see it fails. Perhaps
> that is worth a mention. I think it's also probably worth an example of the
> expected tuning file format like in AgcMeanLuminance::parseTuningData().

Yes, you are right, I'll add documentation for that. In v2 the AutoMode
handling will be removed from that function and replaced by a generic
def parameter.

> 
> 
> Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>

Thanks,
Stefan

> 
> > + *
> > + * \return Zero on success, negative error code otherwise
> > + */
> > +int AwbAlgorithm::parseModeConfigs(const YamlObject &tuningData)
> > +{
> > +	std::vector<ControlValue> availableModes;
> > +
> > +	const YamlObject &yamlModes = tuningData[controls::AwbMode.name()];
> > +	if (!yamlModes.isDictionary()) {
> > +		LOG(Awb, Error)
> > +			<< "AwbModes must be a dictionary.";
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (const auto &[modeName, modeDict] : yamlModes.asDict()) {
> > +		if (controls::AwbModeNameValueMap.find(modeName) ==
> > +		    controls::AwbModeNameValueMap.end()) {
> > +			LOG(Awb, Warning)
> > +				<< "Skipping unknown awb mode '"
> > +				<< modeName << "'";
> > +			continue;
> > +		}
> > +
> > +		if (!modeDict.isDictionary()) {
> > +			LOG(Awb, Error)
> > +				<< "Invalid awb mode '" << modeName << "'";
> > +			return -EINVAL;
> > +		}
> > +
> > +		const auto &modeValue = static_cast<controls::AwbModeEnum>(
> > +			controls::AwbModeNameValueMap.at(modeName));
> > +
> > +		auto &config = modes_[modeValue];
> > +		auto hi = modeDict["hi"].get<double>();
> > +		if (!hi) {
> > +			LOG(Awb, Error) << "Failed to read hi param of mode "
> > +					<< modeName;
> > +			return -EINVAL;
> > +		}
> > +		config.ctHi = *hi;
> > +
> > +		auto lo = modeDict["lo"].get<double>();
> > +		if (!lo) {
> > +			LOG(Awb, Error) << "Failed to read low param of mode "
> > +					<< modeName;
> > +			return -EINVAL;
> > +		}
> > +		config.ctLo = *lo;
> > +
> > +		availableModes.push_back(modeValue);
> > +	}
> > +
> > +	if (modes_.empty()) {
> > +		LOG(Awb, Error) << "No AWB modes configured";
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (modes_.find(controls::AwbAuto) == modes_.end()) {
> > +		LOG(Awb, Error) << "AwbAuto mode is missing in the configuration.";
> > +		return -EINVAL;
> > +	}
> > +
> > +	controls_[&controls::AwbMode] = ControlInfo(availableModes);
> > +
> > +	return 0;
> > +}
> > +
> >   } /* namespace ipa */
> >   } /* namespace libcamera */
> > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> > index 2dd471606ec4..426fdd6dae77 100644
> > --- a/src/ipa/libipa/awb.h
> > +++ b/src/ipa/libipa/awb.h
> > @@ -7,7 +7,11 @@
> >   #pragma once
> > +#include <map>
> > +
> > +#include <libcamera/control_ids.h>
> >   #include <libcamera/controls.h>
> > +
> >   #include "libcamera/internal/yaml_parser.h"
> >   #include "vector.h"
> > @@ -43,7 +47,15 @@ public:
> >   	virtual void handleControls([[maybe_unused]] const ControlList &controls) {}
> >   protected:
> > +	int parseModeConfigs(const YamlObject &tuningData);
> > +
> > +	struct ModeConfig {
> > +		double ctHi;
> > +		double ctLo;
> > +	};
> > +
> >   	ControlInfoMap::Map controls_;
> > +	std::map<controls::AwbModeEnum, AwbAlgorithm::ModeConfig> modes_;
> >   };
> >   } /* namespace ipa */