[libcamera-devel,v2,1/2] ipa: rpi: Add an HDR algorithm to drive multi-channel AGC
diff mbox series

Message ID 20230823144925.2542-2-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • HDR for Raspberry Pi
Related show

Commit Message

David Plowman Aug. 23, 2023, 2:49 p.m. UTC 
This HDR algorithm doesn't combine images in any way, but simply
allows an application to drive the AGC in a multi-channel HDR type of
mode, such as to produce short and long exposure images.

Sufficient plumbing is added to enable the libcamera HDR controls and
metadata to work.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp        |  49 ++++++++++
 src/ipa/rpi/controller/hdr_algorithm.h |  23 +++++
 src/ipa/rpi/controller/hdr_status.h    |  25 +++++
 src/ipa/rpi/controller/meson.build     |   1 +
 src/ipa/rpi/controller/rpi/hdr.cpp     | 127 +++++++++++++++++++++++++
 src/ipa/rpi/controller/rpi/hdr.h       |  42 ++++++++
 6 files changed, 267 insertions(+)
 create mode 100644 src/ipa/rpi/controller/hdr_algorithm.h
 create mode 100644 src/ipa/rpi/controller/hdr_status.h
 create mode 100644 src/ipa/rpi/controller/rpi/hdr.cpp
 create mode 100644 src/ipa/rpi/controller/rpi/hdr.h

Comments

Jacopo Mondi Sept. 16, 2023, 1:25 p.m. UTC | #1 
Hi David

