[libcamera-devel] [PATCH v2 01/10] ipa: move libipa::Algorithm to ipa/ipu3/algorithms
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Tue Aug 17 08:47:12 CEST 2021
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 at ideasonboard.com>
>> Reviewed-by: Kieran Bingham kieran.bingham at 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'
>> ])
>
More information about the libcamera-devel
mailing list