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

[Laurent Pinchart]

On Mon, Aug 12, 2024 at 02:15:29PM +0100, Daniel Scally wrote:
> 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

s/don't/doesn't/

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

Extra blank line.

> > + */
> > +
> > +#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>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

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

-- 
Regards,

Laurent Pinchart