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

17630 diff mbox series

Allow the IPA to 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. At present, only a single index into
this array is used.

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@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Tested-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/raspberrypi.cpp | 65 ++++++++++++++++-------------
 1 file changed, 37 insertions(+), 28 deletions(-)

Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:05)
> Allow the IPA to 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. At present, only a single index into
> this array is used.

Aha, that's what threw me off on the first read through...

I'm weary about the storage of the index in the class, as when we were
doing similar parts in IPU3 / RKISP - I was really worried that keeping
this index 'correct' through different calls may be a bit awkward.

But I think we'll see that in the next patch, as it's only ever zero
here - and this is the 'refactor' patch so:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 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@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp | 65 ++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index b74f1ecf738f..9e7792f5dfbe 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;
> +
>  /* 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
> @@ -320,6 +324,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>  
>         firstStart_ = false;
>         lastRunTimestamp_ = 0;
> +       metadataIdx_ = 0;
>  }
>  
>  void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)
> @@ -539,14 +544,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>());
> @@ -557,24 +563,24 @@ 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, { static_cast<float>(awbStatus->gainR),
>                                                                 static_cast<float>(awbStatus->gainB) });
>                 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,
>                                        { static_cast<int32_t>(blackLevelStatus->blackLevelR),
> @@ -582,7 +588,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
> @@ -593,7 +599,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++)
> @@ -1006,9 +1012,10 @@ 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_);
> +       lastMetadata = std::move(rpiMetadata);
>         fillDeviceStatus(data.controls);
>  
>         if (data.embeddedBufferPresent) {
> @@ -1025,7 +1032,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)
> @@ -1041,7 +1048,7 @@ void IPARPi::prepareISP(const ISPConfig &data)
>                  * current frame, or any other bits of metadata that were added
>                  * in helper_->Prepare().
>                  */
> -               rpiMetadata_.merge(lastMetadata);
> +               rpiMetadata.merge(lastMetadata);
>                 processPending_ = false;
>                 return;
>         }
> @@ -1051,48 +1058,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);
>  
> @@ -1116,11 +1123,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_];
> +

I'm weary when I see constructs like this refering to a index being
updated 'globally' like that.

>         auto it = buffers_.find(bufferId);
>         if (it == buffers_.end()) {
>                 LOG(IPARPI, Error) << "Could not find stats buffer!";
> @@ -1130,11 +1139,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
>