| Message ID | 20211125054259.24792-7-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi JM, Patches seems okay, On 11/25/21 11:12 AM, Jean-Michel Hautbois wrote: > The algorithms are using the same function names with specialized > parameters. Instead of duplicating code, introduce a libipa Algorithm > class which implements a base class with template parameters in libipa, > and use it in each IPA. > > As we now won't need an algorithm class for each IPA, move the > documentation to libipa, and make it agnostic of the IPA used. While at > it, fix the IPU3::Algorithm::Awb documentation. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > v4: use #pragma once > --- > src/ipa/ipu3/algorithms/algorithm.h | 12 ++---- > src/ipa/ipu3/algorithms/awb.cpp | 9 +++++ > src/ipa/ipu3/algorithms/meson.build | 1 - > .../{ipu3/algorithms => libipa}/algorithm.cpp | 38 ++++++++----------- > src/ipa/libipa/algorithm.h | 38 +++++++++++++++++++ > src/ipa/libipa/meson.build | 1 + > 6 files changed, 67 insertions(+), 32 deletions(-) > rename src/ipa/{ipu3/algorithms => libipa}/algorithm.cpp (75%) > create mode 100644 src/ipa/libipa/algorithm.h > > diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h > index 16310ab1..d2eecc78 100644 > --- a/src/ipa/ipu3/algorithms/algorithm.h > +++ b/src/ipa/ipu3/algorithms/algorithm.h > @@ -9,21 +9,15 @@ > > #include <libcamera/ipa/ipu3_ipa_interface.h> > > +#include <libipa/algorithm.h> > + > #include "ipa_context.h" > > namespace libcamera { > > namespace ipa::ipu3 { > > -class Algorithm > -{ > -public: > - virtual ~Algorithm() {} > - > - virtual int configure(IPAContext &context, const IPAConfigInfo &configInfo); > - virtual void prepare(IPAContext &context, ipu3_uapi_params *params); > - virtual void process(IPAContext &context, const ipu3_uapi_stats_3a *stats); > -}; > +using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPAConfigInfo, ipu3_uapi_params, ipu3_uapi_stats_3a>; > > } /* namespace ipa::ipu3 */ > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp > index c7bcb20e..1dc27fc9 100644 > --- a/src/ipa/ipu3/algorithms/awb.cpp > +++ b/src/ipa/ipu3/algorithms/awb.cpp > @@ -193,6 +193,9 @@ Awb::Awb() > > Awb::~Awb() = default; > > +/** > + * \copydoc libcamera::ipa::Algorithm::configure > + */ > int Awb::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > @@ -373,6 +376,9 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats) > } > } > > +/** > + * \copydoc libcamera::ipa::Algorithm::process > + */ > void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) > { > calculateWBGains(stats); > @@ -394,6 +400,9 @@ constexpr uint16_t Awb::threshold(float value) > return value * 8191; > } > > +/** > + * \copydoc libcamera::ipa::Algorithm::prepare > + */ > void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) > { > /* > diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build > index 3ec42f72..4db6ae1d 100644 > --- a/src/ipa/ipu3/algorithms/meson.build > +++ b/src/ipa/ipu3/algorithms/meson.build > @@ -2,7 +2,6 @@ > > ipu3_ipa_algorithms = files([ > 'agc.cpp', > - 'algorithm.cpp', > 'awb.cpp', > 'blc.cpp', > 'tone_mapping.cpp', > diff --git a/src/ipa/ipu3/algorithms/algorithm.cpp b/src/ipa/libipa/algorithm.cpp > similarity index 75% > rename from src/ipa/ipu3/algorithms/algorithm.cpp > rename to src/ipa/libipa/algorithm.cpp > index 3e7e3018..398d5372 100644 > --- a/src/ipa/ipu3/algorithms/algorithm.cpp > +++ b/src/ipa/libipa/algorithm.cpp > @@ -2,7 +2,7 @@ > /* > * Copyright (C) 2021, Ideas On Board > * > - * algorithm.cpp - IPU3 control algorithm interface > + * algorithm.cpp - IPA control algorithm interface > */ > > #include "algorithm.h" > @@ -14,11 +14,15 @@ > > namespace libcamera { > > -namespace ipa::ipu3 { > +namespace ipa { > > /** > * \class Algorithm > - * \brief The base class for all IPU3 algorithms > + * \brief The base class for all IPA algorithms > + * \tparam Context The type of shared IPA context > + * \tparam Config The type of the IPA configuration data > + * \tparam Params The type of the ISP specific parameters > + * \tparam Stats The type of the IPA statistics and ISP results > * > * The Algorithm class defines a standard interface for IPA algorithms. By > * abstracting algorithms, it makes possible the implementation of generic code > @@ -26,6 +30,7 @@ namespace ipa::ipu3 { > */ > > /** > + * \fn Algorithm::configure() > * \brief Configure the Algorithm given an IPAConfigInfo > * \param[in] context The shared IPA context > * \param[in] configInfo The IPA configuration data, received from the pipeline > @@ -39,37 +44,30 @@ namespace ipa::ipu3 { > * > * \return 0 if successful, an error code otherwise > */ > -int Algorithm::configure([[maybe_unused]] IPAContext &context, > - [[maybe_unused]] const IPAConfigInfo &configInfo) > -{ > - return 0; > -} > > /** > + * \fn Algorithm::prepare() > * \brief Fill the \a params buffer with ISP processing parameters for a frame > * \param[in] context The shared IPA context > - * \param[out] params The IPU3 specific parameters. > + * \param[out] params The ISP specific parameters. > * > * This function is called for every frame when the camera is running before it > - * is processed by the ImgU to prepare the ImgU processing parameters for that > + * is processed by the ISP to prepare the ISP processing parameters for that > * frame. > * > * Algorithms shall fill in the parameter structure fields appropriately to > - * configure the ImgU processing blocks that they are responsible for. This > + * configure the ISP processing blocks that they are responsible for. This > * includes setting fields and flags that enable those processing blocks. > */ > -void Algorithm::prepare([[maybe_unused]] IPAContext &context, > - [[maybe_unused]] ipu3_uapi_params *params) > -{ > -} > > /** > + * \fn Algorithm::process() > * \brief Process ISP statistics, and run algorithm operations > * \param[in] context The shared IPA context > - * \param[in] stats The IPU3 statistics and ISP results > + * \param[in] stats The IPA statistics and ISP results > * > * This function is called while camera is running for every frame processed by > - * the ImgU, to process statistics generated from that frame by the ImgU. > + * the ISP, to process statistics generated from that frame by the ISP. > * Algorithms shall use this data to run calculations and update their state > * accordingly. > * > @@ -91,11 +89,7 @@ void Algorithm::prepare([[maybe_unused]] IPAContext &context, > * Care shall be taken to ensure the ordering of access to the information > * such that the algorithms use up to date state as required. > */ > -void Algorithm::process([[maybe_unused]] IPAContext &context, > - [[maybe_unused]] const ipu3_uapi_stats_3a *stats) > -{ > -} > > -} /* namespace ipa::ipu3 */ > +} /* namespace ipa */ > > } /* namespace libcamera */ > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h > new file mode 100644 > index 00000000..766aee5d > --- /dev/null > +++ b/src/ipa/libipa/algorithm.h > @@ -0,0 +1,38 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021, Ideas On Board I think Oy is missing here at the end, that;s what I see in other samples in the code base Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > + * > + * algorithm.h - ISP control algorithm interface > + */ > +#pragma once > + > +namespace libcamera { > + > +namespace ipa { > + > +template<typename Context, typename Config, typename Params, typename Stats> > +class Algorithm > +{ > +public: > + virtual ~Algorithm() {} > + > + virtual int configure([[maybe_unused]] Context &context, > + [[maybe_unused]] const Config &configInfo) > + { > + return 0; > + } > + > + virtual void prepare([[maybe_unused]] Context &context, > + [[maybe_unused]] Params *params) > + { > + } > + > + virtual void process([[maybe_unused]] Context &context, > + [[maybe_unused]] const Stats *stats) > + { > + } > +}; > + > +} /* namespace ipa */ > + > +} /* namespace libcamera */ > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > index 4d073a03..161cc5a1 100644 > --- a/src/ipa/libipa/meson.build > +++ b/src/ipa/libipa/meson.build > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: CC0-1.0 > > libipa_headers = files([ > + 'algorithm.h', > 'camera_sensor_helper.h', > 'histogram.h' > ])
diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h index 16310ab1..d2eecc78 100644 --- a/src/ipa/ipu3/algorithms/algorithm.h +++ b/src/ipa/ipu3/algorithms/algorithm.h @@ -9,21 +9,15 @@ #include <libcamera/ipa/ipu3_ipa_interface.h> +#include <libipa/algorithm.h> + #include "ipa_context.h" namespace libcamera { namespace ipa::ipu3 { -class Algorithm -{ -public: - virtual ~Algorithm() {} - - virtual int configure(IPAContext &context, const IPAConfigInfo &configInfo); - virtual void prepare(IPAContext &context, ipu3_uapi_params *params); - virtual void process(IPAContext &context, const ipu3_uapi_stats_3a *stats); -}; +using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPAConfigInfo, ipu3_uapi_params, ipu3_uapi_stats_3a>; } /* namespace ipa::ipu3 */ diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index c7bcb20e..1dc27fc9 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -193,6 +193,9 @@ Awb::Awb() Awb::~Awb() = default; +/** + * \copydoc libcamera::ipa::Algorithm::configure + */ int Awb::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) { @@ -373,6 +376,9 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats) } } +/** + * \copydoc libcamera::ipa::Algorithm::process + */ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) { calculateWBGains(stats); @@ -394,6 +400,9 @@ constexpr uint16_t Awb::threshold(float value) return value * 8191; } +/** + * \copydoc libcamera::ipa::Algorithm::prepare + */ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) { /* diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build index 3ec42f72..4db6ae1d 100644 --- a/src/ipa/ipu3/algorithms/meson.build +++ b/src/ipa/ipu3/algorithms/meson.build @@ -2,7 +2,6 @@ ipu3_ipa_algorithms = files([ 'agc.cpp', - 'algorithm.cpp', 'awb.cpp', 'blc.cpp', 'tone_mapping.cpp', diff --git a/src/ipa/ipu3/algorithms/algorithm.cpp b/src/ipa/libipa/algorithm.cpp similarity index 75% rename from src/ipa/ipu3/algorithms/algorithm.cpp rename to src/ipa/libipa/algorithm.cpp index 3e7e3018..398d5372 100644 --- a/src/ipa/ipu3/algorithms/algorithm.cpp +++ b/src/ipa/libipa/algorithm.cpp @@ -2,7 +2,7 @@ /* * Copyright (C) 2021, Ideas On Board * - * algorithm.cpp - IPU3 control algorithm interface + * algorithm.cpp - IPA control algorithm interface */ #include "algorithm.h" @@ -14,11 +14,15 @@ namespace libcamera { -namespace ipa::ipu3 { +namespace ipa { /** * \class Algorithm - * \brief The base class for all IPU3 algorithms + * \brief The base class for all IPA algorithms + * \tparam Context The type of shared IPA context + * \tparam Config The type of the IPA configuration data + * \tparam Params The type of the ISP specific parameters + * \tparam Stats The type of the IPA statistics and ISP results * * The Algorithm class defines a standard interface for IPA algorithms. By * abstracting algorithms, it makes possible the implementation of generic code @@ -26,6 +30,7 @@ namespace ipa::ipu3 { */ /** + * \fn Algorithm::configure() * \brief Configure the Algorithm given an IPAConfigInfo * \param[in] context The shared IPA context * \param[in] configInfo The IPA configuration data, received from the pipeline @@ -39,37 +44,30 @@ namespace ipa::ipu3 { * * \return 0 if successful, an error code otherwise */ -int Algorithm::configure([[maybe_unused]] IPAContext &context, - [[maybe_unused]] const IPAConfigInfo &configInfo) -{ - return 0; -} /** + * \fn Algorithm::prepare() * \brief Fill the \a params buffer with ISP processing parameters for a frame * \param[in] context The shared IPA context - * \param[out] params The IPU3 specific parameters. + * \param[out] params The ISP specific parameters. * * This function is called for every frame when the camera is running before it - * is processed by the ImgU to prepare the ImgU processing parameters for that + * is processed by the ISP to prepare the ISP processing parameters for that * frame. * * Algorithms shall fill in the parameter structure fields appropriately to - * configure the ImgU processing blocks that they are responsible for. This + * configure the ISP processing blocks that they are responsible for. This * includes setting fields and flags that enable those processing blocks. */ -void Algorithm::prepare([[maybe_unused]] IPAContext &context, - [[maybe_unused]] ipu3_uapi_params *params) -{ -} /** + * \fn Algorithm::process() * \brief Process ISP statistics, and run algorithm operations * \param[in] context The shared IPA context - * \param[in] stats The IPU3 statistics and ISP results + * \param[in] stats The IPA statistics and ISP results * * This function is called while camera is running for every frame processed by - * the ImgU, to process statistics generated from that frame by the ImgU. + * the ISP, to process statistics generated from that frame by the ISP. * Algorithms shall use this data to run calculations and update their state * accordingly. * @@ -91,11 +89,7 @@ void Algorithm::prepare([[maybe_unused]] IPAContext &context, * Care shall be taken to ensure the ordering of access to the information * such that the algorithms use up to date state as required. */ -void Algorithm::process([[maybe_unused]] IPAContext &context, - [[maybe_unused]] const ipu3_uapi_stats_3a *stats) -{ -} -} /* namespace ipa::ipu3 */ +} /* namespace ipa */ } /* namespace libcamera */ diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h new file mode 100644 index 00000000..766aee5d --- /dev/null +++ b/src/ipa/libipa/algorithm.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2021, Ideas On Board + * + * algorithm.h - ISP control algorithm interface + */ +#pragma once + +namespace libcamera { + +namespace ipa { + +template<typename Context, typename Config, typename Params, typename Stats> +class Algorithm +{ +public: + virtual ~Algorithm() {} + + virtual int configure([[maybe_unused]] Context &context, + [[maybe_unused]] const Config &configInfo) + { + return 0; + } + + virtual void prepare([[maybe_unused]] Context &context, + [[maybe_unused]] Params *params) + { + } + + virtual void process([[maybe_unused]] Context &context, + [[maybe_unused]] const Stats *stats) + { + } +}; + +} /* namespace ipa */ + +} /* namespace libcamera */ diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build index 4d073a03..161cc5a1 100644 --- a/src/ipa/libipa/meson.build +++ b/src/ipa/libipa/meson.build @@ -1,6 +1,7 @@ # SPDX-License-Identifier: CC0-1.0 libipa_headers = files([ + 'algorithm.h', 'camera_sensor_helper.h', 'histogram.h' ])