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

[paul.elder at ideasonboard.com]

Hi Laurent,

On Thu, Jun 09, 2022 at 01:08:08AM +0300, Laurent Pinchart wrote:
> 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,

tbh, yeah, that's what I was envisioning.

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

Yeah, so probably the first hand is better.

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

Hm yeah I think you're right.

Alright, let's go for the first hand then.


Paul

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