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

[Jean-Michel Hautbois]

Hi Laurent,

On 15/03/2021 02:24, Laurent Pinchart wrote:
> 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.

It means class Algorithm would be totally empty, so if I move the
destructor I get :
../src/ipa/libipa/algorithm.cpp:35:12: error: definition of implicitly
declared destructor
Algorithm::~Algorithm() = default;

> 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)
>