[libcamera-devel] [PATCH v2 15/18] libcamera: pipeline: simple: enable use of Soft ISP and Soft IPA

[Andrei Konovalov]

Hi Pavel,

On 15.01.2024 11:15, Pavel Machek wrote:
> Hi!
> 
>>>> +	/*
>>>> +	 * Create SoftwareIsp unconditionally if no converter is used
>>>> +	 * - to be revisited
>>>> +	 */
>>>> +	if (!converter_) {
>>>> +		swIsp_ = SoftwareIspFactoryBase::create(pipe, sensor_->controls());
>>>> +		if (!swIsp_) {
>>>> +			LOG(SimplePipeline, Warning)
>>>> +				<< "Failed to create software ISP, disabling software debayering";
>>>> +			swIsp_.reset();
>>>> +		} else {
>>>> +			swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);
>>>> +			swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);
>>>> +			swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);
>>>> +
>>>> +			swIsp_->getSignalSetSensorControls().connect(this, &SimpleCameraData::setSensorControls);
>>>> +		}
>>>> +	}
>>>> +
>>>
>>> I guess this needs to be revisited before the merge?
>>
>> Currently there is a build-time choice of instantiating:
>>    -Dpipelines=simple/simple -Dipas=simple/simple
>> or not instantiating:
>>    -Dpipelines=simple
>> the Soft ISP and the Soft IPA.
>>
>> Does it need to be a run-time option? How should this work from the user perspective then?
>>
>> For me the only obvious disadvantage of always creating the Soft ISP/IPA if it
>> is enabled in build configuration is that in this case the frames can be captured
>> only in RGB888 or BGR888 formats. Capturing raw bayer data isn't possible, as
>> Soft ISP/IPA always debayers all the raw bayer formats it supports.
> 
> Well, compile-time option is really bad for distros, right? I'm pretty
> sure someone out there uses bayer capture (millipixels, for example),
> and most apps will want RGB888 (cam sdl, for example; likely others).

OK, so the only concern is the ability to capture the raw frames.
This is probably fairly easy to fix.

> So long-term we'll really want to support both. Not sure it needs to
> be solved before merge, but since we had a TODO in the comment, I
> wanted to point it out.

I see. Thanks for the remainder! Let me see if I can fix that before the
next version of the patch set.

Thanks,
Andrei

> Best regards,
> 								Pavel