[PATCH v6 10/18] libcamera: introduce SoftwareIsp

[Milan Zamazal]

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

> On Tue, Mar 19, 2024 at 01:35:57PM +0100, Milan Zamazal wrote:
>> From: Andrey Konovalov <andrey.konovalov at 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);