[libcamera-devel] [libcamera-devel 2/5] ipa: libipa: Add init() function to the Algorithm class

[paul.elder at ideasonboard.com]

Hi,

On Thu, May 26, 2022 at 02:19:39AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Florian,
> 
> Thank you for the patch.
> 
> By the way, you don't need to add "libcamera-devel" manually to the
> subject line, the [libcamera-devel] tag is added by the mailing list.
> 
> On Mon, May 23, 2022 at 11:24:32AM +0200, Florian Sylvestre via libcamera-devel wrote:
> > Add the init() function that will be called during algorithm initialization
> > to provide each algorithm the list of algorithms tuning data.
> > Each algorithm will be responsible to grab their corresponding parameters.
> > 
> > Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> > ---
> >  src/ipa/libipa/algorithm.cpp | 15 +++++++++++++++
> >  src/ipa/libipa/algorithm.h   | 10 +++++++++-
> >  2 files changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp
> > index 398d5372..269a4beb 100644
> > --- a/src/ipa/libipa/algorithm.cpp
> > +++ b/src/ipa/libipa/algorithm.cpp
> > @@ -29,6 +29,21 @@ namespace ipa {
> >   * to manage algorithms regardless of their specific type.
> >   */
> >  
> > +/**
> > + * \fn Algorithm::init()
> > + * \brief Configure the Algorithm with default parameters
> > + * \param[in] context The shared IPA context
> > + * \param[in] params The initial parameters used to tune algorithms
> > + *
> > + * This function is called once before the camera is running to get default
> > + * algorithm parameters.
> 
> This may be understood as meaning it's called once every time the camera
> is started. I'd make this a bit clearer:
> 
>  * This function is called once, when the IPA module is initialized, to
>  * initialize the algorithm. The \a params YamlObject contains IPA module
>  * parameters, typically tuning data for all algorithms. The Algorithm is
>  * responsible for reading the parameters relevant to its configuration.

Yeah that's a good clarification.

> 
> > + *
> > + * Algorithms are responsible to read the parameters given and extract their
> > + * parameter configuration.
> > + *
> > + * \return 0 if successful, an error code otherwise
> > + */
> > +
> >  /**
> >   * \fn Algorithm::configure()
> >   * \brief Configure the Algorithm given an IPAConfigInfo
> > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h
> > index 766aee5d..f5be1caf 100644
> > --- a/src/ipa/libipa/algorithm.h
> > +++ b/src/ipa/libipa/algorithm.h
> > @@ -10,12 +10,20 @@ namespace libcamera {
> >  
> >  namespace ipa {
> >  
> > -template<typename Context, typename Config, typename Params, typename Stats>
> > +template<typename Context, typename TuningData,
> 
> As we're trying to push for YAML usage everywhere through libcamera, I'm
> tempted to drop the additional template argument and use YamlObject
> unconditionally in init() instead of TuningData.
> 
> Does anyone have an opinion on this ?

On one hand, since all information will be representable in a yaml file,
it could be good to unify the tuning data into a YamlObject in
Algorithm.

On the other hand, I think that it's not nice for algorithms to have to
deal with yaml directly. As in, the parser could convert the yaml values
into a "proper" data type that can be operated on.

Back to the first hand, the tuning file is only parsed and operated on
at init time, so the convenience of handling proper data types vs raw
yaml values isn't too vital.

Back to the second hand, maybe for tuning parameters that are common
across platforms could have a unified parser and converter in libipa.

After presenting the hands, I suppose I have to make a choice to be my
opinion... I'd say the second hand. tbh I forsee large tuning
parameters, and although they don't need to be operated on, I think
they'd be converted from yaml to parameters in the same way across
different platforms. So instead of extracting each value with get<>()
(like I see in 5/5), we could have a convenience extractor in libipa,
eventually.

That's just my two cents.

And so for that reason, with the wording fix, I think it's good to go:

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> > +	 typename Config, typename Params, typename Stats>
> > +
> >  class Algorithm
> >  {
> >  public:
> >  	virtual ~Algorithm() {}
> >  
> > +	virtual int init([[maybe_unused]] Context &context,
> > +			 [[maybe_unused]] const TuningData *params)
> > +	{
> > +		return 0;
> > +	}
> > +
> >  	virtual int configure([[maybe_unused]] Context &context,
> >  			      [[maybe_unused]] const Config &configInfo)
> >  	{