[libcamera-devel,RFC,1/2] WIP: ipa: Add Controller and Algorithm skeleton

11346 diff mbox series

In order to instanciate and control IPA algorithms (AWB, AGC, etc.)
there is a need for an IPA algorithm class to define mandatory methods,
and an IPA controller class to operate algorithms together.

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>
---
 include/libcamera/ipa/agc_algorithm.h  | 32 ++++++++++++++++++
 include/libcamera/ipa/awb_algorithm.h  | 27 +++++++++++++++
 include/libcamera/ipa/ipa_algorithm.h  | 46 ++++++++++++++++++++++++++
 include/libcamera/ipa/ipa_controller.h | 39 ++++++++++++++++++++++
 include/libcamera/ipa/meson.build      |  4 +++
 src/ipa/libipa/ipa_algorithm.cpp       | 20 +++++++++++
 src/ipa/libipa/ipa_controller.cpp      | 45 +++++++++++++++++++++++++
 src/ipa/libipa/meson.build             |  2 ++
 8 files changed, 215 insertions(+)
 create mode 100644 include/libcamera/ipa/agc_algorithm.h
 create mode 100644 include/libcamera/ipa/awb_algorithm.h
 create mode 100644 include/libcamera/ipa/ipa_algorithm.h
 create mode 100644 include/libcamera/ipa/ipa_controller.h
 create mode 100644 src/ipa/libipa/ipa_algorithm.cpp
 create mode 100644 src/ipa/libipa/ipa_controller.cpp

Hi Jean-Michel,

Thank you for the patch.

On Fri, Feb 19, 2021 at 06:22:23PM +0100, Jean-Michel Hautbois wrote:
> In order to instanciate and control IPA algorithms (AWB, AGC, etc.)
> there is a need for an IPA algorithm class to define mandatory methods,
> and an IPA controller class to operate algorithms together.
> 
> 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>
> ---
>  include/libcamera/ipa/agc_algorithm.h  | 32 ++++++++++++++++++
>  include/libcamera/ipa/awb_algorithm.h  | 27 +++++++++++++++
>  include/libcamera/ipa/ipa_algorithm.h  | 46 ++++++++++++++++++++++++++
>  include/libcamera/ipa/ipa_controller.h | 39 ++++++++++++++++++++++
>  include/libcamera/ipa/meson.build      |  4 +++
>  src/ipa/libipa/ipa_algorithm.cpp       | 20 +++++++++++
>  src/ipa/libipa/ipa_controller.cpp      | 45 +++++++++++++++++++++++++
>  src/ipa/libipa/meson.build             |  2 ++
>  8 files changed, 215 insertions(+)
>  create mode 100644 include/libcamera/ipa/agc_algorithm.h
>  create mode 100644 include/libcamera/ipa/awb_algorithm.h
>  create mode 100644 include/libcamera/ipa/ipa_algorithm.h
>  create mode 100644 include/libcamera/ipa/ipa_controller.h
>  create mode 100644 src/ipa/libipa/ipa_algorithm.cpp
>  create mode 100644 src/ipa/libipa/ipa_controller.cpp
> 
> diff --git a/include/libcamera/ipa/agc_algorithm.h b/include/libcamera/ipa/agc_algorithm.h
> new file mode 100644
> index 00000000..4dd17103
> --- /dev/null
> +++ b/include/libcamera/ipa/agc_algorithm.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * agc_algorithm.h - AGC/AEC control algorithm interface
> + */
> +#ifndef __LIBCAMERA_AGC_ALGORITHM_H__

This should be __LIBCAMERA_IPA_AGC_ALGORITHM_H__ as the file is in
libcamera/ipa/.

Lots of the comments I'll make here apply in many locations, I won't
repeat them everywhere.

> +#define __LIBCAMERA_AGC_ALGORITHM_H__
> +
> +#include <libcamera/ipa/ipa_algorithm.h>

Should this be considered as an internal header that won't be installed
? It should then use double quotes instead of <>.

