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

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

Commit Message

Milan Zamazal April 4, 2024, 8:46 a.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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart April 7, 2024, 1:02 a.m. UTC | #1 
On Thu, Apr 04, 2024 at 10:46:38AM +0200, 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 01f2a977..d2b88795 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -882,6 +882,33 @@ 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);

Nitpicking, I think this would be more readable aligning the dots:

	Size adjusted = requestedSize.boundedTo(supportedSizes.max)
				     .expandedTo(supportedSizes.min);

	return adjusted.shrunkBy(supportedSizes.min)
		       .alignedDownTo(hStep, vStep)
		       .grownBy(supportedSizes.min);

No need to resubmit just for that.

> +}
> +
> +} /* namespace */
> +
>  CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  {
>  	const CameraSensor *sensor = data_->sensor_.get();
> @@ -998,10 +1025,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;
>  		}
>
Milan Zamazal April 8, 2024, 8:36 a.m. UTC | #2 
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> On Thu, Apr 04, 2024 at 10:46:38AM +0200, 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>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 01f2a977..d2b88795 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -882,6 +882,33 @@ 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);
>
> Nitpicking, I think this would be more readable aligning the dots:

I agree, but LSP has different opinion (seems to prefer tab alignment).
I outsource formatting to LSP, which usually matches libcamera formatting very
well, and live with its recommendations, unless it suggests something clearly
wrong.

It could look better with line breaks after requestedSize and `adjusted' but if
anybody uses a more sane tab width than 8 then it doesn't matter much anyway.

Regards,
Milan

> 	Size adjusted = requestedSize.boundedTo(supportedSizes.max)
> 				     .expandedTo(supportedSizes.min);
>
> 	return adjusted.shrunkBy(supportedSizes.min)
> 		       .alignedDownTo(hStep, vStep)
> 		       .grownBy(supportedSizes.min);
>
> No need to resubmit just for that.
>
>> +}
>> +
>> +} /* namespace */
>> +
>>  CameraConfiguration::Status SimpleCameraConfiguration::validate()
>>  {
>>  	const CameraSensor *sensor = data_->sensor_.get();
>> @@ -998,10 +1025,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;
>>  		}
>>
Laurent Pinchart April 14, 2024, 8:53 p.m. UTC | #3 
Hi Milan,

On Mon, Apr 08, 2024 at 10:36:09AM +0200, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > On Thu, Apr 04, 2024 at 10:46:38AM +0200, 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>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  src/libcamera/pipeline/simple/simple.cpp | 40 ++++++++++++++++++++++--
> >>  1 file changed, 38 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> index 01f2a977..d2b88795 100644
> >> --- a/src/libcamera/pipeline/simple/simple.cpp
> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >> @@ -882,6 +882,33 @@ 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);
> >
> > Nitpicking, I think this would be more readable aligning the dots:
> 
> I agree, but LSP has different opinion (seems to prefer tab alignment).
> I outsource formatting to LSP, which usually matches libcamera formatting very
> well, and live with its recommendations, unless it suggests something clearly
> wrong.

Our checkstyle.py uses clang-format, maybe this is something we should
file as an enhancement request. We consider the clang-format output as
informative and depart from it when needed.

> It could look better with line breaks after requestedSize and `adjusted' but if
> anybody uses a more sane tab width than 8 then it doesn't matter much anyway.

A tab is 8 spaces, anything else is insane :-)

> > 	Size adjusted = requestedSize.boundedTo(supportedSizes.max)
> > 				     .expandedTo(supportedSizes.min);
> >
> > 	return adjusted.shrunkBy(supportedSizes.min)
> > 		       .alignedDownTo(hStep, vStep)
> > 		       .grownBy(supportedSizes.min);
> >
> > No need to resubmit just for that.
> >
> >> +}
> >> +
> >> +} /* namespace */
> >> +
> >>  CameraConfiguration::Status SimpleCameraConfiguration::validate()
> >>  {
> >>  	const CameraSensor *sensor = data_->sensor_.get();
> >> @@ -998,10 +1025,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..d2b88795 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -882,6 +882,33 @@  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 +1025,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;
 		}