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

11367 diff mbox series

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

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

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

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

Hi Jean-Michel,

On Tue, Mar 16, 2021 at 05:02:35PM +0100, Jean-Michel Hautbois wrote:
> On 15/03/2021 02:24, Laurent Pinchart wrote:
> > 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@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;

You need to keep the declaration of the virtual destructor in the class
definition, it's only its definition that is moved to the .cpp file.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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)
> >