[libcamera-devel] [PATCH v4 20/32] ipa: rkisp1: awb: Store per-frame information in frame context
Jacopo Mondi
jacopo at jmondi.org
Wed Sep 21 22:34:04 CEST 2022
Hi Laurent, Kieran
On Tue, Sep 20, 2022 at 11:55:55PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:48)
> > Rework the algorithm's usage of the active state and frame context to
> > store data in the right place.
> >
> > The active state stores two distinct categories of information:
> >
> > - The consolidated value of all algorithm controls. Requests passed to
> > the queueRequest() function store values for controls that the
> > application wants to modify for that particular frame, and the
> > queueRequest() function updates the active state with those values.
> > The active state thus contains a consolidated view of the value of all
> > controls handled by the algorithm.
> >
> > - The value of parameters computed by the algorithm when running in auto
> > mode. Algorithms running in auto mode compute new parameters every
> > time statistics buffers are received (either synchronously, or
> > possibly in a background thread). The latest computed value of those
> > parameters is stored in the active state in the process() function.
> >
> > The frame context also stores two categories of information:
> >
> > - The value of the controls to be applied to the frame. These values are
> > typically set in the queueRequest() function, from the consolidated
> > control values stored in the active state. The frame context thus
> > stores values for all controls related to the algorithm, not limited
> > to the controls specified in the corresponding request, but
> > consolidated from all requests that have been queued so far.
> >
> > For controls that can be specified manually or computed by the
> > algorithm depending on the operation mode (such as the colour gains),
> > the control value will be stored in the frame context in
> > queueRequest() only when operating in manual mode. When operating in
> > auto mode, the values are computed by the algorithm and stored in the
> > frame context in prepare(), just before being stored in the ISP
> > parameters buffer.
> >
> > The queueRequest() function can also store ancillary data in the frame
> > context, such as flags to indicate if (and what) control values have
> > changed compared to the previous request.
> >
> > - Status information computed by the algorithm for a frame. For
> > instance, the colour temperature estimated by the algorithm from ISP
> > statistics calculated on a frame is stored in the frame context for
> > that frame in the process() function.
> >
> > The active state and frame context thus both contain identical members
> > for most control values, but store values that have a different meaning.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/ipa/rkisp1/algorithms/awb.cpp | 75 +++++++++++++++++++------------
> > src/ipa/rkisp1/ipa_context.cpp | 51 +++++++++++++++++----
> > src/ipa/rkisp1/ipa_context.h | 25 +++++++++--
> > 3 files changed, 110 insertions(+), 41 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index a0bbe5cb8afa..bb0f6c27fc7d 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -36,9 +36,12 @@ LOG_DEFINE_CATEGORY(RkISP1Awb)
> > int Awb::configure(IPAContext &context,
> > const IPACameraSensorInfo &configInfo)
> > {
> > - context.activeState.awb.gains.red = 1.0;
> > - context.activeState.awb.gains.blue = 1.0;
> > - context.activeState.awb.gains.green = 1.0;
> > + context.activeState.awb.gains.manual.red = 1.0;
> > + context.activeState.awb.gains.manual.blue = 1.0;
> > + context.activeState.awb.gains.manual.green = 1.0;
> > + context.activeState.awb.gains.automatic.red = 1.0;
> > + context.activeState.awb.gains.automatic.blue = 1.0;
> > + context.activeState.awb.gains.automatic.green = 1.0;
> > context.activeState.awb.autoEnabled = true;
> >
> > /*
> > @@ -75,13 +78,22 @@ uint32_t Awb::estimateCCT(double red, double green, double blue)
> > * \copydoc libcamera::ipa::Algorithm::prepare
> > */
> > void Awb::prepare(IPAContext &context, const uint32_t frame,
> > - [[maybe_unused]] RkISP1FrameContext &frameContext,
> > - rkisp1_params_cfg *params)
> > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)
> > {
> > - params->others.awb_gain_config.gain_green_b = 256 * context.activeState.awb.gains.green;
> > - params->others.awb_gain_config.gain_blue = 256 * context.activeState.awb.gains.blue;
> > - params->others.awb_gain_config.gain_red = 256 * context.activeState.awb.gains.red;
> > - params->others.awb_gain_config.gain_green_r = 256 * context.activeState.awb.gains.green;
> > + /*
> > + * This is the latest time we can read the active state. This is the
> > + * most up-to-date automatic values we can read.
> > + */
> > + if (frameContext.awb.autoEnabled) {
> > + frameContext.awb.gains.red = context.activeState.awb.gains.automatic.red;
> > + frameContext.awb.gains.green = context.activeState.awb.gains.automatic.green;
> > + frameContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue;
> > + }
> > +
> > + params->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green;
> > + params->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue;
> > + params->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red;
> > + params->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green;
> >
> > /* Update the gains. */
> > params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AWB_GAIN;
> > @@ -127,7 +139,7 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,
> > */
> > void Awb::queueRequest(IPAContext &context,
> > [[maybe_unused]] const uint32_t frame,
> > - [[maybe_unused]] RkISP1FrameContext &frameContext,
> > + RkISP1FrameContext &frameContext,
> > const ControlList &controls)
> > {
> > auto &awb = context.activeState.awb;
> > @@ -142,21 +154,29 @@ void Awb::queueRequest(IPAContext &context,
> >
> > const auto &colourGains = controls.get(controls::ColourGains);
> > if (colourGains && !awb.autoEnabled) {
> > - awb.gains.red = (*colourGains)[0];
> > - awb.gains.blue = (*colourGains)[1];
> > + awb.gains.manual.red = (*colourGains)[0];
> > + awb.gains.manual.blue = (*colourGains)[1];
>
> I really do feel like if the application provides a set of manual gains,
> we should store them in the internal manual gains.
>
> Unless perhaps if we say upon transitioning from automatic, to manual
> (Auto-Disabled)if we do not have a manual value in the same request,
> we'll use the most recently computed auto values and hold them until
> told otherwise.
>
> If we don't - we're going to suddenly jump to 'manually' set values that
> are some historic values... It just feels quite wrong to me.
>
I side with Kieran, even if libcamera::AwbEnable, probably because
it's old, doesn't tell much about the expected behaviour. After many
discussions on AEGC, the decision was to re-start from the last
computed vaues in the previous mode, when transitioning from Auto to
Manual or viceversa.
We can easily detect if a transition is happening here and handle it
here maybe ?
>
> >
> > LOG(RkISP1Awb, Debug)
> > - << "Set colour gains to red: " << awb.gains.red
> > - << ", blue: " << awb.gains.blue;
> > + << "Set colour gains to red: " << awb.gains.manual.red
> > + << ", blue: " << awb.gains.manual.blue;
> > + }
> > +
> > + frameContext.awb.autoEnabled = awb.autoEnabled;
> > +
> > + if (!awb.autoEnabled) {
> > + frameContext.awb.gains.red = awb.gains.manual.red;
> > + frameContext.awb.gains.green = 1.0;
> > + frameContext.awb.gains.blue = awb.gains.manual.blue;
> > }
> > }
> >
> > /**
> > * \copydoc libcamera::ipa::Algorithm::process
> > */
> > -void Awb::process([[maybe_unused]] IPAContext &context,
> > +void Awb::process(IPAContext &context,
> > [[maybe_unused]] const uint32_t frame,
> > - [[maybe_unused]] RkISP1FrameContext &frameCtx,
> > + RkISP1FrameContext &frameContext,
> > const rkisp1_stat_buffer *stats)
> > {
> > const rkisp1_cif_isp_stat *params = &stats->params;
> > @@ -187,30 +207,27 @@ void Awb::process([[maybe_unused]] IPAContext &context,
> > double greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean;
> > double blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean;
> >
> > + frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);
> > +
>
> We likely want to store the temperature of each frame to make sure we
> update the metadata, but I wonder if we might also want to track the
> colour temperature in the Active state so that for instance the LSC can
> check what the current temperature is?
>
> If we are able to get the algorithms to update the metadata structure
> directly, then a value like the colour temperature here probably doesn't
> need to go into the frame context, just to be read later.
>
> But for now - I think this is accurate.
>
> I suspect perhaps a "ControlList metadata" could/should go into the base
> FrameContext, so that all calls given a FrameContext can directly update
> the metadata which will then be passed to the pipeline handler to put
> into the request when it completes.
>
> (not this patch, later)
>
> > /* Estimate the red and blue gains to apply in a grey world. */
> > double redGain = greenMean / (redMean + 1);
> > double blueGain = greenMean / (blueMean + 1);
> >
> > /* Filter the values to avoid oscillations. */
> > double speed = 0.2;
> > - redGain = speed * redGain + (1 - speed) * activeState.awb.gains.red;
> > - blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.blue;
> > + redGain = speed * redGain + (1 - speed) * activeState.awb.gains.automatic.red;
> > + blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.automatic.blue;
> >
> > /*
> > * Gain values are unsigned integer value, range 0 to 4 with 8 bit
> > - * fractional part.
> > + * fractional part. Hardcode the green gain to 1.0.
> > */
> > - if (activeState.awb.autoEnabled) {
> > - activeState.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);
> > - activeState.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
> > - }
> > - /* Hardcode the green gain to 1.0. */
> > - activeState.awb.gains.green = 1.0;
> > + activeState.awb.gains.automatic.red = std::clamp(redGain, 0.0, 1023.0 / 256);
> > + activeState.awb.gains.automatic.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
> > + activeState.awb.gains.automatic.green = 1.0;
> >
> > - activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);
> > -
> > - LOG(RkISP1Awb, Debug) << "Gain found for red: " << context.activeState.awb.gains.red
> > - << " and for blue: " << context.activeState.awb.gains.blue;
> > + LOG(RkISP1Awb, Debug) << "Gain found for red: " << activeState.awb.gains.automatic.red
> > + << " and for blue: " << activeState.awb.gains.automatic.blue;
> > }
> >
> > REGISTER_IPA_ALGORITHM(Awb, "Awb")
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 88e0631c8d39..cd1443e1ed46 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -120,17 +120,29 @@ namespace libcamera::ipa::rkisp1 {
> > * \struct IPAActiveState::awb.gains
> > * \brief White balance gains
> > *
> > - * \var IPAActiveState::awb.gains.red
> > - * \brief White balance gain for R channel
> > + * \struct IPAActiveState::awb.gains.manual
> > + * \brief Manual white balance gains (set through requests)
> > *
> > - * \var IPAActiveState::awb.gains.green
> > - * \brief White balance gain for G channel
> > + * \var IPAActiveState::awb.gains.manual.red
> > + * \brief Manual white balance gain for R channel
> > *
> > - * \var IPAActiveState::awb.gains.blue
> > - * \brief White balance gain for B channel
> > + * \var IPAActiveState::awb.gains.manual.green
> > + * \brief Manual white balance gain for G channel
> > *
> > - * \var IPAActiveState::awb.temperatureK
> > - * \brief Estimated color temperature
> > + * \var IPAActiveState::awb.gains.manual.blue
> > + * \brief Manual white balance gain for B channel
> > + *
> > + * \struct IPAActiveState::awb.gains.automatic
> > + * \brief Automatic white balance gains (computed by the algorithm)
> > + *
> > + * \var IPAActiveState::awb.gains.automatic.red
> > + * \brief Automatic white balance gain for R channel
> > + *
> > + * \var IPAActiveState::awb.gains.automatic.green
> > + * \brief Automatic white balance gain for G channel
> > + *
> > + * \var IPAActiveState::awb.gains.automatic.blue
> > + * \brief Automatic white balance gain for B channel
> > *
> > * \var IPAActiveState::awb.autoEnabled
> > * \brief Whether the Auto White Balance algorithm is enabled
> > @@ -201,6 +213,29 @@ namespace libcamera::ipa::rkisp1 {
> > * The gain should be adapted to the sensor specific gain code before applying.
> > */
> >
> > +/**
> > + * \var RkISP1FrameContext::awb
> > + * \brief Automatic White Balance parameters for this frame
> > + *
> > + * \struct RkISP1FrameContext::awb.gains
> > + * \brief White balance gains
> > + *
> > + * \var RkISP1FrameContext::awb.gains.red
> > + * \brief White balance gain for R channel
> > + *
> > + * \var RkISP1FrameContext::awb.gains.green
> > + * \brief White balance gain for G channel
> > + *
> > + * \var RkISP1FrameContext::awb.gains.blue
> > + * \brief White balance gain for B channel
> > + *
> > + * \var RkISP1FrameContext::awb.temperatureK
> > + * \brief Estimated color temperature
> > + *
> > + * \var RkISP1FrameContext::awb.autoEnabled
> > + * \brief Whether the Auto White Balance algorithm is enabled
> > + */
>
> I believe these are all going to get simplified later ... I like that
> ... but if the support class came earlier, it could move to that
> directly in here.
>
> Oh - in fact, it looks like the patch I'm thinking of isn't in this
> series yet :(
>
>
> Ok - so I'm happy enough with this. I look forward to the RGB / Pixel class
> which I think helps simplify this code a fair bit, but it's not required
> for this conversion.
>
>
> I think this patch tells me:
>
> - I need to understand how we globally expect to handle setting manual
> values when an algorithm is configured to use automatic ones.
We surely need to push for a single model for all controls. I haven't
thought too much about which algorithms would not be able to comply and
why, but if no obvious reasons, I think we should aim to be
consistent.
>
> - I look forward to your class that groups R/G/B values
>
> - We should already add a common metadata object to populate, so we can
> report metadata back in completed requests.
>
> - ColourTemperature should go straight to the metadata
>
> - ActiveState should store the ColourTemperature so that LSC can refer
> to it.
>
>
> But all of that can be done on top...
>
> so
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > +
> > /**
> > * \var RkISP1FrameContext::sensor
> > * \brief Sensor configuration that used been used for this frame
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index ecf993cd22d7..d97aae9a97b4 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -56,12 +56,18 @@ struct IPAActiveState {
> >
> > struct {
> > struct {
> > - double red;
> > - double green;
> > - double blue;
> > + struct {
> > + double red;
> > + double green;
> > + double blue;
> > + } manual;
> > + struct {
> > + double red;
> > + double green;
> > + double blue;
> > + } automatic;
> > } gains;
> >
> > - double temperatureK;
> > bool autoEnabled;
> > } awb;
> >
> > @@ -90,6 +96,17 @@ struct RkISP1FrameContext : public FrameContext {
> > double gain;
> > } agc;
> >
> > + struct {
> > + struct {
> > + double red;
> > + double green;
> > + double blue;
> > + } gains;
> > +
> > + double temperatureK;
> > + bool autoEnabled;
> > + } awb;
> > +
> > struct {
> > uint32_t exposure;
> > double gain;
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
More information about the libcamera-devel
mailing list