[{"id":27802,"web_url":"https://patchwork.libcamera.org/comment/27802/","msgid":"","date":"2023-09-16T13:25:15","subject":"Re: [libcamera-devel] [PATCH v2 1/2] ipa: rpi: Add an HDR algorithm\n\tto drive multi-channel AGC","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Wed, Aug 23, 2023 at 03:49:24PM +0100, David Plowman via libcamera-devel wrote:\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> Reviewed-by: Naushir Patuck \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 | 127 +++++++++++++++++++++++++\n> src/ipa/rpi/controller/rpi/hdr.h | 42 ++++++++\n> 6 files changed, 267 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 f7e7ad5e..231e8f96 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> @@ -67,6 +69,7 @@ const ControlInfoMap::Map ipaControls{\n> \t{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },\n> \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> \t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> +\t{ &controls::HdrMode, ControlInfo(controls::HdrModeValues) },\n> \t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> \t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> \t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> @@ -658,9 +661,17 @@ static const std::map AfPauseTable\n> \t{ controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },\n> };\n>\n> +static const std::map HdrModeTable = {\n> +\t{ controls::HdrModeOff, \"Off\" },\n> +\t{ controls::HdrModeMultiExposure, \"MultiExposure\" },\n> +\t{ controls::HdrModeSingleExposure, \"SingleExposure\" },\n> +};\n> +\n> void IpaBase::applyControls(const ControlList &controls)\n> {\n> +\tusing RPiController::AgcAlgorithm;\n> \tusing RPiController::AfAlgorithm;\n> +\tusing RPiController::HdrAlgorithm;\n>\n> \t/* Clear the return metadata buffer. */\n> \tlibcameraMetadata_.clear();\n> @@ -1157,6 +1168,34 @@ void IpaBase::applyControls(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n>\n> +\t\tcase controls::HDR_MODE: {\n> +\t\t\tHdrAlgorithm *hdr = dynamic_cast(controller_.getAlgorithm(\"hdr\"));\n> +\t\t\tif (!hdr) {\n> +\t\t\t\tLOG(IPARPI, Warning) << \"No HDR algorithm available\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tauto mode = HdrModeTable.find(ctrl.second.get());\n> +\t\t\tif (mode == HdrModeTable.end()) {\n> +\t\t\t\tLOG(IPARPI, Warning) << \"Unrecognised HDR mode\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tAgcAlgorithm *agc = dynamic_cast(controller_.getAlgorithm(\"agc\"));\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning) << \"HDR requires an AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tif (hdr->setMode(mode->second) == 0)\n> +\t\t\t\tagc->setActiveChannels(hdr->getChannels());\n> +\t\t\telse\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"HDR mode \" << mode->second << \" not supported\";\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tdefault:\n> \t\t\tLOG(IPARPI, Warning)\n> \t\t\t\t<< \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> @@ -1304,6 +1343,16 @@ void IpaBase::reportMetadata(unsigned int ipaContext)\n> \t\tlibcameraMetadata_.set(controls::AfPauseState, p);\n> \t}\n>\n> +\tconst HdrStatus *hdrStatus = rpiMetadata.getLocked(\"hdr.status\");\n> +\tif (hdrStatus) {\n> +\t\tif (hdrStatus->channel == \"short\")\n> +\t\t\tlibcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelShort);\n> +\t\telse if (hdrStatus->channel == \"long\")\n> +\t\t\tlibcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelLong);\n> +\t\telse\n> +\t\t\tlibcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelNone);\n> +\t}\n> +\n> \tmetadataReady.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> +\tHdrAlgorithm(Controller *controller)\n> +\t\t: Algorithm(controller) {}\n> +\t/* An HDR algorithm must provide the following: */\n> +\tvirtual int setMode(std::string const &modeName) = 0;\n> +\tvirtual 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..120dcb1a\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) 2023 Raspberry Pi Ltd\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> +\tstd::string mode;\n> +\tstd::string channel;\n> +\n> +\t/* Parameters for programming the stitch block (where available). */\n> +\tbool stitch_enable;\n> +\tuint16_t thresholdLo;\n> +\tuint8_t diffPower;\n> +\tdouble motionThreshold;\n\nI'll let you consider if it's worth adding them now or later.\n\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..2700d35e\n> --- /dev/null\n> +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> @@ -0,0 +1,127 @@\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> +\tname = modeName;\n> +\n> +\tif (!params.contains(\"cadence\"))\n> +\t\tLOG(RPiHdr, Fatal) << \"No cadence for HDR mode \" << name;\n> +\tcadence = params[\"cadence\"].getList().value();\n> +\tif (cadence.empty())\n> +\t\tLOG(RPiHdr, Fatal) << \"Empty cadence in HDR mode \" << name;\n> +\n> +\t/*\n> +\t * In the JSON file it's easier to use the channel name as the key, but\n> +\t * for us it's convenient to swap them over.\n> +\t */\n> +\tfor (const auto &[k, v] : params[\"channel_map\"].asDict())\n> +\t\tchannelMap[v.get().value()] = k;\n> +}\n> +\n> +Hdr::Hdr(Controller *controller)\n> +\t: HdrAlgorithm(controller),\n> +\t currentConfig_(nullptr)\n> +{\n> +}\n> +\n> +char const *Hdr::name() const\n> +{\n> +\treturn NAME;\n> +}\n> +\n> +int Hdr::read(const libcamera::YamlObject ¶ms)\n> +{\n> +\t/* Make an \"HDR off\" mode by default so that tuning files don't have to. */\n> +\tHdrConfig &offMode = config_[\"Off\"];\n> +\toffMode.name = \"Off\";\n> +\toffMode.cadence = { 0 };\n\nIsn't \"Off\" part of the config file ?\n \"Off\":\n {\n \"cadence\": [ 0 ]\n },\n\n> +\tcurrentConfig_ = &offMode;\n\nI understand this should be the default\n\n> +\n> +\tfor (const auto &[key, value] : params.asDict())\n> +\t\tconfig_[key].read(value, key);\n\nBut won't \"Off\" be parsed here too ?\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Hdr::setMode(std::string const &mode)\n> +{\n> +\tauto it = config_.find(mode);\n> +\tif (it == config_.end()) {\n> +\t\tLOG(RPiHdr, Warning) << \"No such HDR mode \" << mode;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tcurrentConfig_ = &it->second;\n> +\n> +\treturn 0;\n> +}\n> +\n> +std::vector Hdr::getChannels() const\n> +{\n> +\treturn currentConfig_->cadence;\n> +}\n> +\n> +void Hdr::switchMode([[maybe_unused]] CameraMode const &cameraMode,\n> +\t\t [[maybe_unused]] Metadata *metadata)\n> +{\n> +}\n> +\n> +void Hdr::process([[maybe_unused]] StatisticsPtr &stats, Metadata *imageMetadata)\n> +{\n> +\tif (currentConfig_->name == \"Off\")\n> +\t\treturn;\n> +\n> +\t/* First find out what AGC channel this is, which comes from the delayed_status. */\n> +\tbool channelKnown = false;\n> +\tunsigned int agcChannel = 0;\n> +\t{\n> +\t\tstd::scoped_lock lock(*imageMetadata);\n> +\t\tAgcStatus *agcStatus = imageMetadata->getLocked(\"agc.delayed_status\");\n> +\t\tif (agcStatus) {\n> +\t\t\tchannelKnown = true;\n> +\t\t\tagcChannel = agcStatus->channel;\n> +\t\t}\n> +\t}\n> +\n> +\t/* Fill out the HdrStatus information. */\n> +\tHdrStatus hdrStatus;\n> +\thdrStatus.mode = currentConfig_->name;\n> +\tif (channelKnown) {\n> +\t\t/*\n> +\t\t * Channels that aren't required for HDR processing might come through for\n> +\t\t * various reasons, such as when HDR starts up, or even because the cadence\n> +\t\t * demands some frames that you need for other purposes (e.g. for preview).\n> +\t\t * We'll leave the channel name empty in these cases.\n> +\t\t */\n> +\t\tauto it = currentConfig_->channelMap.find(agcChannel);\n> +\t\tif (it != currentConfig_->channelMap.end())\n> +\t\t\thdrStatus.channel = it->second;\n\nRelated to the discussion about the \"HdrChannel\" metadata, it would be\ntrivial returning the channel index here if I got this right\n\n> +\t}\n> +\n> +\timageMetadata->set(\"hdr.status\", hdrStatus);\n> +}\n> +\n> +/* Register algorithm with the system. */\n> +static Algorithm *create(Controller *controller)\n> +{\n> +\treturn (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\nAll of these are 2023 now\n\n> + *\n> + * hdr.h - HDR control algorithm\n> + */\n> +#pragma once\n> +\n> +#include \n\nand string and map ?\n\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> +\tstd::string name;\n> +\tstd::vector cadence;\n> +\tstd::map channelMap;\n> +\n> +\tvoid read(const libcamera::YamlObject ¶ms, const std::string &name);\n> +};\n> +\n> +class Hdr : public HdrAlgorithm\n> +{\n> +public:\n> +\tHdr(Controller *controller);\n> +\tchar const *name() const override;\n> +\tvoid switchMode(CameraMode const &cameraMode, Metadata *metadata) override;\n> +\tint read(const libcamera::YamlObject ¶ms) override;\n> +\tvoid process(StatisticsPtr &stats, Metadata *imageMetadata) override;\n> +\tint setMode(std::string const &mode) override;\n> +\tstd::vector getChannels() const override;\n> +\n> +private:\n> +\tstd::map config_;\n> +\tconst 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 3FC3EBD160\n\tfor ;\n\tSat, 16 Sep 2023 13:25:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A375B62905;\n\tSat, 16 Sep 2023 15:25:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8B36628E9\n\tfor ;\n\tSat, 16 Sep 2023 15:25:18 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8DAD010A0;\n\tSat, 16 Sep 2023 15:23:44 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694870719;\n\tbh=889ePVyY3d2yn9xOzpWtmrtXKTX86diqQ2s0rk8xZzs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=aH7Hql7VQteUq+Ng8Vimp/u0bNlJ+IKxNAxLI3JffKqRe4ri3hGVdyMPFoQDsvhSQ\n\t8fyDsM8IURKgxQw58N9HjVSqjRfVJgdihu0QOFLSypzmn0LkXNmSWZy+X1ZSpyfzXB\n\tgVBMoAqVjwPWmzhl38zY5JJn/wtnqRCytE455Hsjq8iqIxmW+lXPEvaZ14tqfsQl3y\n\tIiSQfecH+AkfXA6vlpJaRpy9G6a40SBq1TogF7OtjECMTgEPGigIz8T3axiIQSrR92\n\tMr4+xPR9cy6VHtumBV3wAcHcBrXOv0KhTVBmezjlRFlxA27Dg6TkajO8hKtrxJJ7CG\n\tzhtnJY+Mdq3mA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1694870624;\n\tbh=889ePVyY3d2yn9xOzpWtmrtXKTX86diqQ2s0rk8xZzs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZiSRgVRTvxN1Wh16W3dyJ1G/BuNSKbXqbHhJZ1nVFFnnQb+27JJxYWOTgYq5IvZ/h\n\tUg92+CUOWITFwfVpsNZfFTRPRLkpjYLKEFVOVMhApeURxadTNfv52u439uwHNHbFmD\n\tsDGfYfBKUAYr5GhjXL5hEqaeZyTJaUbZQ+WVkC04="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZiSRgVRT\"; dkim-atps=neutral","Date":"Sat, 16 Sep 2023 15:25:15 +0200","To":"David Plowman ","Message-ID":"","References":"<20230823144925.2542-1-david.plowman@raspberrypi.com>\n\t<20230823144925.2542-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230823144925.2542-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] ipa: rpi: Add an HDR algorithm\n\tto drive 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27809,"web_url":"https://patchwork.libcamera.org/comment/27809/","msgid":"","date":"2023-09-18T09:02:17","subject":"Re: [libcamera-devel] [PATCH v2 1/2] ipa: rpi: Add an HDR algorithm\n\tto drive multi-channel AGC","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the review and the questions!\n\nOn Sat, 16 Sept 2023 at 14:25, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Wed, Aug 23, 2023 at 03:49:24PM +0100, David Plowman via libcamera-devel wrote:\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> > Reviewed-by: Naushir Patuck \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 | 127 +++++++++++++++++++++++++\n> > src/ipa/rpi/controller/rpi/hdr.h | 42 ++++++++\n> > 6 files changed, 267 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 f7e7ad5e..231e8f96 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> > @@ -67,6 +69,7 @@ const ControlInfoMap::Map ipaControls{\n> > { &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },\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> > @@ -658,9 +661,17 @@ static const std::map AfPauseTable\n> > { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },\n> > };\n> >\n> > +static const std::map HdrModeTable = {\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> > @@ -1157,6 +1168,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> > @@ -1304,6 +1343,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..120dcb1a\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) 2023 Raspberry Pi Ltd\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> I'll let you consider if it's worth adding them now or later.\n\nYou're right, I could delete those for the moment. Maybe I should do that.\n\n>\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..2700d35e\n> > --- /dev/null\n> > +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > @@ -0,0 +1,127 @@\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> > + /* Make an \"HDR off\" mode by default so that tuning files don't have to. */\n> > + HdrConfig &offMode = config_[\"Off\"];\n> > + offMode.name = \"Off\";\n> > + offMode.cadence = { 0 };\n>\n> Isn't \"Off\" part of the config file ?\n> \"Off\":\n> {\n> \"cadence\": [ 0 ]\n> },\n>\n> > + currentConfig_ = &offMode;\n>\n> I understand this should be the default\n>\n> > +\n> > + for (const auto &[key, value] : params.asDict())\n> > + config_[key].read(value, key);\n>\n> But won't \"Off\" be parsed here too ?\n\nOriginally it didn't set up a default, but Naush suggested it might be\nnice if it did. So I added that, but also felt it would be quite nice\nif applications could override that if they wanted.\n\nOne example I had in mind was an application that mostly wants to do\nSingleExposure-type HDR, but might also want to turn HDR off. Let's\nalso suppose that the application also wants to use standard AGC\ncontrols on the HDR channel (but not on the HDR \"off\" channel).\n\nCurrently that means the HDR channel would have to be channel 0\n(because those other controls only operate on channel 0), meaning\nthey'd have to move HDR \"off\" to channel 1 (for example). So it's all\nabout giving an application complete flexibility about what it can do.\n\nBut it might be helpful if I commented that better - to make it clear\nyou can override the \"Off\" mode if you want, and that it wasn't a\nmistake!\n\n>\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> Related to the discussion about the \"HdrChannel\" metadata, it would be\n> trivial returning the channel index here if I got this right\n\nI think you mean that if we used the HdrMode control values directly,\nthen this would be trivial? That's certainly true, though as a rule we\ntry to avoid using libcamera control values within our own algorithms.\n\nMostly we map libcamera control values onto strings, so maybe that\ncould be the way to go here? Instead of reading an array of AGC\nconfigs, we'd have a dictionary where each config is named \"normal\",\n\"short\" or \"long\" (or indeed anything else).\n\nI think that might be better, and addresses at least some of the\n\"fragility\" concerns. I'll try that in the next version and we can see\nif we prefer it.\n\n>\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> All of these are 2023 now\n\nAh yes, thank you.\n\n>\n> > + *\n> > + * hdr.h - HDR control algorithm\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n>\n> and string and map ?\n\nYep, will check!\n\nThanks again\nDavid\n\n>\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 08D0EBE080\n\tfor ;\n\tMon, 18 Sep 2023 09:02:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3F4716293B;\n\tMon, 18 Sep 2023 11:02:31 +0200 (CEST)","from mail-vs1-xe36.google.com (mail-vs1-xe36.google.com\n\t[IPv6:2607:f8b0:4864:20::e36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE2A461DEC\n\tfor ;\n\tMon, 18 Sep 2023 11:02:29 +0200 (CEST)","by mail-vs1-xe36.google.com with SMTP id\n\tada2fe7eead31-44ee3a547adso2051541137.2\n\tfor ;\n\tMon, 18 Sep 2023 02:02:29 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695027751;\n\tbh=E9yNYJYait1572S7s9nyunSKEISWxihYucDJl6/qKsA=;\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=gxfMeKMxvyF8UFA5/G85NzJO6Q2Zjx6abQjl7T9ZQwAfv5WnrYCNAZAbCrMYnU+VB\n\tEViOEvCODHLBHY8OIxcII7XaLY7cJBiAHe7y039INeOS9xB+I8BVRO+tMTP2G20vm4\n\tBFZSnrD5pSTtJV6VUkK92qP5nnTSFMTfsvgV2M/hC9BZo+UNtaSIlJb241IVvQFjvj\n\tPodgKnttTy2xcnsg+u5TNkbSyKcfgTSCDpaFCfrLI+Vi9M4TYwbqFuObfSiPLmsOyL\n\tXgLY6ySXlZro2r2KimPiR1w2JFAiMk9VYzon+jrGcaOaPrQESX1YlHWhbSXXTm8v91\n\tpjbl4HNbC/AIA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1695027748; x=1695632548;\n\tdarn=lists.libcamera.org; \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=1uYMUSwQDEVWXuje2EpMSLVLRHZ3mbsnIUfLYXu/6po=;\n\tb=eQGtZoTy3joChAgWYB9coQ5yWuMB9b1HGTzvt1y0gFSX+NJkA9AR5HhzzKMCN+Bwz0\n\tk5BT7DRlAQ0PER1cxM3+Xe7O3c/GlY6fa7bhl/n5PlYA4PSXVenpGv2KnNUow9ilAbx5\n\tMhwJMxe8yBTKai9XVZHczGx2GXOLplc5bvMb7PIWyuini1oKKsgO8dqPdkmiEoEgHqLm\n\tMXSloyxpJ/opd+lIDYh6FWX8eLPbkpW0coX+cxA4ztUw7R6M2FztOZLAdcmiVBTlju49\n\t0tLxhGb1fQSRSO/maaFCrCJ0wcw1gLA1NwEszEFzTOH+BEE+jA3TC//plbqEZD+qWLxJ\n\t9cdA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"eQGtZoTy\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1695027748; x=1695632548;\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=1uYMUSwQDEVWXuje2EpMSLVLRHZ3mbsnIUfLYXu/6po=;\n\tb=h+s7h81vxdllOCg4Qsa/gjkKxHYO3Hj389ufYdl26mBy2wfViXGtruFu8qYgFdixpa\n\tbVsurcCpkAN+Iz+aC7DwBOLNHsyKcdAtXFUTLUQC+xzG3+bJFWnvEJmCneafXhfWb9X6\n\tI1K0XtaqiXRZDAifr64Vt5m7map/FC1PU37Al5W65uM248HlN8/FV/Y52ADwKUDrjZxf\n\tdWi4ChS0OQZWYbjGs7AMdmBRJnhXSpf78OhJovKMRpYo7kbMi3eEZR4i5RAmF9vKqYuu\n\tShceezrDsj9BjhiylC7CsWj8BgC9ryqO3smSZAe3Vp0GZNUjVamwDqGBxkoAL83cxU0g\n\t/UfA==","X-Gm-Message-State":"AOJu0YxZ1YC+iAercNKfKFn2vUJ17DkER2y5syVawnyY9+ukVE3pf4Bo\n\tYYKdqFW3RzgLnuXRkwZQnjT51WxeWmxvD3R70SsIHnEWm35wMvlx","X-Google-Smtp-Source":"AGHT+IFkWYoskylWmSt0aL8dIwZAhZu4rw4TE5ZLEOUKLrr60sPIga74GGXkexU+kdFlOfntJETPc4JpU32iBfjT0r8=","X-Received":"by 2002:a67:f4d3:0:b0:44e:98d8:c62e with SMTP id\n\ts19-20020a67f4d3000000b0044e98d8c62emr7246424vsn.33.1695027748252;\n\tMon, 18 Sep 2023 02:02:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20230823144925.2542-1-david.plowman@raspberrypi.com>\n\t<20230823144925.2542-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Mon, 18 Sep 2023 10:02:17 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] ipa: rpi: Add an HDR algorithm\n\tto drive 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\" "}}]