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

[Jacopo Mondi]

Hi Laurent

On Wed, Oct 13, 2021 at 11:30:45AM +0300, Laurent Pinchart wrote:
> 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.
>

Nah, we're talking details

> > 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.
>

Sure, if we'll end up having to maintain the shrunks we'll reconsider
a new helper.

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

I'll base my next frame duration series on top of these patches!

Thanks
  j

> > >  			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