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

[Kieran Bingham]

Quoting Jacopo Mondi (2021-10-14 12:23:17)
> 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.

For chaining calls, I think aligning on the dots is better.

.shrunkBy({1,1}) looks cleaner than the size.width -1 ...

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

But it does seem like this higher level helper might end up being used.
Still, these improvements stand on their own until we figure out if we
do need something else.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

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