[libcamera-devel] libcamera: pipeline: rkisp1: fix crop configuration
diff mbox series

Message ID 20201106203227.1780845-1-helen.koike@collabora.com
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: pipeline: rkisp1: fix crop configuration
Related show

Commit Message

Helen Koike Nov. 6, 2020, 8:32 p.m. UTC 
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@collabora.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 16 +++++++++++++---
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  7 ++++++-
 2 files changed, 19 insertions(+), 4 deletions(-)

Comments

Niklas Söderlund Nov. 12, 2020, 11:57 p.m. UTC | #1 
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@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@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
>
Helen Koike Nov. 13, 2020, 11:53 a.m. UTC | #2 
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@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@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
>>
>

Patch
diff mbox series 

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