| Message ID | 20210812165243.276977-2-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jean-Michel, Thank you for the patch. On Thu, Aug 12, 2021 at 06:52:34PM +0200, Jean-Michel Hautbois wrote: > The abstract Algorithm class was originally placed in libipa as an attempt to > define a generic algorithm container. > This was a little optimistic and pushed a bit far too early. Agreed. I'm looking forward to seeing IPU3 code going back to libipa in the future, but it will be better done after moving forward with the IPU3-specific implementation. > Move the Algorithm class into the IPU3 which is the only user of the class, > as we adapt it to support modular algorithm components for the IPU3. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > Reviewed-by: Kieran Bingham kieran.bingham@ideasonboard.com > --- > src/ipa/ipu3/algorithms/algorithm.h | 24 ++++++++++++++++++ > src/ipa/ipu3/algorithms/meson.build | 4 +++ > src/ipa/ipu3/ipu3_agc.h | 2 +- > src/ipa/ipu3/ipu3_awb.h | 2 +- > src/ipa/ipu3/meson.build | 4 +++ > src/ipa/libipa/algorithm.cpp | 39 ----------------------------- > src/ipa/libipa/algorithm.h | 24 ------------------ > src/ipa/libipa/meson.build | 2 -- > 8 files changed, 34 insertions(+), 67 deletions(-) > create mode 100644 src/ipa/ipu3/algorithms/algorithm.h > create mode 100644 src/ipa/ipu3/algorithms/meson.build > delete mode 100644 src/ipa/libipa/algorithm.cpp > delete mode 100644 src/ipa/libipa/algorithm.h > > diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h > new file mode 100644 > index 00000000..072f01c4 > --- /dev/null > +++ b/src/ipa/ipu3/algorithms/algorithm.h Food for thoughts and not necessarily something to be addressed now, but I wonder if the algorithms subdirectory is needed. > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2021, Ideas On Board > + * > + * algorithm.h - IPU3 control algorithm interface > + */ > +#ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ > +#define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ > + > +namespace libcamera { > + > +namespace ipa::ipu3 { > + > +class Algorithm > +{ > +public: > + virtual ~Algorithm() {} > +}; > + > +} /* namespace ipa::ipu3 */ > + > +} /* namespace libcamera */ > + > +#endif /* __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ */ > diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build > new file mode 100644 > index 00000000..67148333 > --- /dev/null > +++ b/src/ipa/ipu3/algorithms/meson.build > @@ -0,0 +1,4 @@ > +# SPDX-License-Identifier: CC0-1.0 > + > +ipu3_ipa_algorithms = files([ > +]) > diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h > index 9f3d4257..f00b98d6 100644 > --- a/src/ipa/ipu3/ipu3_agc.h > +++ b/src/ipa/ipu3/ipu3_agc.h > @@ -13,7 +13,7 @@ > > #include <libcamera/geometry.h> > > -#include "libipa/algorithm.h" > +#include "algorithms/algorithm.h" > > namespace libcamera { > > diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h > index 122cf68c..ea2d4320 100644 > --- a/src/ipa/ipu3/ipu3_awb.h > +++ b/src/ipa/ipu3/ipu3_awb.h > @@ -13,7 +13,7 @@ > > #include <libcamera/geometry.h> > > -#include "libipa/algorithm.h" > +#include "algorithms/algorithm.h" > > namespace libcamera { > > diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build > index b6364190..fcb27d68 100644 > --- a/src/ipa/ipu3/meson.build > +++ b/src/ipa/ipu3/meson.build > @@ -1,5 +1,7 @@ > # SPDX-License-Identifier: CC0-1.0 > > +subdir('algorithms') > + > ipa_name = 'ipa_ipu3' > > ipu3_ipa_sources = files([ > @@ -8,6 +10,8 @@ ipu3_ipa_sources = files([ > 'ipu3_awb.cpp', > ]) > > +ipu3_ipa_sources += ipu3_ipa_algorithms > + > mod = shared_module(ipa_name, > [ipu3_ipa_sources, libcamera_generated_ipa_headers], > name_prefix : '', > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp > deleted file mode 100644 > index 930f9353..00000000 > --- a/src/ipa/libipa/algorithm.cpp > +++ /dev/null > @@ -1,39 +0,0 @@ > -/* SPDX-License-Identifier: LGPL-2.1-or-later */ > -/* > - * Copyright (C) 2021, Ideas On Board > - * > - * algorithm.cpp - ISP control algorithms > - */ > - > -#include "algorithm.h" > - > -/** > - * \file algorithm.h > - * \brief Algorithm common interface > - */ > - > -namespace libcamera { > - > -/** > - * \brief The IPA namespace > - * > - * The IPA namespace groups all types specific to IPA modules. It serves as the > - * top-level namespace for the IPA library libipa, and also contains > - * module-specific namespaces for IPA modules. > - */ > -namespace ipa { Not documenting the namespace may cause issues with Doxygen. Looks like we need a src/ipa/libipa/libipa.cpp file for this, even if it contains no code. We can leverage it in the future to add more global libipa documentation, and possibly code too. > - > -/** > - * \class Algorithm > - * \brief The base class for all IPA algorithms > - * > - * The Algorithm class defines a standard interface for IPA algorithms. By > - * abstracting algorithms, it makes possible the implementation of generic code > - * to manage algorithms regardless of their specific type. > - */ Could we keep the documentation for this class ? While it's now IPU3-specific, it's still the backbone of the IPA structure, and documenting the design will be useful as a reference for future IPA implementations. > - > -Algorithm::~Algorithm() = default; > - > -} /* namespace ipa */ > - > -} /* namespace libcamera */ > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h > deleted file mode 100644 > index 89cee4c4..00000000 > --- a/src/ipa/libipa/algorithm.h > +++ /dev/null > @@ -1,24 +0,0 @@ > -/* SPDX-License-Identifier: LGPL-2.1-or-later */ > -/* > - * Copyright (C) 2021, Ideas On Board > - * > - * algorithm.h - ISP control algorithm interface > - */ > -#ifndef __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__ > -#define __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__ > - > -namespace libcamera { > - > -namespace ipa { > - > -class Algorithm > -{ > -public: > - virtual ~Algorithm(); > -}; > - > -} /* namespace ipa */ > - > -} /* namespace libcamera */ > - > -#endif /* __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__ */ > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > index 3fda7c00..2a694b1e 100644 > --- a/src/ipa/libipa/meson.build > +++ b/src/ipa/libipa/meson.build > @@ -1,13 +1,11 @@ > # SPDX-License-Identifier: CC0-1.0 > > libipa_headers = files([ > - 'algorithm.h', > 'camera_sensor_helper.h', > 'histogram.h' > ]) > > libipa_sources = files([ > - 'algorithm.cpp', > 'camera_sensor_helper.cpp', > 'histogram.cpp' > ])
Hi Laurent, On 15/08/2021 19:43, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Thu, Aug 12, 2021 at 06:52:34PM +0200, Jean-Michel Hautbois wrote: >> The abstract Algorithm class was originally placed in libipa as an attempt to >> define a generic algorithm container. >> This was a little optimistic and pushed a bit far too early. > > Agreed. I'm looking forward to seeing IPU3 code going back to libipa in > the future, but it will be better done after moving forward with the > IPU3-specific implementation. > >> Move the Algorithm class into the IPU3 which is the only user of the class, >> as we adapt it to support modular algorithm components for the IPU3. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> Reviewed-by: Kieran Bingham kieran.bingham@ideasonboard.com >> --- >> src/ipa/ipu3/algorithms/algorithm.h | 24 ++++++++++++++++++ >> src/ipa/ipu3/algorithms/meson.build | 4 +++ >> src/ipa/ipu3/ipu3_agc.h | 2 +- >> src/ipa/ipu3/ipu3_awb.h | 2 +- >> src/ipa/ipu3/meson.build | 4 +++ >> src/ipa/libipa/algorithm.cpp | 39 ----------------------------- >> src/ipa/libipa/algorithm.h | 24 ------------------ >> src/ipa/libipa/meson.build | 2 -- >> 8 files changed, 34 insertions(+), 67 deletions(-) >> create mode 100644 src/ipa/ipu3/algorithms/algorithm.h >> create mode 100644 src/ipa/ipu3/algorithms/meson.build >> delete mode 100644 src/ipa/libipa/algorithm.cpp >> delete mode 100644 src/ipa/libipa/algorithm.h >> >> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h >> new file mode 100644 >> index 00000000..072f01c4 >> --- /dev/null >> +++ b/src/ipa/ipu3/algorithms/algorithm.h > > Food for thoughts and not necessarily something to be addressed now, but > I wonder if the algorithms subdirectory is needed. > >> @@ -0,0 +1,24 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2021, Ideas On Board >> + * >> + * algorithm.h - IPU3 control algorithm interface >> + */ >> +#ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ >> +#define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ >> + >> +namespace libcamera { >> + >> +namespace ipa::ipu3 { >> + >> +class Algorithm >> +{ >> +public: >> + virtual ~Algorithm() {} >> +}; >> + >> +} /* namespace ipa::ipu3 */ >> + >> +} /* namespace libcamera */ >> + >> +#endif /* __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ */ >> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build >> new file mode 100644 >> index 00000000..67148333 >> --- /dev/null >> +++ b/src/ipa/ipu3/algorithms/meson.build >> @@ -0,0 +1,4 @@ >> +# SPDX-License-Identifier: CC0-1.0 >> + >> +ipu3_ipa_algorithms = files([ >> +]) >> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h >> index 9f3d4257..f00b98d6 100644 >> --- a/src/ipa/ipu3/ipu3_agc.h >> +++ b/src/ipa/ipu3/ipu3_agc.h >> @@ -13,7 +13,7 @@ >> >> #include <libcamera/geometry.h> >> >> -#include "libipa/algorithm.h" >> +#include "algorithms/algorithm.h" >> >> namespace libcamera { >> >> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h >> index 122cf68c..ea2d4320 100644 >> --- a/src/ipa/ipu3/ipu3_awb.h >> +++ b/src/ipa/ipu3/ipu3_awb.h >> @@ -13,7 +13,7 @@ >> >> #include <libcamera/geometry.h> >> >> -#include "libipa/algorithm.h" >> +#include "algorithms/algorithm.h" >> >> namespace libcamera { >> >> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build >> index b6364190..fcb27d68 100644 >> --- a/src/ipa/ipu3/meson.build >> +++ b/src/ipa/ipu3/meson.build >> @@ -1,5 +1,7 @@ >> # SPDX-License-Identifier: CC0-1.0 >> >> +subdir('algorithms') >> + >> ipa_name = 'ipa_ipu3' >> >> ipu3_ipa_sources = files([ >> @@ -8,6 +10,8 @@ ipu3_ipa_sources = files([ >> 'ipu3_awb.cpp', >> ]) >> >> +ipu3_ipa_sources += ipu3_ipa_algorithms >> + >> mod = shared_module(ipa_name, >> [ipu3_ipa_sources, libcamera_generated_ipa_headers], >> name_prefix : '', >> diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp >> deleted file mode 100644 >> index 930f9353..00000000 >> --- a/src/ipa/libipa/algorithm.cpp >> +++ /dev/null >> @@ -1,39 +0,0 @@ >> -/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> -/* >> - * Copyright (C) 2021, Ideas On Board >> - * >> - * algorithm.cpp - ISP control algorithms >> - */ >> - >> -#include "algorithm.h" >> - >> -/** >> - * \file algorithm.h >> - * \brief Algorithm common interface >> - */ >> - >> -namespace libcamera { >> - >> -/** >> - * \brief The IPA namespace >> - * >> - * The IPA namespace groups all types specific to IPA modules. It serves as the >> - * top-level namespace for the IPA library libipa, and also contains >> - * module-specific namespaces for IPA modules. >> - */ >> -namespace ipa { > > Not documenting the namespace may cause issues with Doxygen. Looks like > we need a src/ipa/libipa/libipa.cpp file for this, even if it contains > no code. We can leverage it in the future to add more global libipa > documentation, and possibly code too. > OK, I will create an empty libipa.cpp file with the ipa namespace documented. >> - >> -/** >> - * \class Algorithm >> - * \brief The base class for all IPA algorithms >> - * >> - * The Algorithm class defines a standard interface for IPA algorithms. By >> - * abstracting algorithms, it makes possible the implementation of generic code >> - * to manage algorithms regardless of their specific type. >> - */ > > Could we keep the documentation for this class ? While it's now > IPU3-specific, it's still the backbone of the IPA structure, and > documenting the design will be useful as a reference for future IPA > implementations. > Sure. It means that I need to create a algorithm.cpp file then ;-) ? >> - >> -Algorithm::~Algorithm() = default; >> - >> -} /* namespace ipa */ >> - >> -} /* namespace libcamera */ >> diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h >> deleted file mode 100644 >> index 89cee4c4..00000000 >> --- a/src/ipa/libipa/algorithm.h >> +++ /dev/null >> @@ -1,24 +0,0 @@ >> -/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> -/* >> - * Copyright (C) 2021, Ideas On Board >> - * >> - * algorithm.h - ISP control algorithm interface >> - */ >> -#ifndef __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__ >> -#define __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__ >> - >> -namespace libcamera { >> - >> -namespace ipa { >> - >> -class Algorithm >> -{ >> -public: >> - virtual ~Algorithm(); >> -}; >> - >> -} /* namespace ipa */ >> - >> -} /* namespace libcamera */ >> - >> -#endif /* __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__ */ >> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build >> index 3fda7c00..2a694b1e 100644 >> --- a/src/ipa/libipa/meson.build >> +++ b/src/ipa/libipa/meson.build >> @@ -1,13 +1,11 @@ >> # SPDX-License-Identifier: CC0-1.0 >> >> libipa_headers = files([ >> - 'algorithm.h', >> 'camera_sensor_helper.h', >> 'histogram.h' >> ]) >> >> libipa_sources = files([ >> - 'algorithm.cpp', >> 'camera_sensor_helper.cpp', >> 'histogram.cpp' >> ]) >
diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h new file mode 100644 index 00000000..072f01c4 --- /dev/null +++ b/src/ipa/ipu3/algorithms/algorithm.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2021, Ideas On Board + * + * algorithm.h - IPU3 control algorithm interface + */ +#ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ +#define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ + +namespace libcamera { + +namespace ipa::ipu3 { + +class Algorithm +{ +public: + virtual ~Algorithm() {} +}; + +} /* namespace ipa::ipu3 */ + +} /* namespace libcamera */ + +#endif /* __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ */ diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build new file mode 100644 index 00000000..67148333 --- /dev/null +++ b/src/ipa/ipu3/algorithms/meson.build @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: CC0-1.0 + +ipu3_ipa_algorithms = files([ +]) diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h index 9f3d4257..f00b98d6 100644 --- a/src/ipa/ipu3/ipu3_agc.h +++ b/src/ipa/ipu3/ipu3_agc.h @@ -13,7 +13,7 @@ #include <libcamera/geometry.h> -#include "libipa/algorithm.h" +#include "algorithms/algorithm.h" namespace libcamera { diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h index 122cf68c..ea2d4320 100644 --- a/src/ipa/ipu3/ipu3_awb.h +++ b/src/ipa/ipu3/ipu3_awb.h @@ -13,7 +13,7 @@ #include <libcamera/geometry.h> -#include "libipa/algorithm.h" +#include "algorithms/algorithm.h" namespace libcamera { diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build index b6364190..fcb27d68 100644 --- a/src/ipa/ipu3/meson.build +++ b/src/ipa/ipu3/meson.build @@ -1,5 +1,7 @@ # SPDX-License-Identifier: CC0-1.0 +subdir('algorithms') + ipa_name = 'ipa_ipu3' ipu3_ipa_sources = files([ @@ -8,6 +10,8 @@ ipu3_ipa_sources = files([ 'ipu3_awb.cpp', ]) +ipu3_ipa_sources += ipu3_ipa_algorithms + mod = shared_module(ipa_name, [ipu3_ipa_sources, libcamera_generated_ipa_headers], name_prefix : '', diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp deleted file mode 100644 index 930f9353..00000000 --- a/src/ipa/libipa/algorithm.cpp +++ /dev/null @@ -1,39 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -/* - * Copyright (C) 2021, Ideas On Board - * - * algorithm.cpp - ISP control algorithms - */ - -#include "algorithm.h" - -/** - * \file algorithm.h - * \brief Algorithm common interface - */ - -namespace libcamera { - -/** - * \brief The IPA namespace - * - * The IPA namespace groups all types specific to IPA modules. It serves as the - * top-level namespace for the IPA library libipa, and also contains - * module-specific namespaces for IPA modules. - */ -namespace ipa { - -/** - * \class Algorithm - * \brief The base class for all IPA algorithms - * - * The Algorithm class defines a standard interface for IPA algorithms. By - * abstracting algorithms, it makes possible the implementation of generic code - * to manage algorithms regardless of their specific type. - */ - -Algorithm::~Algorithm() = default; - -} /* namespace ipa */ - -} /* namespace libcamera */ diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h deleted file mode 100644 index 89cee4c4..00000000 --- a/src/ipa/libipa/algorithm.h +++ /dev/null @@ -1,24 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -/* - * Copyright (C) 2021, Ideas On Board - * - * algorithm.h - ISP control algorithm interface - */ -#ifndef __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__ -#define __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__ - -namespace libcamera { - -namespace ipa { - -class Algorithm -{ -public: - virtual ~Algorithm(); -}; - -} /* namespace ipa */ - -} /* namespace libcamera */ - -#endif /* __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__ */ diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build index 3fda7c00..2a694b1e 100644 --- a/src/ipa/libipa/meson.build +++ b/src/ipa/libipa/meson.build @@ -1,13 +1,11 @@ # SPDX-License-Identifier: CC0-1.0 libipa_headers = files([ - 'algorithm.h', 'camera_sensor_helper.h', 'histogram.h' ]) libipa_sources = files([ - 'algorithm.cpp', 'camera_sensor_helper.cpp', 'histogram.cpp' ])