[PATCH v6 10/18] libcamera: introduce SoftwareIsp

[Laurent Pinchart]

On Tue, Apr 02, 2024 at 09:37:10PM +0200, Milan Zamazal wrote:
> 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.

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);

-- 
Regards,

Laurent Pinchart