[libcamera-devel,v2,4/5] ipa: rpi: agc: Add AgcChannelConstraint class
diff mbox series

Message ID 20230823120915.18621-5-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Multi-channel AGC
Related show

Commit Message

David Plowman Aug. 23, 2023, 12:09 p.m. UTC 
A channel constraint is somewhat similar to the upper/lower bound
constraints that we use elsewhere, but these constraints apply between
multiple AGC channels. For example, it lets you say things like "don't
let the channel 1 total exposure be more than 8x that of channel 0",
and so on. By using both an upper and lower bound constraint, you
could fix one AGC channel always to be a fixed ratio of another.

Also read a vector of them (if present) when loading the tuning file.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/controller/rpi/agc_channel.cpp | 42 ++++++++++++++++++++++
 src/ipa/rpi/controller/rpi/agc_channel.h   | 10 ++++++
 2 files changed, 52 insertions(+)

Comments

Jacopo Mondi Aug. 30, 2023, 8:13 a.m. UTC | #1 
Hi David

On Wed, Aug 23, 2023 at 01:09:14PM +0100, David Plowman via libcamera-devel wrote:
> A channel constraint is somewhat similar to the upper/lower bound
> constraints that we use elsewhere, but these constraints apply between
> multiple AGC channels. For example, it lets you say things like "don't
> let the channel 1 total exposure be more than 8x that of channel 0",

I haven't seen an example of a config file, can you make an example of
how you would express the above with UPPER and LOWER ?

