[libcamera-devel] [RFC PATCH v2 1/3] WIP: ipa: Add a common interface for algorithm objects

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Tue, Feb 23, 2021 at 05:40:39PM +0100, Jean-Michel Hautbois wrote:
> In order to instanciate and use algorithms (AWB, AGC, etc.)
> there is a need for a common class to define mandatory methods.
> 
> Instead of reinventing the wheel, reuse what Raspberry Pi has done and
> adapt to the minimum requirements expected.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/libipa/algorithm.cpp | 25 +++++++++++++++++++++++++
>  src/ipa/libipa/algorithm.h   | 29 +++++++++++++++++++++++++++++
>  src/ipa/libipa/meson.build   |  4 +++-
>  3 files changed, 57 insertions(+), 1 deletion(-)
>  create mode 100644 src/ipa/libipa/algorithm.cpp
>  create mode 100644 src/ipa/libipa/algorithm.h
> 
> diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp
> new file mode 100644
> index 00000000..24cd0ae2
> --- /dev/null
> +++ b/src/ipa/libipa/algorithm.cpp
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * 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.
 */

> +
> +void Algorithm::initialise() {}
> +
> +void Algorithm::process() {}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h
> new file mode 100644
> index 00000000..56cd8172
> --- /dev/null
> +++ b/src/ipa/libipa/algorithm.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * 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:
> +	Algorithm()
> +	{
> +	}

There's no need to provide a default constructor when no other
constructors are defined, the compiler will implicitly declare and
define a default constructor.

> +	virtual ~Algorithm() = default;

I'd move this to the .cpp file, to match initialise() and process(). You
can write

Algorithm::~Algorithm() = default;

in the .cpp files.

> +	virtual void initialise();
> +	virtual void process();

These two functions are currently unused, you can drop them.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +};
> +
> +} /* 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 b29ef0f4..585d02a3 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -1,13 +1,15 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  libipa_headers = files([
> +  'algorithm.h',
>  ])
>  
>  libipa_sources = files([
> +    'algorithm.cpp',
>  ])
>  
>  libipa_includes = include_directories('..')
>  
> -libipa = static_library('ipa', libipa_sources,
> +libipa = static_library('ipa', [libipa_sources, libipa_headers],
>                          include_directories : ipa_includes,
>                          dependencies : libcamera_dep)

-- 
Regards,

Laurent Pinchart