[{"id":15717,"web_url":"https://patchwork.libcamera.org/comment/15717/","msgid":"","date":"2021-03-16T10:15:15","subject":"Re: [libcamera-devel] [RFC PATCH 1/1] ipa: raspberrypi: Use\n\tCamHelpers to generalise embedded data parsing","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your work.\n\nOn Wed, 10 Mar 2021 at 17:23, David Plowman \nwrote:\n\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 no longer performs this register parsing, it just has\n> to call the new Prepare() and Process() methods.\n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++\n> src/ipa/raspberrypi/cam_helper.hpp | 14 ++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++---------------------\n> 3 files changed, 84 insertions(+), 67 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> index 0ae0baa0..fc69f4cb 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\nIs this namespace enclosure needed for some reason?\nSeems to compile well enough without it. I wonder if we should add a\ncategory for cam_helper based logging?\n\n\n>\n> static std::map cam_helpers;\n>\n> @@ -45,6 +50,17 @@ CamHelper::~CamHelper()\n> delete parser_;\n> }\n>\n> +void CamHelper::Prepare(const Span &buffer,\n> + const ControlList &controls, Metadata &metadata)\n> +{\n> + parseRegisters(buffer, controls, metadata);\n> +}\n> +\n> +void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,\n> + [[maybe_unused]] Metadata &metadata)\n> +{\n> +}\n> +\n> uint32_t CamHelper::ExposureLines(double exposure_us) const\n> {\n> assert(initialized_);\n> @@ -139,6 +155,39 @@ unsigned int CamHelper::MistrustFramesModeSwitch()\n> const\n> return 0;\n> }\n>\n> +void CamHelper::parseRegisters(const Span &buffer,\n> + const ControlList &controls, Metadata\n> &metadata)\n> +{\n>\n\nBit of a nit-pick (sorry) , but could you consider a few renames:\n\ns/parseRegisters/parseEmbeddedData/\ns/controls/sensorControls/\ns/metadata/controllerMetadata/\n\nTook me a bit of inspection to remind myself what those variables\nrepresented.\nNot too fussed either way though.\n\n+ struct DeviceStatus deviceStatus = {};\n> + bool success = false;\n> + uint32_t exposureLines, gainCode;\n> +\n> + if (buffer.size()) {\n> + parser_->SetBufferSize(buffer.size());\n> + success = parser_->Parse(buffer.data()) ==\n> MdParser::Status::OK &&\n> + parser_->GetExposureLines(exposureLines) ==\n> MdParser::Status::OK &&\n> + parser_->GetGainCode(gainCode) ==\n> MdParser::Status::OK;\n> +\n> + if (!success)\n> + LOG(IPARPI, Error) << \"Embedded buffer parsing\n> failed\";\n> + }\n> +\n> + if (!success) {\n> + exposureLines =\n> controls.get(V4L2_CID_EXPOSURE).get();\n> + gainCode =\n> controls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> + }\n> +\n> + deviceStatus.shutter_speed = Exposure(exposureLines);\n> + deviceStatus.analogue_gain = Gain(gainCode);\n> +\n> + LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> + << deviceStatus.shutter_speed\n> + << \" Gain : \"\n> + << deviceStatus.analogue_gain;\n> +\n> + metadata.Set(\"device.status\", deviceStatus);\n> +}\n> +\n> RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> CamHelperCreateFunc create_func)\n> {\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> index 1b2d6eec..ce5b82d2 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -8,8 +8,13 @@\n>\n> #include \n>\n> +#include \n> +#include \n> +\n> #include \"camera_mode.h\"\n> +#include \"controller/controller.hpp\"\n> #include \"md_parser.hpp\"\n> +#include \"controller/metadata.hpp\"\n>\n\nThis will need to be ordered alphabetically.\n\n\n>\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n>\n> @@ -65,7 +70,10 @@ public:\n> CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> virtual ~CamHelper();\n> void SetCameraMode(const CameraMode &mode);\n> - MdParser &Parser() const { return *parser_; }\n> + virtual void Prepare(const libcamera::Span &buffer,\n> + const libcamera::ControlList &controls,\n> + Metadata &metadata);\n> + virtual void Process(StatisticsPtr &stats, Metadata &metadata);\n> uint32_t ExposureLines(double exposure_us) const;\n> double Exposure(uint32_t exposure_lines) const; // in us\n> virtual uint32_t GetVBlanking(double &exposure_us, double\n> minFrameDuration,\n> @@ -81,6 +89,10 @@ public:\n> virtual unsigned int MistrustFramesModeSwitch() const;\n>\n> protected:\n> + void parseRegisters(const libcamera::Span &buffer,\n> + const libcamera::ControlList &controls,\n> + Metadata &metadata);\n> +\n> MdParser *parser_;\n> CameraMode mode_;\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 7904225a..d699540a 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -103,9 +103,6 @@ private:\n> void returnEmbeddedBuffer(unsigned int bufferId);\n> void prepareISP(const ipa::RPi::ISPConfig &data);\n> void reportMetadata();\n> - bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus\n> &deviceStatus);\n> - void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> - struct DeviceStatus &deviceStatus);\n> void processStats(unsigned int bufferId);\n> void applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration);\n> void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls);\n> @@ -913,35 +910,37 @@ void IPARPi::returnEmbeddedBuffer(unsigned int\n> bufferId)\n>\n> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> {\n> - struct DeviceStatus deviceStatus = {};\n> - bool success = false;\n> + Span embeddedBuffer;\n> + bool returnEmbedded = false;\n> +\n> + rpiMetadata_.Clear();\n>\n> if (data.embeddedBufferPresent) {\n> /*\n> * Pipeline handler has supplied us with an embedded data\n> buffer,\n> - * so parse it and extract the exposure and gain.\n> + * we must pass it to the CamHelper for parsing.\n> */\n> - success = parseEmbeddedData(data.embeddedBufferId,\n> deviceStatus);\n> -\n> - /* Done with embedded data now, return to pipeline handler\n> asap. */\n> - returnEmbeddedBuffer(data.embeddedBufferId);\n> + auto it = buffers_.find(data.embeddedBufferId);\n> + if (it == buffers_.end())\n> + LOG(IPARPI, Error) << \"Could not find embedded\n> buffer!\";\n>\n\nThis really should never happen, perhaps an assert in its place?\n\n\n> + else {\n> + embeddedBuffer = it->second.maps()[0];\n> + returnEmbedded = true;\n> + }\n> }\n>\n> - if (!success) {\n> - /*\n> - * Pipeline handler has not supplied an embedded data\n> buffer,\n> - * or embedded data buffer parsing has failed for some\n> reason,\n> - * so pull the exposure and gain values from the control\n> list.\n> - */\n> - int32_t exposureLines =\n> data.controls.get(V4L2_CID_EXPOSURE).get();\n> - int32_t gainCode =\n> data.controls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> - fillDeviceStatus(exposureLines, gainCode, deviceStatus);\n> - }\n> + /*\n> + * This will add the DeviceStatus to the metadata, and depending\n> on the\n> + * sensor, may do additional custom processing.\n> + */\n> + helper_->Prepare(embeddedBuffer, data.controls, rpiMetadata_);\n> +\n> + /* Done with embedded data now, return to pipeline handler asap. */\n> + if (returnEmbedded)\n> + returnEmbeddedBuffer(data.embeddedBufferId);\n>\n\nI think this should happen unconditionally - but as before, returnEmbedded\nshould never\nbe false. If the earlier code was changed to an assert, you could\nremove returnEmbedded.\n\nRegards,\nNaush\n\n\n\n> ControlList ctrls(ispCtrls_);\n>\n> - rpiMetadata_.Clear();\n> - rpiMetadata_.Set(\"device.status\", deviceStatus);\n> controller_.Prepare(&rpiMetadata_);\n>\n> /* Lock the metadata buffer to avoid constant locks/unlocks. */\n> @@ -991,50 +990,6 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig\n> &data)\n> setIspControls.emit(ctrls);\n> }\n>\n> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus\n> &deviceStatus)\n> -{\n> - auto it = buffers_.find(bufferId);\n> - if (it == buffers_.end()) {\n> - LOG(IPARPI, Error) << \"Could not find embedded buffer!\";\n> - return false;\n> - }\n> -\n> - Span mem = it->second.maps()[0];\n> - helper_->Parser().SetBufferSize(mem.size());\n> - RPiController::MdParser::Status status =\n> helper_->Parser().Parse(mem.data());\n> - if (status != RPiController::MdParser::Status::OK) {\n> - LOG(IPARPI, Error) << \"Embedded Buffer parsing failed,\n> error \" << status;\n> - return false;\n> - } else {\n> - uint32_t exposureLines, gainCode;\n> - if (helper_->Parser().GetExposureLines(exposureLines) !=\n> RPiController::MdParser::Status::OK) {\n> - LOG(IPARPI, Error) << \"Exposure time failed\";\n> - return false;\n> - }\n> -\n> - if (helper_->Parser().GetGainCode(gainCode) !=\n> RPiController::MdParser::Status::OK) {\n> - LOG(IPARPI, Error) << \"Gain failed\";\n> - return false;\n> - }\n> -\n> - fillDeviceStatus(exposureLines, gainCode, deviceStatus);\n> - }\n> -\n> - return true;\n> -}\n> -\n> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> - struct DeviceStatus &deviceStatus)\n> -{\n> - deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> - deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> -\n> - LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> - << deviceStatus.shutter_speed\n> - << \" Gain : \"\n> - << deviceStatus.analogue_gain;\n> -}\n> -\n> void IPARPi::processStats(unsigned int bufferId)\n> {\n> auto it = buffers_.find(bufferId);\n> @@ -1046,6 +1001,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> Span mem = it->second.maps()[0];\n> bcm2835_isp_stats *stats = reinterpret_cast *>(mem.data());\n> RPiController::StatisticsPtr statistics =\n> std::make_shared(*stats);\n> + helper_->Process(statistics, rpiMetadata_);\n> controller_.Process(statistics, &rpiMetadata_);\n>\n> struct AgcStatus agcStatus;\n> --\n> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 6EE36C32E1\n\tfor ;\n\tTue, 16 Mar 2021 10:15:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5F2C68D47;\n\tTue, 16 Mar 2021 11:15:34 +0100 (CET)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D258E602E8\n\tfor ;\n\tTue, 16 Mar 2021 11:15:32 +0100 (CET)","by mail-lf1-x129.google.com with SMTP id p21so61441260lfu.11\n\tfor ;\n\tTue, 16 Mar 2021 03:15:32 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"IYb/aM9v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=mv/2Im9HpamNLOH5yTDhualG/iP8A/UISRBzSoqAEdE=;\n\tb=IYb/aM9vW4eMSt349aVfrvtpdpnSlDSbTEePpRxUpkmz5SU9kBZuMznuGBvBGgUM3u\n\t45W89D7uaW/LVrbIqDgx9kZueKGBchRm3fqWkwjHCx32dPSIQHXLlFCIk28DGv17O2xn\n\tu98s+PEMN6sl8qP/nD0StNSy6ckivRgxKoIf4PN0x9Cluu2rXqyIoMEr5UqKTBw6VIXN\n\tP6KA3gyUqcmZHyzXBNJ5nsevobfTuQfdr45HR/i1Vdt8eftyc25yWBIJznSp+WXF9vml\n\tmeAw56QTrl167bNLHHOEiECum5I4UcoLQksvIzBAdIXrHfbCdKY+m/A4YPhWBn4NmBaO\n\tpdyQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=mv/2Im9HpamNLOH5yTDhualG/iP8A/UISRBzSoqAEdE=;\n\tb=ZOa3Oicg2h7+BbulILLzmS4St8jcDHcq5vlTbemXL0gN77yXwu4nHi+F6+9Q/nkms3\n\tTO/u23igRWphSX3aJeYy85LD1YS3EUorYCnefm6K0Ly2aVlt+Y5/ey1ZK0Ec7RmEA1u6\n\tCuRyc6mjkE02g4OxxUXb20x/bQgflcMJSmG7yDiMHI0twNJd7XgaZZKZewBlwM314saT\n\t7605GFTppl1vf6GT4Zu3F5yw6wwOlfJrYXbdNSaIUJOfyK8uT5OhmxwfSWloGZB6t3n2\n\t4W3R5q8S247r8D97zcpZecJ0njXfLS1UK8b9OXh7j0KMiOeuCotTyAhzpoE8mpbxsZNg\n\t73bQ==","X-Gm-Message-State":"AOAM531Ug75NYwjuZOKxepgbjDSZS8uGQVgu6DALDS5AjTIxrx3c3Ldn\n\tXLVP4jpm00LBotIZ/4Chs9Tv9/wPXm5yIsH/T1W2Tw==","X-Google-Smtp-Source":"ABdhPJxuY2Y9Vm0+SK6wVMTwFN+CN+ZJdz5E5+RGBjJ2Ll7FX1mm8Swi2BHJvlCfskm+u2QXveI2LwHPqu8byw679AI=","X-Received":"by 2002:a19:4116:: with SMTP id\n\to22mr11032585lfa.272.1615889732056; \n\tTue, 16 Mar 2021 03:15:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20210310172348.4312-1-david.plowman@raspberrypi.com>\n\t<20210310172348.4312-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20210310172348.4312-2-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Tue, 16 Mar 2021 10:15:15 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [RFC PATCH 1/1] ipa: raspberrypi: Use\n\tCamHelpers to generalise 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============1562552416871106690==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]