[PATCH v3 08/23] libcamera: software_isp: Define skeletons for IPA refactoring

[Dan Scally]

Hi Milan, what a cool set - thanks - I think it's great to move the Software ISP to the same model 
as the other IPAs.

On 17/07/2024 09:54, Milan Zamazal wrote:
> Software ISP image processing algorithms are currently defined in a
> simplified way, different from other libcamera pipelines.  This is not
> good for several reasons:
>
> - It makes the software ISP code harder to understand due to its
>    different structuring.
> - Adding more algorithms may make the code harder to understand
>    generally.
> - Mass libcamera code changes may not be easily applicable to software
>    ISP.
> - Algorithm sharing with other pipelines is not easily possible.
>
> This patch introduces basic software ISP IPA skeletons structured
> similarly to the other pipelines.  The newly added files are currently
> not used or compiled and the general skeleton structures don't contain
> anything particular.  It is just a preparation step for a larger
> refactoring and the code will be actually used and extended as needed in
> followup patches.
>
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>   src/ipa/simple/algorithms/algorithm.h | 22 +++++++++++
>   src/ipa/simple/ipa_context.cpp        | 53 +++++++++++++++++++++++++++
>   src/ipa/simple/ipa_context.h          | 34 +++++++++++++++++
>   src/ipa/simple/meson.build            |  1 +
>   src/ipa/simple/module.h               | 28 ++++++++++++++
>   5 files changed, 138 insertions(+)
>   create mode 100644 src/ipa/simple/algorithms/algorithm.h
>   create mode 100644 src/ipa/simple/ipa_context.cpp
>   create mode 100644 src/ipa/simple/ipa_context.h
>   create mode 100644 src/ipa/simple/module.h
>
> diff --git a/src/ipa/simple/algorithms/algorithm.h b/src/ipa/simple/algorithms/algorithm.h
> new file mode 100644
> index 00000000..41f63170
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/algorithm.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Red Hat, Inc.
> + *
> + * Software ISP control algorithm interface
> + */
> +
> +#pragma once
> +
> +#include <libipa/algorithm.h>
> +
> +#include "module.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::soft {
> +
> +using Algorithm = libcamera::ipa::Algorithm<Module>;
> +
> +} /* namespace ipa::soft */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp
> new file mode 100644
> index 00000000..3c1c7262
> --- /dev/null
> +++ b/src/ipa/simple/ipa_context.cpp
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + * Copyright (C) 2024 Red Hat Inc.
> + *
> + * Software ISP IPA Context
> + */
> +
> +#include "ipa_context.h"
> +
> +/**
> + * \file ipa_context.h
> + * \brief Context and state information shared between the algorithms
> + */
> +
> +namespace libcamera::ipa::soft {
> +
> +/**
> + * \struct IPASessionConfiguration
> + * \brief Session configuration for the IPA module
> + *
> + * The session configuration contains all IPA configuration parameters that
> + * remain constant during the capture session, from IPA module start to stop.
> + * It is typically set during the configure() operation of the IPA module, but
> + * may also be updated in the start() operation.
> + */
> +
> +/**
> + * \struct IPAActiveState
> + * \brief The active state of the IPA algorithms
> + *
> + * The IPA is fed with the statistics generated from the latest frame processed.
> + * The statistics are then processed by the IPA algorithms to compute parameters
> + * required for the next frame capture and processing. The current state of the
> + * algorithms is reflected through the IPAActiveState to store the values most
> + * recently computed by the IPA algorithms.
> + */
> +
> +/**
> + * \struct IPAContext
> + * \brief Global IPA context data shared between all algorithms
> + *
> + * \var IPAContext::configuration
> + * \brief The IPA session configuration, immutable during the session
> + *
> + * \var IPAContext::frameContexts
> + * \brief Ring buffer of the IPAFrameContext(s)
> + *
> + * \var IPAContext::activeState
> + * \brief The current state of IPA algorithms
> + */
> +
> +} /* namespace libcamera::ipa::soft */
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> new file mode 100644
> index 00000000..bc1235b6
> --- /dev/null
> +++ b/src/ipa/simple/ipa_context.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Red Hat, Inc.
> + *
> + * Simple pipeline IPA Context
> + *
> + */
> +
> +#pragma once
> +
> +#include <libipa/fc_queue.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::soft {
> +
> +struct IPASessionConfiguration {
> +};
> +
> +struct IPAActiveState {
> +};
> +
> +struct IPAFrameContext : public FrameContext {
> +};
> +
> +struct IPAContext {
> +	IPASessionConfiguration configuration;
> +	IPAActiveState activeState;
> +	FCQueue<IPAFrameContext> frameContexts;
> +};
> +
> +} /* namespace ipa::soft */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
> index 33d1c96a..363251fb 100644
> --- a/src/ipa/simple/meson.build
> +++ b/src/ipa/simple/meson.build
> @@ -3,6 +3,7 @@
>   ipa_name = 'ipa_soft_simple'
>   
>   soft_simple_sources = files([
> +    'ipa_context.cpp',
>       'soft_simple.cpp',
>       'black_level.cpp',
>   ])
> diff --git a/src/ipa/simple/module.h b/src/ipa/simple/module.h
> new file mode 100644
> index 00000000..33a7d1db
> --- /dev/null
> +++ b/src/ipa/simple/module.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Red Hat, Inc.
> + *
> + * Software ISP IPA Module
> + */
> +
> +#pragma once
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/software_isp/debayer_params.h"
> +#include "libcamera/internal/software_isp/swisp_stats.h"
> +
> +#include <libipa/module.h>
> +
> +#include "ipa_context.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::soft {
> +
> +using Module = ipa::Module<IPAContext, IPAFrameContext, ControlInfoMap,

The ControlInfoMap here rather than some mojom generated struct surprises me a little, but the rest 
is fine and I assume that will become clear later:


Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>

> +			   DebayerParams, SwIspStats>;
> +
> +} /* namespace ipa::soft */
> +
> +} /* namespace libcamera */