[{"id":27668,"web_url":"https://patchwork.libcamera.org/comment/27668/","msgid":"","date":"2023-08-21T08:56:18","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: rpi: Add an HDR algorithm to\n\tdrive multi-channel AGC","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 this patch.\nOnly minor comments below.\n\nReviewed-by: Naushir Patuck \n\nOn Mon, 31 Jul 2023 at 14:48, David Plowman via libcamera-devel\n wrote:\n>\n> This HDR algorithm doesn't combine images in any way, but simply\n> allows an application to drive the AGC in a multi-channel HDR type of\n> mode, such as to produce short and long exposure images.\n>\n> Sufficient plumbing is added to enable the libcamera HDR controls and\n> metadata to work.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 49 ++++++++++\n> src/ipa/rpi/controller/hdr_algorithm.h | 23 +++++\n> src/ipa/rpi/controller/hdr_status.h | 25 +++++\n> src/ipa/rpi/controller/meson.build | 1 +\n> src/ipa/rpi/controller/rpi/hdr.cpp | 129 +++++++++++++++++++++++++\n> src/ipa/rpi/controller/rpi/hdr.h | 42 ++++++++\n> 6 files changed, 269 insertions(+)\n> create mode 100644 src/ipa/rpi/controller/hdr_algorithm.h\n> create mode 100644 src/ipa/rpi/controller/hdr_status.h\n> create mode 100644 src/ipa/rpi/controller/rpi/hdr.cpp\n> create mode 100644 src/ipa/rpi/controller/rpi/hdr.h\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index f29c32fd..956b1ca0 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -24,6 +24,8 @@\n> #include \"controller/ccm_status.h\"\n> #include \"controller/contrast_algorithm.h\"\n> #include \"controller/denoise_algorithm.h\"\n> +#include \"controller/hdr_algorithm.h\"\n> +#include \"controller/hdr_status.h\"\n> #include \"controller/lux_status.h\"\n> #include \"controller/sharpen_algorithm.h\"\n> #include \"controller/statistics.h\"\n> @@ -63,6 +65,7 @@ const ControlInfoMap::Map ipaControls{\n> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> + { &controls::HdrMode, ControlInfo(controls::HdrModeValues) },\n> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> @@ -651,9 +654,17 @@ static const std::map AfPauseTable\n> { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },\n> };\n>\n> +static const std::map HdrModeTable = {\n\nNo need for static here, it's in the anonymous namespace. Having said that,\nperhaps we want to move HdrModeTable down further in the file where the other\nlibcamera controls -> RPi strings happen?\n\n> + { controls::HdrModeOff, \"Off\" },\n> + { controls::HdrModeMultiExposure, \"MultiExposure\" },\n> + { controls::HdrModeSingleExposure, \"SingleExposure\" },\n> +};\n> +\n> void IpaBase::applyControls(const ControlList &controls)\n> {\n> + using RPiController::AgcAlgorithm;\n> using RPiController::AfAlgorithm;\n> + using RPiController::HdrAlgorithm;\n>\n> /* Clear the return metadata buffer. */\n> libcameraMetadata_.clear();\n> @@ -1092,6 +1103,34 @@ void IpaBase::applyControls(const ControlList &controls)\n> break;\n> }\n>\n> + case controls::HDR_MODE: {\n> + HdrAlgorithm *hdr = dynamic_cast(controller_.getAlgorithm(\"hdr\"));\n> + if (!hdr) {\n> + LOG(IPARPI, Warning) << \"No HDR algorithm available\";\n> + break;\n> + }\n> +\n> + auto mode = HdrModeTable.find(ctrl.second.get());\n> + if (mode == HdrModeTable.end()) {\n> + LOG(IPARPI, Warning) << \"Unrecognised HDR mode\";\n> + break;\n> + }\n> +\n> + AgcAlgorithm *agc = dynamic_cast(controller_.getAlgorithm(\"agc\"));\n> + if (!agc) {\n> + LOG(IPARPI, Warning) << \"HDR requires an AGC algorithm\";\n> + break;\n> + }\n> +\n> + if (hdr->setMode(mode->second) == 0)\n> + agc->setActiveChannels(hdr->getChannels());\n> + else\n> + LOG(IPARPI, Warning)\n> + << \"HDR mode \" << mode->second << \" not supported\";\n> +\n> + break;\n> + }\n> +\n> default:\n> LOG(IPARPI, Warning)\n> << \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> @@ -1239,6 +1278,16 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n> libcameraMetadata_.set(controls::AfPauseState, p);\n> }\n>\n> + const HdrStatus *hdrStatus = rpiMetadata.getLocked(\"hdr.status\");\n> + if (hdrStatus) {\n> + if (hdrStatus->channel == \"short\")\n> + libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelShort);\n> + else if (hdrStatus->channel == \"long\")\n> + libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelLong);\n> + else\n> + libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelNone);\n> + }\n> +\n> metadataReady.emit(libcameraMetadata_);\n> }\n>\n> diff --git a/src/ipa/rpi/controller/hdr_algorithm.h b/src/ipa/rpi/controller/hdr_algorithm.h\n> new file mode 100644\n> index 00000000..5164d50e\n> --- /dev/null\n> +++ b/src/ipa/rpi/controller/hdr_algorithm.h\n> @@ -0,0 +1,23 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2023, Raspberry Pi Ltd\n> + *\n> + * hdr_algorithm.h - HDR control algorithm interface\n> + */\n> +#pragma once\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace RPiController {\n> +\n> +class HdrAlgorithm : public Algorithm\n> +{\n> +public:\n> + HdrAlgorithm(Controller *controller)\n> + : Algorithm(controller) {}\n> + /* An HDR algorithm must provide the following: */\n> + virtual int setMode(std::string const &modeName) = 0;\n> + virtual std::vector getChannels() const = 0;\n> +};\n> +\n> +} /* namespace RPiController */\n> diff --git a/src/ipa/rpi/controller/hdr_status.h b/src/ipa/rpi/controller/hdr_status.h\n> new file mode 100644\n> index 00000000..26b538a0\n> --- /dev/null\n> +++ b/src/ipa/rpi/controller/hdr_status.h\n> @@ -0,0 +1,25 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2022 Raspberry Pi Ltd\n\ns/2022/2023/\n\n> + *\n> + * hdr_status.h - HDR control algorithm status\n> + */\n> +#pragma once\n> +\n> +#include \n> +\n> +/*\n> + * The HDR algorithm process method should post an HdrStatus into the image\n> + * metadata under the tag \"hdr.status\".\n> + */\n> +\n> +struct HdrStatus {\n> + std::string mode;\n> + std::string channel;\n> +\n> + /* Parameters for programming the stitch block (where available). */\n> + bool stitch_enable;\n> + uint16_t thresholdLo;\n> + uint8_t diffPower;\n> + double motionThreshold;\n> +};\n> diff --git a/src/ipa/rpi/controller/meson.build b/src/ipa/rpi/controller/meson.build\n> index 20b9cda9..c9a3e850 100644\n> --- a/src/ipa/rpi/controller/meson.build\n> +++ b/src/ipa/rpi/controller/meson.build\n> @@ -16,6 +16,7 @@ rpi_ipa_controller_sources = files([\n> 'rpi/contrast.cpp',\n> 'rpi/dpc.cpp',\n> 'rpi/geq.cpp',\n> + 'rpi/hdr.cpp',\n> 'rpi/lux.cpp',\n> 'rpi/noise.cpp',\n> 'rpi/sdn.cpp',\n> diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp\n> new file mode 100644\n> index 00000000..2587f622\n> --- /dev/null\n> +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> @@ -0,0 +1,129 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2022 Raspberry Pi Ltd\n> + *\n> + * hdr.cpp - HDR control algorithm\n> + */\n> +\n> +#include \"hdr.h\"\n> +\n> +#include \n> +\n> +#include \"../agc_status.h\"\n> +\n> +using namespace RPiController;\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(RPiHdr)\n> +\n> +#define NAME \"rpi.hdr\"\n> +\n> +void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &modeName)\n> +{\n> + name = modeName;\n> +\n> + if (!params.contains(\"cadence\"))\n> + LOG(RPiHdr, Fatal) << \"No cadence for HDR mode \" << name;\n> + cadence = params[\"cadence\"].getList().value();\n> + if (cadence.empty())\n> + LOG(RPiHdr, Fatal) << \"Empty cadence in HDR mode \" << name;\n> +\n> + /*\n> + * In the JSON file it's easier to use the channel name as the key, but\n> + * for us it's convenient to swap them over.\n> + */\n> + for (const auto &[k, v] : params[\"channel_map\"].asDict())\n> + channelMap[v.get().value()] = k;\n> +}\n> +\n> +Hdr::Hdr(Controller *controller)\n> + : HdrAlgorithm(controller),\n> + currentConfig_(nullptr)\n> +{\n> +}\n> +\n> +char const *Hdr::name() const\n> +{\n> + return NAME;\n> +}\n> +\n> +int Hdr::read(const libcamera::YamlObject ¶ms)\n> +{\n> + for (const auto &[key, value] : params.asDict()) {\n> + HdrConfig hdrConfig;\n> + hdrConfig.read(value, key);\n> + config_[key] = std::move(hdrConfig);\n> + if (!currentConfig_)\n> + currentConfig_ = &config_[key];\n> + }\n> +\n> + if (!currentConfig_)\n> + LOG(RPiHdr, Fatal) << \"No HDR modes read\";\n> +\n> + return 0;\n> +}\n> +\n> +int Hdr::setMode(std::string const &mode)\n> +{\n> + auto it = config_.find(mode);\n> + if (it == config_.end()) {\n> + LOG(RPiHdr, Warning) << \"No such HDR mode \" << mode;\n> + return -1;\n> + }\n> +\n> + currentConfig_ = &it->second;\n> +\n> + return 0;\n> +}\n> +\n> +std::vector Hdr::getChannels() const\n> +{\n> + return currentConfig_->cadence;\n> +}\n> +\n> +void Hdr::switchMode([[maybe_unused]] CameraMode const &cameraMode,\n> + [[maybe_unused]] Metadata *metadata)\n> +{\n> +}\n> +\n> +void Hdr::process([[maybe_unused]] StatisticsPtr &stats, Metadata *imageMetadata)\n> +{\n> + if (currentConfig_->name == \"Off\")\n> + return;\n> +\n> + /* First find out what AGC channel this is, which comes from the delayed_status. */\n> + bool channelKnown = false;\n> + unsigned int agcChannel = 0;\n> + {\n> + std::scoped_lock lock(*imageMetadata);\n> + AgcStatus *agcStatus = imageMetadata->getLocked(\"agc.delayed_status\");\n> + if (agcStatus) {\n> + channelKnown = true;\n> + agcChannel = agcStatus->channel;\n> + }\n> + }\n> +\n> + /* Fill out the HdrStatus information. */\n> + HdrStatus hdrStatus;\n> + hdrStatus.mode = currentConfig_->name;\n> + if (channelKnown) {\n> + /*\n> + * Channels that aren't required for HDR processing might come through for\n> + * various reasons, such as when HDR starts up, or even because the cadence\n> + * demands some frames that you need for other purposes (e.g. for preview).\n> + * We'll leave the channel name empty in these cases.\n> + */\n> + auto it = currentConfig_->channelMap.find(agcChannel);\n> + if (it != currentConfig_->channelMap.end())\n> + hdrStatus.channel = it->second;\n> + }\n> +\n> + imageMetadata->set(\"hdr.status\", hdrStatus);\n> +}\n> +\n> +/* Register algorithm with the system. */\n> +static Algorithm *create(Controller *controller)\n> +{\n> + return (Algorithm *)new Hdr(controller);\n> +}\n> +static RegisterAlgorithm reg(NAME, &create);\n> diff --git a/src/ipa/rpi/controller/rpi/hdr.h b/src/ipa/rpi/controller/rpi/hdr.h\n> new file mode 100644\n> index 00000000..8f6457f2\n> --- /dev/null\n> +++ b/src/ipa/rpi/controller/rpi/hdr.h\n> @@ -0,0 +1,42 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2022, Raspberry Pi Ltd\n> + *\n> + * hdr.h - HDR control algorithm\n> + */\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"../hdr_algorithm.h\"\n> +#include \"../hdr_status.h\"\n> +\n> +/* This is our implementation of an HDR algorithm. */\n> +\n> +namespace RPiController {\n> +\n> +struct HdrConfig {\n> + std::string name;\n> + std::vector cadence;\n> + std::map channelMap;\n> +\n> + void read(const libcamera::YamlObject ¶ms, const std::string &name);\n> +};\n> +\n> +class Hdr : public HdrAlgorithm\n> +{\n> +public:\n> + Hdr(Controller *controller);\n> + char const *name() const override;\n> + void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;\n> + int read(const libcamera::YamlObject ¶ms) override;\n> + void process(StatisticsPtr &stats, Metadata *imageMetadata) override;\n> + int setMode(std::string const &mode) override;\n> + std::vector getChannels() const override;\n> +\n> +private:\n> + std::map config_;\n> + const HdrConfig *currentConfig_;\n> +};\n> +\n> +} /* namespace RPiController */\n> --\n> 2.30.2\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 08A16BF415\n\tfor ;\n\tMon, 21 Aug 2023 08:56:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 661BE627E0;\n\tMon, 21 Aug 2023 10:56:09 +0200 (CEST)","from mail-yw1-x1133.google.com (mail-yw1-x1133.google.com\n\t[IPv6:2607:f8b0:4864:20::1133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F1DB60379\n\tfor ;\n\tMon, 21 Aug 2023 10:56:08 +0200 (CEST)","by mail-yw1-x1133.google.com with SMTP id\n\t00721157ae682-58cd9d9dbf5so42743757b3.0\n\tfor ;\n\tMon, 21 Aug 2023 01:56:08 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692608169;\n\tbh=Mta0mk+5AcpYbn83zvponJJplWVu+Ep1cNnydearnas=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=sLH89U6Yfohr8V9jwc3IfY+u7vYJuuWfgKHlzqrd0QqCa4T18njnCq4Xc2bJHvLa8\n\txsZbKUCOiOrYQIeNDCzLb5740he1PfMqhgB9eNNECqnz9JNO608/ExHAV0Swf4n2ic\n\tt5TDVljA6JKWgHLt7cJNEb/UAA/Dvo8ONvB3cDQVAD8c2ZK1UTwBNUV4Xh8q/3LmwC\n\t349GXQLaHDIhsUi8/aXoU8e3l9k7LKA7tDpCwBtkfdLwRa3jUzrPwtGLriSQgzSClE\n\toLInrV9dguZNdJVO9VXKSbEtjH1+miC5xj13a1g1JklvZkYVw/gql8JjtWT+W0nhSE\n\t/wVwdj9bmZxLw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1692608167; x=1693212967;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=WzbVkV9QY4iw3ISAobnY83zlkNLdinHRrQp4EGIWB54=;\n\tb=EWSlswG8KTKvC4EfnAP0LKMkETRYj8PngU4mvbPGgBczvGOHKQ7hh2jTqKyIxCBxQr\n\tZwjgR3FlERtXYI25CLpzRe3dHZySO3fIcHaai+FrPXPhGw3j9V1qeeki7gQlxvpORKrj\n\t2/4uQJFsfgiSxH+x4lCvAN3ON29chPX689ddDSMjWJ8UBHfmJ/L9jALJ/QucAAH51ONP\n\tDRfoE/Xx6UYv7agPQz0SUJCSHVGhOw3lu7AU6DFJl7NMS+m6Zhzh8NypxDCQeZ4NzAk9\n\tOy/24cDwe84fV5bfLwyBzbVYPHbdpRw6xG6Viwf10p0q8TwIFIK5HDYiV+t+VenL5+cn\n\tjdTw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"EWSlswG8\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1692608167; x=1693212967;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=WzbVkV9QY4iw3ISAobnY83zlkNLdinHRrQp4EGIWB54=;\n\tb=X6W4OLl2sJnR0k90eN0I2D8C7wukzR16mGAH397eX2CIwzj4on2AIHdA2cBuRZDFRj\n\tjlFpwfGbClz70+cfxjrD0AbTxGPmfY0cfTx/401lN4iKwIi6+ClLANGDv9aOc5KwAaEy\n\th0BOnhuwPWtxBMdFIrT/JRE+SQy8JeJOXC7G/PBt6q9wUX1KBpufQtHenfSxmHgI6tXY\n\tF/p/Y/5osVFwZlqKcKOKJ0osQWu+2MoN30L8tXBznUb0T80o1Lnhnx1KEfK84WPIduIN\n\toSHIhmqW6kCBvZrilGsXSyvi3oMweyvGmEVxZuF/0tzwG4MZZ806rkcsYML9l/q8MdeW\n\tnK5g==","X-Gm-Message-State":"AOJu0Ywo98ZfJsCLUiB91oUz0TzawAa0b6UDujN+Sas5loN5oaxVRaa1\n\tymVVdzjZlfn8wnMTu08WbQVH+/JO5YscO8h6qF6vgHnQI1gjPXa4gfoxig==","X-Google-Smtp-Source":"AGHT+IFfaZyxdyEgZjsfoilS3Gjm+eE89qU/KgELpsF5G+zZR7YtyRFRCOvbmXT+5FPMQzJESysnPILW/QUT2jrjsW4=","X-Received":"by 2002:a0d:ea03:0:b0:589:e815:8d71 with SMTP id\n\tt3-20020a0dea03000000b00589e8158d71mr4058207ywe.11.1692608166897;\n\tMon, 21 Aug 2023 01:56:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20230731134833.79687-1-david.plowman@raspberrypi.com>\n\t<20230731134833.79687-2-david.plowman@raspberrypi.com>","In-Reply-To":"<20230731134833.79687-2-david.plowman@raspberrypi.com>","Date":"Mon, 21 Aug 2023 09:56:18 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: rpi: Add an HDR algorithm to\n\tdrive multi-channel AGC","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27671,"web_url":"https://patchwork.libcamera.org/comment/27671/","msgid":"","date":"2023-08-21T09:09:16","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: rpi: Add an HDR algorithm to\n\tdrive multi-channel AGC","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 review.\n\nOn Mon, 21 Aug 2023 at 09:56, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thank you for this patch.\n> Only minor comments below.\n>\n> Reviewed-by: Naushir Patuck \n>\n> On Mon, 31 Jul 2023 at 14:48, David Plowman via libcamera-devel\n> wrote:\n> >\n> > This HDR algorithm doesn't combine images in any way, but simply\n> > allows an application to drive the AGC in a multi-channel HDR type of\n> > mode, such as to produce short and long exposure images.\n> >\n> > Sufficient plumbing is added to enable the libcamera HDR controls and\n> > metadata to work.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/ipa/rpi/common/ipa_base.cpp | 49 ++++++++++\n> > src/ipa/rpi/controller/hdr_algorithm.h | 23 +++++\n> > src/ipa/rpi/controller/hdr_status.h | 25 +++++\n> > src/ipa/rpi/controller/meson.build | 1 +\n> > src/ipa/rpi/controller/rpi/hdr.cpp | 129 +++++++++++++++++++++++++\n> > src/ipa/rpi/controller/rpi/hdr.h | 42 ++++++++\n> > 6 files changed, 269 insertions(+)\n> > create mode 100644 src/ipa/rpi/controller/hdr_algorithm.h\n> > create mode 100644 src/ipa/rpi/controller/hdr_status.h\n> > create mode 100644 src/ipa/rpi/controller/rpi/hdr.cpp\n> > create mode 100644 src/ipa/rpi/controller/rpi/hdr.h\n> >\n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > index f29c32fd..956b1ca0 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -24,6 +24,8 @@\n> > #include \"controller/ccm_status.h\"\n> > #include \"controller/contrast_algorithm.h\"\n> > #include \"controller/denoise_algorithm.h\"\n> > +#include \"controller/hdr_algorithm.h\"\n> > +#include \"controller/hdr_status.h\"\n> > #include \"controller/lux_status.h\"\n> > #include \"controller/sharpen_algorithm.h\"\n> > #include \"controller/statistics.h\"\n> > @@ -63,6 +65,7 @@ const ControlInfoMap::Map ipaControls{\n> > { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> > { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> > { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > + { &controls::HdrMode, ControlInfo(controls::HdrModeValues) },\n> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> > @@ -651,9 +654,17 @@ static const std::map AfPauseTable\n> > { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },\n> > };\n> >\n> > +static const std::map HdrModeTable = {\n>\n> No need for static here, it's in the anonymous namespace. Having said that,\n> perhaps we want to move HdrModeTable down further in the file where the other\n> libcamera controls -> RPi strings happen?\n\nWill do!\n\n>\n> > + { controls::HdrModeOff, \"Off\" },\n> > + { controls::HdrModeMultiExposure, \"MultiExposure\" },\n> > + { controls::HdrModeSingleExposure, \"SingleExposure\" },\n> > +};\n> > +\n> > void IpaBase::applyControls(const ControlList &controls)\n> > {\n> > + using RPiController::AgcAlgorithm;\n> > using RPiController::AfAlgorithm;\n> > + using RPiController::HdrAlgorithm;\n> >\n> > /* Clear the return metadata buffer. */\n> > libcameraMetadata_.clear();\n> > @@ -1092,6 +1103,34 @@ void IpaBase::applyControls(const ControlList &controls)\n> > break;\n> > }\n> >\n> > + case controls::HDR_MODE: {\n> > + HdrAlgorithm *hdr = dynamic_cast(controller_.getAlgorithm(\"hdr\"));\n> > + if (!hdr) {\n> > + LOG(IPARPI, Warning) << \"No HDR algorithm available\";\n> > + break;\n> > + }\n> > +\n> > + auto mode = HdrModeTable.find(ctrl.second.get());\n> > + if (mode == HdrModeTable.end()) {\n> > + LOG(IPARPI, Warning) << \"Unrecognised HDR mode\";\n> > + break;\n> > + }\n> > +\n> > + AgcAlgorithm *agc = dynamic_cast(controller_.getAlgorithm(\"agc\"));\n> > + if (!agc) {\n> > + LOG(IPARPI, Warning) << \"HDR requires an AGC algorithm\";\n> > + break;\n> > + }\n> > +\n> > + if (hdr->setMode(mode->second) == 0)\n> > + agc->setActiveChannels(hdr->getChannels());\n> > + else\n> > + LOG(IPARPI, Warning)\n> > + << \"HDR mode \" << mode->second << \" not supported\";\n> > +\n> > + break;\n> > + }\n> > +\n> > default:\n> > LOG(IPARPI, Warning)\n> > << \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> > @@ -1239,6 +1278,16 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n> > libcameraMetadata_.set(controls::AfPauseState, p);\n> > }\n> >\n> > + const HdrStatus *hdrStatus = rpiMetadata.getLocked(\"hdr.status\");\n> > + if (hdrStatus) {\n> > + if (hdrStatus->channel == \"short\")\n> > + libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelShort);\n> > + else if (hdrStatus->channel == \"long\")\n> > + libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelLong);\n> > + else\n> > + libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelNone);\n> > + }\n> > +\n> > metadataReady.emit(libcameraMetadata_);\n> > }\n> >\n> > diff --git a/src/ipa/rpi/controller/hdr_algorithm.h b/src/ipa/rpi/controller/hdr_algorithm.h\n> > new file mode 100644\n> > index 00000000..5164d50e\n> > --- /dev/null\n> > +++ b/src/ipa/rpi/controller/hdr_algorithm.h\n> > @@ -0,0 +1,23 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2023, Raspberry Pi Ltd\n> > + *\n> > + * hdr_algorithm.h - HDR control algorithm interface\n> > + */\n> > +#pragma once\n> > +\n> > +#include \"algorithm.h\"\n> > +\n> > +namespace RPiController {\n> > +\n> > +class HdrAlgorithm : public Algorithm\n> > +{\n> > +public:\n> > + HdrAlgorithm(Controller *controller)\n> > + : Algorithm(controller) {}\n> > + /* An HDR algorithm must provide the following: */\n> > + virtual int setMode(std::string const &modeName) = 0;\n> > + virtual std::vector getChannels() const = 0;\n> > +};\n> > +\n> > +} /* namespace RPiController */\n> > diff --git a/src/ipa/rpi/controller/hdr_status.h b/src/ipa/rpi/controller/hdr_status.h\n> > new file mode 100644\n> > index 00000000..26b538a0\n> > --- /dev/null\n> > +++ b/src/ipa/rpi/controller/hdr_status.h\n> > @@ -0,0 +1,25 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2022 Raspberry Pi Ltd\n>\n> s/2022/2023/\n\nAh yes, of course!\n\nThanks\nDavid\n\n>\n> > + *\n> > + * hdr_status.h - HDR control algorithm status\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +/*\n> > + * The HDR algorithm process method should post an HdrStatus into the image\n> > + * metadata under the tag \"hdr.status\".\n> > + */\n> > +\n> > +struct HdrStatus {\n> > + std::string mode;\n> > + std::string channel;\n> > +\n> > + /* Parameters for programming the stitch block (where available). */\n> > + bool stitch_enable;\n> > + uint16_t thresholdLo;\n> > + uint8_t diffPower;\n> > + double motionThreshold;\n> > +};\n> > diff --git a/src/ipa/rpi/controller/meson.build b/src/ipa/rpi/controller/meson.build\n> > index 20b9cda9..c9a3e850 100644\n> > --- a/src/ipa/rpi/controller/meson.build\n> > +++ b/src/ipa/rpi/controller/meson.build\n> > @@ -16,6 +16,7 @@ rpi_ipa_controller_sources = files([\n> > 'rpi/contrast.cpp',\n> > 'rpi/dpc.cpp',\n> > 'rpi/geq.cpp',\n> > + 'rpi/hdr.cpp',\n> > 'rpi/lux.cpp',\n> > 'rpi/noise.cpp',\n> > 'rpi/sdn.cpp',\n> > diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > new file mode 100644\n> > index 00000000..2587f622\n> > --- /dev/null\n> > +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > @@ -0,0 +1,129 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2022 Raspberry Pi Ltd\n> > + *\n> > + * hdr.cpp - HDR control algorithm\n> > + */\n> > +\n> > +#include \"hdr.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"../agc_status.h\"\n> > +\n> > +using namespace RPiController;\n> > +using namespace libcamera;\n> > +\n> > +LOG_DEFINE_CATEGORY(RPiHdr)\n> > +\n> > +#define NAME \"rpi.hdr\"\n> > +\n> > +void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &modeName)\n> > +{\n> > + name = modeName;\n> > +\n> > + if (!params.contains(\"cadence\"))\n> > + LOG(RPiHdr, Fatal) << \"No cadence for HDR mode \" << name;\n> > + cadence = params[\"cadence\"].getList().value();\n> > + if (cadence.empty())\n> > + LOG(RPiHdr, Fatal) << \"Empty cadence in HDR mode \" << name;\n> > +\n> > + /*\n> > + * In the JSON file it's easier to use the channel name as the key, but\n> > + * for us it's convenient to swap them over.\n> > + */\n> > + for (const auto &[k, v] : params[\"channel_map\"].asDict())\n> > + channelMap[v.get().value()] = k;\n> > +}\n> > +\n> > +Hdr::Hdr(Controller *controller)\n> > + : HdrAlgorithm(controller),\n> > + currentConfig_(nullptr)\n> > +{\n> > +}\n> > +\n> > +char const *Hdr::name() const\n> > +{\n> > + return NAME;\n> > +}\n> > +\n> > +int Hdr::read(const libcamera::YamlObject ¶ms)\n> > +{\n> > + for (const auto &[key, value] : params.asDict()) {\n> > + HdrConfig hdrConfig;\n> > + hdrConfig.read(value, key);\n> > + config_[key] = std::move(hdrConfig);\n> > + if (!currentConfig_)\n> > + currentConfig_ = &config_[key];\n> > + }\n> > +\n> > + if (!currentConfig_)\n> > + LOG(RPiHdr, Fatal) << \"No HDR modes read\";\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int Hdr::setMode(std::string const &mode)\n> > +{\n> > + auto it = config_.find(mode);\n> > + if (it == config_.end()) {\n> > + LOG(RPiHdr, Warning) << \"No such HDR mode \" << mode;\n> > + return -1;\n> > + }\n> > +\n> > + currentConfig_ = &it->second;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +std::vector Hdr::getChannels() const\n> > +{\n> > + return currentConfig_->cadence;\n> > +}\n> > +\n> > +void Hdr::switchMode([[maybe_unused]] CameraMode const &cameraMode,\n> > + [[maybe_unused]] Metadata *metadata)\n> > +{\n> > +}\n> > +\n> > +void Hdr::process([[maybe_unused]] StatisticsPtr &stats, Metadata *imageMetadata)\n> > +{\n> > + if (currentConfig_->name == \"Off\")\n> > + return;\n> > +\n> > + /* First find out what AGC channel this is, which comes from the delayed_status. */\n> > + bool channelKnown = false;\n> > + unsigned int agcChannel = 0;\n> > + {\n> > + std::scoped_lock lock(*imageMetadata);\n> > + AgcStatus *agcStatus = imageMetadata->getLocked(\"agc.delayed_status\");\n> > + if (agcStatus) {\n> > + channelKnown = true;\n> > + agcChannel = agcStatus->channel;\n> > + }\n> > + }\n> > +\n> > + /* Fill out the HdrStatus information. */\n> > + HdrStatus hdrStatus;\n> > + hdrStatus.mode = currentConfig_->name;\n> > + if (channelKnown) {\n> > + /*\n> > + * Channels that aren't required for HDR processing might come through for\n> > + * various reasons, such as when HDR starts up, or even because the cadence\n> > + * demands some frames that you need for other purposes (e.g. for preview).\n> > + * We'll leave the channel name empty in these cases.\n> > + */\n> > + auto it = currentConfig_->channelMap.find(agcChannel);\n> > + if (it != currentConfig_->channelMap.end())\n> > + hdrStatus.channel = it->second;\n> > + }\n> > +\n> > + imageMetadata->set(\"hdr.status\", hdrStatus);\n> > +}\n> > +\n> > +/* Register algorithm with the system. */\n> > +static Algorithm *create(Controller *controller)\n> > +{\n> > + return (Algorithm *)new Hdr(controller);\n> > +}\n> > +static RegisterAlgorithm reg(NAME, &create);\n> > diff --git a/src/ipa/rpi/controller/rpi/hdr.h b/src/ipa/rpi/controller/rpi/hdr.h\n> > new file mode 100644\n> > index 00000000..8f6457f2\n> > --- /dev/null\n> > +++ b/src/ipa/rpi/controller/rpi/hdr.h\n> > @@ -0,0 +1,42 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2022, Raspberry Pi Ltd\n> > + *\n> > + * hdr.h - HDR control algorithm\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \"../hdr_algorithm.h\"\n> > +#include \"../hdr_status.h\"\n> > +\n> > +/* This is our implementation of an HDR algorithm. */\n> > +\n> > +namespace RPiController {\n> > +\n> > +struct HdrConfig {\n> > + std::string name;\n> > + std::vector cadence;\n> > + std::map channelMap;\n> > +\n> > + void read(const libcamera::YamlObject ¶ms, const std::string &name);\n> > +};\n> > +\n> > +class Hdr : public HdrAlgorithm\n> > +{\n> > +public:\n> > + Hdr(Controller *controller);\n> > + char const *name() const override;\n> > + void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;\n> > + int read(const libcamera::YamlObject ¶ms) override;\n> > + void process(StatisticsPtr &stats, Metadata *imageMetadata) override;\n> > + int setMode(std::string const &mode) override;\n> > + std::vector getChannels() const override;\n> > +\n> > +private:\n> > + std::map config_;\n> > + const HdrConfig *currentConfig_;\n> > +};\n> > +\n> > +} /* namespace RPiController */\n> > --\n> > 2.30.2\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 67D0BBF415\n\tfor ;\n\tMon, 21 Aug 2023 09:09:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4234D627E1;\n\tMon, 21 Aug 2023 11:09:30 +0200 (CEST)","from mail-qv1-xf2f.google.com (mail-qv1-xf2f.google.com\n\t[IPv6:2607:f8b0:4864:20::f2f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E921D627DA\n\tfor ;\n\tMon, 21 Aug 2023 11:09:28 +0200 (CEST)","by mail-qv1-xf2f.google.com with SMTP id\n\t6a1803df08f44-64a70194fbeso18464406d6.0\n\tfor ;\n\tMon, 21 Aug 2023 02:09:28 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692608970;\n\tbh=2cFTjXcTNeZaRPZw0xxqioeds4Dajt5+JDhYo2i7wnY=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=G7uN+ZOKa8b3oaoQlBlENO0Mlnrd7yc0R5sP1tvhuLr18Hdpz66H/bUOTLUgNAjRS\n\t96pIQUQoTUzctYLbQkXPkgalXd74YGDve5USPG0TM+WygSvbVN0855ZzPtdNRE9Erb\n\t39nREvSH8OnqboGOcGPXgdIFCX3KuNsYZ1SPW8ZR6bU+RBrfn6psyGxBDjtAoyXzJT\n\tTrXstWXL5UUIbhzLGnZ9CVIZ2MuQIhUg8Gi1iyOOLXrC+eqtmJZ/bZw9QfcQpii1Eo\n\trdmXCHVofbOBGveV53nwxDHZNDQHFL0JundAlvhJoh3gp/+XaNjYl25jGHgOCKz5hW\n\tTcqslaVnzjKiw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1692608967; x=1693213767;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=y5Wfa5gEPBzD1muxm7FcFKm2ITeGpHXiuoTqTbf4U3g=;\n\tb=rhbfD2aylddMCJfLybb/7WII1TLjEfSXFY5ZJMTyTQQQfLVfa14L77DMS+xwTbV5Nu\n\tKcRA35bzw086hczD0eFHSwKQLAPTUETP2wRe9cw3i8YxkpJk5C6oSFkyIFjYBWp39cdF\n\tIo1zeiJWutq1JLN+DuaIemPpEzJt8wCq9L4ArVtmVyDRUE5zjGLjOmIGXVRSBjGz/Ack\n\twjPDqxhenLpsiA+P2Q/M2JF4oOi3LTKO2kacOFu6WwK6s2sAX3ooMUaX+C2jVbc62t0w\n\tTJAWB92YDp8Mh5A5QduQOHqpzTInDkxCgyWEKJ1f/0Zq4FqSYcxK8pvgfJur7N2mM0nF\n\tSN4Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"rhbfD2ay\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1692608967; x=1693213767;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=y5Wfa5gEPBzD1muxm7FcFKm2ITeGpHXiuoTqTbf4U3g=;\n\tb=PZMwAdZhK1qe6pADhZPA/a6rOHkknOhYSJdoX/tglDWorqgIF6+MoEROY/6cBImHY6\n\tZ13rNVQlPEcXu/2/R4SkKvSJJfBO3SitNR5rPC+sbMIq2PUJeTSoc8UVl2HG4qLGPckc\n\tDD7nuneZO24Cx0eHvnrEd4dA/zfCw84okTi/2OF90hS80UPNCUHfti0mWjPlQpj58M0C\n\tb+1k4JBmMTvp5LXYURclDssqwH6pycPAaIXZNWBWJ5SZtwNsvqUz1p4417eku1ZzYBjN\n\tcR74OEX16ZghauQArr46uA2wA28P94qFA6/P/pj9t9yVrrs0wej3zl37N7GVbCdeHFD8\n\ttVhQ==","X-Gm-Message-State":"AOJu0Yw6kxzho8vWmKwO/cQt3vIaMRc6dD3AsL8UjweWnkf3KgJDIY9C\n\tpPqhNauDNT/uDMTE8o7N4beSBoiQCKAdPyzniBElw6xKPHQ0Znbr","X-Google-Smtp-Source":"AGHT+IHTgEuNCbWLD6/9GuMN7eB94Lo7yxLugXC93Hk30dLtS+GJcH6lMv/n3DyOyWNgBRnM5cPtY7Arz+j9fk00PNI=","X-Received":"by 2002:a0c:f106:0:b0:64a:3ab9:266f with SMTP id\n\ti6-20020a0cf106000000b0064a3ab9266fmr6632795qvl.49.1692608967635;\n\tMon, 21 Aug 2023 02:09:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20230731134833.79687-1-david.plowman@raspberrypi.com>\n\t<20230731134833.79687-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Mon, 21 Aug 2023 10:09:16 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: rpi: Add an HDR algorithm to\n\tdrive multi-channel AGC","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]