| Message ID | 20220523092435.475510-3-fsylvestre@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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. > + * > + * 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 ? > + 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) > {
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@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@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) > > {
Hi Paul, On Fri, May 27, 2022 at 03:46:45PM +0900, paul.elder@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@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@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) > > > {
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@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@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@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) > > > > {
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. + * + * 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, + 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) {
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@baylibre.com> --- src/ipa/libipa/algorithm.cpp | 15 +++++++++++++++ src/ipa/libipa/algorithm.h | 10 +++++++++- 2 files changed, 24 insertions(+), 1 deletion(-)