[v6,10/18] libcamera: introduce SoftwareIsp

19767 diff mbox series

From: Andrey Konovalov <andrey.konovalov@linaro.org>

Doxygen documentation by Dennis Bonke.

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../internal/software_isp/meson.build         |   1 +
 .../internal/software_isp/software_isp.h      |  98 +++++
 src/libcamera/software_isp/meson.build        |   1 +
 src/libcamera/software_isp/software_isp.cpp   | 349 ++++++++++++++++++
 4 files changed, 449 insertions(+)
 create mode 100644 include/libcamera/internal/software_isp/software_isp.h
 create mode 100644 src/libcamera/software_isp/software_isp.cpp

Hi Milan and Andrey,

Thank you for the patch.

On Tue, Mar 19, 2024 at 01:35:57PM +0100, Milan Zamazal wrote:
> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> Doxygen documentation by Dennis Bonke.
> 
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../internal/software_isp/meson.build         |   1 +
>  .../internal/software_isp/software_isp.h      |  98 +++++
>  src/libcamera/software_isp/meson.build        |   1 +
>  src/libcamera/software_isp/software_isp.cpp   | 349 ++++++++++++++++++
>  4 files changed, 449 insertions(+)
>  create mode 100644 include/libcamera/internal/software_isp/software_isp.h
>  create mode 100644 src/libcamera/software_isp/software_isp.cpp
> 
> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
> index a620e16d..508ddddc 100644
> --- a/include/libcamera/internal/software_isp/meson.build
> +++ b/include/libcamera/internal/software_isp/meson.build
> @@ -2,5 +2,6 @@
>  
>  libcamera_internal_headers += files([
>      'debayer_params.h',
> +    'software_isp.h',
>      'swisp_stats.h',
>  ])
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> new file mode 100644
> index 00000000..8d25e979
> --- /dev/null
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * software_isp.h - Simple software ISP implementation
> + */
> +
> +#pragma once
> +
> +#include <functional>
> +#include <initializer_list>
> +#include <map>
> +#include <memory>
> +#include <string>
> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/signal.h>
> +#include <libcamera/base/thread.h>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include <libcamera/ipa/soft_ipa_interface.h>
> +#include <libcamera/ipa/soft_ipa_proxy.h>
> +
> +#include "libcamera/internal/dma_heaps.h"
> +#include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/shared_mem_object.h"
> +#include "libcamera/internal/software_isp/debayer_params.h"
> +
> +namespace libcamera {
> +
> +class DebayerCpu;
> +class FrameBuffer;
> +class PixelFormat;
> +struct StreamConfiguration;
> +
> +LOG_DECLARE_CATEGORY(SoftwareIsp)
> +
> +class SoftwareIsp
> +{
> +public:
> +	SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls);
> +	~SoftwareIsp();
> +
> +	int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }
> +
> +	bool isValid() const;
> +
> +	std::vector<PixelFormat> formats(PixelFormat input);
> +
> +	SizeRange sizes(PixelFormat inputFormat, const Size &inputSize);
> +
> +	std::tuple<unsigned int, unsigned int>
> +	strideAndFrameSize(const PixelFormat &outputFormat, const Size &size);
> +
> +	int configure(const StreamConfiguration &inputCfg,
> +		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> +		      const ControlInfoMap &sensorControls);
> +
> +	int exportBuffers(unsigned int output, unsigned int count,
> +			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> +
> +	void processStats(const ControlList &sensorControls);
> +
> +	int start();
> +	void stop();
> +
> +	int queueBuffers(FrameBuffer *input,
> +			 const std::map<unsigned int, FrameBuffer *> &outputs);
> +
> +	void process(FrameBuffer *input, FrameBuffer *output);
> +
> +	Signal<FrameBuffer *> inputBufferReady;
> +	Signal<FrameBuffer *> outputBufferReady;
> +	Signal<int> ispStatsReady;
> +	Signal<const ControlList &> setSensorControls;
> +
> +private:
> +	void saveIspParams(int dummy);
> +	void setSensorCtrls(const ControlList &sensorControls);
> +	void statsReady(int dummy);
> +	void inputReady(FrameBuffer *input);
> +	void outputReady(FrameBuffer *output);
> +
> +	std::unique_ptr<DebayerCpu> debayer_;
> +	Thread ispWorkerThread_;
> +	SharedMemObject<DebayerParams> sharedParams_;
> +	DebayerParams debayerParams_;
> +	DmaHeap dmaHeap_;
> +
> +	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build
> index 71b46539..e9266e54 100644
> --- a/src/libcamera/software_isp/meson.build
> +++ b/src/libcamera/software_isp/meson.build
> @@ -10,5 +10,6 @@ endif
>  libcamera_sources += files([
>      'debayer.cpp',
>      'debayer_cpu.cpp',
> +    'software_isp.cpp',
>      'swstats_cpu.cpp',
>  ])
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> new file mode 100644
> index 00000000..388b4496
> --- /dev/null
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -0,0 +1,349 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * software_isp.cpp - Simple software ISP implementation
> + */
> +
> +#include "libcamera/internal/software_isp/software_isp.h"
> +
> +#include <sys/mman.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <libcamera/formats.h>
> +#include <libcamera/stream.h>
> +
> +#include "libcamera/internal/bayer_format.h"
> +#include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/ipa_manager.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
> +
> +#include "debayer_cpu.h"
> +
> +/**
> + * \file software_isp.cpp
> + * \brief Simple software ISP implementation
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(SoftwareIsp)
> +
> +/**
> + * \class SoftwareIsp
> + * \brief Class for the Software ISP
> + */
> +
> +/**
> + * \var SoftwareIsp::inputBufferReady
> + * \brief A signal emitted when the input frame buffer completes
> + */
> +
> +/**
> + * \var SoftwareIsp::outputBufferReady
> + * \brief A signal emitted when the output frame buffer completes
> + */
> +
> +/**
> + * \var SoftwareIsp::ispStatsReady
> + * \brief A signal emitted when the statistics for IPA are ready
> + *
> + * The int parameter isn't actually used.

Drop it :-)

> + */
> +
> +/**
> + * \var SoftwareIsp::setSensorControls

To make components reusable, signals should be named based on the event
they report, not based on what the connected slot is expected to do.

> + * \brief A signal emitted when the values to write to the sensor controls are ready
> + */
> +
> +/**
> + * \brief Constructs SoftwareIsp object
> + * \param[in] pipe The pipeline handler in use
> + * \param[in] sensorControls ControlInfoMap describing the controls supported by the sensor
> + */
> +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls)
> +	: debayer_(nullptr),

