[2/7] libcamera: mali-c55: Enable usage of scaler
diff mbox series

Message ID 20240613155949.1041061-3-dan.scally@ideasonboard.com
State Superseded
Headers show
Series
  • Miscellaneous Mali-C55 Pipeline Fixes
Related show

Commit Message

Dan Scally June 13, 2024, 3:59 p.m. UTC 
From: Jacopo Mondi <jacopo.mondi@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@ideasonboard.com>
---
 src/libcamera/pipeline/mali-c55/mali-c55.cpp | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Comments

Kieran Bingham June 13, 2024, 7:25 p.m. UTC | #1 
Quoting Daniel Scally (2024-06-13 16:59:44)
> From: Jacopo Mondi <jacopo.mondi@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@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?

>         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
>
Jacopo Mondi June 24, 2024, 10:59 a.m. UTC | #2 
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@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@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.

> >         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
> >
Kieran Bingham June 24, 2024, 11:48 a.m. UTC | #3 
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@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@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@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
> > >

Patch
diff mbox series 

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 };
 	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);
 }