[PATCH v7 01/18] libcamera: pipeline: simple: fix size adjustment in validate()

[Laurent Pinchart]

Hi Milan,

On Mon, Apr 08, 2024 at 10:36:09AM +0200, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> > On Thu, Apr 04, 2024 at 10:46:38AM +0200, Milan Zamazal wrote:
> >> From: Andrey Konovalov <andrey.konovalov at 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 at linaro.org> # sc8280xp Lenovo x13s
> >> Tested-by: Pavel Machek <pavel at ucw.cz>
> >> Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
> >> Reviewed-by: Pavel Machek <pavel at ucw.cz>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> >> Signed-off-by: Hans de Goede <hdegoede at 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;
> >>  		}
> >>  

-- 
Regards,

Laurent Pinchart