On Wed, Aug 23, 2023 at 03:49:24PM +0100, David Plowman via libcamera-devel wrote:
> This HDR algorithm doesn't combine images in any way, but simply
> allows an application to drive the AGC in a multi-channel HDR type of
> mode, such as to produce short and long exposure images.
>
> Sufficient plumbing is added to enable the libcamera HDR controls and
> metadata to work.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/rpi/common/ipa_base.cpp        |  49 ++++++++++
>  src/ipa/rpi/controller/hdr_algorithm.h |  23 +++++
>  src/ipa/rpi/controller/hdr_status.h    |  25 +++++
>  src/ipa/rpi/controller/meson.build     |   1 +
>  src/ipa/rpi/controller/rpi/hdr.cpp     | 127 +++++++++++++++++++++++++
>  src/ipa/rpi/controller/rpi/hdr.h       |  42 ++++++++
>  6 files changed, 267 insertions(+)
>  create mode 100644 src/ipa/rpi/controller/hdr_algorithm.h
>  create mode 100644 src/ipa/rpi/controller/hdr_status.h
>  create mode 100644 src/ipa/rpi/controller/rpi/hdr.cpp
>  create mode 100644 src/ipa/rpi/controller/rpi/hdr.h
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index f7e7ad5e..231e8f96 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -24,6 +24,8 @@
>  #include "controller/ccm_status.h"
>  #include "controller/contrast_algorithm.h"
>  #include "controller/denoise_algorithm.h"
> +#include "controller/hdr_algorithm.h"
> +#include "controller/hdr_status.h"
>  #include "controller/lux_status.h"
>  #include "controller/sharpen_algorithm.h"
>  #include "controller/statistics.h"
> @@ -67,6 +69,7 @@ const ControlInfoMap::Map ipaControls{
>  	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
>  	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>  	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> +	{ &controls::HdrMode, ControlInfo(controls::HdrModeValues) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>  	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>  	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
> @@ -658,9 +661,17 @@ static const std::map<int32_t, RPiController::AfAlgorithm::AfPause> AfPauseTable
>  	{ controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },
>  };
>
> +static const std::map<int32_t, std::string> HdrModeTable = {
> +	{ controls::HdrModeOff, "Off" },
> +	{ controls::HdrModeMultiExposure, "MultiExposure" },
> +	{ controls::HdrModeSingleExposure, "SingleExposure" },
> +};
> +
>  void IpaBase::applyControls(const ControlList &controls)
>  {
> +	using RPiController::AgcAlgorithm;
>  	using RPiController::AfAlgorithm;
> +	using RPiController::HdrAlgorithm;
>
>  	/* Clear the return metadata buffer. */
>  	libcameraMetadata_.clear();
> @@ -1157,6 +1168,34 @@ void IpaBase::applyControls(const ControlList &controls)
>  			break;
>  		}
>
> +		case controls::HDR_MODE: {
> +			HdrAlgorithm *hdr = dynamic_cast<HdrAlgorithm *>(controller_.getAlgorithm("hdr"));
> +			if (!hdr) {
> +				LOG(IPARPI, Warning) << "No HDR algorithm available";
> +				break;
> +			}
> +
> +			auto mode = HdrModeTable.find(ctrl.second.get<int32_t>());
> +			if (mode == HdrModeTable.end()) {
> +				LOG(IPARPI, Warning) << "Unrecognised HDR mode";
> +				break;
> +			}
> +
> +			AgcAlgorithm *agc = dynamic_cast<AgcAlgorithm *>(controller_.getAlgorithm("agc"));
> +			if (!agc) {
> +				LOG(IPARPI, Warning) << "HDR requires an AGC algorithm";
> +				break;
> +			}
> +
> +			if (hdr->setMode(mode->second) == 0)
> +				agc->setActiveChannels(hdr->getChannels());
> +			else
> +				LOG(IPARPI, Warning)
> +					<< "HDR mode " << mode->second << " not supported";
> +
> +			break;
> +		}
> +
>  		default:
>  			LOG(IPARPI, Warning)
>  				<< "Ctrl " << controls::controls.at(ctrl.first)->name()
> @@ -1304,6 +1343,16 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
>  		libcameraMetadata_.set(controls::AfPauseState, p);
>  	}
>
> +	const HdrStatus *hdrStatus = rpiMetadata.getLocked<HdrStatus>("hdr.status");
> +	if (hdrStatus) {
> +		if (hdrStatus->channel == "short")
> +			libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelShort);
> +		else if (hdrStatus->channel == "long")
> +			libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelLong);
> +		else
> +			libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelNone);
> +	}
> +
>  	metadataReady.emit(libcameraMetadata_);
>  }
>
> diff --git a/src/ipa/rpi/controller/hdr_algorithm.h b/src/ipa/rpi/controller/hdr_algorithm.h
> new file mode 100644
> index 00000000..5164d50e
> --- /dev/null
> +++ b/src/ipa/rpi/controller/hdr_algorithm.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2023, Raspberry Pi Ltd
> + *
> + * hdr_algorithm.h - HDR control algorithm interface
> + */
> +#pragma once
> +
> +#include "algorithm.h"
> +
> +namespace RPiController {
> +
> +class HdrAlgorithm : public Algorithm
> +{
> +public:
> +	HdrAlgorithm(Controller *controller)
> +		: Algorithm(controller) {}
> +	/* An HDR algorithm must provide the following: */
> +	virtual int setMode(std::string const &modeName) = 0;
> +	virtual std::vector<unsigned int> getChannels() const = 0;
> +};
> +
> +} /* namespace RPiController */
> diff --git a/src/ipa/rpi/controller/hdr_status.h b/src/ipa/rpi/controller/hdr_status.h
> new file mode 100644
> index 00000000..120dcb1a
> --- /dev/null
> +++ b/src/ipa/rpi/controller/hdr_status.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2023 Raspberry Pi Ltd
> + *
> + * hdr_status.h - HDR control algorithm status
> + */
> +#pragma once
> +
> +#include <string>
> +
> +/*
> + * The HDR algorithm process method should post an HdrStatus into the image
> + * metadata under the tag "hdr.status".
> + */
> +
> +struct HdrStatus {
> +	std::string mode;
> +	std::string channel;
> +
> +	/* Parameters for programming the stitch block (where available). */
> +	bool stitch_enable;
> +	uint16_t thresholdLo;
> +	uint8_t diffPower;
> +	double motionThreshold;

I'll let you consider if it's worth adding them now or later.

> +};
> diff --git a/src/ipa/rpi/controller/meson.build b/src/ipa/rpi/controller/meson.build
> index 20b9cda9..c9a3e850 100644
> --- a/src/ipa/rpi/controller/meson.build
> +++ b/src/ipa/rpi/controller/meson.build
> @@ -16,6 +16,7 @@ rpi_ipa_controller_sources = files([
>      'rpi/contrast.cpp',
>      'rpi/dpc.cpp',
>      'rpi/geq.cpp',
> +    'rpi/hdr.cpp',
>      'rpi/lux.cpp',
>      'rpi/noise.cpp',
>      'rpi/sdn.cpp',
> diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp
> new file mode 100644
> index 00000000..2700d35e
> --- /dev/null
> +++ b/src/ipa/rpi/controller/rpi/hdr.cpp
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2022 Raspberry Pi Ltd
> + *
> + * hdr.cpp - HDR control algorithm
> + */
> +
> +#include "hdr.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include "../agc_status.h"
> +
> +using namespace RPiController;
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(RPiHdr)
> +
> +#define NAME "rpi.hdr"
> +
> +void HdrConfig::read(const libcamera::YamlObject &params, const std::string &modeName)
> +{
> +	name = modeName;
> +
> +	if (!params.contains("cadence"))
> +		LOG(RPiHdr, Fatal) << "No cadence for HDR mode " << name;
> +	cadence = params["cadence"].getList<unsigned int>().value();
> +	if (cadence.empty())
> +		LOG(RPiHdr, Fatal) << "Empty cadence in HDR mode " << name;
> +
> +	/*
> +	 * In the JSON file it's easier to use the channel name as the key, but
> +	 * for us it's convenient to swap them over.
> +	 */
> +	for (const auto &[k, v] : params["channel_map"].asDict())
> +		channelMap[v.get<unsigned int>().value()] = k;
> +}
> +
> +Hdr::Hdr(Controller *controller)
> +	: HdrAlgorithm(controller),
> +	  currentConfig_(nullptr)
> +{
> +}
> +
> +char const *Hdr::name() const
> +{
> +	return NAME;
> +}
> +
> +int Hdr::read(const libcamera::YamlObject &params)
> +{
> +	/* Make an "HDR off" mode by default so that tuning files don't have to. */
> +	HdrConfig &offMode = config_["Off"];
> +	offMode.name = "Off";
> +	offMode.cadence = { 0 };

Isn't "Off" part of the config file ?
        "Off":
        {
                "cadence": [ 0 ]
        },

> +	currentConfig_ = &offMode;

I understand this should be the default

> +
> +	for (const auto &[key, value] : params.asDict())
> +		config_[key].read(value, key);

But won't "Off" be parsed here too ?

> +
> +	return 0;
> +}
> +
> +int Hdr::setMode(std::string const &mode)
> +{
> +	auto it = config_.find(mode);
> +	if (it == config_.end()) {
> +		LOG(RPiHdr, Warning) << "No such HDR mode " << mode;
> +		return -1;
> +	}
> +
> +	currentConfig_ = &it->second;
> +
> +	return 0;
> +}
> +
> +std::vector<unsigned int> Hdr::getChannels() const
> +{
> +	return currentConfig_->cadence;
> +}
> +
> +void Hdr::switchMode([[maybe_unused]] CameraMode const &cameraMode,
> +		     [[maybe_unused]] Metadata *metadata)
> +{
> +}
> +
> +void Hdr::process([[maybe_unused]] StatisticsPtr &stats, Metadata *imageMetadata)
> +{
> +	if (currentConfig_->name == "Off")
> +		return;
> +
> +	/* First find out what AGC channel this is, which comes from the delayed_status. */
> +	bool channelKnown = false;
> +	unsigned int agcChannel = 0;
> +	{
> +		std::scoped_lock<RPiController::Metadata> lock(*imageMetadata);
> +		AgcStatus *agcStatus = imageMetadata->getLocked<AgcStatus>("agc.delayed_status");
> +		if (agcStatus) {
> +			channelKnown = true;
> +			agcChannel = agcStatus->channel;
> +		}
> +	}
> +
> +	/* Fill out the HdrStatus information. */
> +	HdrStatus hdrStatus;
> +	hdrStatus.mode = currentConfig_->name;
> +	if (channelKnown) {
> +		/*
> +		 * Channels that aren't required for HDR processing might come through for
> +		 * various reasons, such as when HDR starts up, or even because the cadence
> +		 * demands some frames that you need for other purposes (e.g. for preview).
> +		 * We'll leave the channel name empty in these cases.
> +		 */
> +		auto it = currentConfig_->channelMap.find(agcChannel);
> +		if (it != currentConfig_->channelMap.end())
> +			hdrStatus.channel = it->second;

Related to the discussion about the "HdrChannel" metadata, it would be
trivial returning the channel index here if I got this right

> +	}
> +
> +	imageMetadata->set("hdr.status", hdrStatus);
> +}
> +
> +/* Register algorithm with the system. */
> +static Algorithm *create(Controller *controller)
> +{
> +	return (Algorithm *)new Hdr(controller);
> +}
> +static RegisterAlgorithm reg(NAME, &create);
> diff --git a/src/ipa/rpi/controller/rpi/hdr.h b/src/ipa/rpi/controller/rpi/hdr.h
> new file mode 100644
> index 00000000..8f6457f2
> --- /dev/null
> +++ b/src/ipa/rpi/controller/rpi/hdr.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2022, Raspberry Pi Ltd

All of these are 2023 now

> + *
> + * hdr.h - HDR control algorithm
> + */
> +#pragma once
> +
> +#include <vector>

and string and map ?

> +
> +#include "../hdr_algorithm.h"
> +#include "../hdr_status.h"
> +
> +/* This is our implementation of an HDR algorithm. */
> +
> +namespace RPiController {
> +
> +struct HdrConfig {
> +	std::string name;
> +	std::vector<unsigned int> cadence;
> +	std::map<unsigned int, std::string> channelMap;
> +
> +	void read(const libcamera::YamlObject &params, const std::string &name);
> +};
> +
> +class Hdr : public HdrAlgorithm
> +{
> +public:
> +	Hdr(Controller *controller);
> +	char const *name() const override;
> +	void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
> +	int read(const libcamera::YamlObject &params) override;
> +	void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
> +	int setMode(std::string const &mode) override;
> +	std::vector<unsigned int> getChannels() const override;
> +
> +private:
> +	std::map<std::string, HdrConfig> config_;
> +	const HdrConfig *currentConfig_;
> +};
> +
> +} /* namespace RPiController */
> --
> 2.30.2
>
David Plowman Sept. 18, 2023, 9:02 a.m. UTC | #2 
Hi Jacopo

Thanks for the review and the questions!

On Sat, 16 Sept 2023 at 14:25, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi David
>
> On Wed, Aug 23, 2023 at 03:49:24PM +0100, David Plowman via libcamera-devel wrote:
> > This HDR algorithm doesn't combine images in any way, but simply
> > allows an application to drive the AGC in a multi-channel HDR type of
> > mode, such as to produce short and long exposure images.
> >
> > Sufficient plumbing is added to enable the libcamera HDR controls and
> > metadata to work.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp        |  49 ++++++++++
> >  src/ipa/rpi/controller/hdr_algorithm.h |  23 +++++
> >  src/ipa/rpi/controller/hdr_status.h    |  25 +++++
> >  src/ipa/rpi/controller/meson.build     |   1 +
> >  src/ipa/rpi/controller/rpi/hdr.cpp     | 127 +++++++++++++++++++++++++
> >  src/ipa/rpi/controller/rpi/hdr.h       |  42 ++++++++
> >  6 files changed, 267 insertions(+)
> >  create mode 100644 src/ipa/rpi/controller/hdr_algorithm.h
> >  create mode 100644 src/ipa/rpi/controller/hdr_status.h
> >  create mode 100644 src/ipa/rpi/controller/rpi/hdr.cpp
> >  create mode 100644 src/ipa/rpi/controller/rpi/hdr.h
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index f7e7ad5e..231e8f96 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -24,6 +24,8 @@
> >  #include "controller/ccm_status.h"
> >  #include "controller/contrast_algorithm.h"
> >  #include "controller/denoise_algorithm.h"
> > +#include "controller/hdr_algorithm.h"
> > +#include "controller/hdr_status.h"
> >  #include "controller/lux_status.h"
> >  #include "controller/sharpen_algorithm.h"
> >  #include "controller/statistics.h"
> > @@ -67,6 +69,7 @@ const ControlInfoMap::Map ipaControls{
> >       { &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
> >       { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> >       { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> > +     { &controls::HdrMode, ControlInfo(controls::HdrModeValues) },
> >       { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> >       { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> >       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
> > @@ -658,9 +661,17 @@ static const std::map<int32_t, RPiController::AfAlgorithm::AfPause> AfPauseTable
> >       { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },
> >  };
> >
> > +static const std::map<int32_t, std::string> HdrModeTable = {
> > +     { controls::HdrModeOff, "Off" },
> > +     { controls::HdrModeMultiExposure, "MultiExposure" },
> > +     { controls::HdrModeSingleExposure, "SingleExposure" },
> > +};
> > +
> >  void IpaBase::applyControls(const ControlList &controls)
> >  {
> > +     using RPiController::AgcAlgorithm;
> >       using RPiController::AfAlgorithm;
> > +     using RPiController::HdrAlgorithm;
> >
> >       /* Clear the return metadata buffer. */
> >       libcameraMetadata_.clear();
> > @@ -1157,6 +1168,34 @@ void IpaBase::applyControls(const ControlList &controls)
> >                       break;
> >               }
> >
> > +             case controls::HDR_MODE: {
> > +                     HdrAlgorithm *hdr = dynamic_cast<HdrAlgorithm *>(controller_.getAlgorithm("hdr"));
> > +                     if (!hdr) {
> > +                             LOG(IPARPI, Warning) << "No HDR algorithm available";
> > +                             break;
> > +                     }
> > +
> > +                     auto mode = HdrModeTable.find(ctrl.second.get<int32_t>());
> > +                     if (mode == HdrModeTable.end()) {
> > +                             LOG(IPARPI, Warning) << "Unrecognised HDR mode";
> > +                             break;
> > +                     }
> > +
> > +                     AgcAlgorithm *agc = dynamic_cast<AgcAlgorithm *>(controller_.getAlgorithm("agc"));
> > +                     if (!agc) {
> > +                             LOG(IPARPI, Warning) << "HDR requires an AGC algorithm";
> > +                             break;
> > +                     }
> > +
> > +                     if (hdr->setMode(mode->second) == 0)
> > +                             agc->setActiveChannels(hdr->getChannels());
> > +                     else
> > +                             LOG(IPARPI, Warning)
> > +                                     << "HDR mode " << mode->second << " not supported";
> > +
> > +                     break;
> > +             }
> > +
> >               default:
> >                       LOG(IPARPI, Warning)
> >                               << "Ctrl " << controls::controls.at(ctrl.first)->name()
> > @@ -1304,6 +1343,16 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
> >               libcameraMetadata_.set(controls::AfPauseState, p);
> >       }
> >
> > +     const HdrStatus *hdrStatus = rpiMetadata.getLocked<HdrStatus>("hdr.status");
> > +     if (hdrStatus) {
> > +             if (hdrStatus->channel == "short")
> > +                     libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelShort);
> > +             else if (hdrStatus->channel == "long")
> > +                     libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelLong);
> > +             else
> > +                     libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelNone);
> > +     }
> > +
> >       metadataReady.emit(libcameraMetadata_);
> >  }
> >
> > diff --git a/src/ipa/rpi/controller/hdr_algorithm.h b/src/ipa/rpi/controller/hdr_algorithm.h
> > new file mode 100644
> > index 00000000..5164d50e
> > --- /dev/null
> > +++ b/src/ipa/rpi/controller/hdr_algorithm.h
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2023, Raspberry Pi Ltd
> > + *
> > + * hdr_algorithm.h - HDR control algorithm interface
> > + */
> > +#pragma once
> > +
> > +#include "algorithm.h"
> > +
> > +namespace RPiController {
> > +
> > +class HdrAlgorithm : public Algorithm
> > +{
> > +public:
> > +     HdrAlgorithm(Controller *controller)
> > +             : Algorithm(controller) {}
> > +     /* An HDR algorithm must provide the following: */
> > +     virtual int setMode(std::string const &modeName) = 0;
> > +     virtual std::vector<unsigned int> getChannels() const = 0;
> > +};
> > +
> > +} /* namespace RPiController */
> > diff --git a/src/ipa/rpi/controller/hdr_status.h b/src/ipa/rpi/controller/hdr_status.h
> > new file mode 100644
> > index 00000000..120dcb1a
> > --- /dev/null
> > +++ b/src/ipa/rpi/controller/hdr_status.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2023 Raspberry Pi Ltd
> > + *
> > + * hdr_status.h - HDR control algorithm status
> > + */
> > +#pragma once
> > +
> > +#include <string>
> > +
> > +/*
> > + * The HDR algorithm process method should post an HdrStatus into the image
> > + * metadata under the tag "hdr.status".
> > + */
> > +
> > +struct HdrStatus {
> > +     std::string mode;
> > +     std::string channel;
> > +
> > +     /* Parameters for programming the stitch block (where available). */
> > +     bool stitch_enable;
> > +     uint16_t thresholdLo;
> > +     uint8_t diffPower;
> > +     double motionThreshold;
>
> I'll let you consider if it's worth adding them now or later.

You're right, I could delete those for the moment. Maybe I should do that.

>
> > +};
> > diff --git a/src/ipa/rpi/controller/meson.build b/src/ipa/rpi/controller/meson.build
> > index 20b9cda9..c9a3e850 100644
> > --- a/src/ipa/rpi/controller/meson.build
> > +++ b/src/ipa/rpi/controller/meson.build
> > @@ -16,6 +16,7 @@ rpi_ipa_controller_sources = files([
> >      'rpi/contrast.cpp',
> >      'rpi/dpc.cpp',
> >      'rpi/geq.cpp',
> > +    'rpi/hdr.cpp',
> >      'rpi/lux.cpp',
> >      'rpi/noise.cpp',
> >      'rpi/sdn.cpp',
> > diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp
> > new file mode 100644
> > index 00000000..2700d35e
> > --- /dev/null
> > +++ b/src/ipa/rpi/controller/rpi/hdr.cpp
> > @@ -0,0 +1,127 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2022 Raspberry Pi Ltd
> > + *
> > + * hdr.cpp - HDR control algorithm
> > + */
> > +
> > +#include "hdr.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include "../agc_status.h"
> > +
> > +using namespace RPiController;
> > +using namespace libcamera;
> > +
> > +LOG_DEFINE_CATEGORY(RPiHdr)
> > +
> > +#define NAME "rpi.hdr"
> > +
> > +void HdrConfig::read(const libcamera::YamlObject &params, const std::string &modeName)
> > +{
> > +     name = modeName;
> > +
> > +     if (!params.contains("cadence"))
> > +             LOG(RPiHdr, Fatal) << "No cadence for HDR mode " << name;
> > +     cadence = params["cadence"].getList<unsigned int>().value();
> > +     if (cadence.empty())
> > +             LOG(RPiHdr, Fatal) << "Empty cadence in HDR mode " << name;
> > +
> > +     /*
> > +      * In the JSON file it's easier to use the channel name as the key, but
> > +      * for us it's convenient to swap them over.
> > +      */
> > +     for (const auto &[k, v] : params["channel_map"].asDict())
> > +             channelMap[v.get<unsigned int>().value()] = k;
> > +}
> > +
> > +Hdr::Hdr(Controller *controller)
> > +     : HdrAlgorithm(controller),
> > +       currentConfig_(nullptr)
> > +{
> > +}
> > +
> > +char const *Hdr::name() const
> > +{
> > +     return NAME;
> > +}
> > +
> > +int Hdr::read(const libcamera::YamlObject &params)
> > +{
> > +     /* Make an "HDR off" mode by default so that tuning files don't have to. */
> > +     HdrConfig &offMode = config_["Off"];
> > +     offMode.name = "Off";
> > +     offMode.cadence = { 0 };
>
> Isn't "Off" part of the config file ?
>         "Off":
>         {
>                 "cadence": [ 0 ]
>         },
>
> > +     currentConfig_ = &offMode;
>
> I understand this should be the default
>
> > +
> > +     for (const auto &[key, value] : params.asDict())
> > +             config_[key].read(value, key);
>
> But won't "Off" be parsed here too ?

Originally it didn't set up a default, but Naush suggested it might be
nice if it did. So I added that, but also felt it would be quite nice
if applications could override that if they wanted.

One example I had in mind was an application that mostly wants to do
SingleExposure-type HDR, but might also want to turn HDR off. Let's
also suppose that the application also wants to use standard AGC
controls on the HDR channel (but not on the HDR "off" channel).

Currently that means the HDR channel would have to be channel 0
(because those other controls only operate on channel 0), meaning
they'd have to move HDR "off" to channel 1 (for example). So it's all
about giving an application complete flexibility about what it can do.

But it might be helpful if I commented that better - to make it clear
you can override the "Off" mode if you want, and that it wasn't a
mistake!

>
> > +
> > +     return 0;
> > +}
> > +
> > +int Hdr::setMode(std::string const &mode)
> > +{
> > +     auto it = config_.find(mode);
> > +     if (it == config_.end()) {
> > +             LOG(RPiHdr, Warning) << "No such HDR mode " << mode;
> > +             return -1;
> > +     }
> > +
> > +     currentConfig_ = &it->second;
> > +
> > +     return 0;
> > +}
> > +
> > +std::vector<unsigned int> Hdr::getChannels() const
> > +{
> > +     return currentConfig_->cadence;
> > +}
> > +
> > +void Hdr::switchMode([[maybe_unused]] CameraMode const &cameraMode,
> > +                  [[maybe_unused]] Metadata *metadata)
> > +{
> > +}
> > +
> > +void Hdr::process([[maybe_unused]] StatisticsPtr &stats, Metadata *imageMetadata)
> > +{
> > +     if (currentConfig_->name == "Off")
> > +             return;
> > +
> > +     /* First find out what AGC channel this is, which comes from the delayed_status. */
> > +     bool channelKnown = false;
> > +     unsigned int agcChannel = 0;
> > +     {
> > +             std::scoped_lock<RPiController::Metadata> lock(*imageMetadata);
> > +             AgcStatus *agcStatus = imageMetadata->getLocked<AgcStatus>("agc.delayed_status");
> > +             if (agcStatus) {
> > +                     channelKnown = true;
> > +                     agcChannel = agcStatus->channel;
> > +             }
> > +     }
> > +
> > +     /* Fill out the HdrStatus information. */
> > +     HdrStatus hdrStatus;
> > +     hdrStatus.mode = currentConfig_->name;
> > +     if (channelKnown) {
> > +             /*
> > +              * Channels that aren't required for HDR processing might come through for
> > +              * various reasons, such as when HDR starts up, or even because the cadence
> > +              * demands some frames that you need for other purposes (e.g. for preview).
> > +              * We'll leave the channel name empty in these cases.
> > +              */
> > +             auto it = currentConfig_->channelMap.find(agcChannel);
> > +             if (it != currentConfig_->channelMap.end())
> > +                     hdrStatus.channel = it->second;
>
> Related to the discussion about the "HdrChannel" metadata, it would be
> trivial returning the channel index here if I got this right

I think you mean that if we used the HdrMode control values directly,
then this would be trivial? That's certainly true, though as a rule we
try to avoid using libcamera control values within our own algorithms.

Mostly we map libcamera control values onto strings, so maybe that
could be the way to go here? Instead of reading an array of AGC
configs, we'd have a dictionary where each config is named "normal",
"short" or "long" (or indeed anything else).

I think that might be better, and addresses at least some of the
"fragility" concerns. I'll try that in the next version and we can see
if we prefer it.

>
> > +     }
> > +
> > +     imageMetadata->set("hdr.status", hdrStatus);
> > +}
> > +
> > +/* Register algorithm with the system. */
> > +static Algorithm *create(Controller *controller)
> > +{
> > +     return (Algorithm *)new Hdr(controller);
> > +}
> > +static RegisterAlgorithm reg(NAME, &create);
> > diff --git a/src/ipa/rpi/controller/rpi/hdr.h b/src/ipa/rpi/controller/rpi/hdr.h
> > new file mode 100644
> > index 00000000..8f6457f2
> > --- /dev/null
> > +++ b/src/ipa/rpi/controller/rpi/hdr.h
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2022, Raspberry Pi Ltd
>
> All of these are 2023 now

Ah yes, thank you.

>
> > + *
> > + * hdr.h - HDR control algorithm
> > + */
> > +#pragma once
> > +
> > +#include <vector>
>
> and string and map ?

Yep, will check!

Thanks again
David

>
> > +
> > +#include "../hdr_algorithm.h"
> > +#include "../hdr_status.h"
> > +
> > +/* This is our implementation of an HDR algorithm. */
> > +
> > +namespace RPiController {
> > +
> > +struct HdrConfig {
> > +     std::string name;
> > +     std::vector<unsigned int> cadence;
> > +     std::map<unsigned int, std::string> channelMap;
> > +
> > +     void read(const libcamera::YamlObject &params, const std::string &name);
> > +};
> > +
> > +class Hdr : public HdrAlgorithm
> > +{
> > +public:
> > +     Hdr(Controller *controller);
> > +     char const *name() const override;
> > +     void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
> > +     int read(const libcamera::YamlObject &params) override;
> > +     void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
> > +     int setMode(std::string const &mode) override;
> > +     std::vector<unsigned int> getChannels() const override;
> > +
> > +private:
> > +     std::map<std::string, HdrConfig> config_;
> > +     const HdrConfig *currentConfig_;
> > +};
> > +
> > +} /* namespace RPiController */
> > --
> > 2.30.2
> >

Patch
diff mbox series 

diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
index f7e7ad5e..231e8f96 100644
--- a/src/ipa/rpi/common/ipa_base.cpp
+++ b/src/ipa/rpi/common/ipa_base.cpp
@@ -24,6 +24,8 @@ 
 #include "controller/ccm_status.h"
 #include "controller/contrast_algorithm.h"
 #include "controller/denoise_algorithm.h"
+#include "controller/hdr_algorithm.h"
+#include "controller/hdr_status.h"
 #include "controller/lux_status.h"
 #include "controller/sharpen_algorithm.h"
 #include "controller/statistics.h"
@@ -67,6 +69,7 @@  const ControlInfoMap::Map ipaControls{
 	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
 	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
 	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
+	{ &controls::HdrMode, ControlInfo(controls::HdrModeValues) },
 	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
 	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
 	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
@@ -658,9 +661,17 @@  static const std::map<int32_t, RPiController::AfAlgorithm::AfPause> AfPauseTable
 	{ controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },
 };
 
+static const std::map<int32_t, std::string> HdrModeTable = {
+	{ controls::HdrModeOff, "Off" },
+	{ controls::HdrModeMultiExposure, "MultiExposure" },
+	{ controls::HdrModeSingleExposure, "SingleExposure" },
+};
+
 void IpaBase::applyControls(const ControlList &controls)
 {
+	using RPiController::AgcAlgorithm;
 	using RPiController::AfAlgorithm;
+	using RPiController::HdrAlgorithm;
 
 	/* Clear the return metadata buffer. */
 	libcameraMetadata_.clear();
@@ -1157,6 +1168,34 @@  void IpaBase::applyControls(const ControlList &controls)
 			break;
 		}
 
+		case controls::HDR_MODE: {
+			HdrAlgorithm *hdr = dynamic_cast<HdrAlgorithm *>(controller_.getAlgorithm("hdr"));
+			if (!hdr) {
+				LOG(IPARPI, Warning) << "No HDR algorithm available";
+				break;
+			}
+
+			auto mode = HdrModeTable.find(ctrl.second.get<int32_t>());
+			if (mode == HdrModeTable.end()) {
+				LOG(IPARPI, Warning) << "Unrecognised HDR mode";
+				break;
+			}
+
+			AgcAlgorithm *agc = dynamic_cast<AgcAlgorithm *>(controller_.getAlgorithm("agc"));
+			if (!agc) {
+				LOG(IPARPI, Warning) << "HDR requires an AGC algorithm";
+				break;
+			}
+
+			if (hdr->setMode(mode->second) == 0)
+				agc->setActiveChannels(hdr->getChannels());
+			else
+				LOG(IPARPI, Warning)
+					<< "HDR mode " << mode->second << " not supported";
+
+			break;
+		}
+
 		default:
 			LOG(IPARPI, Warning)
 				<< "Ctrl " << controls::controls.at(ctrl.first)->name()
@@ -1304,6 +1343,16 @@  void IpaBase::reportMetadata(unsigned int ipaContext)
 		libcameraMetadata_.set(controls::AfPauseState, p);
 	}
 
+	const HdrStatus *hdrStatus = rpiMetadata.getLocked<HdrStatus>("hdr.status");
+	if (hdrStatus) {
+		if (hdrStatus->channel == "short")
+			libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelShort);
+		else if (hdrStatus->channel == "long")
+			libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelLong);
+		else
+			libcameraMetadata_.set(controls::HdrChannel, controls::HdrChannelNone);
+	}
+
 	metadataReady.emit(libcameraMetadata_);
 }
 
