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

[Jean-Michel Hautbois]

Hi JM,

On 16/03/2021 17:02, Jean-Michel Hautbois wrote:
> 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;

Well, no you keep the declaration in Algorithm class like:
virtual ~Algorithm();

But you declare it in cpp file :-).

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

>>
>>> +};
>>> +
>>> +} /* 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)
>>