[libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Use new Size grownBy() and shrunkBy() helpers

[Laurent Pinchart]

Hi Jacopo,

On Wed, Oct 13, 2021 at 08:43:08AM +0200, Jacopo Mondi wrote:
> On Wed, Oct 13, 2021 at 04:26:30AM +0300, Laurent Pinchart wrote:
> > The Size class has new helpers that can simplify the code in the IPU3
> > pipeline handler. Use them.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 92e869257e53..262b9a23703e 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -438,11 +438,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  			 * \todo Clarify the alignment constraints as explained
> >  			 * in validate()
> >  			 */
> > -			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);
> > -			size.width = utils::alignDown(size.width - 1,
> > -						      IMGU_OUTPUT_WIDTH_MARGIN);
> > -			size.height = utils::alignDown(size.height - 1,
> > -						       IMGU_OUTPUT_HEIGHT_MARGIN);
> > +			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
> > +				.shrunkBy({ 1, 1 })
> > +				.alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > +					       IMGU_OUTPUT_HEIGHT_MARGIN);
> 
> Can you align the dots ?
> 
> 			size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)
> 				               .shrunkBy({ 1, 1 })
> 				               .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> 					                      IMGU_OUTPUT_HEIGHT_MARGIN);
> 
> Same below.
> 
> Or was it intentional ?

I've copied the style from further down in this file, I don't mind it
either way.

> (I'm honest, I found the previous version more immediate compared than
> going through shrunk).

We can skip this patch if you think it doesn't bring any readability
improvement.

> Anyway, there's a repeating pattern of
> 
>                 {size.width - 1, size.height - 1}.alignDownTo(margins)
>                 {size.width + 1, size.height + 1}.alignUpTo(margins)
> 
> Which makes me wonder if what we're looking for is actually a 'Align
> to the next strictly minor/major value' function.

Good question. For IPU3 in particular, I'm still not sure what we'll
need once we complete the (mythical) rework of the ImgU configuration
:-) I haven't checked the other pipeline handlers and IPAs yet, this
series was a by-product of the review of your frame durations series
where I noticed we were missing these helpers.

> >  			pixelFormat = formats::NV12;
> >  			bufferCount = IPU3_BUFFER_COUNT;
> >  			streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
> > @@ -996,8 +995,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >  	 */
> >
> >  	/* The strictly smaller size than the sensor resolution, aligned to margins. */
> > -	Size minSize = Size(sensor->resolution().width - 1,
> > -			    sensor->resolution().height - 1)
> > +	Size minSize = sensor->resolution().shrunkBy({ 1, 1 })
> >  		       .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,
> >  				      IMGU_OUTPUT_HEIGHT_MARGIN);
> >
> > @@ -1005,8 +1003,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >  	 * Either the smallest margin-aligned size larger than the viewfinder
> >  	 * size or the adjusted sensor resolution.
> >  	 */
> > -	minSize = Size(IPU3ViewfinderSize.width + 1,
> > -		       IPU3ViewfinderSize.height + 1)
> > +	minSize = IPU3ViewfinderSize.grownBy({ 1, 1 })
> >  		  .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> >  			       IMGU_OUTPUT_HEIGHT_MARGIN)
> >  		  .boundedTo(minSize);

-- 
Regards,

Laurent Pinchart