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

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 05:31:04PM +0300, Andrei Konovalov wrote:
> On 23.01.2024 17:06, Laurent Pinchart wrote:
> > On Sat, Jan 13, 2024 at 03:22:01PM +0100, 📷-dev 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.
> > 
> > I've always thought the git commit message line length limit of 72
> > characters was small, let's not make it even smaller.
> 
> I was always having hard time calculating the line lengths. Will fix that.

If you use vim, it should be configured correctly by default :-)

> >> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> >> Tested-by: Pavel Machek <pavel at ucw.cz>
> >> ---
> >>   src/libcamera/pipeline/simple/simple.cpp | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> index 911051b2..4d0e7255 100644
> >> --- a/src/libcamera/pipeline/simple/simple.cpp
> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >> @@ -997,10 +997,13 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
> >>   		}
> >>   
> >>   		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
> >> +			Size adjustedSize = pipeConfig_->captureSize;
> > 
> > This deserves a comment to explain the logic.
> > 
> > 			/*
> > 			 * 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 maximum output
> > 			 * size instead.
> > 			 */
> 
> Makes sense, thanks!
> 
> > I'm however wondering, shouldn't we pick the max only when the requested
> > size is larger than outputSizes.max, and pick the min otherwise ?
> 
> Yes, this is better than always using outputSizes.max.
> 
> >> +				adjustedSize = pipeConfig_->outputSizes.max;
> >>   			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