[PATCH v6 10/18] libcamera: introduce SoftwareIsp

[Milan Zamazal]

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

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

[...]

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