[PATCH v4 3/6] ipa: rkisp1: awb: Implement ColourTemperature control

[Stefan Klug]

Hi Laurent,

Thank you for the review. 

On Fri, Aug 30, 2024 at 03:02:07AM +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Tue, Aug 13, 2024 at 10:44:20AM +0200, Stefan Klug wrote:
> > There are many use-cases (tuning-validation, working in static
> > environments) where a manual ColourTemperature control is helpful.
> > Implement that by interpolating and applying the white balance gains
> > from the tuning file according to the requested colour temperature. If
> > colour gains are provided on the same request, they take precedence. As
> > the colour temperature reported in the metadata is always based on the
> > measurements, we don't have to touch that.
> 
> I thought the metadata would report the colour temperature that was set
> manually. This likely calls for a clarification in the control
> documentation.
> 
> Reading https://lists.libcamera.org/pipermail/libcamera-devel/2023-December/039733.html,
> it sounds like the Raspberry Pi implementation would stop evaluating the
> colour temperature when AWB is disabled. Do we need to leave this
> behaviour undocumented and up to pipeline handlers ? I usually try to
> avoid that. Can we standardize on one behaviour that would work for
> everybody ?

This goes hand in hand with what I replied on Patch 1. We also had a
discussion on IRC what you would expect in metadata:
 a) the measurements for the current frame (which get applied on the next), 
 b) the temperature used to process the current frame.

The difficulty is, that if you specify manual ColourGains and manual
ColorCorrectionMatrix, there is no temperature used at all for
processing that frame. So b) can not be realized properly.

By implementing a), the complexity goes away and the behaviour works for
the RPi case you mention above.

The only corner case that is left, is that with regards to PFC, you
don't know when a manually set colour temperature got applied. But I
think we can neglect that one.

> 
> > Note that in the automatic case, the colour gains are still based on the
> > gray world model and the ones from the tuning file get ignored.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index c23f749c192b..d482eda5b541 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -31,6 +31,10 @@ namespace ipa::rkisp1::algorithms {
> >  
> >  LOG_DEFINE_CATEGORY(RkISP1Awb)
> >  
> > +constexpr int32_t kMinColourTemperature = 2500;
> > +constexpr int32_t kMaxColourTemperature = 10000;
> > +constexpr int32_t kDefaultColourTemperature = 6500;
> > +
> >  /* Minimum mean value below which AWB can't operate. */
> >  constexpr double kMeanMinThreshold = 2.0;
> >  
> > @@ -42,8 +46,13 @@ Awb::Awb()
> >  /**
> >   * \copydoc libcamera::ipa::Algorithm::init
> >   */
> > -int Awb::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > +int Awb::init(IPAContext &context, const YamlObject &tuningData)
> >  {
> > +	auto &cmap = context.ctrlMap;
> > +	cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,
> > +							 kMaxColourTemperature,
> > +							 kDefaultColourTemperature);
> > +
> >  	MatrixInterpolator<double, 2, 1> gains;
> >  	int ret = gains.readYaml(tuningData["gains"], "ct", "gains");
> >  	if (ret < 0)
> > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context,
> >  			<< ", blue: " << awb.gains.manual.blue;
> >  	}
> >  
> > +	const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > +	if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) {
> > +		Matrix<double, 2, 1> gains = gains_->get(*colourTemperature);
> > +		awb.gains.manual.red = gains[0][0];
> > +		awb.gains.manual.blue = gains[1][0];
> > +
> > +		LOG(RkISP1Awb, Debug)
> > +			<< "Set colour gains to red: " << awb.gains.manual.red
> > +			<< ", blue: " << awb.gains.manual.blue;
> > +	}
> 
> I wonder if the following would make the logic clearer:
> 
> 	const auto &colourGains = controls.get(controls::ColourGains);
> 	const auto &colourTemperature = controls.get(controls::ColourTemperature);
> 
> 	if (!awb.autoEnabled) {
> 		bool update = false;
> 
> 		if (colourGains) {
> 			awb.gains.manual.red = (*colourGains)[0];
> 			awb.gains.manual.blue = (*colourGains)[1];
> 			update = true;
> 		} else if (colourTemperature && gains_) {
> 			Matrix<double, 2, 1> gains = gains_->get(*colourTemperature);
> 			awb.gains.manual.red = gains[0][0];
> 			awb.gains.manual.blue = gains[1][0];
> 			update = true;
> 		}
> 
> 		if (update)
> 			LOG(RkISP1Awb, Debug)
> 				<< "Set colour gains to red: " << awb.gains.manual.red
> 				<< ", blue: " << awb.gains.manual.blue;
> 	}

If I remember correctly, I tried to stick to the style of low nesting
levels, but I think you are right, it is easier to comprehend. No that I
look at it again, the frameContext access below is also guared with
if(!awb.autoEnable) so we can even do a 

frameContext.awb.autoEnabled = awb.autoEnabled;

if(awb.autoEnable)
	return;

if (colourGains) {
        awb.gains.manual.red = (*colourGains)[0];
        awb.gains.manual.blue = (*colourGains)[1];
        update = true;
} else if (colourTemperature && gains_) {
         Matrix<double, 2, 1> gains = gains_->get(*colourTemperature);
         awb.gains.manual.red = gains[0][0];
         awb.gains.manual.blue = gains[1][0];
         update = true;
}
 
if (update)
          LOG(RkISP1Awb, Debug)
                 << "Set colour gains to red: " << awb.gains.manual.red
                 << ", blue: " << awb.gains.manual.blue;

frameContext.awb.gains.red = awb.gains.manual.red;
frameContext.awb.gains.green = 1.0;
frameContext.awb.gains.blue = awb.gains.manual.blue;


I'll prepare an new patch.

Regards,
Stefan

> 
> > +
> >  	frameContext.awb.autoEnabled = awb.autoEnabled;
> >  
> >  	if (!awb.autoEnabled) {
> 
> -- 
> Regards,
> 
> Laurent Pinchart