[libcamera-devel] [PATCH 07/12] libcamera: ipu3: Register ScalerCrop control

[Jacopo Mondi]

Hi Laurent,

On Mon, Jan 11, 2021 at 01:43:03AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Jan 05, 2021 at 08:05:17PM +0100, Jacopo Mondi wrote:
> > Register the ScalerCrop control in the ImgU pipeline handler computed
> > by using the Viewfinder [1280x720] pipeline output configuration and
> > the sensor resolution as parameters.
> >
> > The ScalerCrop control limits should be updated everytime a new
> > configuration is applied to the sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++--
> >  1 file changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 418301b33a5e..f1329ffb0463 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;
> >  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;
> >  static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;
> >  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;
> > +static constexpr Size IPU3ViewfinderSize(1280, 720);
> >
> >  static const ControlInfoMap::Map IPU3Controls = {
> >  	{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },
> > @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  			 * capped to the maximum sensor resolution and aligned
> >  			 * to the ImgU output constraints.
> >  			 */
> > -			size = sensorResolution.boundedTo({ 1280, 720 })
> > +			size = sensorResolution.boundedTo(IPU3ViewfinderSize)
>
> An unrelated change, but a good one. Could you briefly mention it in the
> commit message ?
>

Why do you think it's unrelated ? As I need the Viewfinder resolution
to be used in multiple places, I just have defined it and used it here
as well.

> >  					       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,
> >  							      IMGU_OUTPUT_HEIGHT_ALIGN);
> >  			pixelFormat = formats::NV12;
> > @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data)
> >   */
> >  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >  {
> > -	const CameraSensor *sensor = data->cio2_.sensor();
> > +	CameraSensor *sensor = data->cio2_.sensor();
> >  	CameraSensorInfo sensorInfo{};
> >
> >  	int ret = sensor->sensorInfo(&sensorInfo);
> > @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >  	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> >  							defExposure);
> >
> > +	/*
> > +	 * Compute the scaler crop limits.
> > +	 *
> > +	 * \todo The scaler crop limits depend on the sensor configuration.  It
> > +	 * should be updated when a new configuration is applied.  To initialize
> > +	 * the control use the 'Viewfinder' configuration (1280x720) as the
> > +	 * pipeline output resolution and the full sensor size as input frame
> > +	 * (see the todo note in the validation function).
>
> s/the validation function/IPU3CameraConfiguration::validate()/
>
> I'm not sure what todo note you're referring too though.

The one that says we currently run with the full frame size as ImgU
input. Otherwise, if a smaller frame is used, the limits should be
updated accordingly.

        validate() :

	 * \todo The image sensor frame size should be selected to optimize
	 * operations based on the sizes of the requested streams. However such
	 * a selection makes the pipeline configuration procedure fail for small
	 * resolutions (for example: 640x480 with OV5670) and causes the capture
	 * operations to stall for some stream size combinations (see the
	 * commit message of the patch that introduced this comment for more
	 * failure examples).
	 *

>
> > +	 */
> > +
> > +	/*
> > +	 * The maximum scaler crop rectangle is the analogue crop used to
> > +	 * produce the maximum frame size.
> > +	 */
> > +	V4L2SubdeviceFormat sensorFormat;
> > +	sensorFormat.size = sensor->resolution();
> > +	ret = sensor->setFormat(&sensorFormat);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Re-fetch the sensor info updated to use the largest resolution. */
> > +	ret = sensor->sensorInfo(&sensorInfo);
> > +	if (ret)
> > +		return ret;
> > +
> > +	const Rectangle &analogueCrop = sensorInfo.analogCrop;
> > +	Rectangle maxCrop = analogueCrop;
> > +
> > +	/*
> > +	 * The minimum crop rectangle is the default viewfinder configuration
> > +	 * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel
>
> Just rescaled, or does the ImgU apply cropping along the pipeline ? If
> it does (or just if you're not sure), a \todo comment would be good.
>

I read this question as: "do we need to take into account any
additional pixels row/columns required by the ImgU for its processing" ?
Am I right ?

In this case I think it's plausible, as in validate() we apply a margin of
64 columns and 32 rows to the CIO2 output frame size to calculate the stream
size limits.

			unsigned int limit;
			limit = utils::alignDown(cio2Configuration_.size.width - 1,
						 IMGU_OUTPUT_WIDTH_MARGIN);
			cfg->size.width = std::clamp(cfg->size.width,
						     IMGU_OUTPUT_MIN_SIZE.width,
						     limit);

			limit = utils::alignDown(cio2Configuration_.size.height - 1,
						 IMGU_OUTPUT_HEIGHT_MARGIN);
			cfg->size.height = std::clamp(cfg->size.height,
						      IMGU_OUTPUT_MIN_SIZE.height,
						      limit);

			cfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN,
					      IMGU_OUTPUT_HEIGHT_ALIGN);

It might make sense to apply the same margins here by adding the same
margins to the viewfinder sizes

> > +	 * array coordinates. As the ImgU cannot up-scale, the minimum selection
> > +	 * rectangle has to be as large as the desired pipeline output size.
> > +	 *
> > +	 * The top-left corner position is not relevant as the minimum crop
> > +	 * rectangle can be placed anywhere inside the analogue crop region.
>
> I would have sworn we had documented it as being (0,0) as a convention,
> but I don't see that in the control documentation. As it's not relevant
> we can set it to anything, but I believe we should standardize the
> behaviour. What do you think would be best, (0,0), the same (x,y)
> position as the maximium value, or centering the minimum rectangle in
> the maximum rectangle ?

I guess it also depends on the platform capabilities. Some platforms
might only be capable of center-zoom (I suspect so as Android has a
property to report the zooming capabilities).

Centering it would be safer option to me. does it make sense ?

>
> The code otherwise looks fine.
>
> > +	 */
> > +	const Size &sensorOutput = sensorInfo.outputSize;
> > +	Size minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize)
> > +				       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,
> > +						      IMGU_OUTPUT_HEIGHT_ALIGN);

Also, aligning the ViewfinderSize downTo() might result in a size
smaller than the input size. It does not happen as the viewfinder
sizes are aligned already

> > +	Rectangle minCrop(minCropSize);
> > +	minCrop.scaledBy(analogueCrop.size(), sensorOutput);
> > +	minCrop.x = analogueCrop.x;
> > +	minCrop.y = analogueCrop.y;
> > +
> > +	controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
> > +
> >  	data->controlInfo_ = std::move(controls);
> >
> >  	return 0;
>
> --
> Regards,
>
> Laurent Pinchart