[PATCH v3 08/23] libcamera: software_isp: Define skeletons for IPA refactoring
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Aug 13 10:32:33 CEST 2024
On Tue, Aug 13, 2024 at 10:26:41AM +0200, Milan Zamazal wrote:
> 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.
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 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 */
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list