> and so on. By using both an upper and lower bound constraint, you
> could fix one AGC channel always to be a fixed ratio of another.
>
> Also read a vector of them (if present) when loading the tuning file.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/rpi/controller/rpi/agc_channel.cpp | 42 ++++++++++++++++++++++
>  src/ipa/rpi/controller/rpi/agc_channel.h   | 10 ++++++
>  2 files changed, 52 insertions(+)
>
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> index ddec611f..44198c2c 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> @@ -173,6 +173,42 @@ readConstraintModes(std::map<std::string, AgcConstraintMode> &constraintModes,
>  	return { 0, first };
>  }
>
> +int AgcChannelConstraint::read(const libcamera::YamlObject &params)
> +{
> +	std::string boundString = params["bound"].get<std::string>("");
> +	transform(boundString.begin(), boundString.end(),
> +		  boundString.begin(), ::toupper);

would this allow "upper" and "UPPER" ?

> +	if (boundString != "UPPER" && boundString != "LOWER") {
> +		LOG(RPiAgc, Error) << "AGC channelconstraint type should be UPPER or LOWER";
> +		return -EINVAL;
> +	}
> +	bound = boundString == "UPPER" ? Bound::UPPER : Bound::LOWER;
> +
> +	auto value = params["factor"].get<double>();
> +	if (!value)
> +		return -EINVAL;

Does this deserve an error message ?

> +	factor = *value;
> +
> +	return 0;
> +}
> +
> +static int readChannelConstraints(std::vector<AgcChannelConstraint> &channelConstraints,
> +				  const libcamera::YamlObject &params)
> +{
> +	int ret = 0;

no need

> +
> +	for (const auto &p : params.asList()) {
> +		AgcChannelConstraint constraint;
> +		ret = constraint.read(p);

int ret = ..

> +		if (ret)
> +			return ret;
> +
> +		channelConstraints.push_back(constraint);
> +	}
> +
> +	return ret;

and return 0

> +}
> +
>  int AgcConfig::read(const libcamera::YamlObject &params)
>  {
>  	LOG(RPiAgc, Debug) << "AgcConfig";
> @@ -191,6 +227,12 @@ int AgcConfig::read(const libcamera::YamlObject &params)
>  	if (ret)
>  		return ret;
>
> +	if (params.contains("channel_constraints")) {
> +		ret = readChannelConstraints(channelConstraints, params["channel_constraints"]);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = yTarget.read(params["y_target"]);
>  	if (ret)
>  		return ret;
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h
> index 0e3d44b0..446125ef 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.h
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h
> @@ -42,11 +42,21 @@ struct AgcConstraint {
>
>  typedef std::vector<AgcConstraint> AgcConstraintMode;
>
> +struct AgcChannelConstraint {
> +	enum class Bound { LOWER = 0,
> +			   UPPER = 1 };
> +	Bound bound;
> +	unsigned int channel;
> +	double factor;
> +	int read(const libcamera::YamlObject &params);
> +};
> +
>  struct AgcConfig {
>  	int read(const libcamera::YamlObject &params);
>  	std::map<std::string, AgcMeteringMode> meteringModes;
>  	std::map<std::string, AgcExposureMode> exposureModes;
>  	std::map<std::string, AgcConstraintMode> constraintModes;
> +	std::vector<AgcChannelConstraint> channelConstraints;
>  	Pwl yTarget;
>  	double speed;
>  	uint16_t startupFrames;
> --
> 2.30.2
>
David Plowman Sept. 11, 2023, 2:43 p.m. UTC | #2 
Hi Jacopo

Thanks for the review.

On Wed, 30 Aug 2023 at 09:13, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi David
>
> On Wed, Aug 23, 2023 at 01:09:14PM +0100, David Plowman via libcamera-devel wrote:
> > A channel constraint is somewhat similar to the upper/lower bound
> > constraints that we use elsewhere, but these constraints apply between
> > multiple AGC channels. For example, it lets you say things like "don't
> > let the channel 1 total exposure be more than 8x that of channel 0",
>
> I haven't seen an example of a config file, can you make an example of
> how you would express the above with UPPER and LOWER ?

Yes, given that I'm not using this feature anywhere (though I expect
other people may want to), then I should supply something. What would
be most appropriate do you think, extending the commit message,
comments in the source file or something different?

>
> > and so on. By using both an upper and lower bound constraint, you
> > could fix one AGC channel always to be a fixed ratio of another.
> >
> > Also read a vector of them (if present) when loading the tuning file.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/rpi/controller/rpi/agc_channel.cpp | 42 ++++++++++++++++++++++
> >  src/ipa/rpi/controller/rpi/agc_channel.h   | 10 ++++++
> >  2 files changed, 52 insertions(+)
> >
> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > index ddec611f..44198c2c 100644
> > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > @@ -173,6 +173,42 @@ readConstraintModes(std::map<std::string, AgcConstraintMode> &constraintModes,
> >       return { 0, first };
> >  }
> >
> > +int AgcChannelConstraint::read(const libcamera::YamlObject &params)
> > +{
> > +     std::string boundString = params["bound"].get<std::string>("");
> > +     transform(boundString.begin(), boundString.end(),
> > +               boundString.begin(), ::toupper);
>
> would this allow "upper" and "UPPER" ?

Yes, or "Upper" or even "uPpEr" if you want! But failing just on
account of the case doesn't seem very helpful.

>
> > +     if (boundString != "UPPER" && boundString != "LOWER") {
> > +             LOG(RPiAgc, Error) << "AGC channelconstraint type should be UPPER or LOWER";
> > +             return -EINVAL;
> > +     }
> > +     bound = boundString == "UPPER" ? Bound::UPPER : Bound::LOWER;
> > +
> > +     auto value = params["factor"].get<double>();
> > +     if (!value)
> > +             return -EINVAL;
>
> Does this deserve an error message ?

Good point, I think it should.

>
> > +     factor = *value;
> > +
> > +     return 0;
> > +}
> > +
> > +static int readChannelConstraints(std::vector<AgcChannelConstraint> &channelConstraints,
> > +                               const libcamera::YamlObject &params)
> > +{
> > +     int ret = 0;
>
> no need
>
> > +
> > +     for (const auto &p : params.asList()) {
> > +             AgcChannelConstraint constraint;
> > +             ret = constraint.read(p);
>
> int ret = ..
>
> > +             if (ret)
> > +                     return ret;
> > +
> > +             channelConstraints.push_back(constraint);
> > +     }
> > +
> > +     return ret;
>
> and return 0

Yep, I can make those changes!

Thanks
David

>
> > +}
> > +
> >  int AgcConfig::read(const libcamera::YamlObject &params)
> >  {
> >       LOG(RPiAgc, Debug) << "AgcConfig";
> > @@ -191,6 +227,12 @@ int AgcConfig::read(const libcamera::YamlObject &params)
> >       if (ret)
> >               return ret;
> >
> > +     if (params.contains("channel_constraints")) {
> > +             ret = readChannelConstraints(channelConstraints, params["channel_constraints"]);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       ret = yTarget.read(params["y_target"]);
> >       if (ret)
> >               return ret;
> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h
> > index 0e3d44b0..446125ef 100644
> > --- a/src/ipa/rpi/controller/rpi/agc_channel.h
> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.h
> > @@ -42,11 +42,21 @@ struct AgcConstraint {
> >
> >  typedef std::vector<AgcConstraint> AgcConstraintMode;
> >
> > +struct AgcChannelConstraint {
> > +     enum class Bound { LOWER = 0,
> > +                        UPPER = 1 };
> > +     Bound bound;
> > +     unsigned int channel;
> > +     double factor;
> > +     int read(const libcamera::YamlObject &params);
> > +};
> > +
> >  struct AgcConfig {
> >       int read(const libcamera::YamlObject &params);
> >       std::map<std::string, AgcMeteringMode> meteringModes;
> >       std::map<std::string, AgcExposureMode> exposureModes;
> >       std::map<std::string, AgcConstraintMode> constraintModes;
> > +     std::vector<AgcChannelConstraint> channelConstraints;
> >       Pwl yTarget;
> >       double speed;
> >       uint16_t startupFrames;
> > --
> > 2.30.2
> >

Patch
diff mbox series 

diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
index ddec611f..44198c2c 100644
--- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
+++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
@@ -173,6 +173,42 @@  readConstraintModes(std::map<std::string, AgcConstraintMode> &constraintModes,
 	return { 0, first };
 }
 
+int AgcChannelConstraint::read(const libcamera::YamlObject &params)
+{
+	std::string boundString = params["bound"].get<std::string>("");
+	transform(boundString.begin(), boundString.end(),
+		  boundString.begin(), ::toupper);
+	if (boundString != "UPPER" && boundString != "LOWER") {
+		LOG(RPiAgc, Error) << "AGC channelconstraint type should be UPPER or LOWER";
+		return -EINVAL;
+	}
+	bound = boundString == "UPPER" ? Bound::UPPER : Bound::LOWER;
+
+	auto value = params["factor"].get<double>();
+	if (!value)
+		return -EINVAL;
+	factor = *value;
+
+	return 0;
+}
+
+static int readChannelConstraints(std::vector<AgcChannelConstraint> &channelConstraints,
+				  const libcamera::YamlObject &params)
+{
+	int ret = 0;
+
+	for (const auto &p : params.asList()) {
+		AgcChannelConstraint constraint;
+		ret = constraint.read(p);
+		if (ret)
+			return ret;
+
+		channelConstraints.push_back(constraint);
+	}
+
+	return ret;
+}
+
 int AgcConfig::read(const libcamera::YamlObject &params)
 {
 	LOG(RPiAgc, Debug) << "AgcConfig";
@@ -191,6 +227,12 @@  int AgcConfig::read(const libcamera::YamlObject &params)
 	if (ret)
 		return ret;
 
+	if (params.contains("channel_constraints")) {
+		ret = readChannelConstraints(channelConstraints, params["channel_constraints"]);
+		if (ret)
+			return ret;
+	}
+
 	ret = yTarget.read(params["y_target"]);
 	if (ret)
 		return ret;
diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h
index 0e3d44b0..446125ef 100644
--- a/src/ipa/rpi/controller/rpi/agc_channel.h
+++ b/src/ipa/rpi/controller/rpi/agc_channel.h
@@ -42,11 +42,21 @@  struct AgcConstraint {
 
 typedef std::vector<AgcConstraint> AgcConstraintMode;
 
+struct AgcChannelConstraint {
+	enum class Bound { LOWER = 0,
+			   UPPER = 1 };
+	Bound bound;
+	unsigned int channel;
+	double factor;
+	int read(const libcamera::YamlObject &params);
+};
+
 struct AgcConfig {
 	int read(const libcamera::YamlObject &params);
 	std::map<std::string, AgcMeteringMode> meteringModes;
 	std::map<std::string, AgcExposureMode> exposureModes;
 	std::map<std::string, AgcConstraintMode> constraintModes;
+	std::vector<AgcChannelConstraint> channelConstraints;
 	Pwl yTarget;
 	double speed;
 	uint16_t startupFrames;