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

20670 diff mbox series

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@redhat.com>
Reviewed-by: Umang Jain <umang.jain@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

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@redhat.com>
> Reviewed-by: Umang Jain <umang.jain@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@ideasonboard.com>

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

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@redhat.com>
> > Reviewed-by: Umang Jain <umang.jain@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@ideasonboard.com>

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

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

Hi Laurent,

thank you for review.

Laurent Pinchart <laurent.pinchart@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@redhat.com>
>> > Reviewed-by: Umang Jain <umang.jain@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@ideasonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> > +			   DebayerParams, SwIspStats>;
>> > +
>> > +} /* namespace ipa::soft */
>> > +
>> > +} /* namespace libcamera */

On Tue, Aug 13, 2024 at 10:26:41AM +0200, Milan Zamazal wrote:
> Hi Laurent,
> 
> thank you for review.
> 
> Laurent Pinchart <laurent.pinchart@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.

I'm not sure what I misread. Please ignore my comment.

> >> > 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@redhat.com>
> >> > Reviewed-by: Umang Jain <umang.jain@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@ideasonboard.com>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> >> > +			   DebayerParams, SwIspStats>;
> >> > +
> >> > +} /* namespace ipa::soft */
> >> > +
> >> > +} /* namespace libcamera */
>