[libcamera-devel] [PATCH v2 3/3] ipa: ipu3: Fill AGC and AWB metadata in algorithms
Jacopo Mondi
jacopo at jmondi.org
Thu Oct 20 20:00:20 CEST 2022
Hi Kieran, Laurent
On Thu, Oct 20, 2022 at 05:11:42PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-10-20 11:33:55)
> > Hi Jacopo,
> >
> > On Thu, Oct 20, 2022 at 09:40:39AM +0200, Jacopo Mondi wrote:
> > > On Wed, Oct 19, 2022 at 11:56:14PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > Fill the frame metadata in the AGC and AWB algorithm's prepare()
> > > > function. This removes the need to fill metadata manually in the IPA
> > > > module's processStatsBuffer() function.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > > src/ipa/ipu3/algorithms/agc.cpp | 16 +++++++++++++++-
> > > > src/ipa/ipu3/algorithms/awb.cpp | 10 ++++++++++
> > > > src/ipa/ipu3/ipa_context.cpp | 3 +++
> > > > src/ipa/ipu3/ipa_context.h | 1 +
> > > > src/ipa/ipu3/ipu3.cpp | 13 +------------
> > > > 5 files changed, 30 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > > > index f4e559bf8b84..b5309bdbea25 100644
> > > > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > > > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > > > @@ -14,6 +14,7 @@
> > > > #include <libcamera/base/log.h>
> > > > #include <libcamera/base/utils.h>
> > > >
> > > > +#include <libcamera/control_ids.h>
> > > > #include <libcamera/ipa/core_ipa_interface.h>
> > > >
> > > > #include "libipa/histogram.h"
> > > > @@ -328,7 +329,7 @@ double Agc::estimateLuminance(IPAActiveState &activeState,
> > > > void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > IPAFrameContext &frameContext,
> > > > const ipu3_uapi_stats_3a *stats,
> > > > - [[maybe_unused]] ControlList &metadata)
> > > > + ControlList &metadata)
> > > > {
> > > > /*
> > > > * Estimate the gain needed to have the proportion of pixels in a given
> > > > @@ -365,6 +366,19 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > >
> > > > computeExposure(context, frameContext, yGain, iqMeanGain);
> > > > frameCount_++;
> > > > +
> > > > + utils::Duration exposureTime = context.configuration.sensor.lineDuration
> > > > + * frameContext.sensor.exposure;
> > > > + metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> > > > + metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
> > > > +
> > > > + /* \todo Use VBlank value calculated from each frame exposure. */
> > >
> > > What block will adjust the frame duration ? AEGC for sure can ends up
> > > enlarging blankings, what other component will handle control of
> > > vblank ?
> >
> > I think it will be AGC only.
>
> Yes, I don't think we would split AEC and AGC. This agc algorithm deals
> with both auto-exposure, and auto-gain. Perhaps it should be named aegc
> ... ?
>
Actually I was thinking about handling the of
controls::FrameDurationLimits which I presume should clamp the
exposure and the total frame length (hence changing the vblank here
mentioned in the comment).
RPi handles that in the IPA main module for example, that's why I was
wondering if the comment actually applies. Nothing relevant, I was mostly
thinking out loud..
>
> >
> > > Nothing that interefere with this patch
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > >
> > > > + uint32_t vTotal = context.configuration.sensor.size.height
> > > > + + context.configuration.sensor.defVBlank;
> > > > + utils::Duration frameDuration = context.configuration.sensor.lineDuration
> > > > + * vTotal;
> > > > + metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> > > > +
> > > > }
> > > >
> > > > REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> > > > index 6452b6a1acd2..dd7ebc07c534 100644
> > > > --- a/src/ipa/ipu3/algorithms/awb.cpp
> > > > +++ b/src/ipa/ipu3/algorithms/awb.cpp
> > > > @@ -11,6 +11,8 @@
> > > >
> > > > #include <libcamera/base/log.h>
> > > >
> > > > +#include <libcamera/control_ids.h>
> > > > +
> > > > /**
> > > > * \file awb.h
> > > > */
> > > > @@ -403,6 +405,14 @@ void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > context.activeState.awb.gains.green = asyncResults_.greenGain;
> > > > context.activeState.awb.gains.red = asyncResults_.redGain;
> > > > context.activeState.awb.temperatureK = asyncResults_.temperatureK;
> > > > +
> > > > + metadata.set(controls::AwbEnable, true);
> > > > + metadata.set(controls::ColourGains, {
> > > > + static_cast<float>(context.activeState.awb.gains.red),
> > > > + static_cast<float>(context.activeState.awb.gains.blue)
> > > > + });
> > > > + metadata.set(controls::ColourTemperature,
> > > > + context.activeState.awb.temperatureK);
>
> Thanks,
>
> I think this all makes sense for the moment.
>
>
>
> > > > }
> > > >
> > > > constexpr uint16_t Awb::threshold(float value)
> > > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > > > index 68f017b04751..959f314f5452 100644
> > > > --- a/src/ipa/ipu3/ipa_context.cpp
> > > > +++ b/src/ipa/ipu3/ipa_context.cpp
> > > > @@ -111,6 +111,9 @@ namespace libcamera::ipa::ipu3 {
> > > > *
> > > > * \var IPASessionConfiguration::sensor.defVBlank
> > > > * \brief The default vblank value of the sensor
> > > > + *
> > > > + * \var IPASessionConfiguration::sensor.size
> > > > + * \brief Sensor output resolution
> > > > */
> > > >
> > > > /**
> > > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > > > index 36099353e9f2..e9a3863b1693 100644
> > > > --- a/src/ipa/ipu3/ipa_context.h
> > > > +++ b/src/ipa/ipu3/ipa_context.h
> > > > @@ -41,6 +41,7 @@ struct IPASessionConfiguration {
> > > > struct {
> > > > int32_t defVBlank;
> > > > utils::Duration lineDuration;
> > > > + Size size;
> > > > } sensor;
> > > > };
> > > >
> > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > > index bc0f6007baca..a9a2b49ca95b 100644
> > > > --- a/src/ipa/ipu3/ipu3.cpp
> > > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > > @@ -502,6 +502,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > > > /* Initialise the sensor configuration. */
> > > > context_.configuration.sensor.lineDuration = sensorInfo_.minLineLength
> > > > * 1.0s / sensorInfo_.pixelRate;
> > > > + context_.configuration.sensor.size = sensorInfo_.outputSize;
> > > >
> > > > /*
> > > > * Compute the sensor V4L2 controls to be used by the algorithms and
> > > > @@ -628,8 +629,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > > > frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > > > frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> > > >
> > > > - double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
> > > > - int32_t vBlank = context_.configuration.sensor.defVBlank;
> > > > ControlList metadata(controls::controls);
> > > >
> > > > for (auto const &algo : algorithms())
> > > > @@ -637,16 +636,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > > >
> > > > setControls(frame);
> > > >
> > > > - /* \todo Use VBlank value calculated from each frame exposure. */
> > > > - int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
> > > > - metadata.set(controls::FrameDuration, frameDuration);
> > > > -
> > > > - metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> > > > -
> > > > - metadata.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
> > > > -
> > > > - metadata.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
> > > > -
>
> I'm half worrying about what would happen if the agc was optional and
> disabled... But I also think it shouldn't be all too optional right now.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > > > /*
> > > > * \todo The Metadata provides a path to getting extended data
> > > > * out to the application. Further data such as a simplifed Histogram
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
More information about the libcamera-devel
mailing list