<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 20 Oct 2022 at 17:01, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Naushir Patuck via libcamera-devel (2022-10-20 10:25:50)<br>
> Hi Laurent,<br>
> <br>
> This change doesn't affect the RPi platform, but I just wanted to give a<br>
> quick comment.<br>
> <br>
> On Wed, 19 Oct 2022 at 21:56, Laurent Pinchart via libcamera-devel <<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<br>
> <br>
> > Fill the frame metadata in the AGC and AWB algorithm's prepare()<br>
> > function. This removes the need to fill metadata manually in the IPA<br>
> > module's processStatsBuffer() function.<br>
> ><br>
> > Signed-off-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> > ---<br>
> >  src/ipa/ipu3/algorithms/agc.cpp | 16 +++++++++++++++-<br>
> >  src/ipa/ipu3/algorithms/awb.cpp | 10 ++++++++++<br>
> >  src/ipa/ipu3/ipa_context.cpp    |  3 +++<br>
> >  src/ipa/ipu3/ipa_context.h      |  1 +<br>
> >  src/ipa/ipu3/ipu3.cpp           | 13 +------------<br>
> >  5 files changed, 30 insertions(+), 13 deletions(-)<br>
> ><br>
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp<br>
> > b/src/ipa/ipu3/algorithms/agc.cpp<br>
> > index f4e559bf8b84..b5309bdbea25 100644<br>
> > --- a/src/ipa/ipu3/algorithms/agc.cpp<br>
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp<br>
> > @@ -14,6 +14,7 @@<br>
> >  #include <libcamera/base/log.h><br>
> >  #include <libcamera/base/utils.h><br>
> ><br>
> > +#include <libcamera/control_ids.h><br>
> >  #include <libcamera/ipa/core_ipa_interface.h><br>
> ><br>
> >  #include "libipa/histogram.h"<br>
> > @@ -328,7 +329,7 @@ double Agc::estimateLuminance(IPAActiveState<br>
> > &activeState,<br>
> >  void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t<br>
> > frame,<br>
> >                   IPAFrameContext &frameContext,<br>
> >                   const ipu3_uapi_stats_3a *stats,<br>
> > -                 [[maybe_unused]] ControlList &metadata)<br>
> > +                 ControlList &metadata)<br>
> >  {<br>
> >         /*<br>
> >          * Estimate the gain needed to have the proportion of pixels in a<br>
> > given<br>
> > @@ -365,6 +366,19 @@ void Agc::process(IPAContext &context,<br>
> > [[maybe_unused]] const uint32_t frame,<br>
> ><br>
> >         computeExposure(context, frameContext, yGain, iqMeanGain);<br>
> >         frameCount_++;<br>
> > +<br>
> > +       utils::Duration exposureTime =<br>
> > context.configuration.sensor.lineDuration<br>
> > +                                    * frameContext.sensor.exposure;<br>
> > +       metadata.set(controls::AnalogueGain, frameContext.sensor.gain);<br>
> > +       metadata.set(controls::ExposureTime,<br>
> > exposureTime.get<std::micro>());<br>
> ><br>
> <br>
> Putting the metadata.set() calls into the algorithm source code like above<br>
> would<br>
> mean that the metadata is populated with the values the AGC requested. This<br>
> could be wrong because:<br>
<br>
The frame context stores multiple copies of the exposure and gain.<br>
<br>
frameContext.sensor.{exposure,gain} are the values that are determined<br>
to have been set according to delayed controls. These get set currently<br>
in<br>
<br>
void IPAIPU3::processStatsBuffer(const uint32_t frame,<br>
                                 [[maybe_unused]] const int64_t frameTimestamp,<br>
                                 const uint32_t bufferId, const ControlList &sensorControls)<br>
<br>
<br>
It looks like we don't actually store what the algorithm specifically<br>
wanted on that frame, so we're only storing what the agc 'currently'<br>
would like (in the active state) and here we get from delayed controls<br>
what was 'actually' set for the frame.<br>
<br>
So I believe it's correct.<br></blockquote><div><br></div><div>Ah, that all makes sense now, thanks for explaining!</div><div><br></div><div>Naush</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> <br>
> 1) If the sensor quantises the gain and/or shutter speed values from what<br>
> AGC asked for.<br>
> 2) Unless I am mistaken, this does not account for sensor delays.<br>
<br>
The results for the frame are passed from the pipeline handler to the<br>
IPA from delayed controls. So it should all be handled.<br>
<br>
<br>
> I am assuming the metadata list here is what is returned back via the<br>
> completed<br>
> request, is that correct? Of course, my understanding of this change could<br>
> be completely<br>
> wrong, so please ignore the noise :-)<br>
<br>
Yes, this completed metadata is what the application should receive.<br>
In this instance, it's a bit long winded, as the exposure/gains are<br>
being sent from the pipeline handler, into the IPA, stored in the frame<br>
context.sensor{} ... and then assigned to the metadata by the AGC<br>
algorithm, which then returns the completed metadata back to the<br>
pipeline handler for the request to be delivered to the application.<br>
<br>
--<br>
Kieran<br>
<br>
<br>
> <br>
> Regards,<br>
> Naush<br>
> <br>
> <br>
> > +<br>
> > +       /* \todo Use VBlank value calculated from each frame exposure. */<br>
> > +       uint32_t vTotal = context.configuration.sensor.size.height<br>
> > +                       + context.configuration.sensor.defVBlank;<br>
> > +       utils::Duration frameDuration =<br>
> > context.configuration.sensor.lineDuration<br>
> > +                                     * vTotal;<br>
> > +       metadata.set(controls::FrameDuration,<br>
> > frameDuration.get<std::micro>());<br>
> > +<br>
> >  }<br>
> ><br>
> >  REGISTER_IPA_ALGORITHM(Agc, "Agc")<br>
> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp<br>
> > b/src/ipa/ipu3/algorithms/awb.cpp<br>
> > index 6452b6a1acd2..dd7ebc07c534 100644<br>
> > --- a/src/ipa/ipu3/algorithms/awb.cpp<br>
> > +++ b/src/ipa/ipu3/algorithms/awb.cpp<br>
> > @@ -11,6 +11,8 @@<br>
> ><br>
> >  #include <libcamera/base/log.h><br>
> ><br>
> > +#include <libcamera/control_ids.h><br>
> > +<br>
> >  /**<br>
> >   * \file awb.h<br>
> >   */<br>
> > @@ -403,6 +405,14 @@ void Awb::process(IPAContext &context,<br>
> > [[maybe_unused]] const uint32_t frame,<br>
> >         context.activeState.awb.gains.green = asyncResults_.greenGain;<br>
> >         context.activeState.awb.gains.red = asyncResults_.redGain;<br>
> >         context.activeState.awb.temperatureK = asyncResults_.temperatureK;<br>
> > +<br>
> > +       metadata.set(controls::AwbEnable, true);<br>
> > +       metadata.set(controls::ColourGains, {<br>
> > +<br>
> >  static_cast<float>(context.activeState.awb.gains.red),<br>
> > +<br>
> >  static_cast<float>(context.activeState.awb.gains.blue)<br>
> > +               });<br>
> > +       metadata.set(controls::ColourTemperature,<br>
> > +                    context.activeState.awb.temperatureK);<br>
> >  }<br>
> ><br>
> >  constexpr uint16_t Awb::threshold(float value)<br>
> > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp<br>
> > index 68f017b04751..959f314f5452 100644<br>
> > --- a/src/ipa/ipu3/ipa_context.cpp<br>
> > +++ b/src/ipa/ipu3/ipa_context.cpp<br>
> > @@ -111,6 +111,9 @@ namespace libcamera::ipa::ipu3 {<br>
> >   *<br>
> >   * \var IPASessionConfiguration::sensor.defVBlank<br>
> >   * \brief The default vblank value of the sensor<br>
> > + *<br>
> > + * \var IPASessionConfiguration::sensor.size<br>
> > + * \brief Sensor output resolution<br>
> >   */<br>
> ><br>
> >  /**<br>
> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h<br>
> > index 36099353e9f2..e9a3863b1693 100644<br>
> > --- a/src/ipa/ipu3/ipa_context.h<br>
> > +++ b/src/ipa/ipu3/ipa_context.h<br>
> > @@ -41,6 +41,7 @@ struct IPASessionConfiguration {<br>
> >         struct {<br>
> >                 int32_t defVBlank;<br>
> >                 utils::Duration lineDuration;<br>
> > +               Size size;<br>
> >         } sensor;<br>
> >  };<br>
> ><br>
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp<br>
> > index bc0f6007baca..a9a2b49ca95b 100644<br>
> > --- a/src/ipa/ipu3/ipu3.cpp<br>
> > +++ b/src/ipa/ipu3/ipu3.cpp<br>
> > @@ -502,6 +502,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,<br>
> >         /* Initialise the sensor configuration. */<br>
> >         context_.configuration.sensor.lineDuration =<br>
> > sensorInfo_.minLineLength<br>
> >                                                    * 1.0s /<br>
> > sensorInfo_.pixelRate;<br>
> > +       context_.configuration.sensor.size = sensorInfo_.outputSize;<br>
> ><br>
> >         /*<br>
> >          * Compute the sensor V4L2 controls to be used by the algorithms<br>
> > and<br>
> > @@ -628,8 +629,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,<br>
> >         frameContext.sensor.exposure =<br>
> > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();<br>
> >         frameContext.sensor.gain =<br>
> > camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());<br>
> ><br>
> > -       double lineDuration =<br>
> > context_.configuration.sensor.lineDuration.get<std::micro>();<br>
> > -       int32_t vBlank = context_.configuration.sensor.defVBlank;<br>
> >         ControlList metadata(controls::controls);<br>
> ><br>
> >         for (auto const &algo : algorithms())<br>
> > @@ -637,16 +636,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,<br>
> ><br>
> >         setControls(frame);<br>
> ><br>
> > -       /* \todo Use VBlank value calculated from each frame exposure. */<br>
> > -       int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) *<br>
> > lineDuration;<br>
> > -       metadata.set(controls::FrameDuration, frameDuration);<br>
> > -<br>
> > -       metadata.set(controls::AnalogueGain, frameContext.sensor.gain);<br>
> > -<br>
> > -       metadata.set(controls::ColourTemperature,<br>
> > context_.activeState.awb.temperatureK);<br>
> > -<br>
> > -       metadata.set(controls::ExposureTime, frameContext.sensor.exposure<br>
> > * lineDuration);<br>
> > -<br>
> >         /*<br>
> >          * \todo The Metadata provides a path to getting extended data<br>
> >          * out to the application. Further data such as a simplifed<br>
> > Histogram<br>
> > --<br>
> > Regards,<br>
> ><br>
> > Laurent Pinchart<br>
> ><br>
> ><br>
</blockquote></div></div>