[{"id":15904,"web_url":"https://patchwork.libcamera.org/comment/15904/","msgid":"","date":"2021-03-25T16:07:42","subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata 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 Mon, 22 Mar 2021 at 16:01, 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 only needs to call the new Prepare() and Process()\n> methods, and to supply exposure/gain values from the controls when the\n> CamHelper does not.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 44 +++++++++++++++\n> src/ipa/raspberrypi/cam_helper.hpp | 11 +++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 83 ++++++++++-------------------\n> 3 files changed, 83 insertions(+), 55 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> index 0ae0baa0..ce6482ba 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\nI don't think this namespace scope is needed. Seems to compile fine\nwithout.\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> + Metadata &metadata)\n>\n+{\n> + parseEmbeddedData(buffer, metadata);\n> +}\n>\n\n\nWould it be useful to pass in the ControlList that contains the sensor setup\n(from DelayedControls) into this function? Would the CamHelper ever want\nto know these things?\n\nIf the answer is yes, you could also pull in the case that deals with\nnon-embedded\ndata where we pull values from the ControlList. So something like:\n\nvoid CamHelper::Prepare(const Span &buffer,\nMetadata &metadata, ControlList &SensorControls)\n{\nbool success = false;\n\nif (buffer.size())\nsuccess = parseEmbeddedData(buffer, metadata);\n\nif (!success) {\nstruct DeviceStatus deviceStatus = {};\nint32_t exposureLines =\nsensorControls.get(V4L2_CID_EXPOSURE).get();\nint32_t gainCode =\nsensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n\ndeviceStatus.shutter_speed = Exposure(exposureLines);\ndeviceStatus.analogue_gain = Gain(gainCode);\nmetadata.Set(\"device.status\", deviceStatus);\n}\n}\n\nWhat do you think? Might be a bit neater to have deviecStatus set in one\nplace only.\nWith or without this change and the fixup for the namespace scoping above:\n\nReviewed-by: Naushir Patuck \n\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,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch()\n> const\n> return 0;\n> }\n>\n> +void CamHelper::parseEmbeddedData(const Span &buffer,\n> + Metadata &metadata)\n>\n+{\n> + if (buffer.size()) {\n> + struct DeviceStatus deviceStatus = {};\n> + bool success = false;\n> + uint32_t exposureLines, gainCode;\n> +\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> + deviceStatus.shutter_speed =\n> 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> + } else\n> + LOG(IPARPI, Error) << \"Embedded buffer parsing\n> failed\";\n> + }\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..930ea39a 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> 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> + 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 +87,9 @@ public:\n> virtual unsigned int MistrustFramesModeSwitch() const;\n>\n> protected:\n> + void parseEmbeddedData(const libcamera::Span &buffer,\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..e08b47c0 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -103,8 +103,7 @@ 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> + void fillDeviceStatus(const ControlList &sensorControls,\n> struct DeviceStatus &deviceStatus);\n> void processStats(unsigned int bufferId);\n> void applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration);\n> @@ -913,35 +912,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> +\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> + ASSERT(it != buffers_.end());\n> + embeddedBuffer = it->second.maps()[0];\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 may add the DeviceStatus to the metadata, and depending on\n> the\n> + * sensor, may do additional custom processing.\n> + */\n> + helper_->Prepare(embeddedBuffer, rpiMetadata_);\n> +\n> + /* Add DeviceStatus metadata if helper_->Prepare() didn't. */\n> + DeviceStatus deviceStatus = {};\n> + if (rpiMetadata_.Get(\"device.status\", deviceStatus))\n> + fillDeviceStatus(data.controls, deviceStatus);\n> +\n> + /* Done with embedded data now, return to pipeline handler asap. */\n> + if (data.embeddedBufferPresent)\n> + returnEmbeddedBuffer(data.embeddedBufferId);\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,41 +992,12 @@ 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> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls,\n> struct DeviceStatus &deviceStatus)\n> {\n> + int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> + int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> +\n> deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> deviceStatus.analogue_gain = helper_->Gain(gainCode);\n>\n> @@ -1033,6 +1005,8 @@ void IPARPi::fillDeviceStatus(uint32_t\n> exposureLines, uint32_t gainCode,\n> << deviceStatus.shutter_speed\n> << \" Gain : \"\n> << deviceStatus.analogue_gain;\n> +\n> + rpiMetadata_.Set(\"device.status\", deviceStatus);\n> }\n>\n> void IPARPi::processStats(unsigned int bufferId)\n> @@ -1046,6 +1020,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 2E5F7BDC66\n\tfor ;\n\tThu, 25 Mar 2021 16:08:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AF9168D6A;\n\tThu, 25 Mar 2021 17:08:01 +0100 (CET)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFBCF602D5\n\tfor ;\n\tThu, 25 Mar 2021 17:07:59 +0100 (CET)","by mail-lj1-x22b.google.com with SMTP id z8so3778959ljm.12\n\tfor ;\n\tThu, 25 Mar 2021 09:07:59 -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=\"bHI0WKt1\"; 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=G03mf/NNLqq8u0BzLJ1k0/NSPIcJlBxgKLt7WCYqqKs=;\n\tb=bHI0WKt1Sk3zuoI49l56iyUHCvNJoycr84spdLhFadewL0dTx+LVp2gJG9wrlc7ZSR\n\tKAls40ksHETz1mP+3UqLUoUIGhgBC+MTdT3hXJz2Q1BQQuGP4iSAg9dNY6ITMKeuB5mz\n\tvaHR6spIAmOPy+pNkiad4zqCd07mlNymcyhqyGlE6HFs4oV4TeME3i8YBvO13yIVcTY5\n\tRpwJnqrLfBPItvOuvUQ/e6jENwl7T0X5B7/Q4YE0dDOB/+ggM1Q+z5YoTknJW+Wt30+W\n\t9ZZ/8y6x7yO+BOeBfxISp9hldmUpoVq6lpaTRPqAXBuNyTGpXDBzc2grChhpFpKQm7un\n\tWYXg==","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=G03mf/NNLqq8u0BzLJ1k0/NSPIcJlBxgKLt7WCYqqKs=;\n\tb=YYspxej7tYSydcORcVLJ9xufAjHdtUKpzFZVeVcVLRiyA7+aTITDMBltQIApOB8WcA\n\tuPjXkmz39mAkG87B9ccSyfDBbYBtI1Kf8tjAt1oz6i/PHLREH4xH1mSba1u8/r4akZzn\n\tlEZxF4X3+6DWvNps+dAM7XMFtRbQtLr/h2f7AeB7VKXAVKK4245Dv/TblhGE4nV8kV3w\n\t+fFMGoEp7OxQICLaqIiVOnYcvp2tqHijLEzncnTk5Hnnv5mTubTr5PUhTqWkjR/vrDJg\n\ti69qnKxKPoQyxQmCXVzEZt9+ORXWF/rYPaiMMw9e/fIpuLZojbbcIMvaA6q52VhI+edR\n\tswxg==","X-Gm-Message-State":"AOAM530kY1Hr0vPZiC65VYGk6CamufRK29sqM4I+zqIQI+yTuDlS8u5W\n\t8IQ2NWnv081iBYylVh3RN7MQ9APSmQFcQgX2zWRERA==","X-Google-Smtp-Source":"ABdhPJyfjgXZb+Sp+bVxjGrNZVlIX8OaVYRQLsnDKaTDOoZT6C3kyRPOjWfqFVeUcO8ABJactQgtdCiDRGNkgN5hMAo=","X-Received":"by 2002:a2e:89d4:: with SMTP id\n\tc20mr6243037ljk.169.1616688479076; \n\tThu, 25 Mar 2021 09:07:59 -0700 (PDT)","MIME-Version":"1.0","References":"<20210322160148.32291-1-david.plowman@raspberrypi.com>\n\t<20210322160148.32291-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20210322160148.32291-2-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Thu, 25 Mar 2021 16:07:42 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata 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=\"===============7763393055338051364==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15938,"web_url":"https://patchwork.libcamera.org/comment/15938/","msgid":"","date":"2021-03-26T09:32:40","subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata parsing","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the feedback!\n\nOn Thu, 25 Mar 2021 at 16:08, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thank you for your work.\n>\n> On Mon, 22 Mar 2021 at 16:01, David Plowman wrote:\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 only needs to call the new Prepare() and Process()\n>> methods, and to supply exposure/gain values from the controls when the\n>> CamHelper does not.\n>>\n>> Signed-off-by: David Plowman \n>> ---\n>> src/ipa/raspberrypi/cam_helper.cpp | 44 +++++++++++++++\n>> src/ipa/raspberrypi/cam_helper.hpp | 11 +++-\n>> src/ipa/raspberrypi/raspberrypi.cpp | 83 ++++++++++-------------------\n>> 3 files changed, 83 insertions(+), 55 deletions(-)\n>>\n>> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n>> index 0ae0baa0..ce6482ba 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>\n> I don't think this namespace scope is needed. Seems to compile fine without.\n\nFunny one, this. It builds like that for me too but when I run it (for\nexample, qcam), it bombs out with\n\nbuild/src/qcam/qcam: symbol lookup error:\n/home/pi/libcamera/build/src/ipa/raspberrypi/ipa_rpi.so: undefined\nsymbol: _Z17logCategoryIPARPIv\n\nIt seems to be looking for logCategory::IPARPI when I think it should\nbe libcamera::logCategory::IPARPI. I guess I'm not clear why the\nlinker doesn't flag that up... anyone know?\n\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>> + Metadata &metadata)\n>>\n>> +{\n>> + parseEmbeddedData(buffer, metadata);\n>> +}\n>\n>\n>\n> Would it be useful to pass in the ControlList that contains the sensor setup\n> (from DelayedControls) into this function? Would the CamHelper ever want\n> to know these things?\n>\n> If the answer is yes, you could also pull in the case that deals with non-embedded\n> data where we pull values from the ControlList. So something like:\n>\n> void CamHelper::Prepare(const Span &buffer,\n> Metadata &metadata, ControlList &SensorControls)\n> {\n> bool success = false;\n>\n> if (buffer.size())\n> success = parseEmbeddedData(buffer, metadata);\n>\n> if (!success) {\n> struct DeviceStatus deviceStatus = {};\n> int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>\n> deviceStatus.shutter_speed = Exposure(exposureLines);\n> deviceStatus.analogue_gain = Gain(gainCode);\n> metadata.Set(\"device.status\", deviceStatus);\n> }\n> }\n>\n> What do you think? Might be a bit neater to have deviecStatus set in one place only.\n\nInteresting point, and in fact, my previous version of this patch\n(labelled as \"RFC\") did exactly this. Obviously I back-tracked on\nthat, for the following reasons.\n\nThe version you've outlined above is nice because it makes the IPA\nfile (raspberrypi.cpp) a bit tidier, functions like\nCamHelper::Exposure and CamHelper::Gain only need to be called once.\nThe DeviceStatus only gets set in one place.\n\nThe version I went for in the end has the advantage that it makes\nCamHelper::Prepare responsible only for parsing sensor metadata.\nThere's nothing it has to remember to do if, for example, some\nparticular metadata isn't there.\n\nThis means that the CamHelper::Prepare for my mysterious new sensor\ndoesn't have to do anything extra. It just parses the sensor's\nstatistics buffer and nothing else. As this kind of code - CamHelpers\n- is likely to get duplicated more in future, potentially by 3rd\nparties, I was keener to avoid little \"gotchas\" in there, and slight\nuglification of raspberrypi.cpp seemed reasonable in return.\n\nBut it's a close call and I'm definitely swayable...\n\n> With or without this change and the fixup for the namespace scoping above:\n>\n> Reviewed-by: Naushir Patuck \n\nThanks!\n\nDavid\n\n>\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,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n>> return 0;\n>> }\n>>\n>> +void CamHelper::parseEmbeddedData(const Span &buffer,\n>> + Metadata &metadata)\n>>\n>> +{\n>> + if (buffer.size()) {\n>> + struct DeviceStatus deviceStatus = {};\n>> + bool success = false;\n>> + uint32_t exposureLines, gainCode;\n>> +\n>> + parser_->SetBufferSize(buffer.size());\n>> + success = parser_->Parse(buffer.data()) == MdParser::Status::OK &&\n>> + parser_->GetExposureLines(exposureLines) == MdParser::Status::OK &&\n>> + parser_->GetGainCode(gainCode) == MdParser::Status::OK;\n>> +\n>> + if (success) {\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>> + } else\n>> + LOG(IPARPI, Error) << \"Embedded buffer parsing failed\";\n>> + }\n>> +}\n>> +\n>> RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n>> CamHelperCreateFunc create_func)\n>> {\n>> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n>> index 1b2d6eec..930ea39a 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>> 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>> + 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 minFrameDuration,\n>> @@ -81,6 +87,9 @@ public:\n>> virtual unsigned int MistrustFramesModeSwitch() const;\n>>\n>> protected:\n>> + void parseEmbeddedData(const libcamera::Span &buffer,\n>> + Metadata &metadata);\n>> +\n>> MdParser *parser_;\n>> CameraMode mode_;\n>>\n>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> index 7904225a..e08b47c0 100644\n>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> @@ -103,8 +103,7 @@ 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 &deviceStatus);\n>> - void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n>> + void fillDeviceStatus(const ControlList &sensorControls,\n>> struct DeviceStatus &deviceStatus);\n>> void processStats(unsigned int bufferId);\n>> void applyFrameDurations(double minFrameDuration, double maxFrameDuration);\n>> @@ -913,35 +912,37 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n>>\n>> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n>> {\n>> - struct DeviceStatus deviceStatus = {};\n>> - bool success = false;\n>> + Span embeddedBuffer;\n>> +\n>> + rpiMetadata_.Clear();\n>>\n>> if (data.embeddedBufferPresent) {\n>> /*\n>> * Pipeline handler has supplied us with an embedded data 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, deviceStatus);\n>> -\n>> - /* Done with embedded data now, return to pipeline handler asap. */\n>> - returnEmbeddedBuffer(data.embeddedBufferId);\n>> + auto it = buffers_.find(data.embeddedBufferId);\n>> + ASSERT(it != buffers_.end());\n>> + embeddedBuffer = it->second.maps()[0];\n>> }\n>>\n>> - if (!success) {\n>> - /*\n>> - * Pipeline handler has not supplied an embedded data buffer,\n>> - * or embedded data buffer parsing has failed for some reason,\n>> - * so pull the exposure and gain values from the control list.\n>> - */\n>> - int32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get();\n>> - int32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> - fillDeviceStatus(exposureLines, gainCode, deviceStatus);\n>> - }\n>> + /*\n>> + * This may add the DeviceStatus to the metadata, and depending on the\n>> + * sensor, may do additional custom processing.\n>> + */\n>> + helper_->Prepare(embeddedBuffer, rpiMetadata_);\n>> +\n>> + /* Add DeviceStatus metadata if helper_->Prepare() didn't. */\n>> + DeviceStatus deviceStatus = {};\n>> + if (rpiMetadata_.Get(\"device.status\", deviceStatus))\n>> + fillDeviceStatus(data.controls, deviceStatus);\n>> +\n>> + /* Done with embedded data now, return to pipeline handler asap. */\n>> + if (data.embeddedBufferPresent)\n>> + returnEmbeddedBuffer(data.embeddedBufferId);\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,41 +992,12 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n>> setIspControls.emit(ctrls);\n>> }\n>>\n>> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &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 = helper_->Parser().Parse(mem.data());\n>> - if (status != RPiController::MdParser::Status::OK) {\n>> - LOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n>> - return false;\n>> - } else {\n>> - uint32_t exposureLines, gainCode;\n>> - if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n>> - LOG(IPARPI, Error) << \"Exposure time failed\";\n>> - return false;\n>> - }\n>> -\n>> - if (helper_->Parser().GetGainCode(gainCode) != 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>> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls,\n>> struct DeviceStatus &deviceStatus)\n>> {\n>> + int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> + int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> +\n>> deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n>> deviceStatus.analogue_gain = helper_->Gain(gainCode);\n>>\n>> @@ -1033,6 +1005,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n>> << deviceStatus.shutter_speed\n>> << \" Gain : \"\n>> << deviceStatus.analogue_gain;\n>> +\n>> + rpiMetadata_.Set(\"device.status\", deviceStatus);\n>> }\n>>\n>> void IPARPi::processStats(unsigned int bufferId)\n>> @@ -1046,6 +1020,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 = 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","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 55BF3BDC66\n\tfor ;\n\tFri, 26 Mar 2021 09:32:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 73BC068D6E;\n\tFri, 26 Mar 2021 10:32:55 +0100 (CET)","from mail-oo1-xc29.google.com (mail-oo1-xc29.google.com\n\t[IPv6:2607:f8b0:4864:20::c29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB0286051B\n\tfor ;\n\tFri, 26 Mar 2021 10:32:53 +0100 (CET)","by mail-oo1-xc29.google.com with SMTP id\n\tc12-20020a4ae24c0000b02901bad05f40e4so1157233oot.4\n\tfor ;\n\tFri, 26 Mar 2021 02:32:53 -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=\"WPy/Cg3S\"; 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=n8/W7v75RXHkExWmbBGH56y3qp+cM9bx21fsHR13rc8=;\n\tb=WPy/Cg3SPWwgRLu73md3WGmx8D8P3p3WIh+MnaUU+Bsst4VPrJLXwG53d4s/LRE1ds\n\tKHiAteAsoP6Q4vmicBGIzeiAKV7/jZijLOlmtuTRxEe6qsWdveGrQ6g852i38Cy+NxSt\n\tlg3cZNfoLBpnom6i4VRHSLSD6g5h2Qu5rByWF/DILZkDV/2vKBzXO1G9pGPka3fr1FfK\n\tDd1fQQzi30f/16t5bU2akgdQhuA6uOByaSnQyZd6RZz2TpkBDXs9WfNctLhhaaTh5CGX\n\tpfhqYbZ9ETi7xJdn1LypqQfco9cRwX00pRiqDM2SdsHWI9J28RuQ/JMEYZA4ElXfkPFz\n\tZTgg==","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=n8/W7v75RXHkExWmbBGH56y3qp+cM9bx21fsHR13rc8=;\n\tb=n8S7/S1XNX9Xh0KNXN5dWJTj4Qmvtzy1nfYI4/XmFqPoCkZZfD7YxvlqAeVv5U6+hY\n\toCVE1+muSzJj7VN4a7xZGX1eXvEreq4+f/25XnyOmzwNSahgephbXKCcahx3L43eNhwI\n\tAXpHvqJFWTs770KEiZCILVijdfBoCIIKeU3/hBkYJm2Ek8sBkdvt+uDv8gmojdJICXWO\n\tzOdTZbBF9kAG0NNWNw4Q9E6Mu+v7BMxLKvLUfwWAN749KhFz9Jqt3kzJv8NJkwedwymg\n\tNkk/dRSTiMZlscdj8409vY4lDqhY7tYdOOVHaMfDAGE8K1AnmtJ5c5Rf+dmFjsjNnZoQ\n\t/hLQ==","X-Gm-Message-State":"AOAM530nGtGuFZ6up6nBpbO+TkONurtH89pOxbNhY4ElgesPE30CRFBK\n\tMqREqLcDT0Z0NXmBb4XXPxxDni8hvNl2qAljy0mBUQ==","X-Google-Smtp-Source":"ABdhPJxS1Ib0GgllbZNMCTuZAwUi4mhOlTEvWZhzfvnWtaH03N4twR9hK2ioL9Q43iV8lWV9W6wnuQs/claK1KFV3qU=","X-Received":"by 2002:a4a:d553:: with SMTP id\n\tq19mr10551241oos.28.1616751172142; \n\tFri, 26 Mar 2021 02:32:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20210322160148.32291-1-david.plowman@raspberrypi.com>\n\t<20210322160148.32291-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Fri, 26 Mar 2021 09:32:40 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata 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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15939,"web_url":"https://patchwork.libcamera.org/comment/15939/","msgid":"","date":"2021-03-26T09:57:10","subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata parsing","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nOn Fri, 26 Mar 2021 at 09:32, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for the feedback!\n>\n> On Thu, 25 Mar 2021 at 16:08, Naushir Patuck \n> wrote:\n> >\n> > Hi David,\n> >\n> > Thank you for your work.\n> >\n> > On Mon, 22 Mar 2021 at 16:01, David Plowman <\n> david.plowman@raspberrypi.com> wrote:\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 only needs to call the new Prepare() and Process()\n> >> methods, and to supply exposure/gain values from the controls when the\n> >> CamHelper does not.\n> >>\n> >> Signed-off-by: David Plowman \n> >> ---\n> >> src/ipa/raspberrypi/cam_helper.cpp | 44 +++++++++++++++\n> >> src/ipa/raspberrypi/cam_helper.hpp | 11 +++-\n> >> src/ipa/raspberrypi/raspberrypi.cpp | 83 ++++++++++-------------------\n> >> 3 files changed, 83 insertions(+), 55 deletions(-)\n> >>\n> >> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> >> index 0ae0baa0..ce6482ba 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> >\n> > I don't think this namespace scope is needed. Seems to compile fine\n> without.\n>\n> Funny one, this. It builds like that for me too but when I run it (for\n> example, qcam), it bombs out with\n>\n> build/src/qcam/qcam: symbol lookup error:\n> /home/pi/libcamera/build/src/ipa/raspberrypi/ipa_rpi.so: undefined\n> symbol: _Z17logCategoryIPARPIv\n>\n> It seems to be looking for logCategory::IPARPI when I think it should\n> be libcamera::logCategory::IPARPI. I guess I'm not clear why the\n> linker doesn't flag that up... anyone know?\n>\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> >> + Metadata &metadata)\n> >>\n> >> +{\n> >> + parseEmbeddedData(buffer, metadata);\n> >> +}\n> >\n> >\n> >\n> > Would it be useful to pass in the ControlList that contains the sensor\n> setup\n> > (from DelayedControls) into this function? Would the CamHelper ever want\n> > to know these things?\n> >\n> > If the answer is yes, you could also pull in the case that deals with\n> non-embedded\n> > data where we pull values from the ControlList. So something like:\n> >\n> > void CamHelper::Prepare(const Span &buffer,\n> > Metadata &metadata, ControlList &SensorControls)\n> > {\n> > bool success = false;\n> >\n> > if (buffer.size())\n> > success = parseEmbeddedData(buffer, metadata);\n> >\n> > if (!success) {\n> > struct DeviceStatus deviceStatus = {};\n> > int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >\n> > deviceStatus.shutter_speed = Exposure(exposureLines);\n> > deviceStatus.analogue_gain = Gain(gainCode);\n> > metadata.Set(\"device.status\", deviceStatus);\n> > }\n> > }\n> >\n> > What do you think? Might be a bit neater to have deviecStatus set in one\n> place only.\n>\n> Interesting point, and in fact, my previous version of this patch\n> (labelled as \"RFC\") did exactly this. Obviously I back-tracked on\n> that, for the following reasons.\n>\n> The version you've outlined above is nice because it makes the IPA\n> file (raspberrypi.cpp) a bit tidier, functions like\n> CamHelper::Exposure and CamHelper::Gain only need to be called once.\n> The DeviceStatus only gets set in one place.\n>\n> The version I went for in the end has the advantage that it makes\n> CamHelper::Prepare responsible only for parsing sensor metadata.\n> There's nothing it has to remember to do if, for example, some\n> particular metadata isn't there.\n>\n> This means that the CamHelper::Prepare for my mysterious new sensor\n> doesn't have to do anything extra. It just parses the sensor's\n> statistics buffer and nothing else. As this kind of code - CamHelpers\n> - is likely to get duplicated more in future, potentially by 3rd\n> parties, I was keener to avoid little \"gotchas\" in there, and slight\n> uglification of raspberrypi.cpp seemed reasonable in return.\n>\n> But it's a close call and I'm definitely swayable...\n>\n\nPerhaps it's a bit premature to think about this, but what if some stuff\nrunning in\nprepare() (e.g. fancy phase detect AF) needs the exposure and gain values\nused\nfor the sensor, and we don't have embedded data to give us these values.\nYou could\npossibly run this fancy AF in process() where you do get the values, but\nthen you add\na frame's worth of latency.\n\nIt also possibly makes the code a bit cleaner for an up-coming change for\nimx477 ultra\nlong exposures where I have to disable parsing when the exposure factor is\nset. But\nagain, I have not written code for this yet to tell for sure.\n\nI'll leave it to you to choose :-)\n\nNaush\n\n\n>\n> > With or without this change and the fixup for the namespace scoping\n> above:\n> >\n> > Reviewed-by: Naushir Patuck \n>\n> Thanks!\n>\n> David\n>\n> >\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,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch()\n> const\n> >> return 0;\n> >> }\n> >>\n> >> +void CamHelper::parseEmbeddedData(const Span &buffer,\n> >> + Metadata &metadata)\n> >>\n> >> +{\n> >> + if (buffer.size()) {\n> >> + struct DeviceStatus deviceStatus = {};\n> >> + bool success = false;\n> >> + uint32_t exposureLines, gainCode;\n> >> +\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> >> + deviceStatus.shutter_speed =\n> Exposure(exposureLines);\n> >> + deviceStatus.analogue_gain = Gain(gainCode);\n> >> +\n> >> + LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> >> + << deviceStatus.shutter_speed\n> >> + << \" Gain : \"\n> >> + <<\n> deviceStatus.analogue_gain;\n> >> +\n> >> + metadata.Set(\"device.status\", deviceStatus);\n> >> + } else\n> >> + LOG(IPARPI, Error) << \"Embedded buffer parsing\n> failed\";\n> >> + }\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..930ea39a 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> >> 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> >> + 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 +87,9 @@ public:\n> >> virtual unsigned int MistrustFramesModeSwitch() const;\n> >>\n> >> protected:\n> >> + void parseEmbeddedData(const libcamera::Span &buffer,\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..e08b47c0 100644\n> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> @@ -103,8 +103,7 @@ 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\n> DeviceStatus &deviceStatus);\n> >> - void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> >> + void fillDeviceStatus(const ControlList &sensorControls,\n> >> struct DeviceStatus &deviceStatus);\n> >> void processStats(unsigned int bufferId);\n> >> void applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration);\n> >> @@ -913,35 +912,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> >> +\n> >> + rpiMetadata_.Clear();\n> >>\n> >> if (data.embeddedBufferPresent) {\n> >> /*\n> >> * Pipeline handler has supplied us with an embedded\n> data 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\n> handler asap. */\n> >> - returnEmbeddedBuffer(data.embeddedBufferId);\n> >> + auto it = buffers_.find(data.embeddedBufferId);\n> >> + ASSERT(it != buffers_.end());\n> >> + embeddedBuffer = it->second.maps()[0];\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\n> control 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 may add the DeviceStatus to the metadata, and depending\n> on the\n> >> + * sensor, may do additional custom processing.\n> >> + */\n> >> + helper_->Prepare(embeddedBuffer, rpiMetadata_);\n> >> +\n> >> + /* Add DeviceStatus metadata if helper_->Prepare() didn't. */\n> >> + DeviceStatus deviceStatus = {};\n> >> + if (rpiMetadata_.Get(\"device.status\", deviceStatus))\n> >> + fillDeviceStatus(data.controls, deviceStatus);\n> >> +\n> >> + /* Done with embedded data now, return to pipeline handler\n> asap. */\n> >> + if (data.embeddedBufferPresent)\n> >> + returnEmbeddedBuffer(data.embeddedBufferId);\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,41 +992,12 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig\n> &data)\n> >> setIspControls.emit(ctrls);\n> >> }\n> >>\n> >> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct\n> DeviceStatus &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\n> gainCode,\n> >> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls,\n> >> struct DeviceStatus &deviceStatus)\n> >> {\n> >> + int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> >> + int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >> +\n> >> deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> >> deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> >>\n> >> @@ -1033,6 +1005,8 @@ void IPARPi::fillDeviceStatus(uint32_t\n> exposureLines, uint32_t gainCode,\n> >> << deviceStatus.shutter_speed\n> >> << \" Gain : \"\n> >> << deviceStatus.analogue_gain;\n> >> +\n> >> + rpiMetadata_.Set(\"device.status\", deviceStatus);\n> >> }\n> >>\n> >> void IPARPi::processStats(unsigned int bufferId)\n> >> @@ -1046,6 +1020,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 A9292C32EA\n\tfor ;\n\tFri, 26 Mar 2021 09:57:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B3FC68D6E;\n\tFri, 26 Mar 2021 10:57:28 +0100 (CET)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 143206051B\n\tfor ;\n\tFri, 26 Mar 2021 10:57:27 +0100 (CET)","by mail-lf1-x12a.google.com with SMTP id a198so6795827lfd.7\n\tfor ;\n\tFri, 26 Mar 2021 02:57:27 -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=\"EhglSGQk\"; 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=D0XLsMj/RIRyMsBkcgoronQqPmR/tVHmAkAXN2DjZuY=;\n\tb=EhglSGQkTyp0Zi1KY0ibVc9TgFetmxyuDITe/JzRAyhxSxfwNBE0qXfGrt6yylnZte\n\tiso2wHknSOx2bP5ZH7fCLdMCW9qXO7K9OH5ywIPpHsmOwrrZD2sLj9pbddK5DT5Snx9b\n\tbPUHi/OUd7bQ5Ktw1zyj0LDpBKLvuCrL7hIk5UyZKzTF/l0Wj+WpwQeFDjW5aiHTKRYo\n\tBeqajC65cdK+/WoCdV8hlfVJDwCMNGnQxqlcZhABc2lW/IwKQZ54IXLhN0SAoH5j1HCT\n\t3dH+wu25SF11c2+Myj3Y2sgqDk5Jn2aZOFSsZJDZSwqEfjAWhzsLFUh/1poXBR4ECg2d\n\tLl0g==","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=D0XLsMj/RIRyMsBkcgoronQqPmR/tVHmAkAXN2DjZuY=;\n\tb=r6t0HN0vPRMvQRo4pxu5Giid/VYOKuo7tV67N2okbweN8Q7aSLL3SQCabiSP9qdM2Z\n\tcBjps3BIiOC0Oo2H9/Q3OpGu5lv3G191FHuKSs/klxmArKtdmrxGjPej3rDxWh2wDQ4y\n\tqpvK0eELCLSDOVWkYnK6ZXaOjGDEQ/nm1I6MfYIgN0rMlgX1sCHrOTR+A8LPlSB4mlp+\n\tEcfyKGBv0Q2RqxsMongVNTlLPjhi1Fi2p0dPkWdCPJoCfgkFcy4lt1Uo8s8vb3bt5xpe\n\t1W4iz81bz5KRXHBtqxdvbVD27p7uuPvskR9PySf5p/FycpJbW6PhLqzWyX2C3+ptMCXY\n\tC7kw==","X-Gm-Message-State":"AOAM531kbIUeLIKr7J3ANP345VhY7re8L0QO+vo5inRD02UYAu+mhCwH\n\t9zvUKPVDlbC532niNq+Pr6xAhigEQyJPGOVLQuBp/A==","X-Google-Smtp-Source":"ABdhPJyz0//28VdFLZ5YNatCDWGBqX2XNyFkzhj2zms1esJqsqELEAgO0jv2nQC+58avenOl37kFgcJWPpWInhl389s=","X-Received":"by 2002:a05:6512:210b:: with SMTP id\n\tq11mr7552562lfr.133.1616752646349; \n\tFri, 26 Mar 2021 02:57:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20210322160148.32291-1-david.plowman@raspberrypi.com>\n\t<20210322160148.32291-2-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 26 Mar 2021 09:57:10 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata 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=\"===============5799420699930073714==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15940,"web_url":"https://patchwork.libcamera.org/comment/15940/","msgid":"","date":"2021-03-26T11:07:12","subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata parsing","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nOn Fri, 26 Mar 2021 at 09:57, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> On Fri, 26 Mar 2021 at 09:32, David Plowman wrote:\n>>\n>> Hi Naush\n>>\n>> Thanks for the feedback!\n>>\n>> On Thu, 25 Mar 2021 at 16:08, Naushir Patuck wrote:\n>> >\n>> > Hi David,\n>> >\n>> > Thank you for your work.\n>> >\n>> > On Mon, 22 Mar 2021 at 16:01, David Plowman wrote:\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 only needs to call the new Prepare() and Process()\n>> >> methods, and to supply exposure/gain values from the controls when the\n>> >> CamHelper does not.\n>> >>\n>> >> Signed-off-by: David Plowman \n>> >> ---\n>> >> src/ipa/raspberrypi/cam_helper.cpp | 44 +++++++++++++++\n>> >> src/ipa/raspberrypi/cam_helper.hpp | 11 +++-\n>> >> src/ipa/raspberrypi/raspberrypi.cpp | 83 ++++++++++-------------------\n>> >> 3 files changed, 83 insertions(+), 55 deletions(-)\n>> >>\n>> >> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n>> >> index 0ae0baa0..ce6482ba 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>> >\n>> > I don't think this namespace scope is needed. Seems to compile fine without.\n>>\n>> Funny one, this. It builds like that for me too but when I run it (for\n>> example, qcam), it bombs out with\n>>\n>> build/src/qcam/qcam: symbol lookup error:\n>> /home/pi/libcamera/build/src/ipa/raspberrypi/ipa_rpi.so: undefined\n>> symbol: _Z17logCategoryIPARPIv\n>>\n>> It seems to be looking for logCategory::IPARPI when I think it should\n>> be libcamera::logCategory::IPARPI. I guess I'm not clear why the\n>> linker doesn't flag that up... anyone know?\n>>\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>> >> + Metadata &metadata)\n>> >>\n>> >> +{\n>> >> + parseEmbeddedData(buffer, metadata);\n>> >> +}\n>> >\n>> >\n>> >\n>> > Would it be useful to pass in the ControlList that contains the sensor setup\n>> > (from DelayedControls) into this function? Would the CamHelper ever want\n>> > to know these things?\n>> >\n>> > If the answer is yes, you could also pull in the case that deals with non-embedded\n>> > data where we pull values from the ControlList. So something like:\n>> >\n>> > void CamHelper::Prepare(const Span &buffer,\n>> > Metadata &metadata, ControlList &SensorControls)\n>> > {\n>> > bool success = false;\n>> >\n>> > if (buffer.size())\n>> > success = parseEmbeddedData(buffer, metadata);\n>> >\n>> > if (!success) {\n>> > struct DeviceStatus deviceStatus = {};\n>> > int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> > int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> >\n>> > deviceStatus.shutter_speed = Exposure(exposureLines);\n>> > deviceStatus.analogue_gain = Gain(gainCode);\n>> > metadata.Set(\"device.status\", deviceStatus);\n>> > }\n>> > }\n>> >\n>> > What do you think? Might be a bit neater to have deviecStatus set in one place only.\n>>\n>> Interesting point, and in fact, my previous version of this patch\n>> (labelled as \"RFC\") did exactly this. Obviously I back-tracked on\n>> that, for the following reasons.\n>>\n>> The version you've outlined above is nice because it makes the IPA\n>> file (raspberrypi.cpp) a bit tidier, functions like\n>> CamHelper::Exposure and CamHelper::Gain only need to be called once.\n>> The DeviceStatus only gets set in one place.\n>>\n>> The version I went for in the end has the advantage that it makes\n>> CamHelper::Prepare responsible only for parsing sensor metadata.\n>> There's nothing it has to remember to do if, for example, some\n>> particular metadata isn't there.\n>>\n>> This means that the CamHelper::Prepare for my mysterious new sensor\n>> doesn't have to do anything extra. It just parses the sensor's\n>> statistics buffer and nothing else. As this kind of code - CamHelpers\n>> - is likely to get duplicated more in future, potentially by 3rd\n>> parties, I was keener to avoid little \"gotchas\" in there, and slight\n>> uglification of raspberrypi.cpp seemed reasonable in return.\n>>\n>> But it's a close call and I'm definitely swayable...\n>\n>\n> Perhaps it's a bit premature to think about this, but what if some stuff running in\n> prepare() (e.g. fancy phase detect AF) needs the exposure and gain values used\n> for the sensor, and we don't have embedded data to give us these values. You could\n> possibly run this fancy AF in process() where you do get the values, but then you add\n> a frame's worth of latency.\n>\n> It also possibly makes the code a bit cleaner for an up-coming change for imx477 ultra\n> long exposures where I have to disable parsing when the exposure factor is set. But\n> again, I have not written code for this yet to tell for sure.\n\nThat's an interesting thought. Generally I'm quite strongly opposed to\n\"algorithm\" code creeping into the helpers, the helpers should be\nrestricted to the minimum \"parsing\" duties. To take the AF example,\nCamHelper::Prepare would put the PDAF information into some kind of\n\"pdaf\" entry in our RPi metadata. Hopefully many sensors would be able\nto re-use this \"pdaf\" metadata. Then there'd be a bona fide \"pdaf\"\nalgorithm, loaded as usual via the json file, that would interpret it.\n\nOn the subject of long exposures, how can you tell whether you're in\nthe long exposure case - are the values enough? I'm thinking about\nalways reading the exposure/gain from the controls and filling in the\nDeviceStatus *before* we call CamHelper::Prepare. That way you have\nthe values, and you can overwrite them if you know better. Would that\nbe helpful to the imx477?\n\nThanks!\nDavid\n\n>\n> I'll leave it to you to choose :-)\n>\n> Naush\n>\n>>\n>>\n>> > With or without this change and the fixup for the namespace scoping above:\n>> >\n>> > Reviewed-by: Naushir Patuck \n>>\n>> Thanks!\n>>\n>> David\n>>\n>> >\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,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n>> >> return 0;\n>> >> }\n>> >>\n>> >> +void CamHelper::parseEmbeddedData(const Span &buffer,\n>> >> + Metadata &metadata)\n>> >>\n>> >> +{\n>> >> + if (buffer.size()) {\n>> >> + struct DeviceStatus deviceStatus = {};\n>> >> + bool success = false;\n>> >> + uint32_t exposureLines, gainCode;\n>> >> +\n>> >> + parser_->SetBufferSize(buffer.size());\n>> >> + success = parser_->Parse(buffer.data()) == MdParser::Status::OK &&\n>> >> + parser_->GetExposureLines(exposureLines) == MdParser::Status::OK &&\n>> >> + parser_->GetGainCode(gainCode) == MdParser::Status::OK;\n>> >> +\n>> >> + if (success) {\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>> >> + } else\n>> >> + LOG(IPARPI, Error) << \"Embedded buffer parsing failed\";\n>> >> + }\n>> >> +}\n>> >> +\n>> >> RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n>> >> CamHelperCreateFunc create_func)\n>> >> {\n>> >> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n>> >> index 1b2d6eec..930ea39a 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>> >> 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>> >> + 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 minFrameDuration,\n>> >> @@ -81,6 +87,9 @@ public:\n>> >> virtual unsigned int MistrustFramesModeSwitch() const;\n>> >>\n>> >> protected:\n>> >> + void parseEmbeddedData(const libcamera::Span &buffer,\n>> >> + Metadata &metadata);\n>> >> +\n>> >> MdParser *parser_;\n>> >> CameraMode mode_;\n>> >>\n>> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> >> index 7904225a..e08b47c0 100644\n>> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> >> @@ -103,8 +103,7 @@ 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 &deviceStatus);\n>> >> - void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n>> >> + void fillDeviceStatus(const ControlList &sensorControls,\n>> >> struct DeviceStatus &deviceStatus);\n>> >> void processStats(unsigned int bufferId);\n>> >> void applyFrameDurations(double minFrameDuration, double maxFrameDuration);\n>> >> @@ -913,35 +912,37 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n>> >>\n>> >> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n>> >> {\n>> >> - struct DeviceStatus deviceStatus = {};\n>> >> - bool success = false;\n>> >> + Span embeddedBuffer;\n>> >> +\n>> >> + rpiMetadata_.Clear();\n>> >>\n>> >> if (data.embeddedBufferPresent) {\n>> >> /*\n>> >> * Pipeline handler has supplied us with an embedded data 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, deviceStatus);\n>> >> -\n>> >> - /* Done with embedded data now, return to pipeline handler asap. */\n>> >> - returnEmbeddedBuffer(data.embeddedBufferId);\n>> >> + auto it = buffers_.find(data.embeddedBufferId);\n>> >> + ASSERT(it != buffers_.end());\n>> >> + embeddedBuffer = it->second.maps()[0];\n>> >> }\n>> >>\n>> >> - if (!success) {\n>> >> - /*\n>> >> - * Pipeline handler has not supplied an embedded data buffer,\n>> >> - * or embedded data buffer parsing has failed for some reason,\n>> >> - * so pull the exposure and gain values from the control list.\n>> >> - */\n>> >> - int32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get();\n>> >> - int32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> >> - fillDeviceStatus(exposureLines, gainCode, deviceStatus);\n>> >> - }\n>> >> + /*\n>> >> + * This may add the DeviceStatus to the metadata, and depending on the\n>> >> + * sensor, may do additional custom processing.\n>> >> + */\n>> >> + helper_->Prepare(embeddedBuffer, rpiMetadata_);\n>> >> +\n>> >> + /* Add DeviceStatus metadata if helper_->Prepare() didn't. */\n>> >> + DeviceStatus deviceStatus = {};\n>> >> + if (rpiMetadata_.Get(\"device.status\", deviceStatus))\n>> >> + fillDeviceStatus(data.controls, deviceStatus);\n>> >> +\n>> >> + /* Done with embedded data now, return to pipeline handler asap. */\n>> >> + if (data.embeddedBufferPresent)\n>> >> + returnEmbeddedBuffer(data.embeddedBufferId);\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,41 +992,12 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n>> >> setIspControls.emit(ctrls);\n>> >> }\n>> >>\n>> >> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &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 = helper_->Parser().Parse(mem.data());\n>> >> - if (status != RPiController::MdParser::Status::OK) {\n>> >> - LOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n>> >> - return false;\n>> >> - } else {\n>> >> - uint32_t exposureLines, gainCode;\n>> >> - if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n>> >> - LOG(IPARPI, Error) << \"Exposure time failed\";\n>> >> - return false;\n>> >> - }\n>> >> -\n>> >> - if (helper_->Parser().GetGainCode(gainCode) != 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>> >> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls,\n>> >> struct DeviceStatus &deviceStatus)\n>> >> {\n>> >> + int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> >> + int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> >> +\n>> >> deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n>> >> deviceStatus.analogue_gain = helper_->Gain(gainCode);\n>> >>\n>> >> @@ -1033,6 +1005,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n>> >> << deviceStatus.shutter_speed\n>> >> << \" Gain : \"\n>> >> << deviceStatus.analogue_gain;\n>> >> +\n>> >> + rpiMetadata_.Set(\"device.status\", deviceStatus);\n>> >> }\n>> >>\n>> >> void IPARPi::processStats(unsigned int bufferId)\n>> >> @@ -1046,6 +1020,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 = 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","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 B1D85C32EA\n\tfor ;\n\tFri, 26 Mar 2021 11:07:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D3C2168D47;\n\tFri, 26 Mar 2021 12:07:27 +0100 (CET)","from mail-ot1-x32c.google.com (mail-ot1-x32c.google.com\n\t[IPv6:2607:f8b0:4864:20::32c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDE59602D7\n\tfor ;\n\tFri, 26 Mar 2021 12:07:25 +0100 (CET)","by mail-ot1-x32c.google.com with SMTP id\n\ty19-20020a0568301d93b02901b9f88a238eso4818022oti.11\n\tfor ;\n\tFri, 26 Mar 2021 04:07:25 -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=\"fM9C7yad\"; 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=VQmdVdmJZ1ofJ0ezK3RQ14dsxqVglyNjb/BdQw4rCHY=;\n\tb=fM9C7yadg4bopYP573RxOZ7aUSTqKvAf+UmdvsoBcPxvooUOapN2wEuBuxe9NxtwKh\n\tJTmtl+5nEjiMDuP5UoXRbRFoPSjcvxBMDiHgC1Bor/GcwQxn1KUURbI+e8X1f0LRWUYc\n\tLLN5iN5RwWuDZUCWfnyZxyJTvsf0JxBZesTmXrJWHBnPN1RF99s86C34nRsayIeRZYHK\n\tRRuLijsjPPlvZP4qJDjR8tb4mkLZPTtXMofdWz+CHlmXT0Cg9uQfZy6POV1/tCjpEI1y\n\t0zA6PuOQhAgeHuK6YQIwGfGPCDeexPb4GKQnuXYjr+Jz59FDWb5mJI4RWfuWC4nhg48O\n\tpFbw==","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=VQmdVdmJZ1ofJ0ezK3RQ14dsxqVglyNjb/BdQw4rCHY=;\n\tb=bbB2yQBhvz5UwE8u/k66wUs444NvBGAANRJe69slSXDUiaU0VY5D2qG17kbHaHqbRc\n\tUf9o3V/7Ko4NXYwfnhyc+5gK1ZAwWYEsKJoth8n2ASVbvvOfxxpv1UVGGiiAi/EYAtsA\n\tuo6Eg+02HPBpvm0aQxe0nC2B1BSE6yAjQX78Kc6ec+VZkK4nnFhsd2y92Z1J01WeCNCV\n\t3tcd6dvXpM/DDgQtc87fU1CLwMoN1I9zYarFQOWc66gaY/2ZxfSDSAYmTaANgUJ7khdt\n\tplmzJbAo9hkgAD6WpqvmoCwe+/mfH2qWY3ov0qTVh0Zl2kj/RaVCj0ggGDhsml3aS7/b\n\tU8fw==","X-Gm-Message-State":"AOAM531WnztmJjWqn8KycILwfe3RPM8TQLSh+fZZ8vcIWmEiYU43wqKh\n\tyGKe/a7vSH187iE0TH7Yqz2Qnogu6V8eZXXUYN/KNg==","X-Google-Smtp-Source":"ABdhPJwD7oA4e9L75Exr1lnYP/tdT72AcH+DD7R0CeGcMAf4wd980erz8Kl+//xA6+tW6hTkwjYMQS/6cKm2FPecHZ8=","X-Received":"by 2002:a05:6830:8c:: with SMTP id\n\ta12mr10732803oto.317.1616756844109; \n\tFri, 26 Mar 2021 04:07:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20210322160148.32291-1-david.plowman@raspberrypi.com>\n\t<20210322160148.32291-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Fri, 26 Mar 2021 11:07:12 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata 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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15941,"web_url":"https://patchwork.libcamera.org/comment/15941/","msgid":"","date":"2021-03-26T11:14:56","subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata parsing","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nOn Fri, 26 Mar 2021 at 11:07, David Plowman \nwrote:\n\n> Hi Naush\n>\n> On Fri, 26 Mar 2021 at 09:57, Naushir Patuck \n> wrote:\n> >\n> > Hi David,\n> >\n> > On Fri, 26 Mar 2021 at 09:32, David Plowman <\n> david.plowman@raspberrypi.com> wrote:\n> >>\n> >> Hi Naush\n> >>\n> >> Thanks for the feedback!\n> >>\n> >> On Thu, 25 Mar 2021 at 16:08, Naushir Patuck \n> wrote:\n> >> >\n> >> > Hi David,\n> >> >\n> >> > Thank you for your work.\n> >> >\n> >> > On Mon, 22 Mar 2021 at 16:01, David Plowman <\n> david.plowman@raspberrypi.com> wrote:\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\n> when\n> >> >> necessary.\n> >> >>\n> >> >> The IPA itself only needs to call the new Prepare() and Process()\n> >> >> methods, and to supply exposure/gain values from the controls when\n> the\n> >> >> CamHelper does not.\n> >> >>\n> >> >> Signed-off-by: David Plowman \n> >> >> ---\n> >> >> src/ipa/raspberrypi/cam_helper.cpp | 44 +++++++++++++++\n> >> >> src/ipa/raspberrypi/cam_helper.hpp | 11 +++-\n> >> >> src/ipa/raspberrypi/raspberrypi.cpp | 83\n> ++++++++++-------------------\n> >> >> 3 files changed, 83 insertions(+), 55 deletions(-)\n> >> >>\n> >> >> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> >> >> index 0ae0baa0..ce6482ba 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> >> >\n> >> > I don't think this namespace scope is needed. Seems to compile fine\n> without.\n> >>\n> >> Funny one, this. It builds like that for me too but when I run it (for\n> >> example, qcam), it bombs out with\n> >>\n> >> build/src/qcam/qcam: symbol lookup error:\n> >> /home/pi/libcamera/build/src/ipa/raspberrypi/ipa_rpi.so: undefined\n> >> symbol: _Z17logCategoryIPARPIv\n> >>\n> >> It seems to be looking for logCategory::IPARPI when I think it should\n> >> be libcamera::logCategory::IPARPI. I guess I'm not clear why the\n> >> linker doesn't flag that up... anyone know?\n> >>\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> >> >> + Metadata &metadata)\n> >> >>\n> >> >> +{\n> >> >> + parseEmbeddedData(buffer, metadata);\n> >> >> +}\n> >> >\n> >> >\n> >> >\n> >> > Would it be useful to pass in the ControlList that contains the\n> sensor setup\n> >> > (from DelayedControls) into this function? Would the CamHelper ever\n> want\n> >> > to know these things?\n> >> >\n> >> > If the answer is yes, you could also pull in the case that deals with\n> non-embedded\n> >> > data where we pull values from the ControlList. So something like:\n> >> >\n> >> > void CamHelper::Prepare(const Span &buffer,\n> >> > Metadata &metadata, ControlList &SensorControls)\n> >> > {\n> >> > bool success = false;\n> >> >\n> >> > if (buffer.size())\n> >> > success = parseEmbeddedData(buffer, metadata);\n> >> >\n> >> > if (!success) {\n> >> > struct DeviceStatus deviceStatus = {};\n> >> > int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> >> > int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >> >\n> >> > deviceStatus.shutter_speed = Exposure(exposureLines);\n> >> > deviceStatus.analogue_gain = Gain(gainCode);\n> >> > metadata.Set(\"device.status\", deviceStatus);\n> >> > }\n> >> > }\n> >> >\n> >> > What do you think? Might be a bit neater to have deviecStatus set in\n> one place only.\n> >>\n> >> Interesting point, and in fact, my previous version of this patch\n> >> (labelled as \"RFC\") did exactly this. Obviously I back-tracked on\n> >> that, for the following reasons.\n> >>\n> >> The version you've outlined above is nice because it makes the IPA\n> >> file (raspberrypi.cpp) a bit tidier, functions like\n> >> CamHelper::Exposure and CamHelper::Gain only need to be called once.\n> >> The DeviceStatus only gets set in one place.\n> >>\n> >> The version I went for in the end has the advantage that it makes\n> >> CamHelper::Prepare responsible only for parsing sensor metadata.\n> >> There's nothing it has to remember to do if, for example, some\n> >> particular metadata isn't there.\n> >>\n> >> This means that the CamHelper::Prepare for my mysterious new sensor\n> >> doesn't have to do anything extra. It just parses the sensor's\n> >> statistics buffer and nothing else. As this kind of code - CamHelpers\n> >> - is likely to get duplicated more in future, potentially by 3rd\n> >> parties, I was keener to avoid little \"gotchas\" in there, and slight\n> >> uglification of raspberrypi.cpp seemed reasonable in return.\n> >>\n> >> But it's a close call and I'm definitely swayable...\n> >\n> >\n> > Perhaps it's a bit premature to think about this, but what if some stuff\n> running in\n> > prepare() (e.g. fancy phase detect AF) needs the exposure and gain\n> values used\n> > for the sensor, and we don't have embedded data to give us these\n> values. You could\n> > possibly run this fancy AF in process() where you do get the values, but\n> then you add\n> > a frame's worth of latency.\n> >\n> > It also possibly makes the code a bit cleaner for an up-coming change\n> for imx477 ultra\n> > long exposures where I have to disable parsing when the exposure factor\n> is set. But\n> > again, I have not written code for this yet to tell for sure.\n>\n> That's an interesting thought. Generally I'm quite strongly opposed to\n> \"algorithm\" code creeping into the helpers, the helpers should be\n> restricted to the minimum \"parsing\" duties. To take the AF example,\n> CamHelper::Prepare would put the PDAF information into some kind of\n> \"pdaf\" entry in our RPi metadata. Hopefully many sensors would be able\n> to re-use this \"pdaf\" metadata. Then there'd be a bona fide \"pdaf\"\n> algorithm, loaded as usual via the json file, that would interpret it.\n>\n\nYes I agree that algorithm stuff should end up in the Controller, so maybe\nthis is not something we deal with.\n\n\n>\n> On the subject of long exposures, how can you tell whether you're in\n> the long exposure case - are the values enough?\n\n\nUnfortunately not. We have to use the fact that the helper (specifically\nGetVBlanking) will setup the long exposure factor based on the requested\nexposure time. Once this is done, we store state in the CamHelper to\nabort parsing until long exposure is turned off.\n\n\n> I'm thinking about\n> always reading the exposure/gain from the controls and filling in the\n> DeviceStatus *before* we call CamHelper::Prepare. That way you have\n> the values, and you can overwrite them if you know better. Would that\n> be helpful to the imx477?\n>\n\nProbably won't make much difference to imx477, but this might be a bit\nneater\nthan we have it now. I think we should make this change!\n\nRegards,\nNaush\n\n\n\n>\n> Thanks!\n> David\n>\n> >\n> > I'll leave it to you to choose :-)\n> >\n> > Naush\n> >\n> >>\n> >>\n> >> > With or without this change and the fixup for the namespace scoping\n> above:\n> >> >\n> >> > Reviewed-by: Naushir Patuck \n> >>\n> >> Thanks!\n> >>\n> >> David\n> >>\n> >> >\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,34 @@ unsigned int\n> CamHelper::MistrustFramesModeSwitch() const\n> >> >> return 0;\n> >> >> }\n> >> >>\n> >> >> +void CamHelper::parseEmbeddedData(const Span &buffer,\n> >> >> + Metadata &metadata)\n> >> >>\n> >> >> +{\n> >> >> + if (buffer.size()) {\n> >> >> + struct DeviceStatus deviceStatus = {};\n> >> >> + bool success = false;\n> >> >> + uint32_t exposureLines, gainCode;\n> >> >> +\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> >> >> + deviceStatus.shutter_speed =\n> Exposure(exposureLines);\n> >> >> + deviceStatus.analogue_gain = Gain(gainCode);\n> >> >> +\n> >> >> + LOG(IPARPI, Debug) << \"Metadata - Exposure :\n> \"\n> >> >> + <<\n> deviceStatus.shutter_speed\n> >> >> + << \" Gain : \"\n> >> >> + <<\n> deviceStatus.analogue_gain;\n> >> >> +\n> >> >> + metadata.Set(\"device.status\", deviceStatus);\n> >> >> + } else\n> >> >> + LOG(IPARPI, Error) << \"Embedded buffer\n> parsing failed\";\n> >> >> + }\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..930ea39a 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> >> >> CamHelper(MdParser *parser, unsigned int\n> 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> >> >> + Metadata &metadata);\n> >> >> + virtual void Process(StatisticsPtr &stats, Metadata\n> &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 +87,9 @@ public:\n> >> >> virtual unsigned int MistrustFramesModeSwitch() const;\n> >> >>\n> >> >> protected:\n> >> >> + void parseEmbeddedData(const libcamera::Span\n> &buffer,\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..e08b47c0 100644\n> >> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> >> @@ -103,8 +103,7 @@ 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\n> DeviceStatus &deviceStatus);\n> >> >> - void fillDeviceStatus(uint32_t exposureLines, uint32_t\n> gainCode,\n> >> >> + void fillDeviceStatus(const ControlList &sensorControls,\n> >> >> struct DeviceStatus &deviceStatus);\n> >> >> void processStats(unsigned int bufferId);\n> >> >> void applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration);\n> >> >> @@ -913,35 +912,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> >> >> +\n> >> >> + rpiMetadata_.Clear();\n> >> >>\n> >> >> if (data.embeddedBufferPresent) {\n> >> >> /*\n> >> >> * Pipeline handler has supplied us with an embedded\n> data 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\n> handler asap. */\n> >> >> - returnEmbeddedBuffer(data.embeddedBufferId);\n> >> >> + auto it = buffers_.find(data.embeddedBufferId);\n> >> >> + ASSERT(it != buffers_.end());\n> >> >> + embeddedBuffer = it->second.maps()[0];\n> >> >> }\n> >> >>\n> >> >> - if (!success) {\n> >> >> - /*\n> >> >> - * Pipeline handler has not supplied an embedded\n> data buffer,\n> >> >> - * or embedded data buffer parsing has failed for\n> some reason,\n> >> >> - * so pull the exposure and gain values from the\n> control 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,\n> deviceStatus);\n> >> >> - }\n> >> >> + /*\n> >> >> + * This may add the DeviceStatus to the metadata, and\n> depending on the\n> >> >> + * sensor, may do additional custom processing.\n> >> >> + */\n> >> >> + helper_->Prepare(embeddedBuffer, rpiMetadata_);\n> >> >> +\n> >> >> + /* Add DeviceStatus metadata if helper_->Prepare() didn't. */\n> >> >> + DeviceStatus deviceStatus = {};\n> >> >> + if (rpiMetadata_.Get(\"device.status\", deviceStatus))\n> >> >> + fillDeviceStatus(data.controls, deviceStatus);\n> >> >> +\n> >> >> + /* Done with embedded data now, return to pipeline handler\n> asap. */\n> >> >> + if (data.embeddedBufferPresent)\n> >> >> + returnEmbeddedBuffer(data.embeddedBufferId);\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> */\n> >> >> @@ -991,41 +992,12 @@ void IPARPi::prepareISP(const\n> ipa::RPi::ISPConfig &data)\n> >> >> setIspControls.emit(ctrls);\n> >> >> }\n> >> >>\n> >> >> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct\n> DeviceStatus &deviceStatus)\n> >> >> -{\n> >> >> - auto it = buffers_.find(bufferId);\n> >> >> - if (it == buffers_.end()) {\n> >> >> - LOG(IPARPI, Error) << \"Could not find embedded\n> 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\n> failed, error \" << status;\n> >> >> - return false;\n> >> >> - } else {\n> >> >> - uint32_t exposureLines, gainCode;\n> >> >> - if\n> (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,\n> deviceStatus);\n> >> >> - }\n> >> >> -\n> >> >> - return true;\n> >> >> -}\n> >> >> -\n> >> >> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t\n> gainCode,\n> >> >> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls,\n> >> >> struct DeviceStatus &deviceStatus)\n> >> >> {\n> >> >> + int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> >> >> + int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >> >> +\n> >> >> deviceStatus.shutter_speed =\n> helper_->Exposure(exposureLines);\n> >> >> deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> >> >>\n> >> >> @@ -1033,6 +1005,8 @@ void IPARPi::fillDeviceStatus(uint32_t\n> exposureLines, uint32_t gainCode,\n> >> >> << deviceStatus.shutter_speed\n> >> >> << \" Gain : \"\n> >> >> << deviceStatus.analogue_gain;\n> >> >> +\n> >> >> + rpiMetadata_.Set(\"device.status\", deviceStatus);\n> >> >> }\n> >> >>\n> >> >> void IPARPi::processStats(unsigned int bufferId)\n> >> >> @@ -1046,6 +1020,7 @@ void IPARPi::processStats(unsigned int\n> bufferId)\n> >> >> Span mem = it->second.maps()[0];\n> >> >> bcm2835_isp_stats *stats =\n> 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 AB423BDC66\n\tfor ;\n\tFri, 26 Mar 2021 11:15:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4FED68D6A;\n\tFri, 26 Mar 2021 12:15:14 +0100 (CET)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CB85602D7\n\tfor ;\n\tFri, 26 Mar 2021 12:15:13 +0100 (CET)","by mail-lf1-x12e.google.com with SMTP id w37so7058278lfu.13\n\tfor ;\n\tFri, 26 Mar 2021 04:15:13 -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=\"nsZrQfqU\"; 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=0eg9yE1STdkzGIdYnftTTkBH3FOb9itUgy3o4B7+NOw=;\n\tb=nsZrQfqUu2v+JYb8rnnQR52Ep9CfuNO0TXEYoOBNHXDLwHeRk/i/qFikvmkNby9nUc\n\taoFJOGWCYuWj4lli4jLDe3DWRtgYvBmukEM3xsWMryLRRGtQ+yVPD6NUc4ahOK1+zBG3\n\tudyW5f+G3uyBb5xyAxrDr95Ip0YZHnLm49yPUpIhnAaBDS+OCHgPvAlDZkibQOjZVWfb\n\t6ins+8G76j0vR2+1BSkcW0N9ugOppbgrKCcdTt1889VU8LoiuofDTA/KM4D7WFttHR2S\n\tFyfQrd5fZCGMeQadlkz3hl+yYfILpPqyNNJ670W62zbQ7WLTKTioU5QUXFzmoQE1prKp\n\t/PoQ==","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=0eg9yE1STdkzGIdYnftTTkBH3FOb9itUgy3o4B7+NOw=;\n\tb=YT3dw/zPgzn5Q81hnC7P9qB8h0EZwNBtxFAx0v29RlSKmmSGmrKztzrtZWnlewGyMo\n\tm+fRfN773CKgo6o18co4YpYPd6GiV3Rd1aUeknpj2Knj/ZTR1m0yvPJFd/Y4Naof70c3\n\tmUTw7uWvV2SZznLZJR1OadQjFlFVWoJa0aHnMhCTTzhCA9Ejr9pEmKTg4gZsJD/VEmNE\n\t6Qh95ZM86oRgUCMBowqyc4qt4vaajF2rgxBxkcF6LmYBGpwc9P1yKsblpggX+m6HfOix\n\tsDh7vp1BtBhZpmvQvxH2GQzrMk88De3ssA95aySLGSgch6fvpjbZLywYNJ9Jm/Fgjyh4\n\tC0Rg==","X-Gm-Message-State":"AOAM532pfWYYNc1iu8Ypn48+kdcaEZnfx/K3IHk2UtmJXsU/XLpq/J62\n\tXVi3lDoqvfVQFIKvUoLaDgk+i27GlZhXOF+743/lNLkC9bgftg==","X-Google-Smtp-Source":"ABdhPJwaCWkjCV/YZUVTtC5uqHuNkUTdv6xt9RQmvYpooMEiOPIibBPzZmzaD58gfxd5wiwLFlxey1cavQfLTEiqLK8=","X-Received":"by 2002:a19:4116:: with SMTP id\n\to22mr7995861lfa.272.1616757312666; \n\tFri, 26 Mar 2021 04:15:12 -0700 (PDT)","MIME-Version":"1.0","References":"<20210322160148.32291-1-david.plowman@raspberrypi.com>\n\t<20210322160148.32291-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 26 Mar 2021 11:14:56 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata 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=\"===============6224730146736824018==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15945,"web_url":"https://patchwork.libcamera.org/comment/15945/","msgid":"","date":"2021-03-26T13:28:38","subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata parsing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David and Naush,\n\nOn Fri, Mar 26, 2021 at 11:14:56AM +0000, Naushir Patuck wrote:\n> On Fri, 26 Mar 2021 at 11:07, David Plowman wrote:\n> > On Fri, 26 Mar 2021 at 09:57, Naushir Patuck wrote:\n> >> On Fri, 26 Mar 2021 at 09:32, David Plowman wrote:\n> >>> On Thu, 25 Mar 2021 at 16:08, Naushir Patuck wrote:\n> >>>> On Mon, 22 Mar 2021 at 16:01, David Plowman wrote:\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 only needs to call the new Prepare() and Process()\n> >>>>> methods, and to supply exposure/gain values from the controls when the\n> >>>>> CamHelper does not.\n> >>>>>\n> >>>>> Signed-off-by: David Plowman \n> >>>>> ---\n> >>>>> src/ipa/raspberrypi/cam_helper.cpp | 44 +++++++++++++++\n> >>>>> src/ipa/raspberrypi/cam_helper.hpp | 11 +++-\n> >>>>> src/ipa/raspberrypi/raspberrypi.cpp | 83 ++++++++++-------------------\n> >>>>> 3 files changed, 83 insertions(+), 55 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> >>>>> index 0ae0baa0..ce6482ba 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> >>>> I don't think this namespace scope is needed. Seems to compile fine without.\n> >>>\n> >>> Funny one, this. It builds like that for me too but when I run it (for\n> >>> example, qcam), it bombs out with\n> >>>\n> >>> build/src/qcam/qcam: symbol lookup error:\n> >>> /home/pi/libcamera/build/src/ipa/raspberrypi/ipa_rpi.so: undefined\n> >>> symbol: _Z17logCategoryIPARPIv\n> >>>\n> >>> It seems to be looking for logCategory::IPARPI when I think it should\n> >>> be libcamera::logCategory::IPARPI. I guess I'm not clear why the\n> >>> linker doesn't flag that up... anyone know?\n\nThat's an interesting one, haven't seen it before. I would also have\nexpected the linker to complain.\n\nThis causes the linker to complain:\n\ndiff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build\nindex d1397a3211f0..687b1f5d6412 100644\n--- a/src/ipa/raspberrypi/meson.build\n+++ b/src/ipa/raspberrypi/meson.build\n@@ -46,6 +46,7 @@ mod = shared_module(ipa_name,\n name_prefix : '',\n include_directories : rpi_ipa_includes,\n dependencies : rpi_ipa_deps,\n+ link_args : ['-Wl,--no-undefined'],\n link_with : libipa,\n install : true,\n install_dir : ipa_install_dir)\n\nI haven't investigated whether this is something we should enable\nglobally though, and why the default is to allow undefined symbols.\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> >>>>> + Metadata &metadata)\n> >>>>>\n> >>>>> +{\n> >>>>> + parseEmbeddedData(buffer, metadata);\n> >>>>> +}\n> >>>>\n> >>>> Would it be useful to pass in the ControlList that contains the sensor setup\n> >>>> (from DelayedControls) into this function? Would the CamHelper ever want\n> >>>> to know these things?\n> >>>>\n> >>>> If the answer is yes, you could also pull in the case that deals with non-embedded\n> >>>> data where we pull values from the ControlList. So something like:\n> >>>>\n> >>>> void CamHelper::Prepare(const Span &buffer,\n> >>>> Metadata &metadata, ControlList &SensorControls)\n> >>>> {\n> >>>> bool success = false;\n> >>>>\n> >>>> if (buffer.size())\n> >>>> success = parseEmbeddedData(buffer, metadata);\n> >>>>\n> >>>> if (!success) {\n> >>>> struct DeviceStatus deviceStatus = {};\n> >>>> int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> >>>> int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >>>>\n> >>>> deviceStatus.shutter_speed = Exposure(exposureLines);\n> >>>> deviceStatus.analogue_gain = Gain(gainCode);\n> >>>> metadata.Set(\"device.status\", deviceStatus);\n> >>>> }\n> >>>> }\n> >>>>\n> >>>> What do you think? Might be a bit neater to have deviecStatus set in one place only.\n> >>>\n> >>> Interesting point, and in fact, my previous version of this patch\n> >>> (labelled as \"RFC\") did exactly this. Obviously I back-tracked on\n> >>> that, for the following reasons.\n> >>>\n> >>> The version you've outlined above is nice because it makes the IPA\n> >>> file (raspberrypi.cpp) a bit tidier, functions like\n> >>> CamHelper::Exposure and CamHelper::Gain only need to be called once.\n> >>> The DeviceStatus only gets set in one place.\n> >>>\n> >>> The version I went for in the end has the advantage that it makes\n> >>> CamHelper::Prepare responsible only for parsing sensor metadata.\n> >>> There's nothing it has to remember to do if, for example, some\n> >>> particular metadata isn't there.\n> >>>\n> >>> This means that the CamHelper::Prepare for my mysterious new sensor\n> >>> doesn't have to do anything extra. It just parses the sensor's\n> >>> statistics buffer and nothing else. As this kind of code - CamHelpers\n> >>> - is likely to get duplicated more in future, potentially by 3rd\n> >>> parties, I was keener to avoid little \"gotchas\" in there, and slight\n> >>> uglification of raspberrypi.cpp seemed reasonable in return.\n> >>>\n> >>> But it's a close call and I'm definitely swayable...\n> >>\n> >> Perhaps it's a bit premature to think about this, but what if some\n> >> stuff running in prepare() (e.g. fancy phase detect AF) needs the\n> >> exposure and gain values used for the sensor, and we don't have\n> >> embedded data to give us these values. You could possibly run this\n> >> fancy AF in process() where you do get the values, but then you add\n> >> a frame's worth of latency.\n> >>\n> >> It also possibly makes the code a bit cleaner for an up-coming\n> >> change for imx477 ultra long exposures where I have to disable\n> >> parsing when the exposure factor is set. But again, I have not\n> >> written code for this yet to tell for sure.\n> >\n> > That's an interesting thought. Generally I'm quite strongly opposed\n> > to \"algorithm\" code creeping into the helpers, the helpers should be\n> > restricted to the minimum \"parsing\" duties. To take the AF example,\n> > CamHelper::Prepare would put the PDAF information into some kind of\n> > \"pdaf\" entry in our RPi metadata. Hopefully many sensors would be\n> > able to re-use this \"pdaf\" metadata. Then there'd be a bona fide\n> > \"pdaf\" algorithm, loaded as usual via the json file, that would\n> > interpret it.\n> \n> Yes I agree that algorithm stuff should end up in the Controller, so maybe\n> this is not something we deal with.\n\nI agree too, especially given that sensor helpers should be moved to\nlibipa.\n\n> > On the subject of long exposures, how can you tell whether you're in\n> > the long exposure case - are the values enough?\n> \n> Unfortunately not. We have to use the fact that the helper (specifically\n> GetVBlanking) will setup the long exposure factor based on the requested\n> exposure time. Once this is done, we store state in the CamHelper to\n> abort parsing until long exposure is turned off.\n> \n> I'm thinking about always reading the exposure/gain from the controls\n> and filling in the DeviceStatus *before* we call CamHelper::Prepare.\n> That way you have the values, and you can overwrite them if you know\n> better. Would that be helpful to the imx477?\n> \n> Probably won't make much difference to imx477, but this might be a bit\n> neater than we have it now. I think we should make this change!\n> \n> >> I'll leave it to you to choose :-)\n> >>\n> >>>> With or without this change and the fixup for the namespace scoping above:\n> >>>>\n> >>>> Reviewed-by: Naushir Patuck \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,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n> >>>>> return 0;\n> >>>>> }\n> >>>>>\n> >>>>> +void CamHelper::parseEmbeddedData(const Span &buffer,\n> >>>>> + Metadata &metadata)\n> >>>>>\n> >>>>> +{\n> >>>>> + if (buffer.size()) {\n> >>>>> + struct DeviceStatus deviceStatus = {};\n> >>>>> + bool success = false;\n> >>>>> + uint32_t exposureLines, gainCode;\n> >>>>> +\n> >>>>> + parser_->SetBufferSize(buffer.size());\n> >>>>> + success = parser_->Parse(buffer.data()) == MdParser::Status::OK &&\n> >>>>> + parser_->GetExposureLines(exposureLines) == MdParser::Status::OK &&\n> >>>>> + parser_->GetGainCode(gainCode) == MdParser::Status::OK;\n> >>>>> +\n> >>>>> + if (success) {\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> >>>>> + } else\n> >>>>> + LOG(IPARPI, Error) << \"Embedded buffer parsing failed\";\n> >>>>> + }\n> >>>>> +}\n> >>>>> +\n> >>>>> RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> >>>>> CamHelperCreateFunc create_func)\n> >>>>> {\n> >>>>> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> >>>>> index 1b2d6eec..930ea39a 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> >>>>> 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> >>>>> + 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 minFrameDuration,\n> >>>>> @@ -81,6 +87,9 @@ public:\n> >>>>> virtual unsigned int MistrustFramesModeSwitch() const;\n> >>>>>\n> >>>>> protected:\n> >>>>> + void parseEmbeddedData(const libcamera::Span &buffer,\n> >>>>> + Metadata &metadata);\n> >>>>> +\n> >>>>> MdParser *parser_;\n> >>>>> CameraMode mode_;\n> >>>>>\n> >>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>> index 7904225a..e08b47c0 100644\n> >>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>> @@ -103,8 +103,7 @@ 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 &deviceStatus);\n> >>>>> - void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> >>>>> + void fillDeviceStatus(const ControlList &sensorControls,\n> >>>>> struct DeviceStatus &deviceStatus);\n> >>>>> void processStats(unsigned int bufferId);\n> >>>>> void applyFrameDurations(double minFrameDuration, double maxFrameDuration);\n> >>>>> @@ -913,35 +912,37 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)\n> >>>>>\n> >>>>> void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> >>>>> {\n> >>>>> - struct DeviceStatus deviceStatus = {};\n> >>>>> - bool success = false;\n> >>>>> + Span embeddedBuffer;\n> >>>>> +\n> >>>>> + rpiMetadata_.Clear();\n> >>>>>\n> >>>>> if (data.embeddedBufferPresent) {\n> >>>>> /*\n> >>>>> * Pipeline handler has supplied us with an embedded data 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, deviceStatus);\n> >>>>> -\n> >>>>> - /* Done with embedded data now, return to pipeline handler asap. */\n> >>>>> - returnEmbeddedBuffer(data.embeddedBufferId);\n> >>>>> + auto it = buffers_.find(data.embeddedBufferId);\n> >>>>> + ASSERT(it != buffers_.end());\n> >>>>> + embeddedBuffer = it->second.maps()[0];\n> >>>>> }\n> >>>>>\n> >>>>> - if (!success) {\n> >>>>> - /*\n> >>>>> - * Pipeline handler has not supplied an embedded data buffer,\n> >>>>> - * or embedded data buffer parsing has failed for some reason,\n> >>>>> - * so pull the exposure and gain values from the control list.\n> >>>>> - */\n> >>>>> - int32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get();\n> >>>>> - int32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >>>>> - fillDeviceStatus(exposureLines, gainCode, deviceStatus);\n> >>>>> - }\n> >>>>> + /*\n> >>>>> + * This may add the DeviceStatus to the metadata, and depending on the\n> >>>>> + * sensor, may do additional custom processing.\n> >>>>> + */\n> >>>>> + helper_->Prepare(embeddedBuffer, rpiMetadata_);\n> >>>>> +\n> >>>>> + /* Add DeviceStatus metadata if helper_->Prepare() didn't. */\n> >>>>> + DeviceStatus deviceStatus = {};\n> >>>>> + if (rpiMetadata_.Get(\"device.status\", deviceStatus))\n> >>>>> + fillDeviceStatus(data.controls, deviceStatus);\n> >>>>> +\n> >>>>> + /* Done with embedded data now, return to pipeline handler asap. */\n> >>>>> + if (data.embeddedBufferPresent)\n> >>>>> + returnEmbeddedBuffer(data.embeddedBufferId);\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,41 +992,12 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)\n> >>>>> setIspControls.emit(ctrls);\n> >>>>> }\n> >>>>>\n> >>>>> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &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 = helper_->Parser().Parse(mem.data());\n> >>>>> - if (status != RPiController::MdParser::Status::OK) {\n> >>>>> - LOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n> >>>>> - return false;\n> >>>>> - } else {\n> >>>>> - uint32_t exposureLines, gainCode;\n> >>>>> - if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n> >>>>> - LOG(IPARPI, Error) << \"Exposure time failed\";\n> >>>>> - return false;\n> >>>>> - }\n> >>>>> -\n> >>>>> - if (helper_->Parser().GetGainCode(gainCode) != 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> >>>>> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls,\n> >>>>> struct DeviceStatus &deviceStatus)\n> >>>>> {\n> >>>>> + int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> >>>>> + int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >>>>> +\n> >>>>> deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> >>>>> deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> >>>>>\n> >>>>> @@ -1033,6 +1005,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,\n> >>>>> << deviceStatus.shutter_speed\n> >>>>> << \" Gain : \"\n> >>>>> << deviceStatus.analogue_gain;\n> >>>>> +\n> >>>>> + rpiMetadata_.Set(\"device.status\", deviceStatus);\n> >>>>> }\n> >>>>>\n> >>>>> void IPARPi::processStats(unsigned int bufferId)\n> >>>>> @@ -1046,6 +1020,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 = std::make_shared(*stats);\n> >>>>> + helper_->Process(statistics, rpiMetadata_);\n> >>>>> controller_.Process(statistics, &rpiMetadata_);\n> >>>>>\n> >>>>> struct 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 9629DC32EB\n\tfor ;\n\tFri, 26 Mar 2021 13:29:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EBC5868D6A;\n\tFri, 26 Mar 2021 14:29:23 +0100 (CET)","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 8C610602D7\n\tfor ;\n\tFri, 26 Mar 2021 14:29:22 +0100 (CET)","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 DE5AB443;\n\tFri, 26 Mar 2021 14:29:21 +0100 (CET)"],"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=\"lFaJ9BAx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616765362;\n\tbh=ByWHCvX5k02ez9JanE+uIiPvTeAQL4N7AqA8rAzhkW4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lFaJ9BAxJPrq3yrd96GxZ+HHdvrT9j6CuzDcEDTc6ib67WrGSTKnvx68OsktksyLQ\n\t5DWVXgS+n9VkFt6FW6NZZ0wyEqcmvBHZO5aNSgiwd8636BolW64lBPMGj42S+H+7y3\n\t25Vl7Eu5d/ZNp2I8XBqD2KQwp9I/ykVJGyyGf4YY=","Date":"Fri, 26 Mar 2021 15:28:38 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210322160148.32291-1-david.plowman@raspberrypi.com>\n\t<20210322160148.32291-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] ipa: raspberrypi: Use CamHelpers\n\tto generalise sensor metadata 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":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]