[{"id":25660,"web_url":"https://patchwork.libcamera.org/comment/25660/","msgid":"<166695918067.3974115.1365119550951145321@Monstersaurus>","date":"2022-10-28T12:13:00","subject":"Re: [libcamera-devel] [PATCH v4 5/7] ipa: raspberrypi: Use an array\n\tof RPiController::Metadata objects","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:05)\n> Allow the IPA to cycle through an array of RPiController::Metadata objects, one\n> per prepare()/process() invocation, when running the controller algorithms. This\n> allows historical metadata objects to be retained, and subsequently passed into\n> the controller algorithms on future frames. At present, only a single index into\n> this array is used.\n\nAha, that's what threw me off on the first read through...\n\nI'm weary about the storage of the index in the class, as when we were\ndoing similar parts in IPU3 / RKISP - I was really worried that keeping\nthis index 'correct' through different calls may be a bit awkward.\n\nBut I think we'll see that in the next patch, as it's only ever zero\nhere - and this is the 'refactor' patch so:\n\n\nReviewed-by: Kieran Bingham \n\n> This change provides a route to fixing a problem with the AGC algorithm, where\n> if a manual shutter/gain is requested, the algorithm does not currently retain\n> any context of the total exposure that it has calculated. As a result, the\n> wrong digital gain would be applied when the frame with the manual shutter/gain\n> is processed by the ISP.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Tested-by: David Plowman \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 65 ++++++++++++++++-------------\n> 1 file changed, 37 insertions(+), 28 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index b74f1ecf738f..9e7792f5dfbe 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -57,6 +57,9 @@ namespace libcamera {\n> using namespace std::literals::chrono_literals;\n> using utils::Duration;\n> \n> +/* Number of metadata objects available in the context list. */\n> +constexpr unsigned int numMetadataContexts = 16;\n> +\n> /* Configure the sensor with these values initially. */\n> constexpr double defaultAnalogueGain = 1.0;\n> constexpr Duration defaultExposureTime = 20.0ms;\n> @@ -163,7 +166,8 @@ private:\n> /* Raspberry Pi controller specific defines. */\n> std::unique_ptr helper_;\n> RPiController::Controller controller_;\n> - RPiController::Metadata rpiMetadata_;\n> + std::array rpiMetadata_;\n> + unsigned int metadataIdx_;\n> \n> /*\n> * We count frames to decide if the frame must be hidden (e.g. from\n> @@ -320,6 +324,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> \n> firstStart_ = false;\n> lastRunTimestamp_ = 0;\n> + metadataIdx_ = 0;\n> }\n> \n> void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)\n> @@ -539,14 +544,15 @@ void IPARPi::signalIspPrepare(const ISPConfig &data)\n> \n> void IPARPi::reportMetadata()\n> {\n> - std::unique_lock lock(rpiMetadata_);\n> + RPiController::Metadata &rpiMetadata = rpiMetadata_[metadataIdx_];\n> + std::unique_lock lock(rpiMetadata);\n> \n> /*\n> * Certain information about the current frame and how it will be\n> * processed can be extracted and placed into the libcamera metadata\n> * buffer, where an application could query it.\n> */\n> - DeviceStatus *deviceStatus = rpiMetadata_.getLocked(\"device.status\");\n> + DeviceStatus *deviceStatus = rpiMetadata.getLocked(\"device.status\");\n> if (deviceStatus) {\n> libcameraMetadata_.set(controls::ExposureTime,\n> deviceStatus->shutterSpeed.get());\n> @@ -557,24 +563,24 @@ void IPARPi::reportMetadata()\n> libcameraMetadata_.set(controls::SensorTemperature, *deviceStatus->sensorTemperature);\n> }\n> \n> - AgcStatus *agcStatus = rpiMetadata_.getLocked(\"agc.status\");\n> + AgcStatus *agcStatus = rpiMetadata.getLocked(\"agc.status\");\n> if (agcStatus) {\n> libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n> libcameraMetadata_.set(controls::DigitalGain, agcStatus->digitalGain);\n> }\n> \n> - LuxStatus *luxStatus = rpiMetadata_.getLocked(\"lux.status\");\n> + LuxStatus *luxStatus = rpiMetadata.getLocked(\"lux.status\");\n> if (luxStatus)\n> libcameraMetadata_.set(controls::Lux, luxStatus->lux);\n> \n> - AwbStatus *awbStatus = rpiMetadata_.getLocked(\"awb.status\");\n> + AwbStatus *awbStatus = rpiMetadata.getLocked(\"awb.status\");\n> if (awbStatus) {\n> libcameraMetadata_.set(controls::ColourGains, { static_cast(awbStatus->gainR),\n> static_cast(awbStatus->gainB) });\n> libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK);\n> }\n> \n> - BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked(\"black_level.status\");\n> + BlackLevelStatus *blackLevelStatus = rpiMetadata.getLocked(\"black_level.status\");\n> if (blackLevelStatus)\n> libcameraMetadata_.set(controls::SensorBlackLevels,\n> { static_cast(blackLevelStatus->blackLevelR),\n> @@ -582,7 +588,7 @@ void IPARPi::reportMetadata()\n> static_cast(blackLevelStatus->blackLevelG),\n> static_cast(blackLevelStatus->blackLevelB) });\n> \n> - FocusStatus *focusStatus = rpiMetadata_.getLocked(\"focus.status\");\n> + FocusStatus *focusStatus = rpiMetadata.getLocked(\"focus.status\");\n> if (focusStatus && focusStatus->num == 12) {\n> /*\n> * We get a 4x3 grid of regions by default. Calculate the average\n> @@ -593,7 +599,7 @@ void IPARPi::reportMetadata()\n> libcameraMetadata_.set(controls::FocusFoM, focusFoM);\n> }\n> \n> - CcmStatus *ccmStatus = rpiMetadata_.getLocked(\"ccm.status\");\n> + CcmStatus *ccmStatus = rpiMetadata.getLocked(\"ccm.status\");\n> if (ccmStatus) {\n> float m[9];\n> for (unsigned int i = 0; i < 9; i++)\n> @@ -1006,9 +1012,10 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> {\n> int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);\n> RPiController::Metadata lastMetadata;\n> + RPiController::Metadata &rpiMetadata = rpiMetadata_[metadataIdx_];\n> Span embeddedBuffer;\n> \n> - lastMetadata = std::move(rpiMetadata_);\n> + lastMetadata = std::move(rpiMetadata);\n> fillDeviceStatus(data.controls);\n> \n> if (data.embeddedBufferPresent) {\n> @@ -1025,7 +1032,7 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> * This may overwrite the DeviceStatus using values from the sensor\n> * metadata, and may also do additional custom processing.\n> */\n> - helper_->prepare(embeddedBuffer, rpiMetadata_);\n> + helper_->prepare(embeddedBuffer, rpiMetadata);\n> \n> /* Done with embedded data now, return to pipeline handler asap. */\n> if (data.embeddedBufferPresent)\n> @@ -1041,7 +1048,7 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> * current frame, or any other bits of metadata that were added\n> * in helper_->Prepare().\n> */\n> - rpiMetadata_.merge(lastMetadata);\n> + rpiMetadata.merge(lastMetadata);\n> processPending_ = false;\n> return;\n> }\n> @@ -1051,48 +1058,48 @@ void IPARPi::prepareISP(const ISPConfig &data)\n> \n> ControlList ctrls(ispCtrls_);\n> \n> - controller_.prepare(&rpiMetadata_);\n> + controller_.prepare(&rpiMetadata);\n> \n> /* Lock the metadata buffer to avoid constant locks/unlocks. */\n> - std::unique_lock lock(rpiMetadata_);\n> + std::unique_lock lock(rpiMetadata);\n> \n> - AwbStatus *awbStatus = rpiMetadata_.getLocked(\"awb.status\");\n> + AwbStatus *awbStatus = rpiMetadata.getLocked(\"awb.status\");\n> if (awbStatus)\n> applyAWB(awbStatus, ctrls);\n> \n> - CcmStatus *ccmStatus = rpiMetadata_.getLocked(\"ccm.status\");\n> + CcmStatus *ccmStatus = rpiMetadata.getLocked(\"ccm.status\");\n> if (ccmStatus)\n> applyCCM(ccmStatus, ctrls);\n> \n> - AgcStatus *dgStatus = rpiMetadata_.getLocked(\"agc.status\");\n> + AgcStatus *dgStatus = rpiMetadata.getLocked(\"agc.status\");\n> if (dgStatus)\n> applyDG(dgStatus, ctrls);\n> \n> - AlscStatus *lsStatus = rpiMetadata_.getLocked(\"alsc.status\");\n> + AlscStatus *lsStatus = rpiMetadata.getLocked(\"alsc.status\");\n> if (lsStatus)\n> applyLS(lsStatus, ctrls);\n> \n> - ContrastStatus *contrastStatus = rpiMetadata_.getLocked(\"contrast.status\");\n> + ContrastStatus *contrastStatus = rpiMetadata.getLocked(\"contrast.status\");\n> if (contrastStatus)\n> applyGamma(contrastStatus, ctrls);\n> \n> - BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked(\"black_level.status\");\n> + BlackLevelStatus *blackLevelStatus = rpiMetadata.getLocked(\"black_level.status\");\n> if (blackLevelStatus)\n> applyBlackLevel(blackLevelStatus, ctrls);\n> \n> - GeqStatus *geqStatus = rpiMetadata_.getLocked(\"geq.status\");\n> + GeqStatus *geqStatus = rpiMetadata.getLocked(\"geq.status\");\n> if (geqStatus)\n> applyGEQ(geqStatus, ctrls);\n> \n> - DenoiseStatus *denoiseStatus = rpiMetadata_.getLocked(\"denoise.status\");\n> + DenoiseStatus *denoiseStatus = rpiMetadata.getLocked(\"denoise.status\");\n> if (denoiseStatus)\n> applyDenoise(denoiseStatus, ctrls);\n> \n> - SharpenStatus *sharpenStatus = rpiMetadata_.getLocked(\"sharpen.status\");\n> + SharpenStatus *sharpenStatus = rpiMetadata.getLocked(\"sharpen.status\");\n> if (sharpenStatus)\n> applySharpen(sharpenStatus, ctrls);\n> \n> - DpcStatus *dpcStatus = rpiMetadata_.getLocked(\"dpc.status\");\n> + DpcStatus *dpcStatus = rpiMetadata.getLocked(\"dpc.status\");\n> if (dpcStatus)\n> applyDPC(dpcStatus, ctrls);\n> \n> @@ -1116,11 +1123,13 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n> \n> LOG(IPARPI, Debug) << \"Metadata - \" << deviceStatus;\n> \n> - rpiMetadata_.set(\"device.status\", deviceStatus);\n> + rpiMetadata_[metadataIdx_].set(\"device.status\", deviceStatus);\n> }\n> \n> void IPARPi::processStats(unsigned int bufferId)\n> {\n> + RPiController::Metadata &rpiMetadata = rpiMetadata_[metadataIdx_];\n> +\n\nI'm weary when I see constructs like this refering to a index being\nupdated 'globally' like that.\n\n> auto it = buffers_.find(bufferId);\n> if (it == buffers_.end()) {\n> LOG(IPARPI, Error) << \"Could not find stats buffer!\";\n> @@ -1130,11 +1139,11 @@ void IPARPi::processStats(unsigned int bufferId)\n> Span mem = it->second.planes()[0];\n> bcm2835_isp_stats *stats = reinterpret_cast(mem.data());\n> RPiController::StatisticsPtr statistics = std::make_shared(*stats);\n> - helper_->process(statistics, rpiMetadata_);\n> - controller_.process(statistics, &rpiMetadata_);\n> + helper_->process(statistics, rpiMetadata);\n> + controller_.process(statistics, &rpiMetadata);\n> \n> struct AgcStatus agcStatus;\n> - if (rpiMetadata_.get(\"agc.status\", agcStatus) == 0) {\n> + if (rpiMetadata.get(\"agc.status\", agcStatus) == 0) {\n> ControlList ctrls(sensorCtrls_);\n> applyAGC(&agcStatus, ctrls);\n> \n> -- \n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 85959BDB16\n\tfor ;\n\tFri, 28 Oct 2022 12:13:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6B6262FD9;\n\tFri, 28 Oct 2022 14:13:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B175761F4A\n\tfor ;\n\tFri, 28 Oct 2022 14:13:03 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2CA0F2F5;\n\tFri, 28 Oct 2022 14:13:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666959185;\n\tbh=few8CVJ2fAHRnB4MiREAGO0uKGi4AGzcfstQkS12HEw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=Y0/it3tFEkpIbyqsBJTf/wks87ixFHk+CnsbA7xQodZi8kE38JS096VQbGUYGxVBu\n\tqkNGq4WHyoUlKOvVj/54x+m0AeZM9TO5tyNFA/LN9pD2FhTgnEn4E2h7siRsU/tExI\n\tHTllAHEeCKFRPbUtRHwE1uMDWNqc+Fy4QOgjAnEa2iDaZg4nrZAE2nbQBUwh1Fr9QA\n\t6Dq+QLI5n0rgACU9Jf2ABC2Zox3qhqERrt5p4i6xPdHhUefoBwhLMzXyx0pDer3RIk\n\tDQz50yau0xyG62DFafLKO3sU/hga4aRrX74pdQ6ywa817DlkSJiC3U85QFgcEeu7sf\n\tVro93/hRMehGw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666959183;\n\tbh=few8CVJ2fAHRnB4MiREAGO0uKGi4AGzcfstQkS12HEw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=wE16adXg7r15b2reT7wH+B/khMg3sUPPa8xVmrjWAE2mgTjUoQcL4f6YmOyuMvtJ9\n\txrG+nA35Brn1ipyiOaqGTejlTrqtrilAta2/VP+2vy/mFL9xUncjurZPzegPPHiXRS\n\tEC2loYWRm9c4/U8m9udtv/aidWOfaq43ThF8iOSQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wE16adXg\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221019090107.19975-6-naush@raspberrypi.com>","References":"<20221019090107.19975-1-naush@raspberrypi.com>\n\t<20221019090107.19975-6-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 28 Oct 2022 13:13:00 +0100","Message-ID":"<166695918067.3974115.1365119550951145321@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 5/7] ipa: raspberrypi: Use an array\n\tof RPiController::Metadata objects","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]