[PATCH v2 04/19] libcamera: software_isp: Define skeletons for IPA refactoring

[Umang Jain]

Hi Milan

Thank you for the patch.

On 03/07/24 11:21 pm, 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>
> ---
>   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..0898e591
> --- /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 captured
> + * by the hardware. The statistics are then processed by the IPA algorithms to

I am not very sure here, but this line above reguarding the stats being 
generated by hardware needs to be defined in soft-IPA context.
Do you have any thoughts on that ?

Other than that, LGTM:

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> + * compute ISP parameters required for the next frame capture. 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,
> +			   DebayerParams, SwIspStats>;
> +
> +} /* namespace ipa::soft */
> +
> +} /* namespace libcamera */