[libcamera-devel] [PATCH v2 06/10] ipa: ipu3: convert AWB to the new algorithm interface
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Mon Aug 16 09:55:20 CEST 2021
Hi Laurent,
On 16/08/2021 01:51, Laurent Pinchart wrote:
> 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.
>
I like the idea of naming the structures...
>>> + } 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 :-/ ?
>> 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.
>>>
>>> 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();
>
More information about the libcamera-devel
mailing list