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

[David Plowman]

Hi Jacopo

Thanks for the review.

On Wed, 30 Aug 2023 at 09:13, Jacopo Mondi
<jacopo.mondi at 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 at raspberrypi.com>
> > Reviewed-by: Naushir Patuck <naush at 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
> >