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

[Milan Zamazal]

Hi Laurent,

thank you for review.

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

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

? "structures" is plural.

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

OK, I'll remove it.

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