Not needed, the default unique_ptr<> constructor will handle that.

> +	  debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f },
> +	  dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)
> +{
> +	if (!dmaHeap_.isValid()) {
> +		LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object";
> +		return;
> +	}
> +
> +	sharedParams_ = SharedMemObject<DebayerParams>("softIsp_params");
> +	if (!sharedParams_) {
> +		LOG(SoftwareIsp, Error) << "Failed to create shared memory for parameters";
> +		return;
> +	}
> +
> +	auto stats = std::make_unique<SwStatsCpu>();
> +	if (!stats->isValid()) {
> +		LOG(SoftwareIsp, Error) << "Failed to create SwStatsCpu object";
> +		return;
> +	}
> +	stats->statsReady.connect(this, &SoftwareIsp::statsReady);
> +
> +	debayer_ = std::make_unique<DebayerCpu>(std::move(stats));
> +	debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady);
> +	debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);
> +
> +	ipa_ = IPAManager::createIPA<ipa::soft::IPAProxySoft>(pipe, 0, 0);
> +	if (!ipa_) {
> +		LOG(SoftwareIsp, Error)
> +			<< "Creating IPA for software ISP failed";
> +		debayer_.reset();

Is this needed, or can we let the destructor handle it ?

> +		return;
> +	}
> +
> +	int ret = ipa_->init(IPASettings{ "No cfg file", "No sensor model" },

The change from "No sensor model" to using the CameraSensor class, as
done in patch 18/18, should be squashed in this patch.

> +			     debayer_->getStatsFD(),
> +			     sharedParams_.fd(),
> +			     sensorControls);
> +	if (ret) {
> +		LOG(SoftwareIsp, Error) << "IPA init failed";
> +		debayer_.reset();
> +		return;
> +	}
> +
> +	ipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);
> +	ipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);
> +
> +	debayer_->moveToThread(&ispWorkerThread_);
> +}
> +
> +SoftwareIsp::~SoftwareIsp()
> +{
> +	/* make sure to destroy the DebayerCpu before the ispWorkerThread_ is gone */
> +	debayer_.reset();
> +}
> +
> +/**
> + * \fn int SoftwareIsp::loadConfiguration([[maybe_unused]] const std::string &filename)
> + * \brief Load a configuration from a file
> + * \param[in] filename The file to load the configuration data from
> + *
> + * Currently is a stub doing nothing and always returning "success".
> + *
> + * \return 0 on success
> + */
> +
> +/**
> + * \brief Process the statistics gathered
> + * \param[in] sensorControls The sensor controls
> + *
> + * Requests the IPA to calculate new parameters for ISP and new control
> + * values for the sensor.
> + */
> +void SoftwareIsp::processStats(const ControlList &sensorControls)
> +{
> +	ASSERT(ipa_);
> +	ipa_->processStats(sensorControls);
> +}
> +
> +/**
> + * \brief Check the validity of Software Isp object
> + * \return True if Software Isp is valid, false otherwise
> + */
> +bool SoftwareIsp::isValid() const
> +{
> +	return !!debayer_;
> +}
> +
> +/**
> +  * \brief Get the output formats supported for the given input format
> +  * \param[in] inputFormat The input format
> +  * \return All the supported output formats or an empty vector if there are none
> +  */
> +std::vector<PixelFormat> SoftwareIsp::formats(PixelFormat inputFormat)
> +{
> +	ASSERT(debayer_ != nullptr);

You can write

	ASSERT(debayer_);

Same below.

> +
> +	return debayer_->formats(inputFormat);
> +}
> +
> +/**
> + * \brief Get the supported output sizes for the given input format and size
> + * \param[in] inputFormat The input format
> + * \param[in] inputSize The input frame size
> + * \return The valid size range or an empty range if there are none
> + */
> +SizeRange SoftwareIsp::sizes(PixelFormat inputFormat, const Size &inputSize)
> +{
> +	ASSERT(debayer_ != nullptr);
> +
> +	return debayer_->sizes(inputFormat, inputSize);
> +}
> +
> +/**
> + * Get the output stride and the frame size in bytes for the given output format and size
> + * \param[in] outputFormat The output format
> + * \param[in] size The output size (width and height in pixels)
> + * \return A tuple of the stride and the frame size in bytes, or a tuple of 0,0
> + * if there is no valid output config
> + */
> +std::tuple<unsigned int, unsigned int>
> +SoftwareIsp::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)
> +{
> +	ASSERT(debayer_ != nullptr);
> +
> +	return debayer_->strideAndFrameSize(outputFormat, size);
> +}
> +
> +/**
> + * \brief Configure the SoftwareIsp object according to the passed in parameters
> + * \param[in] inputCfg The input configuration
> + * \param[in] outputCfgs The output configurations
> + * \param[in] sensorControls ControlInfoMap of the controls supported by the sensor
> + * \return 0 on success, a negative errno on failure
> + */
> +int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
> +			   const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> +			   const ControlInfoMap &sensorControls)
> +{
> +	ASSERT(ipa_ != nullptr && debayer_ != nullptr);

Is there a specific reason to check ipa_ here ?

> +
> +	int ret = ipa_->configure(sensorControls);
> +	if (ret < 0)
> +		return ret;
> +
> +	return debayer_->configure(inputCfg, outputCfgs);
> +}
> +
> +/**
> + * \brief Export the buffers from the Software ISP
> + * \param[in] output Output stream index exporting the buffers
> + * \param[in] count Number of buffers to allocate
> + * \param[out] buffers Vector to store the allocated buffers
> + * \return The number of allocated buffers on success or a negative error code
> + * otherwise
> + */
> +int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,
> +			       std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> +	ASSERT(debayer_ != nullptr);
> +
> +	/* single output for now */
> +	if (output >= 1)
> +		return -EINVAL;
> +
> +	for (unsigned int i = 0; i < count; i++) {
> +		const std::string name = "frame-" + std::to_string(i);
> +		const size_t frameSize = debayer_->frameSize();
> +
> +		FrameBuffer::Plane outPlane;
> +		outPlane.fd = SharedFD(dmaHeap_.alloc(name.c_str(), frameSize));
> +		if (!outPlane.fd.isValid()) {
> +			LOG(SoftwareIsp, Error)
> +				<< "failed to allocate a dma_buf";
> +			return -ENOMEM;
> +		}
> +		outPlane.offset = 0;
> +		outPlane.length = frameSize;
> +
> +		std::vector<FrameBuffer::Plane> planes{ outPlane };
> +		buffers->emplace_back(std::make_unique<FrameBuffer>(std::move(planes)));
> +	}
> +
> +	return count;
> +}
> +
> +/**
> + * \brief Queue buffers to Software ISP
> + * \param[in] input The input framebuffer
> + * \param[in] outputs The container holding the output stream indexes and
> + * their respective frame buffer outputs
> + * \return 0 on success, a negative errno on failure
> + */
> +int SoftwareIsp::queueBuffers(FrameBuffer *input,
> +			      const std::map<unsigned int, FrameBuffer *> &outputs)
> +{
> +	unsigned int mask = 0;
> +
> +	/*
> +	 * Validate the outputs as a sanity check: at least one output is
> +	 * required, all outputs must reference a valid stream and no two
> +	 * outputs can reference the same stream.
> +	 */
> +	if (outputs.empty())
> +		return -EINVAL;
> +
> +	for (auto [index, buffer] : outputs) {
> +		if (!buffer)
> +			return -EINVAL;
> +		if (index >= 1) /* only single stream atm */
> +			return -EINVAL;
> +		if (mask & (1 << index))
> +			return -EINVAL;
> +
> +		mask |= 1 << index;
> +	}
> +
> +	process(input, outputs.at(0));
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Starts the Software ISP streaming operation
> + * \return 0 on success, any other value indicates an error
> + */
> +int SoftwareIsp::start()
> +{
> +	int ret = ipa_->start();
> +	if (ret)
> +		return ret;
> +
> +	ispWorkerThread_.start();
> +	return 0;
> +}
> +
> +/**
> + * \brief Stops the Software ISP streaming operation
> + */
> +void SoftwareIsp::stop()
> +{
> +	ispWorkerThread_.exit();
> +	ispWorkerThread_.wait();
> +
> +	ipa_->stop();
> +}
> +
> +/**
> + * \brief Passes the input framebuffer to the ISP worker to process
> + * \param[in] input The input framebuffer
> + * \param[out] output The framebuffer to write the processed frame to
> + */
> +void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output)
> +{
> +	debayer_->invokeMethod(&DebayerCpu::process,
> +			       ConnectionTypeQueued, input, output, debayerParams_);
> +}
> +
> +void SoftwareIsp::saveIspParams([[maybe_unused]] int dummy)
> +{
> +	debayerParams_ = *sharedParams_;
> +}
> +
> +void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)
> +{
> +	setSensorControls.emit(sensorControls);
> +}
> +
> +void SoftwareIsp::statsReady(int dummy)
> +{
> +	ispStatsReady.emit(dummy);
> +}
> +
> +void SoftwareIsp::inputReady(FrameBuffer *input)
> +{
> +	inputBufferReady.emit(input);
> +}
> +
> +void SoftwareIsp::outputReady(FrameBuffer *output)
> +{
> +	outputBufferReady.emit(output);
> +}
> +
> +} /* namespace libcamera */

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

