[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 11:38:23 CEST 2021
Hi Jean-Michel,
On Mon, Aug 16, 2021 at 09:55:20AM +0200, Jean-Michel Hautbois wrote:
> On 16/08/2021 01:51, Laurent Pinchart wrote:
> > 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.
>
> I like the idea of naming the structures...
There will always be disagreements on personal preferences :-) My view
in this case is that is clutters the structure definition for little
gain, but I'm sure that's at least partly caused by coding experience
that is very personal.
> >>> + } 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.
>
> Indeed, and that's really painful. That's why I was tempted to separate
> into algorithms even if they access the same structure in the prepare()
> call they will never set all the parameters, only the ones they are
> responsible for. But then it means we need to make sure we have all the
> defaults. Spaghetti dish you said :-/ ?
I don't think it's too bad, as long as we make sure that the design
ensures that everything will be initialized in all cases.
> >> 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 ?
>
> This is something which is a bad comprehension (from me) in the first
> place. I did not want to change that here, as it is a move.
> You are right, a gain of 1.0 is mapped to 8192. Doing so would lead to
> greenish frames as we don't have the corresponding CCM to compensate.
Could you give it a try and share the result ?
> >>>
> >>> 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