<div dir="ltr"><div dir="ltr">Hi Davd,<div><br></div><div>Thanks for the review of this series!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 23 Sept 2022 at 11:38, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.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">HI Naush<br>
<br>
Thanks for the patch.<br>
<br>
On Mon, 5 Sept 2022 at 08:40, Naushir Patuck via libcamera-devel<br>
<<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<br>
><br>
> Cycle through an array of RPiController::Metadata objects, one per<br>
> prepare()/process() invocation, when running the controller algorithms. This<br>
> allows historical metadata objects to be retained, and subsequently passed into<br>
> the controller algorithms on future frames.<br>
><br>
> This change provides a route to fixing a problem with the AGC algorithm, where<br>
> if a manual shutter/gain is requested, the algorithm does not currently retain<br>
> any context of the total exposure that it has calculated. As a result, the<br>
> wrong digital gain would be applied when the frame with the manual shutter/gain<br>
> is processed by the ISP.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 69 +++++++++++++++++------------<br>
> 1 file changed, 40 insertions(+), 29 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 8d731435764e..63cccda901c1 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -57,6 +57,9 @@ namespace libcamera {<br>
> using namespace std::literals::chrono_literals;<br>
> using utils::Duration;<br>
><br>
> +/* Number of metadata objects available in the context list. */<br>
> +constexpr unsigned int numMetadataContexts = 16;<br>
<br>
Just curious as to what determines this number. Presumably it only has<br>
to be bigger than the max delay we have? Can anything else affect it,<br>
like when the IPAs skip frames at high framerates?<br></blockquote><div><br></div><div>It's actually the max delay + the number of dropped frames at startup.</div><div>I chose 16 because it matches the DelayedControl ring buffer size.... I think!</div><div><br></div><div>I will post a v3 with your other amendments + address Umang's comments early next week.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
But anyway, it all looked good to me.<br>
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Thanks!<br>
David<br>
<br>
> +<br>
> /* Configure the sensor with these values initially. */<br>
> constexpr double defaultAnalogueGain = 1.0;<br>
> constexpr Duration defaultExposureTime = 20.0ms;<br>
> @@ -163,7 +166,8 @@ private:<br>
> /* Raspberry Pi controller specific defines. */<br>
> std::unique_ptr<RPiController::CamHelper> helper_;<br>
> RPiController::Controller controller_;<br>
> - RPiController::Metadata rpiMetadata_;<br>
> + std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;<br>
> + unsigned int metadataIdx_;<br>
><br>
> /*<br>
> * We count frames to decide if the frame must be hidden (e.g. from<br>
> @@ -319,6 +323,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)<br>
><br>
> firstStart_ = false;<br>
> lastRunTimestamp_ = 0;<br>
> + metadataIdx_ = 0;<br>
> }<br>
><br>
> void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)<br>
> @@ -513,6 +518,7 @@ void IPARPi::signalStatReady(uint32_t bufferId)<br>
> reportMetadata();<br>
><br>
> statsMetadataComplete.emit(bufferId & MaskID, libcameraMetadata_);<br>
> + metadataIdx_ = (metadataIdx_ + 1) % rpiMetadata_.size();<br>
> }<br>
><br>
> void IPARPi::signalQueueRequest(const ControlList &controls)<br>
> @@ -536,14 +542,15 @@ void IPARPi::signalIspPrepare(const ISPConfig &data)<br>
><br>
> void IPARPi::reportMetadata()<br>
> {<br>
> - std::unique_lock<RPiController::Metadata> lock(rpiMetadata_);<br>
> + RPiController::Metadata &rpiMetadata = rpiMetadata_[metadataIdx_];<br>
> + std::unique_lock<RPiController::Metadata> lock(rpiMetadata);<br>
><br>
> /*<br>
> * Certain information about the current frame and how it will be<br>
> * processed can be extracted and placed into the libcamera metadata<br>
> * buffer, where an application could query it.<br>
> */<br>
> - DeviceStatus *deviceStatus = rpiMetadata_.getLocked<DeviceStatus>("device.status");<br>
> + DeviceStatus *deviceStatus = rpiMetadata.getLocked<DeviceStatus>("device.status");<br>
> if (deviceStatus) {<br>
> libcameraMetadata_.set(controls::ExposureTime,<br>
> deviceStatus->shutterSpeed.get<std::micro>());<br>
> @@ -554,17 +561,17 @@ void IPARPi::reportMetadata()<br>
> libcameraMetadata_.set(controls::SensorTemperature, *deviceStatus->sensorTemperature);<br>
> }<br>
><br>
> - AgcStatus *agcStatus = rpiMetadata_.getLocked<AgcStatus>("agc.status");<br>
> + AgcStatus *agcStatus = rpiMetadata.getLocked<AgcStatus>("agc.status");<br>
> if (agcStatus) {<br>
> libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);<br>
> libcameraMetadata_.set(controls::DigitalGain, agcStatus->digitalGain);<br>
> }<br>
><br>
> - LuxStatus *luxStatus = rpiMetadata_.getLocked<LuxStatus>("lux.status");<br>
> + LuxStatus *luxStatus = rpiMetadata.getLocked<LuxStatus>("lux.status");<br>
> if (luxStatus)<br>
> libcameraMetadata_.set(controls::Lux, luxStatus->lux);<br>
><br>
> - AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status");<br>
> + AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status");<br>
> if (awbStatus) {<br>
> libcameraMetadata_.set(controls::ColourGains,<br>
> Span<const float, 2>({ static_cast<float>(awbStatus->gainR),<br>
> @@ -572,7 +579,7 @@ void IPARPi::reportMetadata()<br>
> libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK);<br>
> }<br>
><br>
> - BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status");<br>
> + BlackLevelStatus *blackLevelStatus = rpiMetadata.getLocked<BlackLevelStatus>("black_level.status");<br>
> if (blackLevelStatus)<br>
> libcameraMetadata_.set(controls::SensorBlackLevels,<br>
> Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR),<br>
> @@ -580,7 +587,7 @@ void IPARPi::reportMetadata()<br>
> static_cast<int32_t>(blackLevelStatus->blackLevelG),<br>
> static_cast<int32_t>(blackLevelStatus->blackLevelB) }));<br>
><br>
> - FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status");<br>
> + FocusStatus *focusStatus = rpiMetadata.getLocked<FocusStatus>("focus.status");<br>
> if (focusStatus && focusStatus->num == 12) {<br>
> /*<br>
> * We get a 4x3 grid of regions by default. Calculate the average<br>
> @@ -591,7 +598,7 @@ void IPARPi::reportMetadata()<br>
> libcameraMetadata_.set(controls::FocusFoM, focusFoM);<br>
> }<br>
><br>
> - CcmStatus *ccmStatus = rpiMetadata_.getLocked<CcmStatus>("ccm.status");<br>
> + CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status");<br>
> if (ccmStatus) {<br>
> float m[9];<br>
> for (unsigned int i = 0; i < 9; i++)<br>
> @@ -1002,10 +1009,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)<br>
> void IPARPi::prepareISP(const ISPConfig &data)<br>
> {<br>
> int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);<br>
> - RPiController::Metadata lastMetadata;<br>
> + RPiController::Metadata &rpiMetadata = rpiMetadata_[metadataIdx_];<br>
> Span<uint8_t> embeddedBuffer;<br>
><br>
> - lastMetadata = std::move(rpiMetadata_);<br>
> + rpiMetadata.clear();<br>
> fillDeviceStatus(data.controls);<br>
><br>
> if (data.embeddedBufferPresent) {<br>
> @@ -1022,7 +1029,7 @@ void IPARPi::prepareISP(const ISPConfig &data)<br>
> * This may overwrite the DeviceStatus using values from the sensor<br>
> * metadata, and may also do additional custom processing.<br>
> */<br>
> - helper_->prepare(embeddedBuffer, rpiMetadata_);<br>
> + helper_->prepare(embeddedBuffer, rpiMetadata);<br>
><br>
> /* Done with embedded data now, return to pipeline handler asap. */<br>
> if (data.embeddedBufferPresent)<br>
> @@ -1038,7 +1045,9 @@ void IPARPi::prepareISP(const ISPConfig &data)<br>
> * current frame, or any other bits of metadata that were added<br>
> * in helper_->Prepare().<br>
> */<br>
> - rpiMetadata_.merge(lastMetadata);<br>
> + RPiController::Metadata &lastMetadata =<br>
> + rpiMetadata_[metadataIdx_ == 0 ? rpiMetadata_.size() - 1 : metadataIdx_ - 1];<br>
> + rpiMetadata.mergeCopy(lastMetadata);<br>
> processPending_ = false;<br>
> return;<br>
> }<br>
> @@ -1048,48 +1057,48 @@ void IPARPi::prepareISP(const ISPConfig &data)<br>
><br>
> ControlList ctrls(ispCtrls_);<br>
><br>
> - controller_.prepare(&rpiMetadata_);<br>
> + controller_.prepare(&rpiMetadata);<br>
><br>
> /* Lock the metadata buffer to avoid constant locks/unlocks. */<br>
> - std::unique_lock<RPiController::Metadata> lock(rpiMetadata_);<br>
> + std::unique_lock<RPiController::Metadata> lock(rpiMetadata);<br>
><br>
> - AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status");<br>
> + AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status");<br>
> if (awbStatus)<br>
> applyAWB(awbStatus, ctrls);<br>
><br>
> - CcmStatus *ccmStatus = rpiMetadata_.getLocked<CcmStatus>("ccm.status");<br>
> + CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status");<br>
> if (ccmStatus)<br>
> applyCCM(ccmStatus, ctrls);<br>
><br>
> - AgcStatus *dgStatus = rpiMetadata_.getLocked<AgcStatus>("agc.status");<br>
> + AgcStatus *dgStatus = rpiMetadata.getLocked<AgcStatus>("agc.status");<br>
> if (dgStatus)<br>
> applyDG(dgStatus, ctrls);<br>
><br>
> - AlscStatus *lsStatus = rpiMetadata_.getLocked<AlscStatus>("alsc.status");<br>
> + AlscStatus *lsStatus = rpiMetadata.getLocked<AlscStatus>("alsc.status");<br>
> if (lsStatus)<br>
> applyLS(lsStatus, ctrls);<br>
><br>
> - ContrastStatus *contrastStatus = rpiMetadata_.getLocked<ContrastStatus>("contrast.status");<br>
> + ContrastStatus *contrastStatus = rpiMetadata.getLocked<ContrastStatus>("contrast.status");<br>
> if (contrastStatus)<br>
> applyGamma(contrastStatus, ctrls);<br>
><br>
> - BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status");<br>
> + BlackLevelStatus *blackLevelStatus = rpiMetadata.getLocked<BlackLevelStatus>("black_level.status");<br>
> if (blackLevelStatus)<br>
> applyBlackLevel(blackLevelStatus, ctrls);<br>
><br>
> - GeqStatus *geqStatus = rpiMetadata_.getLocked<GeqStatus>("geq.status");<br>
> + GeqStatus *geqStatus = rpiMetadata.getLocked<GeqStatus>("geq.status");<br>
> if (geqStatus)<br>
> applyGEQ(geqStatus, ctrls);<br>
><br>
> - DenoiseStatus *denoiseStatus = rpiMetadata_.getLocked<DenoiseStatus>("denoise.status");<br>
> + DenoiseStatus *denoiseStatus = rpiMetadata.getLocked<DenoiseStatus>("denoise.status");<br>
> if (denoiseStatus)<br>
> applyDenoise(denoiseStatus, ctrls);<br>
><br>
> - SharpenStatus *sharpenStatus = rpiMetadata_.getLocked<SharpenStatus>("sharpen.status");<br>
> + SharpenStatus *sharpenStatus = rpiMetadata.getLocked<SharpenStatus>("sharpen.status");<br>
> if (sharpenStatus)<br>
> applySharpen(sharpenStatus, ctrls);<br>
><br>
> - DpcStatus *dpcStatus = rpiMetadata_.getLocked<DpcStatus>("dpc.status");<br>
> + DpcStatus *dpcStatus = rpiMetadata.getLocked<DpcStatus>("dpc.status");<br>
> if (dpcStatus)<br>
> applyDPC(dpcStatus, ctrls);<br>
><br>
> @@ -1111,11 +1120,13 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)<br>
><br>
> LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;<br>
><br>
> - rpiMetadata_.set("device.status", deviceStatus);<br>
> + rpiMetadata_[metadataIdx_].set("device.status", deviceStatus);<br>
> }<br>
><br>
> void IPARPi::processStats(unsigned int bufferId)<br>
> {<br>
> + RPiController::Metadata &rpiMetadata = rpiMetadata_[metadataIdx_];<br>
> +<br>
> auto it = buffers_.find(bufferId);<br>
> if (it == buffers_.end()) {<br>
> LOG(IPARPI, Error) << "Could not find stats buffer!";<br>
> @@ -1125,11 +1136,11 @@ void IPARPi::processStats(unsigned int bufferId)<br>
> Span<uint8_t> mem = it->second.planes()[0];<br>
> bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data());<br>
> RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats);<br>
> - helper_->process(statistics, rpiMetadata_);<br>
> - controller_.process(statistics, &rpiMetadata_);<br>
> + helper_->process(statistics, rpiMetadata);<br>
> + controller_.process(statistics, &rpiMetadata);<br>
><br>
> struct AgcStatus agcStatus;<br>
> - if (rpiMetadata_.get("agc.status", agcStatus) == 0) {<br>
> + if (rpiMetadata.get("agc.status", agcStatus) == 0) {<br>
> ControlList ctrls(sensorCtrls_);<br>
> applyAGC(&agcStatus, ctrls);<br>
><br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>