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

[Niklas Söderlund]

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)// ?

For the patch itself,

Tested-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

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

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

-- 
Regards,
Niklas Söderlund