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

[Laurent Pinchart]

Hi Paul,

On Fri, May 27, 2022 at 03:46:45PM +0900, paul.elder at ideasonboard.com wrote:
> 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.

How would you envision this ? A set of structures that would be specific
to particular algorithms (and to a particular IPA module) ? If so,
someone would need to convert the YAML data to that structure, and
that's exactly what the Algorithm::init() function does :-) If we were
to split that out of the algorithm implementation, wouldn't we end up
with code that is algorithm-specific but would live elsewhere ?

> 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.

That's possible, although I wouldn't expect it to be very common. In any
case, the Algorithm::init() function could call helpers from libipa if
needed.

> 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)
> > >  	{

-- 
Regards,

Laurent Pinchart