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

[Laurent Pinchart]

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

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