> +
> +namespace libcamera {

The IPA IPC generated code uses an ipa namespace, should we do the same
here ? To be clear, that would be

namespace libcamera {

namespace ipa {

> +
> +class AgcAlgorithm : public IPAAlgorithm
> +{
> +public:
> +	AgcAlgorithm(IPAController *controller)
> +		: IPAAlgorithm(controller) {}

The usual coding style is

	AgcAlgorithm(IPAController *controller)
		: IPAAlgorithm(controller)
	{
	}

> +	/* An AGC algorithm must provide the following: */

I'd drop this, as it's implied by virtual ... = 0.

> +	virtual unsigned int GetConvergenceFrames() const = 0;

Functions should start with a lower-case letter. Getter shouldn't start with
'get', so this should be

	virtual unsigned int convergenceFrames() const = 0;

> +	virtual void SetEv(double ev) = 0;
> +	virtual void SetFlickerPeriod(double flicker_period) = 0;

Variables should use camelCase.

> +	virtual void SetFixedShutter(double fixed_shutter) = 0; // microseconds
> +	virtual void SetMaxShutter(double max_shutter) = 0; // microseconds

We use C-style comments.

Instead of documenting the function parameter in the .h file, it should
be documented in a doxygen comment block in a .cpp file. This can go in
ipa_algorithm.cpp, or you can create an agc_algorithm.cpp with
documentation only for this purpose. The latter would likely scale
better.

We can also consider switching to std::chrono::microseconds, that will
increase type safety, at the cost of not allowing sub-microseconds
resolution, which is probably fine here, but maybe we could then use
nanoseconds instead.

We should also consider if we want to always express all times in IPA
code in the same unit.

> +	virtual void SetFixedAnalogueGain(double fixed_analogue_gain) = 0;
> +	virtual void SetMeteringMode(std::string const &metering_mode_name) = 0;

We usually put the const keyword before the type.

> +	virtual void SetExposureMode(std::string const &exposure_mode_name) = 0;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_AGC_ALGORITHM_H__ */
> diff --git a/include/libcamera/ipa/awb_algorithm.h b/include/libcamera/ipa/awb_algorithm.h
> new file mode 100644
> index 00000000..37464d12
> --- /dev/null
> +++ b/include/libcamera/ipa/awb_algorithm.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * awb_algorithm.h - AWB control algorithm interface
> + */
> +#ifndef __LIBCAMERA_AWB_ALGORITHM_H__
> +#define __LIBCAMERA_AWB_ALGORITHM_H__
> +
> +#include <libcamera/ipa/ipa_algorithm.h>
> +
> +namespace libcamera {
> +
> +class AwbAlgorithm : public IPAAlgorithm
> +{
> +public:
> +	AwbAlgorithm(IPAController *controller)
> +		: IPAAlgorithm(controller) {}
> +	/* An AWB algorithm must provide the following: */
> +	virtual unsigned int GetConvergenceFrames() const = 0;
> +	virtual void SetMode(std::string const &mode_name) = 0;
> +	virtual void SetManualGains(double manual_r, double manual_b) = 0;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_AWB_ALGORITHM_H__ */
> diff --git a/include/libcamera/ipa/ipa_algorithm.h b/include/libcamera/ipa/ipa_algorithm.h
> new file mode 100644
> index 00000000..e48b99a6
> --- /dev/null
> +++ b/include/libcamera/ipa/ipa_algorithm.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * ipa_algorithm.h - ISP control algorithm interface
> + */
> +#ifndef __LIBCAMERA_IPA_ALGORITHM_H__
> +#define __LIBCAMERA_IPA_ALGORITHM_H__
> +
> +/* All algorithms should be derived from this class and made available to the
> + * Controller. */

/*
 * All algorithms should be derived from this class and made available
 * to the Controller.
 */

But this should go to the .cpp file, in a doxygen comment.

> +
> +#include <map>
> +#include <memory>
> +#include <string>
> +
> +#include "ipa_controller.h"

#include "libcamera/ipa/ipa_controller.h"

> +
> +namespace libcamera {
> +
> +/* This defines the basic interface for all control algorithms. */
> +
> +class IPAAlgorithm
> +{
> +public:
> +	IPAAlgorithm(IPAController *controller)
> +		: controller_(controller), paused_(false)
> +	{
> +	}
> +	virtual ~IPAAlgorithm() = default;
> +	virtual char const *Name() const = 0;
> +	virtual bool IsPaused() const { return paused_; }
> +	virtual void Pause() { paused_ = true; }
> +	virtual void Resume() { paused_ = false; }
> +	virtual void Initialise();
> +	virtual void Prepare();
> +	virtual void Process();
> +
> +private:
> +	[[maybe_unused]] IPAController *controller_;

Is the [[maybe_unused]] needed ? Or, rather, is this field even needed ?
You don't use it in this series, so I'd drop it. You could then remove
the controller parameter from the constructor.

> +	bool paused_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_ALGORITHM_H__ */
> diff --git a/include/libcamera/ipa/ipa_controller.h b/include/libcamera/ipa/ipa_controller.h
> new file mode 100644
> index 00000000..953cad4a
> --- /dev/null
> +++ b/include/libcamera/ipa/ipa_controller.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * ipa_controller.h - ISP controller interface
> + */
> +#ifndef __LIBCAMERA_IPA_CONTROLLER_H__
> +#define __LIBCAMERA_IPA_CONTROLLER_H__
> +
> +// The Controller is simply a container for a collecting together a number of
> +// "control algorithms" (such as AWB etc.) and for running them all in a
> +// convenient manner.
> +
> +#include <string>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +class IPAAlgorithm;
> +typedef std::unique_ptr<IPAAlgorithm> IPAAlgorithmPtr;
> +
> +class IPAController
> +{
> +public:
> +	IPAController();
> +	~IPAController();

A blank line would be nice here.

https://en.cppreference.com/w/cpp/language/rule_of_three

The copy constructor and copy assignment operator should be defined as
deleted. As there's also no use case for moving the IPAController, you
can use the LIBCAMERA_DISABLE_COPY_AND_MOVE() macro defined in class.h.

> +	IPAAlgorithm *CreateAlgorithm(char const *name);
> +	void Initialise();
> +	void Prepare();
> +	void Process();
> +	IPAAlgorithm *GetAlgorithm(std::string const &name) const;

I'd use the same type for the name argument to both CreateAlgorithm()
and GetAlgorithm().

Actually, neither functions are used, so you can drop them. And, reading
patch 2/2, the IPAController class is instantiated, but not used yet.
Should it be dropped for now ?

> +
> +protected:
> +	std::vector<IPAAlgorithmPtr> algorithms_;

The IPAAlgorithmPtr type is only used here, and hinders readability as
it requires looking it up. You can just use

	std::vector<std::unique_ptr<IPAAlgorithm>> algorithms_;

> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_CONTROLLER_H__ */
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index a4d3f868..e56d8b00 100644
> --- a/include/libcamera/ipa/meson.build
> +++ b/include/libcamera/ipa/meson.build
> @@ -1,6 +1,10 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  libcamera_ipa_headers = files([
> +    'agc_algorithm.h',
> +    'awb_algorithm.h',
> +    'ipa_algorithm.h',
> +    'ipa_controller.h',

These should go to libipa_headers.

>      'ipa_controls.h',
>      'ipa_interface.h',
>      'ipa_module_info.h',
> diff --git a/src/ipa/libipa/ipa_algorithm.cpp b/src/ipa/libipa/ipa_algorithm.cpp
> new file mode 100644
> index 00000000..16fb29ce
> --- /dev/null
> +++ b/src/ipa/libipa/ipa_algorithm.cpp
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * ipa_algorithm.cpp - ISP control algorithms
> + */
> +#include <iostream>

We have a logging infrastructure.

> +
> +#include <libcamera/ipa/ipa_algorithm.h>
> +
> +using namespace libcamera;

Let's open the libcamera namespace instead

namespace libcamera {

> +
> +void IPAAlgorithm::Initialise()
> +{
> +	std::cout << "Entering: " << __func__ << std::endl;

I'm not sure if logging all calls to this function will be very useful,
especially given that it will always print the name of this function,
without any information about the derived class. Let's just drop it.

> +}
> +
> +void IPAAlgorithm::Prepare() {}

void IPAAlgorithm::Prepare()
{
}

> +
> +void IPAAlgorithm::Process() {}
> diff --git a/src/ipa/libipa/ipa_controller.cpp b/src/ipa/libipa/ipa_controller.cpp
> new file mode 100644
> index 00000000..e2cde8ce
> --- /dev/null
> +++ b/src/ipa/libipa/ipa_controller.cpp
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * ipa_controller.cpp - ISP controller
> + */
> +
> +#include "libcamera/internal/log.h"
> +
> +#include <libcamera/ipa/ipa_algorithm.h>
> +#include <libcamera/ipa/ipa_controller.h>
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(IPAController)
> +
> +IPAController::IPAController() {}

This can also be written

IPAController::IPAController() = default;

> +
> +IPAController::~IPAController() {}

Here too.

> +
> +IPAAlgorithm *IPAController::CreateAlgorithm(char const *name)
> +{
> +	LOG(IPAController, Error) << "Create algorithm " << name;
> +	return nullptr;
> +}
> +
> +void IPAController::Initialise()
> +{
> +	for (auto &algo : algorithms_)

Let's avoid auto when possible:

	for (IPAAlgorithm &algo : algorithms_)

> +		algo->Initialise();
> +}
> +
> +void IPAController::Prepare()
> +{
> +	for (auto &algo : algorithms_)
> +		if (!algo->IsPaused())
> +			algo->Prepare();

	for (auto &algo : algorithms_) {
		if (!algo->IsPaused())
			algo->Prepare();
	}

I foresee a need for a more complex scheduling implementation that will
take into account dependencies between algorithms. This will be an
interesting research topic.

> +}
> +
> +void IPAController::Process()
> +{
> +	for (auto &algo : algorithms_)
> +		if (!algo->IsPaused())
> +			algo->Process();
> +}
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index b29ef0f4..1693e489 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -4,6 +4,8 @@ libipa_headers = files([
>  ])
>  
>  libipa_sources = files([
> +    'ipa_algorithm.cpp',
> +    'ipa_controller.cpp',
>  ])
>  
>  libipa_includes = include_directories('..')