[v6,01/18] libcamera: pipeline: simple: fix size adjustment in validate()
diff mbox series

Message ID 20240319123622.675599-2-mzamazal@redhat.com
State Accepted
Headers show
Series
  • libcamera: introduce Software ISP and Software IPA
Related show

Commit Message

Milan Zamazal March 19, 2024, 12:35 p.m. UTC 
From: Andrey Konovalov <andrey.konovalov@linaro.org>

SimpleCameraConfiguration::validate() adjusts the configuration of its
streams (if the size is not in the outputSizes) to the captureSize. But
the captureSize itself can be not in the outputSizes, and then the
adjusted configuration won't be valid resulting in camera configuration
failure.

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 37 ++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart March 20, 2024, 12:17 p.m. UTC | #1 
Hi Milan,

Thank you for the patch.

On Tue, Mar 19, 2024 at 01:35:48PM +0100, Milan Zamazal wrote:
> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> SimpleCameraConfiguration::validate() adjusts the configuration of its
> streams (if the size is not in the outputSizes) to the captureSize. But
> the captureSize itself can be not in the outputSizes, and then the
> adjusted configuration won't be valid resulting in camera configuration
> failure.
> 
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 37 ++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 01f2a977..04e77f7e 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -882,6 +882,30 @@ SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
>  {
>  }
>  
> +namespace {
> +
> +static Size adjustSize(const Size &requestedSize, const SizeRange &supportedSizes)
> +{
> +	ASSERT(supportedSizes.min <= supportedSizes.max);
> +
> +	if (supportedSizes.min == supportedSizes.max)
> +		return supportedSizes.max;
> +
> +	unsigned int hStep = supportedSizes.hStep;
> +	unsigned int vStep = supportedSizes.vStep;
> +
> +	if (hStep == 0)
> +		hStep = supportedSizes.max.width - supportedSizes.min.width;
> +	if (vStep == 0)
> +		vStep = supportedSizes.max.height - supportedSizes.min.height;
> +
> +	Size adjusted = requestedSize.boundedTo(supportedSizes.max).expandedTo(supportedSizes.min);
> +
> +	return adjusted.shrunkBy(supportedSizes.min).alignedDownTo(hStep, vStep).grownBy(supportedSizes.min);

Those lines are getting long. You can write

	Size adjusted = requestedSize.boundedTo(supportedSizes.max)
				     .expandedTo(supportedSizes.min);
	return adjusted.shrunkBy(supportedSizes.min)
		       .alignedDownTo(hStep, vStep)
		       .grownBy(supportedSizes.min);

or even

	return requestedSize.boundedTo(supportedSizes.max)
			    .expandedTo(supportedSizes.min)
			    .shrunkBy(supportedSizes.min)
			    .alignedDownTo(hStep, vStep)
			    .grownBy(supportedSizes.min);

I can change this when applying if you tell me which version you like
best (if any).

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +}
> +
> +} /* namespace */
> +
>  CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  {
>  	const CameraSensor *sensor = data_->sensor_.get();
> @@ -998,10 +1022,19 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  		}
>  
>  		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
> +			Size adjustedSize = pipeConfig_->captureSize;
> +			/*
> +			 * The converter (when present) may not be able to output
> +			 * a size identical to its input size. The capture size is thus
> +			 * not guaranteed to be a valid output size. In such cases, use
> +			 * the smaller valid output size closest to the requested.
> +			 */
> +			if (!pipeConfig_->outputSizes.contains(adjustedSize))
> +				adjustedSize = adjustSize(cfg.size, pipeConfig_->outputSizes);
>  			LOG(SimplePipeline, Debug)
>  				<< "Adjusting size from " << cfg.size
> -				<< " to " << pipeConfig_->captureSize;
> -			cfg.size = pipeConfig_->captureSize;
> +				<< " to " << adjustedSize;
> +			cfg.size = adjustedSize;
>  			status = Adjusted;
>  		}
>
Andrei Konovalov March 20, 2024, 12:53 p.m. UTC | #2 
Hi Laurent and Milan,