diff --git a/src/ipa/rpi/controller/hdr_algorithm.h b/src/ipa/rpi/controller/hdr_algorithm.h
new file mode 100644
index 00000000..5164d50e
--- /dev/null
+++ b/src/ipa/rpi/controller/hdr_algorithm.h
@@ -0,0 +1,23 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2023, Raspberry Pi Ltd
+ *
+ * hdr_algorithm.h - HDR control algorithm interface
+ */
+#pragma once
+
+#include "algorithm.h"
+
+namespace RPiController {
+
+class HdrAlgorithm : public Algorithm
+{
+public:
+	HdrAlgorithm(Controller *controller)
+		: Algorithm(controller) {}
+	/* An HDR algorithm must provide the following: */
+	virtual int setMode(std::string const &modeName) = 0;
+	virtual std::vector<unsigned int> getChannels() const = 0;
+};
+
+} /* namespace RPiController */
diff --git a/src/ipa/rpi/controller/hdr_status.h b/src/ipa/rpi/controller/hdr_status.h
new file mode 100644
index 00000000..120dcb1a
--- /dev/null
+++ b/src/ipa/rpi/controller/hdr_status.h
@@ -0,0 +1,25 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2023 Raspberry Pi Ltd
+ *
+ * hdr_status.h - HDR control algorithm status
+ */
+#pragma once
+
+#include <string>
+
+/*
+ * The HDR algorithm process method should post an HdrStatus into the image
+ * metadata under the tag "hdr.status".
+ */
+
+struct HdrStatus {
+	std::string mode;
+	std::string channel;
+
+	/* Parameters for programming the stitch block (where available). */
+	bool stitch_enable;
+	uint16_t thresholdLo;
+	uint8_t diffPower;
+	double motionThreshold;
+};
diff --git a/src/ipa/rpi/controller/meson.build b/src/ipa/rpi/controller/meson.build
index 20b9cda9..c9a3e850 100644
--- a/src/ipa/rpi/controller/meson.build
+++ b/src/ipa/rpi/controller/meson.build
@@ -16,6 +16,7 @@  rpi_ipa_controller_sources = files([
     'rpi/contrast.cpp',
     'rpi/dpc.cpp',
     'rpi/geq.cpp',
+    'rpi/hdr.cpp',
     'rpi/lux.cpp',
     'rpi/noise.cpp',
     'rpi/sdn.cpp',
diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp
new file mode 100644
index 00000000..2700d35e
--- /dev/null
+++ b/src/ipa/rpi/controller/rpi/hdr.cpp
@@ -0,0 +1,127 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2022 Raspberry Pi Ltd
+ *
+ * hdr.cpp - HDR control algorithm
+ */
+
+#include "hdr.h"
+
+#include <libcamera/base/log.h>
+
+#include "../agc_status.h"
+
+using namespace RPiController;
+using namespace libcamera;
+
+LOG_DEFINE_CATEGORY(RPiHdr)
+
+#define NAME "rpi.hdr"
+
+void HdrConfig::read(const libcamera::YamlObject &params, const std::string &modeName)
+{
+	name = modeName;
+
+	if (!params.contains("cadence"))
+		LOG(RPiHdr, Fatal) << "No cadence for HDR mode " << name;
+	cadence = params["cadence"].getList<unsigned int>().value();
+	if (cadence.empty())
+		LOG(RPiHdr, Fatal) << "Empty cadence in HDR mode " << name;
+
+	/*
+	 * In the JSON file it's easier to use the channel name as the key, but
+	 * for us it's convenient to swap them over.
+	 */
+	for (const auto &[k, v] : params["channel_map"].asDict())
+		channelMap[v.get<unsigned int>().value()] = k;
+}
+
+Hdr::Hdr(Controller *controller)
+	: HdrAlgorithm(controller),
+	  currentConfig_(nullptr)
+{
+}
+
+char const *Hdr::name() const
+{
+	return NAME;
+}
+
+int Hdr::read(const libcamera::YamlObject &params)
+{
+	/* Make an "HDR off" mode by default so that tuning files don't have to. */
+	HdrConfig &offMode = config_["Off"];
+	offMode.name = "Off";
+	offMode.cadence = { 0 };
+	currentConfig_ = &offMode;
+
+	for (const auto &[key, value] : params.asDict())
+		config_[key].read(value, key);
+
+	return 0;
+}
+
+int Hdr::setMode(std::string const &mode)
+{
+	auto it = config_.find(mode);
+	if (it == config_.end()) {
+		LOG(RPiHdr, Warning) << "No such HDR mode " << mode;
+		return -1;
+	}
+
+	currentConfig_ = &it->second;
+
+	return 0;
+}
+
+std::vector<unsigned int> Hdr::getChannels() const
+{
+	return currentConfig_->cadence;
+}
+
+void Hdr::switchMode([[maybe_unused]] CameraMode const &cameraMode,
+		     [[maybe_unused]] Metadata *metadata)
+{
+}
+
+void Hdr::process([[maybe_unused]] StatisticsPtr &stats, Metadata *imageMetadata)
+{
+	if (currentConfig_->name == "Off")
+		return;
+
+	/* First find out what AGC channel this is, which comes from the delayed_status. */
+	bool channelKnown = false;
+	unsigned int agcChannel = 0;
+	{
+		std::scoped_lock<RPiController::Metadata> lock(*imageMetadata);
+		AgcStatus *agcStatus = imageMetadata->getLocked<AgcStatus>("agc.delayed_status");
+		if (agcStatus) {
+			channelKnown = true;
+			agcChannel = agcStatus->channel;
+		}
+	}
+
+	/* Fill out the HdrStatus information. */
+	HdrStatus hdrStatus;
+	hdrStatus.mode = currentConfig_->name;
+	if (channelKnown) {
+		/*
+		 * Channels that aren't required for HDR processing might come through for
+		 * various reasons, such as when HDR starts up, or even because the cadence
+		 * demands some frames that you need for other purposes (e.g. for preview).
+		 * We'll leave the channel name empty in these cases.
+		 */
+		auto it = currentConfig_->channelMap.find(agcChannel);
+		if (it != currentConfig_->channelMap.end())
+			hdrStatus.channel = it->second;
+	}
+
+	imageMetadata->set("hdr.status", hdrStatus);
+}
+
+/* Register algorithm with the system. */
+static Algorithm *create(Controller *controller)
+{
+	return (Algorithm *)new Hdr(controller);
+}
+static RegisterAlgorithm reg(NAME, &create);
diff --git a/src/ipa/rpi/controller/rpi/hdr.h b/src/ipa/rpi/controller/rpi/hdr.h
new file mode 100644
index 00000000..8f6457f2
--- /dev/null
+++ b/src/ipa/rpi/controller/rpi/hdr.h
@@ -0,0 +1,42 @@ 
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2022, Raspberry Pi Ltd
+ *
+ * hdr.h - HDR control algorithm
+ */
+#pragma once
+
+#include <vector>
+
+#include "../hdr_algorithm.h"
+#include "../hdr_status.h"
+
+/* This is our implementation of an HDR algorithm. */
+
+namespace RPiController {
+
+struct HdrConfig {
+	std::string name;
+	std::vector<unsigned int> cadence;
+	std::map<unsigned int, std::string> channelMap;
+
+	void read(const libcamera::YamlObject &params, const std::string &name);
+};
+
+class Hdr : public HdrAlgorithm
+{
+public:
+	Hdr(Controller *controller);
+	char const *name() const override;
+	void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
+	int read(const libcamera::YamlObject &params) override;
+	void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
+	int setMode(std::string const &mode) override;
+	std::vector<unsigned int> getChannels() const override;
+
+private:
+	std::map<std::string, HdrConfig> config_;
+	const HdrConfig *currentConfig_;
+};
+
+} /* namespace RPiController */