[{"id":16837,"web_url":"https://patchwork.libcamera.org/comment/16837/","msgid":"","date":"2021-05-07T23:27:46","subject":"Re: [libcamera-devel] [PATCH v4 2/2] ipa: raspberrypi: Use\n\tCamHelpers to generalise sensor embedded data parsing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Fri, May 07, 2021 at 12:37:28PM +0100, David Plowman wrote:\n> CamHelpers get virtual Prepare() and Process() methods, running just\n> before and just after the ISP, just like Raspberry Pi Algorithms.\n> \n> The Prepare() method is able to parse the register dumps in embedded\n> data buffers, and can be specialised to perform custom processing when\n> necessary.\n> \n> The IPA itself only needs to call the new Prepare() and Process()\n> methods. It must fill in the DeviceStatus from the controls first, in\n> case the Prepare() method does not supply its own values.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 54 +++++++++++++++++++\n> src/ipa/raspberrypi/cam_helper.hpp | 11 +++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 80 ++++++++++-------------------\n> 3 files changed, 90 insertions(+), 55 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 0ae0baa0..09917f3c 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -17,6 +17,11 @@\n> #include \"md_parser.hpp\"\n> \n> using namespace RPiController;\n> +using namespace libcamera;\n> +\n> +namespace libcamera {\n> +LOG_DECLARE_CATEGORY(IPARPI)\n> +}\n> \n> static std::map cam_helpers;\n> \n> @@ -45,6 +50,17 @@ CamHelper::~CamHelper()\n> \tdelete parser_;\n> }\n> \n> +void CamHelper::Prepare(Span buffer,\n> +\t\t\tMetadata &metadata)\n> +{\n> +\tparseEmbeddedData(buffer, metadata);\n> +}\n> +\n> +void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,\n> +\t\t\t[[maybe_unused]] Metadata &metadata)\n> +{\n> +}\n> +\n> uint32_t CamHelper::ExposureLines(double exposure_us) const\n> {\n> \tassert(initialized_);\n> @@ -139,6 +155,44 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n> \treturn 0;\n> }\n> \n> +void CamHelper::parseEmbeddedData(Span buffer,\n> +\t\t\t\t Metadata &metadata)\n> +{\n> +\tif (buffer.empty())\n> +\t\treturn;\n> +\n> +\tuint32_t exposureLines, gainCode;\n> +\n> +\tif (parser_->Parse(buffer) != MdParser::Status::OK ||\n> +\t parser_->GetExposureLines(exposureLines) != MdParser::Status::OK ||\n> +\t parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> +\t\tLOG(IPARPI, Error) << \"Embedded data buffer parsing failed\";\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t * Overwrite the exposure/gain values in the DeviceStatus, as\n> +\t * we know better. Fetch it first in case any other fields were\n> +\t * set meaningfully.\n> +\t */\n> +\tDeviceStatus deviceStatus;\n> +\n> +\tif (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> +\t\tLOG(IPARPI, Error) << \"DeviceStatus not found\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tdeviceStatus.shutter_speed = Exposure(exposureLines);\n> +\tdeviceStatus.analogue_gain = Gain(gainCode);\n> +\n> +\tLOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> +\t\t\t << deviceStatus.shutter_speed\n> +\t\t\t << \" Gain : \"\n> +\t\t\t << deviceStatus.analogue_gain;\n> +\n> +\tmetadata.Set(\"device.status\", deviceStatus);\n> +}\n> +\n> RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> \t\t\t\t CamHelperCreateFunc create_func)\n> {\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index c3ed5362..a52f3f0b 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -8,7 +8,11 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \"camera_mode.h\"\n> +#include \"controller/controller.hpp\"\n> +#include \"controller/metadata.hpp\"\n> #include \"md_parser.hpp\"\n> \n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> @@ -65,7 +69,9 @@ public:\n> \tCamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> \tvirtual ~CamHelper();\n> \tvoid SetCameraMode(const CameraMode &mode);\n> -\tMdParser &Parser() const { return *parser_; }\n> +\tvirtual void Prepare(libcamera::Span buffer,\n> +\t\t\t Metadata &metadata);\n> +\tvirtual void Process(StatisticsPtr &stats, Metadata &metadata);\n> \tuint32_t ExposureLines(double exposure_us) const;\n> \tdouble Exposure(uint32_t exposure_lines) const; // in us\n> \tvirtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,\n> @@ -81,6 +87,9 @@ public:\n> \tvirtual unsigned int MistrustFramesModeSwitch() const;\n> \n> protected:\n> +\tvoid parseEmbeddedData(libcamera::Span buffer,\n> +\t\t\t Metadata &metadata);\n> +\n> \tMdParser *parser_;\n> \tCameraMode mode_;\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index dad6395f..bb55f931 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -101,9 +101,7 @@ private:\n> \tvoid returnEmbeddedBuffer(unsigned int bufferId);\n> \tvoid prepareISP(const ipa::RPi::ISPConfig &data);\n> \tvoid reportMetadata();\n> -\tbool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);\n> -\tvoid fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> -\t\t\t struct DeviceStatus &deviceStatus);\n> +\tvoid fillDeviceStatus(const ControlList &sensorControls);\n> \tvoid processStats(unsigned int bufferId);\n> \tvoid applyFrameDurations(double minFrameDuration, double maxFrameDuration);\n> \tvoid applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> @@ -894,35 +892,34 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> \n> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> {\n> -\tstruct DeviceStatus deviceStatus = {};\n> -\tbool success = false;\n> +\tSpan embeddedBuffer;\n> +\n> +\trpiMetadata_.Clear();\n> +\n> +\tfillDeviceStatus(data.controls);\n> \n> \tif (data.embeddedBufferPresent) {\n> \t\t/*\n> \t\t * Pipeline handler has supplied us with an embedded data buffer,\n> -\t\t * so parse it and extract the exposure and gain.\n> +\t\t * we must pass it to the CamHelper for parsing.\n> \t\t */\n> -\t\tsuccess = parseEmbeddedData(data.embeddedBufferId, deviceStatus);\n> -\n> -\t\t/* Done with embedded data now, return to pipeline handler asap. */\n> -\t\treturnEmbeddedBuffer(data.embeddedBufferId);\n> +\t\tauto it = buffers_.find(data.embeddedBufferId);\n> +\t\tASSERT(it != buffers_.end());\n> +\t\tembeddedBuffer = it->second.maps()[0];\n> \t}\n> \n> -\tif (!success) {\n> -\t\t/*\n> -\t\t * Pipeline handler has not supplied an embedded data buffer,\n> -\t\t * or embedded data buffer parsing has failed for some reason,\n> -\t\t * so pull the exposure and gain values from the control list.\n> -\t\t */\n> -\t\tint32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get();\n> -\t\tint32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> -\t\tfillDeviceStatus(exposureLines, gainCode, deviceStatus);\n> -\t}\n> +\t/*\n> +\t * This may overwrite the DeviceStatus using values from the sensor\n> +\t * metadata, and may also do additional custom processing.\n> +\t */\n> +\thelper_->Prepare(embeddedBuffer, rpiMetadata_);\n> +\n> +\t/* Done with embedded data now, return to pipeline handler asap. */\n> +\tif (data.embeddedBufferPresent)\n> +\t\treturnEmbeddedBuffer(data.embeddedBufferId);\n> \n> \tControlList ctrls(ispCtrls_);\n> \n> -\trpiMetadata_.Clear();\n> -\trpiMetadata_.Set(\"device.status\", deviceStatus);\n> \tcontroller_.Prepare(&rpiMetadata_);\n> \n> \t/* Lock the metadata buffer to avoid constant locks/unlocks. */\n> @@ -972,41 +969,13 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> \t\tsetIspControls.emit(ctrls);\n> }\n> \n> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus)\n> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n> {\n> -\tauto it = buffers_.find(bufferId);\n> -\tif (it == buffers_.end()) {\n> -\t\tLOG(IPARPI, Error) << \"Could not find embedded buffer!\";\n> -\t\treturn false;\n> -\t}\n> -\n> -\tSpan mem = it->second.maps()[0];\n> -\thelper_->Parser().SetBufferSize(mem.size());\n> -\tRPiController::MdParser::Status status = helper_->Parser().Parse(mem.data());\n> -\tif (status != RPiController::MdParser::Status::OK) {\n> -\t\tLOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n> -\t\treturn false;\n> -\t} else {\n> -\t\tuint32_t exposureLines, gainCode;\n> -\t\tif (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n> -\t\t\tLOG(IPARPI, Error) << \"Exposure time failed\";\n> -\t\t\treturn false;\n> -\t\t}\n> -\n> -\t\tif (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {\n> -\t\t\tLOG(IPARPI, Error) << \"Gain failed\";\n> -\t\t\treturn false;\n> -\t\t}\n> -\n> -\t\tfillDeviceStatus(exposureLines, gainCode, deviceStatus);\n> -\t}\n> +\tDeviceStatus deviceStatus = {};\n> \n> -\treturn true;\n> -}\n> +\tint32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> +\tint32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> \n> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> -\t\t\t struct DeviceStatus &deviceStatus)\n> -{\n> \tdeviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> \tdeviceStatus.analogue_gain = helper_->Gain(gainCode);\n> \n> @@ -1014,6 +983,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> \t\t\t << deviceStatus.shutter_speed\n> \t\t\t << \" Gain : \"\n> \t\t\t << deviceStatus.analogue_gain;\n> +\n> +\trpiMetadata_.Set(\"device.status\", deviceStatus);\n> }\n> \n> void IPARPi::processStats(unsigned int bufferId)\n> @@ -1027,6 +998,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> \tSpan mem = it->second.maps()[0];\n> \tbcm2835_isp_stats *stats = reinterpret_cast(mem.data());\n> \tRPiController::StatisticsPtr statistics = std::make_shared(*stats);\n> +\thelper_->Process(statistics, rpiMetadata_);\n> \tcontroller_.Process(statistics, &rpiMetadata_);\n> \n> \tstruct AgcStatus agcStatus;","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 AAD41BF829\n\tfor ;\n\tFri, 7 May 2021 23:27:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3A0D6891B;\n\tSat, 8 May 2021 01:27:55 +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 A884368901\n\tfor ;\n\tSat, 8 May 2021 01:27:53 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1885A3D7;\n\tSat, 8 May 2021 01:27:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oXStw4sa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620430073;\n\tbh=ZLY0Y8E3MxDr7+6T+q3Uo6CZHpiV4UK24yrxHvotgXs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oXStw4saKP8tYUQAXi9lD5MSo3b5dJfmFmuwiTSLdXG/U79H/u9RqQBvh570fkEWj\n\tgFH5wEs3vvg4Jrl4tHc8RCRBV7qXggWQKmQfSHmulvsY3XkEtfG1kxWAG444aBudDn\n\t/tO9W0XEVZ0fBJG1dvMlcJHpUGzkeGoJ6jt60oUk=","Date":"Sat, 8 May 2021 02:27:46 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210507113728.14037-1-david.plowman@raspberrypi.com>\n\t<20210507113728.14037-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210507113728.14037-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 2/2] ipa: raspberrypi: Use\n\tCamHelpers to generalise sensor embedded data parsing","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]