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

18907 diff mbox series

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>
---
 src/ipa/rpi/controller/rpi/agc_channel.cpp | 42 ++++++++++++++++++++++
 src/ipa/rpi/controller/rpi/agc_channel.h   | 10 ++++++
 2 files changed, 52 insertions(+)

Hi David,

Thank you for your work.

On Mon, 31 Jul 2023 at 10:46, David Plowman via libcamera-devel
<libcamera-devel@lists.libcamera.org> 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",
> 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>
> ---
>  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..ed8a3b30 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;
> +}
> +
> +int readChannelConstraints(std::vector<AgcChannelConstraint> channelConstraints,
> +                          const libcamera::YamlObject &params)

Make this function static?


> +{
> +       int ret = 0;
> +
> +       for (const auto &p : params.asList()) {
> +               AgcChannelConstraint constraint;
> +               ret = constraint.read(p);
> +               if (ret)
> +                       return ret;
> +
> +               channelConstraints.push_back(std::move(constraint));

Do we need to move here?  Probably no practical difference though.

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> +       }
> +
> +       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;
> --
> 2.30.2
>

Hi Naush

Thanks for the review.

On Tue, 22 Aug 2023 at 13:46, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi David,
>
> Thank you for your work.
>
> On Mon, 31 Jul 2023 at 10:46, David Plowman via libcamera-devel
> <libcamera-devel@lists.libcamera.org> 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",
> > 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>
> > ---
> >  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..ed8a3b30 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;
> > +}
> > +
> > +int readChannelConstraints(std::vector<AgcChannelConstraint> channelConstraints,
> > +                          const libcamera::YamlObject &params)
>
> Make this function static?

Will do.

>
>
> > +{
> > +       int ret = 0;
> > +
> > +       for (const auto &p : params.asList()) {
> > +               AgcChannelConstraint constraint;
> > +               ret = constraint.read(p);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               channelConstraints.push_back(std::move(constraint));
>
> Do we need to move here?  Probably no practical difference though.

Yep, will change!

Thanks
David

>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>
> > +       }
> > +
> > +       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;
> > --
> > 2.30.2
> >