[libcamera-devel] [PATCH v2 5/7] ipa: raspberrypi: Use an array of RPiController::Metadata objects

Naushir Patuck naush at raspberrypi.com
Fri Sep 23 15:24:32 CEST 2022


Hi Davd,

Thanks for the review of this series!

On Fri, 23 Sept 2022 at 11:38, David Plowman <david.plowman at raspberrypi.com>
wrote:

> HI Naush
>
> Thanks for the patch.
>
> On Mon, 5 Sept 2022 at 08:40, Naushir Patuck via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Cycle through an array of RPiController::Metadata objects, one per
> > prepare()/process() invocation, when running the controller algorithms.
> This
> > allows historical metadata objects to be retained, and subsequently
> passed into
> > the controller algorithms on future frames.
> >
> > This change provides a route to fixing a problem with the AGC algorithm,
> where
> > if a manual shutter/gain is requested, the algorithm does not currently
> retain
> > any context of the total exposure that it has calculated. As a result,
> the
> > wrong digital gain would be applied when the frame with the manual
> shutter/gain
> > is processed by the ISP.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/raspberrypi.cpp | 69 +++++++++++++++++------------
> >  1 file changed, 40 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 8d731435764e..63cccda901c1 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -57,6 +57,9 @@ namespace libcamera {
> >  using namespace std::literals::chrono_literals;
> >  using utils::Duration;
> >
> > +/* Number of metadata objects available in the context list. */
> > +constexpr unsigned int numMetadataContexts = 16;
>
> Just curious as to what determines this number. Presumably it only has
> to be bigger than the max delay we have? Can anything else affect it,
> like when the IPAs skip frames at high framerates?
>

It's actually the max delay + the number of dropped frames at startup.
I chose 16 because it matches the DelayedControl ring buffer size.... I
think!

I will post a v3 with your other amendments + address Umang's comments
early next week.

Regards,
Naush


>
> But anyway, it all looked good to me.
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> > +
> >  /* Configure the sensor with these values initially. */
> >  constexpr double defaultAnalogueGain = 1.0;
> >  constexpr Duration defaultExposureTime = 20.0ms;
> > @@ -163,7 +166,8 @@ private:
> >         /* Raspberry Pi controller specific defines. */
> >         std::unique_ptr<RPiController::CamHelper> helper_;
> >         RPiController::Controller controller_;
> > -       RPiController::Metadata rpiMetadata_;
> > +       std::array<RPiController::Metadata, numMetadataContexts>
> rpiMetadata_;
> > +       unsigned int metadataIdx_;
> >
> >         /*
> >          * We count frames to decide if the frame must be hidden (e.g.
> from
> > @@ -319,6 +323,7 @@ void IPARPi::start(const ControlList &controls,
> StartConfig *startConfig)
> >
> >         firstStart_ = false;
> >         lastRunTimestamp_ = 0;
> > +       metadataIdx_ = 0;
> >  }
> >
> >  void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)
> > @@ -513,6 +518,7 @@ void IPARPi::signalStatReady(uint32_t bufferId)
> >         reportMetadata();
> >
> >         statsMetadataComplete.emit(bufferId & MaskID,
> libcameraMetadata_);
> > +       metadataIdx_ = (metadataIdx_ + 1) % rpiMetadata_.size();
> >  }
> >
> >  void IPARPi::signalQueueRequest(const ControlList &controls)
> > @@ -536,14 +542,15 @@ void IPARPi::signalIspPrepare(const ISPConfig
> &data)
> >
> >  void IPARPi::reportMetadata()
> >  {
> > -       std::unique_lock<RPiController::Metadata> lock(rpiMetadata_);
> > +       RPiController::Metadata &rpiMetadata =
> rpiMetadata_[metadataIdx_];
> > +       std::unique_lock<RPiController::Metadata> lock(rpiMetadata);
> >
> >         /*
> >          * Certain information about the current frame and how it will be
> >          * processed can be extracted and placed into the libcamera
> metadata
> >          * buffer, where an application could query it.
> >          */
> > -       DeviceStatus *deviceStatus =
> rpiMetadata_.getLocked<DeviceStatus>("device.status");
> > +       DeviceStatus *deviceStatus =
> rpiMetadata.getLocked<DeviceStatus>("device.status");
> >         if (deviceStatus) {
> >                 libcameraMetadata_.set(controls::ExposureTime,
> >
> deviceStatus->shutterSpeed.get<std::micro>());
> > @@ -554,17 +561,17 @@ void IPARPi::reportMetadata()
> >
>  libcameraMetadata_.set(controls::SensorTemperature,
> *deviceStatus->sensorTemperature);
> >         }
> >
> > -       AgcStatus *agcStatus =
> rpiMetadata_.getLocked<AgcStatus>("agc.status");
> > +       AgcStatus *agcStatus =
> rpiMetadata.getLocked<AgcStatus>("agc.status");
> >         if (agcStatus) {
> >                 libcameraMetadata_.set(controls::AeLocked,
> agcStatus->locked);
> >                 libcameraMetadata_.set(controls::DigitalGain,
> agcStatus->digitalGain);
> >         }
> >
> > -       LuxStatus *luxStatus =
> rpiMetadata_.getLocked<LuxStatus>("lux.status");
> > +       LuxStatus *luxStatus =
> rpiMetadata.getLocked<LuxStatus>("lux.status");
> >         if (luxStatus)
> >                 libcameraMetadata_.set(controls::Lux, luxStatus->lux);
> >
> > -       AwbStatus *awbStatus =
> rpiMetadata_.getLocked<AwbStatus>("awb.status");
> > +       AwbStatus *awbStatus =
> rpiMetadata.getLocked<AwbStatus>("awb.status");
> >         if (awbStatus) {
> >                 libcameraMetadata_.set(controls::ColourGains,
> >                                        Span<const float, 2>({
> static_cast<float>(awbStatus->gainR),
> > @@ -572,7 +579,7 @@ void IPARPi::reportMetadata()
> >                 libcameraMetadata_.set(controls::ColourTemperature,
> awbStatus->temperatureK);
> >         }
> >
> > -       BlackLevelStatus *blackLevelStatus =
> rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status");
> > +       BlackLevelStatus *blackLevelStatus =
> rpiMetadata.getLocked<BlackLevelStatus>("black_level.status");
> >         if (blackLevelStatus)
> >                 libcameraMetadata_.set(controls::SensorBlackLevels,
> >                                        Span<const int32_t, 4>({
> static_cast<int32_t>(blackLevelStatus->blackLevelR),
> > @@ -580,7 +587,7 @@ void IPARPi::reportMetadata()
> >
>  static_cast<int32_t>(blackLevelStatus->blackLevelG),
> >
>  static_cast<int32_t>(blackLevelStatus->blackLevelB) }));
> >
> > -       FocusStatus *focusStatus =
> rpiMetadata_.getLocked<FocusStatus>("focus.status");
> > +       FocusStatus *focusStatus =
> rpiMetadata.getLocked<FocusStatus>("focus.status");
> >         if (focusStatus && focusStatus->num == 12) {
> >                 /*
> >                  * We get a 4x3 grid of regions by default. Calculate
> the average
> > @@ -591,7 +598,7 @@ void IPARPi::reportMetadata()
> >                 libcameraMetadata_.set(controls::FocusFoM, focusFoM);
> >         }
> >
> > -       CcmStatus *ccmStatus =
> rpiMetadata_.getLocked<CcmStatus>("ccm.status");
> > +       CcmStatus *ccmStatus =
> rpiMetadata.getLocked<CcmStatus>("ccm.status");
> >         if (ccmStatus) {
> >                 float m[9];
> >                 for (unsigned int i = 0; i < 9; i++)
> > @@ -1002,10 +1009,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned int
> bufferId)
> >  void IPARPi::prepareISP(const ISPConfig &data)
> >  {
> >         int64_t frameTimestamp =
> data.controls.get(controls::SensorTimestamp).value_or(0);
> > -       RPiController::Metadata lastMetadata;
> > +       RPiController::Metadata &rpiMetadata =
> rpiMetadata_[metadataIdx_];
> >         Span<uint8_t> embeddedBuffer;
> >
> > -       lastMetadata = std::move(rpiMetadata_);
> > +       rpiMetadata.clear();
> >         fillDeviceStatus(data.controls);
> >
> >         if (data.embeddedBufferPresent) {
> > @@ -1022,7 +1029,7 @@ void IPARPi::prepareISP(const ISPConfig &data)
> >          * This may overwrite the DeviceStatus using values from the
> sensor
> >          * metadata, and may also do additional custom processing.
> >          */
> > -       helper_->prepare(embeddedBuffer, rpiMetadata_);
> > +       helper_->prepare(embeddedBuffer, rpiMetadata);
> >
> >         /* Done with embedded data now, return to pipeline handler asap.
> */
> >         if (data.embeddedBufferPresent)
> > @@ -1038,7 +1045,9 @@ void IPARPi::prepareISP(const ISPConfig &data)
> >                  * current frame, or any other bits of metadata that
> were added
> >                  * in helper_->Prepare().
> >                  */
> > -               rpiMetadata_.merge(lastMetadata);
> > +               RPiController::Metadata &lastMetadata =
> > +                       rpiMetadata_[metadataIdx_ == 0 ?
> rpiMetadata_.size() - 1 : metadataIdx_ - 1];
> > +               rpiMetadata.mergeCopy(lastMetadata);
> >                 processPending_ = false;
> >                 return;
> >         }
> > @@ -1048,48 +1057,48 @@ void IPARPi::prepareISP(const ISPConfig &data)
> >
> >         ControlList ctrls(ispCtrls_);
> >
> > -       controller_.prepare(&rpiMetadata_);
> > +       controller_.prepare(&rpiMetadata);
> >
> >         /* Lock the metadata buffer to avoid constant locks/unlocks. */
> > -       std::unique_lock<RPiController::Metadata> lock(rpiMetadata_);
> > +       std::unique_lock<RPiController::Metadata> lock(rpiMetadata);
> >
> > -       AwbStatus *awbStatus =
> rpiMetadata_.getLocked<AwbStatus>("awb.status");
> > +       AwbStatus *awbStatus =
> rpiMetadata.getLocked<AwbStatus>("awb.status");
> >         if (awbStatus)
> >                 applyAWB(awbStatus, ctrls);
> >
> > -       CcmStatus *ccmStatus =
> rpiMetadata_.getLocked<CcmStatus>("ccm.status");
> > +       CcmStatus *ccmStatus =
> rpiMetadata.getLocked<CcmStatus>("ccm.status");
> >         if (ccmStatus)
> >                 applyCCM(ccmStatus, ctrls);
> >
> > -       AgcStatus *dgStatus =
> rpiMetadata_.getLocked<AgcStatus>("agc.status");
> > +       AgcStatus *dgStatus =
> rpiMetadata.getLocked<AgcStatus>("agc.status");
> >         if (dgStatus)
> >                 applyDG(dgStatus, ctrls);
> >
> > -       AlscStatus *lsStatus =
> rpiMetadata_.getLocked<AlscStatus>("alsc.status");
> > +       AlscStatus *lsStatus =
> rpiMetadata.getLocked<AlscStatus>("alsc.status");
> >         if (lsStatus)
> >                 applyLS(lsStatus, ctrls);
> >
> > -       ContrastStatus *contrastStatus =
> rpiMetadata_.getLocked<ContrastStatus>("contrast.status");
> > +       ContrastStatus *contrastStatus =
> rpiMetadata.getLocked<ContrastStatus>("contrast.status");
> >         if (contrastStatus)
> >                 applyGamma(contrastStatus, ctrls);
> >
> > -       BlackLevelStatus *blackLevelStatus =
> rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status");
> > +       BlackLevelStatus *blackLevelStatus =
> rpiMetadata.getLocked<BlackLevelStatus>("black_level.status");
> >         if (blackLevelStatus)
> >                 applyBlackLevel(blackLevelStatus, ctrls);
> >
> > -       GeqStatus *geqStatus =
> rpiMetadata_.getLocked<GeqStatus>("geq.status");
> > +       GeqStatus *geqStatus =
> rpiMetadata.getLocked<GeqStatus>("geq.status");
> >         if (geqStatus)
> >                 applyGEQ(geqStatus, ctrls);
> >
> > -       DenoiseStatus *denoiseStatus =
> rpiMetadata_.getLocked<DenoiseStatus>("denoise.status");
> > +       DenoiseStatus *denoiseStatus =
> rpiMetadata.getLocked<DenoiseStatus>("denoise.status");
> >         if (denoiseStatus)
> >                 applyDenoise(denoiseStatus, ctrls);
> >
> > -       SharpenStatus *sharpenStatus =
> rpiMetadata_.getLocked<SharpenStatus>("sharpen.status");
> > +       SharpenStatus *sharpenStatus =
> rpiMetadata.getLocked<SharpenStatus>("sharpen.status");
> >         if (sharpenStatus)
> >                 applySharpen(sharpenStatus, ctrls);
> >
> > -       DpcStatus *dpcStatus =
> rpiMetadata_.getLocked<DpcStatus>("dpc.status");
> > +       DpcStatus *dpcStatus =
> rpiMetadata.getLocked<DpcStatus>("dpc.status");
> >         if (dpcStatus)
> >                 applyDPC(dpcStatus, ctrls);
> >
> > @@ -1111,11 +1120,13 @@ void IPARPi::fillDeviceStatus(const ControlList
> &sensorControls)
> >
> >         LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;
> >
> > -       rpiMetadata_.set("device.status", deviceStatus);
> > +       rpiMetadata_[metadataIdx_].set("device.status", deviceStatus);
> >  }
> >
> >  void IPARPi::processStats(unsigned int bufferId)
> >  {
> > +       RPiController::Metadata &rpiMetadata =
> rpiMetadata_[metadataIdx_];
> > +
> >         auto it = buffers_.find(bufferId);
> >         if (it == buffers_.end()) {
> >                 LOG(IPARPI, Error) << "Could not find stats buffer!";
> > @@ -1125,11 +1136,11 @@ void IPARPi::processStats(unsigned int bufferId)
> >         Span<uint8_t> mem = it->second.planes()[0];
> >         bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats
> *>(mem.data());
> >         RPiController::StatisticsPtr statistics =
> std::make_shared<bcm2835_isp_stats>(*stats);
> > -       helper_->process(statistics, rpiMetadata_);
> > -       controller_.process(statistics, &rpiMetadata_);
> > +       helper_->process(statistics, rpiMetadata);
> > +       controller_.process(statistics, &rpiMetadata);
> >
> >         struct AgcStatus agcStatus;
> > -       if (rpiMetadata_.get("agc.status", agcStatus) == 0) {
> > +       if (rpiMetadata.get("agc.status", agcStatus) == 0) {
> >                 ControlList ctrls(sensorCtrls_);
> >                 applyAGC(&agcStatus, ctrls);
> >
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220923/db0604ad/attachment.htm>


More information about the libcamera-devel mailing list