[PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with YamlObject::get()

[Kieran Bingham]

Quoting Laurent Pinchart (2024-06-13 12:17:29)
> On Thu, Jun 13, 2024 at 12:06:33PM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2024-06-13 02:39:43)
> > > Now that deserializing a Pwl object from YAML data is possible using the
> > > YamlObject::get() function, replace all usage of Pwl::readYaml() to
> > > prepare for its removal.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  src/ipa/rpi/controller/rpi/af.cpp          | 2 +-
> > >  src/ipa/rpi/controller/rpi/agc_channel.cpp | 9 +++++----
> > >  src/ipa/rpi/controller/rpi/awb.cpp         | 3 ++-
> > >  src/ipa/rpi/controller/rpi/ccm.cpp         | 6 +++---
> > >  src/ipa/rpi/controller/rpi/contrast.cpp    | 4 +++-
> > >  src/ipa/rpi/controller/rpi/geq.cpp         | 6 +++---
> > >  src/ipa/rpi/controller/rpi/hdr.cpp         | 4 ++--
> > >  src/ipa/rpi/controller/rpi/tonemap.cpp     | 2 +-
> > >  8 files changed, 20 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/src/ipa/rpi/controller/rpi/af.cpp b/src/ipa/rpi/controller/rpi/af.cpp
> > > index 304629d6d4e5..5ca76dd98b4b 100644
> > > --- a/src/ipa/rpi/controller/rpi/af.cpp
> > > +++ b/src/ipa/rpi/controller/rpi/af.cpp
> > > @@ -139,7 +139,7 @@ int Af::CfgParams::read(const libcamera::YamlObject &params)
> > >         readNumber<uint32_t>(skipFrames, params, "skip_frames");
> > >  
> > >         if (params.contains("map"))
> > > -               map.readYaml(params["map"]);
> > > +               map = params["map"].get<ipa::Pwl>(ipa::Pwl{});
> > 
> > I look forward to your resurrection of the default constructor candidate
> > in the optionals!
> 
> Me too :-) The above would then be written
> 
>                 map = params["map"].get<ipa::Pwl>().value_or(utils::defopt);
> 
> I'll let you decide if you like it better or not.

Hrm ... it's ... longer ;-) I thought it would  mean we could write:
		map = params["map"].get<ipa::Pwl>();

But oh well, lets see later.


> 
> > >         else
> > >                 LOG(RPiAf, Warning) << "No map defined";
> > >  
> > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > > index a381dd972215..cf2565a83836 100644
> > > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > > @@ -130,7 +130,8 @@ int AgcConstraint::read(const libcamera::YamlObject &params)
> > >                 return -EINVAL;
> > >         qHi = *value;
> > >  
> > > -       return yTarget.readYaml(params["y_target"]);
> > > +       yTarget = params["y_target"].get<ipa::Pwl>(ipa::Pwl{});
> > > +       return yTarget.empty() ? -EINVAL : 0;
> > 
> > Utlimately this is the biggest change here. Instead of returning an
> > error - it will return a default construction or the result. Pwl is fine
> > here as we can check that it's empty or not - but I wonder what happens
> > on more complex structures like a Matrix where we default construct to
> > an identity ...
> > 
> > How would that be handled?
> 
> With a tiny bit more code:
> 
>         std::optional<ipa::Pwl> pwl = params["y_target"].get<ipa::Pwl>();
>         if (!pwl)
>                 return -EINVAL;
> 
>         yTarget = *pwl;
> 
> > It seems because Vector::readYaml wasn't used yet - I can't see an
> > equivelent implemention for that concept.
> > 
> > I'm curious how we make this expressive for the three conditions though:
> > 
> >  1. Get succeeds, value returned.
> 
>         std::optional<ipa::Pwl> pwl = params["y_target"].get<ipa::Pwl>();
>         if (pwl)
>                 /* Success */
> 
> For types that include a way to check if they are valid, you can also
> 
>         ipa::Pwl pwl = params["y_target"].get<ipa::Pwl>(ipa::Pwl{});
>         if (!pwl.empty())
>                 /* Success */
> 
> >  2. Get fails desired value returned.
> 
> I assume you mean default value returned ?

Yes,


> 
>         ipa::Pwl pwl = params["y_target"].get<ipa::Pwl>(ipa::Pwl{});
> 
> >  3. Get fails Do not want a default value, and knowing it failed is
> >     important.
> 
>         std::optional<ipa::Pwl> pwl = params["y_target"].get<ipa::Pwl>();
>         if (!pwl)
>                 /* Failure */
> 

Ok - perfect - all cases handled!

Thanks for the clarification.

--
Kieran