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

[Milan Zamazal]

Hi Umang,

thank you for review.

Umang Jain <umang.jain at ideasonboard.com> writes:

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

This copy of a text from the hardware pipelines is confusing, I'll
adjust the paragraph.

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