[libcamera-devel] [PATCH v2 6/7] libcamera: v4l2_subdevice: Add selection support

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Aug 27, 2019 at 11:50:06AM +0200, Jacopo Mondi wrote:
> Currently the selection API are implemented by wrappers of the
> setSelection method. As we add support for more targets, adding new
> wrapper does not scale well. Make the setSelection operation public and
> implement getSelection, and require callers to specify the selection
> target in the operation parameters list.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/include/v4l2_subdevice.h |   9 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp   |   4 +-
>  src/libcamera/v4l2_subdevice.cpp       | 113 ++++++++++++++++---------
>  3 files changed, 78 insertions(+), 48 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 9c077674f997..b69b93280894 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -41,8 +41,10 @@ public:
>  
>  	const MediaEntity *entity() const { return entity_; }
>  
> -	int setCrop(unsigned int pad, Rectangle *rect);
> -	int setCompose(unsigned int pad, Rectangle *rect);
> +	int getSelection(unsigned int pad, unsigned int target,
> +			 Rectangle *rect);
> +	int setSelection(unsigned int pad, unsigned int target,
> +			 Rectangle *rect);
>  
>  	ImageFormats formats(unsigned int pad);
>  
> @@ -60,9 +62,6 @@ private:
>  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
>  					    unsigned int code);
>  
> -	int setSelection(unsigned int pad, unsigned int target,
> -			 Rectangle *rect);
> -
>  	const MediaEntity *entity_;
>  };
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 827906d5cd2e..bd027c7f10be 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1071,11 +1071,11 @@ int ImgUDevice::configureInput(const Size &size,
>  		.w = inputFormat->size.width,
>  		.h = inputFormat->size.height,
>  	};
> -	ret = imgu_->setCrop(PAD_INPUT, &rect);
> +	ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect);
>  	if (ret)
>  		return ret;
>  
> -	ret = imgu_->setCompose(PAD_INPUT, &rect);
> +	ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &rect);
>  	if (ret)
>  		return ret;
>  
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index a188298de34c..262dfa23ee57 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -127,25 +127,87 @@ int V4L2Subdevice::open()
>   */
>  
>  /**
> - * \brief Set a crop rectangle on one of the V4L2 subdevice pads
> - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> - * \param[inout] rect The rectangle describing crop target area
> + * \brief Get the V4L2_SEL_TGT_* selection \a target rectangle on \a pad

I would drop V4L2_SEL_TGT_* here as it's mentioned in the target
parameter.

> + * \param[in] pad The 0-indexed pad number to get the selection target from

s/target/rectangle/

> + * \param[in] target The selection target V4L2_SEL_TGT_* as defined by the V4L2
> + * selection API
> + * \param[out] rect The rectangle describing the returned selection target

s/selection/selection rectangle/ or s/\a target/rectangle/

> + *
> + * This operations wraps the VIDIOC_SUBDEV_G_SELECTION ioctl, applied to one of
> + * the V4L2_SEL_TGT_* targets defined by the V4L2 API on the subdevice \a pad.
> + * The retrieved selection rectangle is returned in the output parameter \a
> + * rect.
> + *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
> +int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> +				Rectangle *rect)
>  {
> -	return setSelection(pad, V4L2_SEL_TGT_CROP, rect);
> +	struct v4l2_subdev_selection sel = {};
> +
> +	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	sel.pad = pad;
> +	sel.target = target;
> +
> +	int ret = ioctl(VIDIOC_SUBDEV_G_SELECTION, &sel);
> +	if (ret < 0) {
> +		LOG(V4L2, Error)
> +			<< "Unable to get rectangle " << target << " on pad "

Maybe s/rectangle/selection rectangle/ ?

All these comments apply to setSelection() below.

With those small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +			<< pad << ": " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	rect->x = sel.r.left;
> +	rect->y = sel.r.top;
> +	rect->w = sel.r.width;
> +	rect->h = sel.r.height;
> +
> +	return 0;
>  }
>  
>  /**
> - * \brief Set a compose rectangle on one of the V4L2 subdevice pads
> - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> - * \param[inout] rect The rectangle describing the compose target area
> + * \brief Set the V4L2_SEL_TGT_* selection \a target rectangle on \a pad
> + * \param[in] pad The 0-indexed pad number to set the selection target on
> + * \param[in] target The selection target V4L2_SEL_TGT_* as defined by the V4L2
> + * selection API
> + * \param[inout] rect The rectangle to be applied to the the selection \a target
> + *
> + * This operations wraps the VIDIOC_SUBDEV_S_SELECTION ioctl, applied to one of
> + * the V4L2_SEL_TGT_* targets defined by the V4L2 API on the subdevice \a pad.
> + * The selection rectangle to apply is described by the parameter \a rect,
> + * which is updated to reflect what has been actually applied on the subdevice
> + * when the method returns.
> + *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> +int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> +				Rectangle *rect)
>  {
> -	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> +	struct v4l2_subdev_selection sel = {};
> +
> +	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	sel.pad = pad;
> +	sel.target = target;
> +
> +	sel.r.left = rect->x;
> +	sel.r.top = rect->y;
> +	sel.r.width = rect->w;
> +	sel.r.height = rect->h;
> +
> +	int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
> +	if (ret < 0) {
> +		LOG(V4L2, Error)
> +			<< "Unable to set rectangle " << target << " on pad "
> +			<< pad << ": " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	rect->x = sel.r.left;
> +	rect->y = sel.r.top;
> +	rect->w = sel.r.width;
> +	rect->h = sel.r.height;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -329,35 +391,4 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
>  	return sizes;
>  }
>  
> -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> -				Rectangle *rect)
> -{
> -	struct v4l2_subdev_selection sel = {};
> -
> -	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -	sel.pad = pad;
> -	sel.target = target;
> -	sel.flags = 0;
> -
> -	sel.r.left = rect->x;
> -	sel.r.top = rect->y;
> -	sel.r.width = rect->w;
> -	sel.r.height = rect->h;
> -
> -	int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
> -	if (ret < 0) {
> -		LOG(V4L2, Error)
> -			<< "Unable to set rectangle " << target << " on pad "
> -			<< pad << ": " << strerror(-ret);
> -		return ret;
> -	}
> -
> -	rect->x = sel.r.left;
> -	rect->y = sel.r.top;
> -	rect->w = sel.r.width;
> -	rect->h = sel.r.height;
> -
> -	return 0;
> -}
> -
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart