[PATCH 2/7] libcamera: mali-c55: Enable usage of scaler

[Kieran Bingham]

Quoting Jacopo Mondi (2024-06-24 11:59:39)
> Hi Kieran
> 
> On Thu, Jun 13, 2024 at 08:25:43PM GMT, Kieran Bingham wrote:
> > Quoting Daniel Scally (2024-06-13 16:59:44)
> > > From: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > >
> > > The Mali C55 ISP has a resizing pipeline that allows to crop and scale
> > > images.
> > >
> > > So far the mali-c55 pipeline has only supported cropping without using
> > > the scaling functionalities.
> > >
> > > Now that the kernel has gained support for the scaling operations, make
> > > the libcamera pipeline use it by combining it with a first cropping step
> > > to align the input and output images FOV ratio, and then scale to the
> > > desired output size.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > Missing SoB.
> >
> > > ---
> > >  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 18 +++++++++++++++---
> > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > index 9442d17c..c4f1afbc 100644
> > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > @@ -715,16 +715,28 @@ int PipelineHandlerMaliC55::configureProcessedStream(MaliC55CameraData *data,
> > >         if (ret)
> > >                 return ret;
> > >
> > > -       /* \todo Configure the resizer crop/compose rectangles. */
> > > -       Rectangle ispCrop = { 0, 0, config.size };
> > > +       /*
> > > +        * Compute the scaler-in to scaler-out ratio: first center-crop to align
> > > +        * the FOV to the desired resolution, then scale to the desired size.
> > > +        */
> > > +       Size scalerIn = subdevFormat.size.boundedToAspectRatio(config.size);
> > > +       int xCrop = (subdevFormat.size.width - scalerIn.width) / 2;
> > > +       int yCrop = (subdevFormat.size.height - scalerIn.height) / 2;
> > > +       Rectangle ispCrop = { xCrop, yCrop, scalerIn };
> >
> > Don't we have geometry helpers to do centering like this?
> >
> 
> Our helpers allow to center a Size in a Point:
> 
>         Rectangle centeredTo(const Point &center) const;
> 
> Here I have a size obtained by rounding down a rectangle's dimention
> to an aspect ratio, and I'm just creating a rectangle out of it,
> centered in the original one.
> 
> I could create an helper that does that, but sincerely, it's really
> two lines of code.

Aren't all the geometry helpers 'just two lines of code' ? :-)
Otherwise we could just open code all of the geometry functions...

That's the point of the helpers though I thought... To convey intent
over the action.

But you're right - I guess we don't have a:

Rectangle Size::centeredIn(const Rectangle &rect)

to allow:
	ispCrop = scalerIn.centredIn(Rectangle(subdevFormat.size));

or a 
	ispCrop = subdevFormat.size.boundedToAspectRatio(config.size)
				   .centeredIn(config.size);

Either of which would be more self documenting?

Maybe "boundedAndCenteredInAspectRatio(Size)" becomes a bit too much of
a mouthful :D


I guess the final option would be with existing code
	ispCrop = subdevFormat.size.boundedToAspectRatio(config.size)
				   .centeredTo(Rectangle(config.size).center());


But that might need extra construtors as Rectangle(Size) is marked
explict, and I think 'centeredIn(size)' (or 'centeredIn(rect)') is a better
way to convey the intents.


Anyway, it's all just syntatic sugar so SoB aside depending on who
posts:

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



> > >         ret = pipe->resizer->setSelection(0, V4L2_SEL_TGT_CROP, &ispCrop);
> > >         if (ret)
> > >                 return ret;
> > >
> > > -       ret = pipe->resizer->setSelection(0, V4L2_SEL_TGT_COMPOSE, &ispCrop);
> > > +       Rectangle ispCompose = { 0, 0, config.size };
> > > +       ret = pipe->resizer->setSelection(0, V4L2_SEL_TGT_COMPOSE, &ispCompose);
> > >         if (ret)
> > >                 return ret;
> > >
> > > +       /*
> > > +        * The source pad format size comes directly from the sink
> > > +        * compose rectangle.
> > > +        */
> > > +       subdevFormat.size = ispCompose.size();
> > >         subdevFormat.code = maliC55FmtToCode.find(config.pixelFormat)->second;
> > >         return pipe->resizer->setFormat(1, &subdevFormat);
> > >  }
> > > --
> > > 2.30.2
> > >