[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: fix crop configuration

[Helen Koike]

Hi Niklas,

On 11/12/20 8:57 PM, Niklas Söderlund wrote:
> Hi Helen,
> 
> Thanks for your work.
> 
> On 2020-11-06 17:32:27 -0300, Helen Koike wrote:
>> Crop rectangle was not being configured on the isp output pad nor in the
>> resizer input pad, causing an unecessary crop in the image and an
>> unecessary scaling by the resizer when streaming with a higher
>> resolution then the default 800x600.
>>
>> Example:
>>     cam -c 1 -C -s width=3280,height=2464
>>
>> In the pipeline:
>>     sensor->isp->resizer->dma_engine
>>
>> isp output crop is set to 800x600, which limits the output format to
>> 800x600, which is propagated to the resizer input format set to 800x600,
>> and the resizer output format is set to the desired end resolution
>> 3280x2464.
>>
>> Signed-off-by: Helen Koike <helen.koike at collabora.com>
>> ---
>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 16 +++++++++++++---
>>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  7 ++++++-
>>  2 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index c74a2e9b..10d44400 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -698,17 +698,27 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	LOG(RkISP1, Debug) << "ISP input pad configured with " << format.toString();
>> +	LOG(RkISP1, Debug)
>> +		<< "ISP input pad configured with " << format.toString()
>> +		<< " crop " << rect.toString();
>>  
>>  	/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */
>>  	format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
>> -	LOG(RkISP1, Debug) << "Configuring ISP output pad with " << format.toString();
>> +	LOG(RkISP1, Debug)
>> +		<< "Configuring ISP output pad with " << format.toString()
>> +		<< " crop (img stabilizer) " << rect.toString();
> 
> I understand the crop rectangle may be used to implement image 
> stabilization, but it's not yet. Is there any specific reason you 
> mention it here or would it make sens to s/(img stabilizer)// ?

There is no specific reason, just to help identifying which part of the
topology this configuration applies.
We can remove it, no problem.

> 
> For the patch itself,
> 
> Tested-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

Thanks!

> 
> I will wait for us to figure out the best wording here, but I can fix it 
> while applying once we do.

Thanks.
Helen

> 
>> +
>> +	ret = isp_->setSelection(2, V4L2_SEL_TGT_CROP, &rect);
>> +	if (ret < 0)
>> +		return ret;
>>  
>>  	ret = isp_->setFormat(2, &format);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString();
>> +	LOG(RkISP1, Debug)
>> +		<< "ISP output pad configured with " << format.toString()
>> +		<< " crop (img stabilizer) " << rect.toString();
>>  
>>  	for (const StreamConfiguration &cfg : *config) {
>>  		if (cfg.stream() == &data->mainPathStream_)
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>> index e98515c8..50c0747c 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>> @@ -117,9 +117,14 @@ int RkISP1Path::configure(const StreamConfiguration &config,
>>  	if (ret < 0)
>>  		return ret;
>>  
>> +	Rectangle rect(0, 0, ispFormat.size);
>> +	ret = resizer_->setSelection(0, V4L2_SEL_TGT_CROP, &rect);
>> +	if (ret < 0)
>> +		return ret;
>> +
>>  	LOG(RkISP1, Debug)
>>  		<< "Configured " << name_ << " resizer input pad with "
>> -		<< ispFormat.toString();
>> +		<< ispFormat.toString() << " crop " << rect.toString();
>>  
>>  	ispFormat.size = config.size;
>>  
>> -- 
>> 2.29.2
>>
>