[libcamera-devel] [PATCH v2 06/10] ipa: ipu3: convert AWB to the new algorithm interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 16 01:51:37 CEST 2021
Hi Jean-Michel,
Thank you for the patch.
On Fri, Aug 13, 2021 at 10:49:56AM +0100, Kieran Bingham wrote:
> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> > When the stats are received, pass it with the context to the existing
s/it/them/
> > AWB algorithm. IPAFrameContext now has a new structure to store the
> > gains calculated byt the AWB algorithm.
>
> s/byt/by/
>
> >
> > When a EventFillParams event is received, call prepare() and set the new
>
> /When a/When an/
>
> > gains accordingly in the params structure.
> > There is no more a need for the IPU3Awb::initialise() function, as the
> > params are always set in prepare().
> >
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > ---
> > src/ipa/ipu3/ipa_context.h | 8 ++++
> > src/ipa/ipu3/ipu3.cpp | 5 +--
> > src/ipa/ipu3/ipu3_awb.cpp | 76 ++++++++++++++++++++------------------
> > src/ipa/ipu3/ipu3_awb.h | 6 +--
> > 4 files changed, 54 insertions(+), 41 deletions(-)
> >
> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > index e995c663..7bb4f598 100644
> > --- a/src/ipa/ipu3/ipa_context.h
> > +++ b/src/ipa/ipu3/ipa_context.h
> > @@ -38,6 +38,14 @@ struct IPAFrameContext {
> > double gain;
> > } agc;
> >
> > + struct Awb {
> > + struct Gains {
> > + double redGain;
> > + double greenGain;
> > + double blueGain;
>
> They're all in a structure called gains, can they be named simpler as
> just 'red', 'green', 'blue' ?
Agree. Also, we don't necessarily need to name the structures here.
> > + } gains;
> > + } awb;
> > +
> > struct Contrast {
> > uint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];
> > } contrast;
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index e09adfc3..dba5554d 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -230,7 +230,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> > }
> >
> > awbAlgo_ = std::make_unique<IPU3Awb>();
> > - awbAlgo_->initialise(params_, context_.configuration.grid.bdsOutputSize, context_.configuration.grid.bdsGrid);
> >
> > agcAlgo_ = std::make_unique<IPU3Agc>();
> > agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);
> > @@ -311,7 +310,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> > algo->prepare(context_, params_);
> >
> > if (agcAlgo_->updateControls())
> > - awbAlgo_->updateWbParameters(params_);
> > + awbAlgo_->prepare(context_, params_);
> >
> > *params = params_;
> >
> > @@ -337,7 +336,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> > gain = context_.frameContext.agc.gain;
> > gain_ = camHelper_->gainCode(gain);
> >
> > - awbAlgo_->calculateWBGains(stats);
> > + awbAlgo_->process(context_, stats);
> >
> > if (agcAlgo_->updateControls())
> > setControls(frame);
> > diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> > index 0f3bcdc9..b422b095 100644
> > --- a/src/ipa/ipu3/ipu3_awb.cpp
> > +++ b/src/ipa/ipu3/ipu3_awb.cpp
> > @@ -121,42 +121,14 @@ IPU3Awb::IPU3Awb()
> > asyncResults_.greenGain = 1.0;
> > asyncResults_.redGain = 1.0;
> > asyncResults_.temperatureK = 4500;
> > +
> > + zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
> > }
> >
> > IPU3Awb::~IPU3Awb()
> > {
> > }
> >
> > -void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)
> > -{
> > - params.use.acc_awb = 1;
> > - params.acc_param.awb.config.rgbs_thr_gr = 8191;
> > - params.acc_param.awb.config.rgbs_thr_r = 8191;
> > - params.acc_param.awb.config.rgbs_thr_gb = 8191;
> > - params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;
> > - awbGrid_ = bdsGrid;
> > -
> > - params.use.acc_bnr = 1;
> > - params.acc_param.bnr = imguCssBnrDefaults;
> > - /**
> > - * Optical center is column (respectively row) startminus X (respectively Y) center.
> > - * For the moment use BDS as a first approximation, but it should
> > - * be calculated based on Shading (SHD) parameters.
> > - */
> > - params.acc_param.bnr.column_size = bdsOutputSize.width;
> > - params.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);
> > - params.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);
> > - params.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset
> > - * params.acc_param.bnr.opt_center.x_reset;
> > - params.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset
> > - * params.acc_param.bnr.opt_center.y_reset;
> > -
> > - params.use.acc_ccm = 1;
> > - params.acc_param.ccm = imguCssCcmDefault;
> > -
> > - zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
> > -}
> > -
> > /**
> > * The function estimates the correlated color temperature using
> > * from RGB color space input.
> > @@ -303,17 +275,51 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> > }
> > }
> >
> > -void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms)
> > +void IPU3Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> > +{
> > + calculateWBGains(stats);
>
> I'd put a new line here.
>
> Perhaps a comment here saying "
>
> /*
> * Gains are only recalculated if enough zones were detected.
> * The results are cached, so if no results were calculated, we set the
> * cached values from asyncResults_ here.
> */
>
>
> > + context.frameContext.awb.gains.blueGain = asyncResults_.blueGain;
> > + context.frameContext.awb.gains.greenGain = asyncResults_.greenGain;
> > + context.frameContext.awb.gains.redGain = asyncResults_.redGain;
> > +}
> > +
> > +void IPU3Awb::prepare(IPAContext &context, ipu3_uapi_params ¶ms)
> > {
> > + params.use.acc_awb = 1;
>
> Set enable/use flags last... - or in this instance, after setting the
> awb parameters.
On this topic, btw, I tend to set structure fields in the order in which
they are defined in the structure. I suppose it's a matter of personal
taste.
> > + params.acc_param.awb.config.rgbs_thr_gr = 8191;
> > + params.acc_param.awb.config.rgbs_thr_r = 8191;
> > + params.acc_param.awb.config.rgbs_thr_gb = 8191;
> > + params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;
Line wrap.
> > + awbGrid_ = context.configuration.grid.bdsGrid;
> > +
> > + params.use.acc_bnr = 1;
>
> And this shows that there are modules to further break out of this...
Agreed, I think Bayer noise reduction should be moved to a separate
module, but note that the ImgU groups parameters in a weird way.
params.acc_param.bnr contains much more than noise reduction parameters
(namely at least white balance, lens shading correction and bad pixel
correction). acc_bnr should thus be set if any of those algorithms are
enabled, but we then need to make sure that default values are
programmed for all other algorithms.
> Anything that has it's own 'module' on the IPU3 should have it's own
> corresponding module in the code base I think.
It's a good starting point, but it can sometimes make sense to group or
split modules in different ways, to match how the algorithms are
implemented. I haven't checked, but for instance the AWB and CCM
parameters may make sense to control from a single algorithm.
> > + params.acc_param.bnr = imguCssBnrDefaults;
Based on the above comment, maybe this should be moved to ipu3.cpp (in a
separate patch though).
> new line?
>
> > + /**
>
> s#/**#/*#
>
> > + * Optical center is column (respectively row) startminus X (respectively Y) center.
I know this is moved from a different location, but I have trouble
understanding the comment. The typo s/startminus/start minus/ could
already be fixed here. Oh, and please reflow the comment to 80 columns.
> > + * For the moment use BDS as a first approximation, but it should
> > + * be calculated based on Shading (SHD) parameters.
> > + */
> > + Size &bdsOutputSize = context.configuration.grid.bdsOutputSize;
> > + params.acc_param.bnr.column_size = bdsOutputSize.width;
> > + params.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);
> > + params.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);
> > + params.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset
> > + * params.acc_param.bnr.opt_center.x_reset;
> > + params.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset
> > + * params.acc_param.bnr.opt_center.y_reset;
> > +
> > + params.use.acc_ccm = 1;
>
> Same, but keep both BNR and CCM here in this patch, and they can move
> out to their own modules separately.
>
> Fairly trivial comments in this patch.
>
> With those handled as best as you see fit,
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > + params.acc_param.ccm = imguCssCcmDefault;
> > +
> > /*
> > * Green gains should not be touched and considered 1.
> > * Default is 16, so do not change it at all.
> > * 4096 is the value for a gain of 1.0
> > */
> > - params.acc_param.bnr.wb_gains.gr = 16;
> > - params.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;
> > - params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;
> > - params.acc_param.bnr.wb_gains.gb = 16;
> > + params.acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.greenGain;
> > + params.acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.redGain;
> > + params.acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blueGain;
> > + params.acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.greenGain;
Does this mean that a gain of 1.0 is encoded as 16 for the green
channels and 4096 for the red and blue channels ?
This also doesn't match the comment in intel-ipu3.h that states
/**
* struct ipu3_uapi_bnr_static_config_wb_gains_config - White balance gains
*
* @gr: white balance gain for Gr channel.
* @r: white balance gain for R channel.
* @b: white balance gain for B channel.
* @gb: white balance gain for Gb channel.
*
* Precision u3.13, range [0, 8). White balance correction is done by applying
* a multiplicative gain to each color channels prior to BNR.
*/
This indicates that 1.0 maps to 8192. Do we have evidence that this is
incorrect ?
> >
> > LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
> >
> > diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> > index eeb2920b..4de3fae2 100644
> > --- a/src/ipa/ipu3/ipu3_awb.h
> > +++ b/src/ipa/ipu3/ipu3_awb.h
> > @@ -29,9 +29,8 @@ public:
> > IPU3Awb();
> > ~IPU3Awb();
> >
> > - void initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
> > - void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> > - void updateWbParameters(ipu3_uapi_params ¶ms);
> > + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;
> > + void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> >
> > struct Ipu3AwbCell {
> > unsigned char greenRedAvg;
> > @@ -72,6 +71,7 @@ public:
> > };
> >
> > private:
> > + void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> > void generateZones(std::vector<RGB> &zones);
> > void generateAwbStats(const ipu3_uapi_stats_3a *stats);
> > void clearAwbStats();
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list