[libcamera-devel] [PATCH v4 20/32] ipa: rkisp1: awb: Store per-frame information in frame context
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 22 22:52:28 CEST 2022
Hello,
On Thu, Sep 22, 2022 at 10:12:51AM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2022-09-21 21:34:04)
> > 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.
I'd like to have this discussion in the context of the new AWB and AEGC
controls, on top of this patch series. I think this patch provides a
good base for the discussion actually, as we'll be able to see the
effect on the code and test the runtime behaviour.
> > We can easily detect if a transition is happening here and handle it
> > here maybe ?
>
> Yes, I think it can be detected here. I would have:
>
> const auto &awbEnable = controls.get(controls::AwbEnable);
> if (awbEnable && *awbEnable != awb.autoEnabled) {
> awb.autoEnabled = *awbEnable;
>
> LOG(RkISP1Awb, Debug)
> << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
>
> + /*
> + * Pre-set manual controls with the current state to
> + * support switching to auto disabled mode when the user
> + * does not provide manual values in the same request.
> + */
> + if (!awb.autoEnabled) {
> + awb.gains.manual.red = awb.gains.automatic.red;
> + awb.gains.manual.blue = awb.gains.automatic.blue;
> + }
> }
>
> > > >
> > > > 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?
>
> I see this gets introduced later anyway - so it's fine.
>
> > > 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.
I agree, it should be stored in metadata. I'll try to implement that,
but on top of this series as it's quite large already.
> > > 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 like the idea that switching from automatic to manual without
> specifying a manual value will take the most up to date automatic values
> and lock them in as the manual values at that point. Then applications
> can of course update them to new manual values (even in the request that
> causes the transition) - or ... it simply disables the automatic
> operation from that point, and doesn't cause any visual switch to some
> historic manual value.
>
> > > - I look forward to your class that groups R/G/B values
That one is a big rabbit hole :-)
> > > - 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
Will do so on top.
> > > - 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