> On Tue, Mar 19, 2024 at 01:35:57PM +0100, Milan Zamazal wrote:
>> From: Andrey Konovalov <andrey.konovalov@linaro.org>

[...]

>> + * \var SoftwareIsp::setSensorControls
>
> To make components reusable, signals should be named based on the event
> they report, not based on what the connected slot is expected to do.

The current naming is consistent with similar libcamera code in other places.
If we want to change it, it should be done at once everywhere.

[...]

>> + debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f },
>> +	  dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)
>> +{
>> +	if (!dmaHeap_.isValid()) {
>> +		LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object";
>> +		return;
>> +	}
>> +
>> +	sharedParams_ = SharedMemObject<DebayerParams>("softIsp_params");
>> +	if (!sharedParams_) {
>> +		LOG(SoftwareIsp, Error) << "Failed to create shared memory for parameters";
>> +		return;
>> +	}
>> +
>> +	auto stats = std::make_unique<SwStatsCpu>();
>> +	if (!stats->isValid()) {
>> +		LOG(SoftwareIsp, Error) << "Failed to create SwStatsCpu object";
>> +		return;
>> +	}
>> +	stats->statsReady.connect(this, &SoftwareIsp::statsReady);
>> +
>> +	debayer_ = std::make_unique<DebayerCpu>(std::move(stats));
>> +	debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady);
>> +	debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);
>> +
>> +	ipa_ = IPAManager::createIPA<ipa::soft::IPAProxySoft>(pipe, 0, 0);
>> +	if (!ipa_) {
>> +		LOG(SoftwareIsp, Error)
>> +			<< "Creating IPA for software ISP failed";
>> +		debayer_.reset();
>
> Is this needed, or can we let the destructor handle it ?

I think it is needed, debayer_ should be set to a consistent, i.e. default,
state before leaving the constructor.  It doesn't know when the destructor will
be called.

[...]

>> +/**
>> + * \brief Configure the SoftwareIsp object according to the passed in parameters
>> + * \param[in] inputCfg The input configuration
>> + * \param[in] outputCfgs The output configurations
>> + * \param[in] sensorControls ControlInfoMap of the controls supported by the sensor
>> + * \return 0 on success, a negative errno on failure
>> + */
>> +int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>> +			   const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
>> +			   const ControlInfoMap &sensorControls)
>> +{
>> +	ASSERT(ipa_ != nullptr && debayer_ != nullptr);
>
> Is there a specific reason to check ipa_ here ?

I don't know the answer but I'd say it doesn't harm and better to catch this
here than letting it simply crash:

>> +
>> +	int ret = ipa_->configure(sensorControls);

On Tue, Apr 02, 2024 at 09:37:10PM +0200, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> 
> > On Tue, Mar 19, 2024 at 01:35:57PM +0100, Milan Zamazal wrote:
> >> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> [...]
> 
> >> + * \var SoftwareIsp::setSensorControls
> >
> > To make components reusable, signals should be named based on the event
> > they report, not based on what the connected slot is expected to do.
> 
> The current naming is consistent with similar libcamera code in other places.
> If we want to change it, it should be done at once everywhere.

Indeed, I thought we had fixed that already.

> [...]
> 
> >> + debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f },
> >> +	  dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)
> >> +{
> >> +	if (!dmaHeap_.isValid()) {
> >> +		LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object";
> >> +		return;
> >> +	}
> >> +
> >> +	sharedParams_ = SharedMemObject<DebayerParams>("softIsp_params");
> >> +	if (!sharedParams_) {
> >> +		LOG(SoftwareIsp, Error) << "Failed to create shared memory for parameters";
> >> +		return;
> >> +	}
> >> +
> >> +	auto stats = std::make_unique<SwStatsCpu>();
> >> +	if (!stats->isValid()) {
> >> +		LOG(SoftwareIsp, Error) << "Failed to create SwStatsCpu object";
> >> +		return;
> >> +	}
> >> +	stats->statsReady.connect(this, &SoftwareIsp::statsReady);
> >> +
> >> +	debayer_ = std::make_unique<DebayerCpu>(std::move(stats));
> >> +	debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady);
> >> +	debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);
> >> +
> >> +	ipa_ = IPAManager::createIPA<ipa::soft::IPAProxySoft>(pipe, 0, 0);
> >> +	if (!ipa_) {
> >> +		LOG(SoftwareIsp, Error)
> >> +			<< "Creating IPA for software ISP failed";
> >> +		debayer_.reset();
> >
> > Is this needed, or can we let the destructor handle it ?
> 
> I think it is needed, debayer_ should be set to a consistent, i.e. default,
> state before leaving the constructor.  It doesn't know when the destructor will
> be called.

I don't mind keeping it.

> [...]
> 
> >> +/**
> >> + * \brief Configure the SoftwareIsp object according to the passed in parameters
> >> + * \param[in] inputCfg The input configuration
> >> + * \param[in] outputCfgs The output configurations
> >> + * \param[in] sensorControls ControlInfoMap of the controls supported by the sensor
> >> + * \return 0 on success, a negative errno on failure
> >> + */
> >> +int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
> >> +			   const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> >> +			   const ControlInfoMap &sensorControls)
> >> +{
> >> +	ASSERT(ipa_ != nullptr && debayer_ != nullptr);
> >
> > Is there a specific reason to check ipa_ here ?
> 
> I don't know the answer but I'd say it doesn't harm and better to catch this
> here than letting it simply crash:

If debayer_ is not null, I don't see how ipa_ could be null, so I think
we can drop that part of the check.

> >> +
> >> +	int ret = ipa_->configure(sensorControls);

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

> On Tue, Apr 02, 2024 at 09:37:10PM +0200, Milan Zamazal wrote:
>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>> 
>
>> > On Tue, Mar 19, 2024 at 01:35:57PM +0100, Milan Zamazal wrote:
>> >> From: Andrey Konovalov <andrey.konovalov@linaro.org>

[...]

>> >> +	ipa_ = IPAManager::createIPA<ipa::soft::IPAProxySoft>(pipe, 0, 0);
>> >> +	if (!ipa_) {
>> >> +		LOG(SoftwareIsp, Error)
>> >> +			<< "Creating IPA for software ISP failed";
>> >> +		debayer_.reset();
>> >
>> > Is this needed, or can we let the destructor handle it ?
>> 
>> I think it is needed, debayer_ should be set to a consistent, i.e. default,
>> state before leaving the constructor.  It doesn't know when the destructor will
>> be called.
>
> I don't mind keeping it.
>
>> [...]
>> 
>> >> +/**
>> >> + * \brief Configure the SoftwareIsp object according to the passed in parameters
>> >> + * \param[in] inputCfg The input configuration
>> >> + * \param[in] outputCfgs The output configurations
>> >> + * \param[in] sensorControls ControlInfoMap of the controls supported by the sensor
>> >> + * \return 0 on success, a negative errno on failure
>> >> + */
>> >> +int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>> >> +			   const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
>> >> +			   const ControlInfoMap &sensorControls)
>> >> +{
>> >> +	ASSERT(ipa_ != nullptr && debayer_ != nullptr);
>> >
>> > Is there a specific reason to check ipa_ here ?
>> 
>> I don't know the answer but I'd say it doesn't harm and better to catch this
>> here than letting it simply crash:
>
> If debayer_ is not null, I don't see how ipa_ could be null, 

In the current code yes.  But I cannot see this documented anywhere and it's not
something that SoftwareIsp::configure can derive itself, so it's only an
implicit assumption based on current implementation.  Such assumptions are
notoriously dangerous and if the implementation gets changed e.g. by dropping
debayer_.reset() calls or other early quit in the constructor then debayer_ can
be non-null and ipa_ null.  And it's easy to forget about updating the assertion
in configure().

> so I think we can drop that part of the check.

So I think we should keep it. :-)

>> >> +
>> >> +	int ret = ipa_->configure(sensorControls);