[libcamera-devel] [PATCH v2 06/11] libcamera: ipu3: Register ScalerCrop control

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jan 25, 2021 at 03:19:21PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 25, 2021 at 01:31:09PM +0200, Laurent Pinchart wrote:
> > > +	/*
> > > +	 * Re-scale in the sensor's native coordinates. Report (0,0) as
> > > +	 * top-left corner as we allow application to feely pan the crop area.
> >
> > s/feely/freely/
> >
> > > +	 */
> > > +	Rectangle minCrop(minSize);
> > > +	minCrop.scaledBy(analogueCrop.size(), sensorInfo.outputSize);
> >
> > scaledBy() is a const function that returns a new rectangle. You
> > probably want scaleBy() here (or see below for an alternative).
> >
> > > +	minCrop.x = 0;
> > > +	minCrop.y = 0;
> >
> > (x,y) are set to 0 by the constructor, so you could write
> >
> > 	Rectangle minCrop = minSize.scaledBy(analogueCrop.size(),
> > 					     sensorInfo.outputSize);
> 
> I went for the minCrop initialization as 'scaledBy' is not a Size
> class method.

Oops, indeed.

> In facts, if preferred, I could work around it with
>        Rectangle minCrop = Rectangle(minSize).scaledBy(analogueCrop.size(),
>                                               sensorInfo.outputSize);
> 
> Which I don't mind

Up to you. If you prefer keeping the existing code, just remember to
s/scaledBy/scaleBy/

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > +
> > > +	controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
> > > +
> > >  	data->controlInfo_ = std::move(controls);
> > >
> > >  	return 0;

-- 
Regards,

Laurent Pinchart