On 20.03.2024 15:17, Laurent Pinchart wrote:
> Hi Milan,
> 
> Thank you for the patch.
> 
> On Tue, Mar 19, 2024 at 01:35:48PM +0100, Milan Zamazal wrote:
>> From: Andrey Konovalov <andrey.konovalov@linaro.org>
>>
>> SimpleCameraConfiguration::validate() adjusts the configuration of its
>> streams (if the size is not in the outputSizes) to the captureSize. But
>> the captureSize itself can be not in the outputSizes, and then the
>> adjusted configuration won't be valid resulting in camera configuration
>> failure.
>>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel@ucw.cz>
>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   src/libcamera/pipeline/simple/simple.cpp | 37 ++++++++++++++++++++++--
>>   1 file changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 01f2a977..04e77f7e 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -882,6 +882,30 @@ SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
>>   {
>>   }
>>   
>> +namespace {
>> +
>> +static Size adjustSize(const Size &requestedSize, const SizeRange &supportedSizes)
>> +{
>> +	ASSERT(supportedSizes.min <= supportedSizes.max);
>> +
>> +	if (supportedSizes.min == supportedSizes.max)
>> +		return supportedSizes.max;
>> +
>> +	unsigned int hStep = supportedSizes.hStep;
>> +	unsigned int vStep = supportedSizes.vStep;
>> +
>> +	if (hStep == 0)
>> +		hStep = supportedSizes.max.width - supportedSizes.min.width;
>> +	if (vStep == 0)
>> +		vStep = supportedSizes.max.height - supportedSizes.min.height;
>> +
>> +	Size adjusted = requestedSize.boundedTo(supportedSizes.max).expandedTo(supportedSizes.min);
>> +
>> +	return adjusted.shrunkBy(supportedSizes.min).alignedDownTo(hStep, vStep).grownBy(supportedSizes.min);
> 
> Those lines are getting long. You can write
> 
> 	Size adjusted = requestedSize.boundedTo(supportedSizes.max)
> 				     .expandedTo(supportedSizes.min);
> 	return adjusted.shrunkBy(supportedSizes.min)
> 		       .alignedDownTo(hStep, vStep)
> 		       .grownBy(supportedSizes.min);
> 
> or even
> 
> 	return requestedSize.boundedTo(supportedSizes.max)
> 			    .expandedTo(supportedSizes.min)
> 			    .shrunkBy(supportedSizes.min)
> 			    .alignedDownTo(hStep, vStep)
> 			    .grownBy(supportedSizes.min);
> 
> I can change this when applying if you tell me which version you like
> best (if any).

As for me, the both versions look good.
The first version matching my taste a tiny bit better probably.

Thanks,
Andrei

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +}
>> +
>> +} /* namespace */
>> +
>>   CameraConfiguration::Status SimpleCameraConfiguration::validate()
>>   {
>>   	const CameraSensor *sensor = data_->sensor_.get();
>> @@ -998,10 +1022,19 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>>   		}
>>   
>>   		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
>> +			Size adjustedSize = pipeConfig_->captureSize;
>> +			/*
>> +			 * The converter (when present) may not be able to output
>> +			 * a size identical to its input size. The capture size is thus
>> +			 * not guaranteed to be a valid output size. In such cases, use
>> +			 * the smaller valid output size closest to the requested.
>> +			 */
>> +			if (!pipeConfig_->outputSizes.contains(adjustedSize))
>> +				adjustedSize = adjustSize(cfg.size, pipeConfig_->outputSizes);
>>   			LOG(SimplePipeline, Debug)
>>   				<< "Adjusting size from " << cfg.size
>> -				<< " to " << pipeConfig_->captureSize;
>> -			cfg.size = pipeConfig_->captureSize;
>> +				<< " to " << adjustedSize;
>> +			cfg.size = adjustedSize;
>>   			status = Adjusted;
>>   		}
>>   
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 01f2a977..04e77f7e 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -882,6 +882,30 @@  SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,
 {
 }
 
+namespace {
+
+static Size adjustSize(const Size &requestedSize, const SizeRange &supportedSizes)
+{
+	ASSERT(supportedSizes.min <= supportedSizes.max);
+
+	if (supportedSizes.min == supportedSizes.max)
+		return supportedSizes.max;
+
+	unsigned int hStep = supportedSizes.hStep;
+	unsigned int vStep = supportedSizes.vStep;
+
+	if (hStep == 0)
+		hStep = supportedSizes.max.width - supportedSizes.min.width;
+	if (vStep == 0)
+		vStep = supportedSizes.max.height - supportedSizes.min.height;
+
+	Size adjusted = requestedSize.boundedTo(supportedSizes.max).expandedTo(supportedSizes.min);
+
+	return adjusted.shrunkBy(supportedSizes.min).alignedDownTo(hStep, vStep).grownBy(supportedSizes.min);
+}
+
+} /* namespace */
+
 CameraConfiguration::Status SimpleCameraConfiguration::validate()
 {
 	const CameraSensor *sensor = data_->sensor_.get();
@@ -998,10 +1022,19 @@  CameraConfiguration::Status SimpleCameraConfiguration::validate()
 		}
 
 		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
+			Size adjustedSize = pipeConfig_->captureSize;
+			/*
+			 * The converter (when present) may not be able to output
+			 * a size identical to its input size. The capture size is thus
+			 * not guaranteed to be a valid output size. In such cases, use
+			 * the smaller valid output size closest to the requested.
+			 */
+			if (!pipeConfig_->outputSizes.contains(adjustedSize))
+				adjustedSize = adjustSize(cfg.size, pipeConfig_->outputSizes);
 			LOG(SimplePipeline, Debug)
 				<< "Adjusting size from " << cfg.size
-				<< " to " << pipeConfig_->captureSize;
-			cfg.size = pipeConfig_->captureSize;
+				<< " to " << adjustedSize;
+			cfg.size = adjustedSize;
 			status = Adjusted;
 		}