[libcamera-devel,v4,1/7] libcamera: v4l2_subdevice: Expose setSelection()
diff mbox series

Message ID 20200427213236.333777-2-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: Add CameraSensorInfo
Related show

Commit Message

Jacopo Mondi April 27, 2020, 9:32 p.m. UTC 
Expose V4L2Subdevice::setSelection() method and drop
V4L2Subdevice::setCrop() and V4L2Subdevice::setComopse() as wrapping
each target with a single function does not provide any benefit.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/v4l2_subdevice.h |  7 +--
 src/libcamera/pipeline/ipu3/ipu3.cpp   |  4 +-
 src/libcamera/v4l2_subdevice.cpp       | 76 ++++++++++----------------
 3 files changed, 34 insertions(+), 53 deletions(-)

Comments

Niklas Söderlund April 27, 2020, 11:25 p.m. UTC | #1 
Hi Jacopo,

Thanks for your patch.

On 2020-04-27 23:32:30 +0200, Jacopo Mondi wrote:
> Expose V4L2Subdevice::setSelection() method and drop
> V4L2Subdevice::setCrop() and V4L2Subdevice::setComopse() as wrapping
> each target with a single function does not provide any benefit.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/include/v4l2_subdevice.h |  7 +--
>  src/libcamera/pipeline/ipu3/ipu3.cpp   |  4 +-
>  src/libcamera/v4l2_subdevice.cpp       | 76 ++++++++++----------------
>  3 files changed, 34 insertions(+), 53 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 4a04eadfb1f9..9a5c812db9b6 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -46,8 +46,8 @@ public:
>  
>  	const MediaEntity *entity() const { return entity_; }
>  
> -	int setCrop(unsigned int pad, Rectangle *rect);
> -	int setCompose(unsigned int pad, Rectangle *rect);
> +	int setSelection(unsigned int pad, unsigned int target,
> +			 Rectangle *rect);
>  
>  	ImageFormats formats(unsigned int pad);
>  
> @@ -67,9 +67,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 b45900159d06..ff33f15f9eac 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1135,11 +1135,11 @@ int ImgUDevice::configureInput(const Size &size,
>  		.width = inputFormat->size.width,
>  		.height = 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 5a479a96b795..432e89eacbd3 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -134,27 +134,42 @@ int V4L2Subdevice::open()
>   */
>  
>  /**
> - * \brief Set a crop rectangle on one of the V4L2 subdevice pads
> + * \brief Set selection rectangle \a rect for \a target
>   * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> - * \param[inout] rect The rectangle describing crop target area
> + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> + * \param[inout] rect The selection rectangle to be applied
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
> +int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> +				Rectangle *rect)
>  {
> -	return setSelection(pad, V4L2_SEL_TGT_CROP, rect);
> -}
> +	struct v4l2_subdev_selection sel = {};
>  
> -/**
> - * \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
> - * \return 0 on success or a negative error code otherwise
> - */
> -int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> -{
> -	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> -}
> +	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->width;
> +	sel.r.height = rect->height;
> +
> +	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->width = sel.r.width;
> +	rect->height = sel.r.height;
> +
> +	return 0;
> +}
>  /**
>   * \brief Enumerate all media bus codes and frame sizes on a \a pad
>   * \param[in] pad The 0-indexed pad number to enumerate formats on
> @@ -343,35 +358,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->width;
> -	sel.r.height = rect->height;
> -
> -	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->width = sel.r.width;
> -	rect->height = sel.r.height;
> -
> -	return 0;
> -}
> -
>  } /* namespace libcamera */
> -- 
> 2.26.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 28, 2020, 1:36 a.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Mon, Apr 27, 2020 at 11:32:30PM +0200, Jacopo Mondi wrote:
> Expose V4L2Subdevice::setSelection() method and drop
> V4L2Subdevice::setCrop() and V4L2Subdevice::setComopse() as wrapping
> each target with a single function does not provide any benefit.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_subdevice.h |  7 +--
>  src/libcamera/pipeline/ipu3/ipu3.cpp   |  4 +-
>  src/libcamera/v4l2_subdevice.cpp       | 76 ++++++++++----------------
>  3 files changed, 34 insertions(+), 53 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 4a04eadfb1f9..9a5c812db9b6 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -46,8 +46,8 @@ public:
>  
>  	const MediaEntity *entity() const { return entity_; }
>  
> -	int setCrop(unsigned int pad, Rectangle *rect);
> -	int setCompose(unsigned int pad, Rectangle *rect);
> +	int setSelection(unsigned int pad, unsigned int target,
> +			 Rectangle *rect);

A note on API design, not intended to be applied to this patch: with two
integer parameters for pad and target, there's a risk that the caller
would use them in the wrong order. If we defined

	enum V4L2SelectionTarget {
		V4L2SelectionTargetCrop = V4L2_SEL_TGT_CROP,
		...
	};

and turned the function into

	int setSelection(unsigned int pad, V4L2SelectionTarget target,
			 Rectangle *rect);

then the compiler would tell us if we called

	subdev->setSelection(V4L2SelectionTargetCrop, 0, &rect);

>  
>  	ImageFormats formats(unsigned int pad);
>  
> @@ -67,9 +67,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 b45900159d06..ff33f15f9eac 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1135,11 +1135,11 @@ int ImgUDevice::configureInput(const Size &size,
>  		.width = inputFormat->size.width,
>  		.height = 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 5a479a96b795..432e89eacbd3 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -134,27 +134,42 @@ int V4L2Subdevice::open()
>   */
>  
>  /**
> - * \brief Set a crop rectangle on one of the V4L2 subdevice pads
> + * \brief Set selection rectangle \a rect for \a target
>   * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> - * \param[inout] rect The rectangle describing crop target area
> + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> + * \param[inout] rect The selection rectangle to be applied

Maybe you could add

  * \todo Define a V4L2SelectionTarget enum for the selection target

here to remember the comment above.

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

(and no need to submit a new version just for this, you can fix when
pushing if you agree with the comment.

>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
> +int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> +				Rectangle *rect)
>  {
> -	return setSelection(pad, V4L2_SEL_TGT_CROP, rect);
> -}
> +	struct v4l2_subdev_selection sel = {};
>  
> -/**
> - * \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
> - * \return 0 on success or a negative error code otherwise
> - */
> -int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> -{
> -	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> -}
> +	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->width;
> +	sel.r.height = rect->height;
> +
> +	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->width = sel.r.width;
> +	rect->height = sel.r.height;
> +
> +	return 0;
> +}
>  /**
>   * \brief Enumerate all media bus codes and frame sizes on a \a pad
>   * \param[in] pad The 0-indexed pad number to enumerate formats on
> @@ -343,35 +358,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->width;
> -	sel.r.height = rect->height;
> -
> -	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->width = sel.r.width;
> -	rect->height = sel.r.height;
> -
> -	return 0;
> -}
> -
>  } /* namespace libcamera */
Jacopo Mondi April 28, 2020, 7:10 a.m. UTC | #3 
HI Laurent,

On Tue, Apr 28, 2020 at 04:36:43AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Apr 27, 2020 at 11:32:30PM +0200, Jacopo Mondi wrote:
> > Expose V4L2Subdevice::setSelection() method and drop
> > V4L2Subdevice::setCrop() and V4L2Subdevice::setComopse() as wrapping
> > each target with a single function does not provide any benefit.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_subdevice.h |  7 +--
> >  src/libcamera/pipeline/ipu3/ipu3.cpp   |  4 +-
> >  src/libcamera/v4l2_subdevice.cpp       | 76 ++++++++++----------------
> >  3 files changed, 34 insertions(+), 53 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index 4a04eadfb1f9..9a5c812db9b6 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -46,8 +46,8 @@ public:
> >
> >  	const MediaEntity *entity() const { return entity_; }
> >
> > -	int setCrop(unsigned int pad, Rectangle *rect);
> > -	int setCompose(unsigned int pad, Rectangle *rect);
> > +	int setSelection(unsigned int pad, unsigned int target,
> > +			 Rectangle *rect);
>
> A note on API design, not intended to be applied to this patch: with two
> integer parameters for pad and target, there's a risk that the caller
> would use them in the wrong order. If we defined
>
> 	enum V4L2SelectionTarget {
> 		V4L2SelectionTargetCrop = V4L2_SEL_TGT_CROP,
> 		...
> 	};
>
> and turned the function into
>
> 	int setSelection(unsigned int pad, V4L2SelectionTarget target,
> 			 Rectangle *rect);
>
> then the compiler would tell us if we called
>
> 	subdev->setSelection(V4L2SelectionTargetCrop, 0, &rect);
>
> >
> >  	ImageFormats formats(unsigned int pad);
> >
> > @@ -67,9 +67,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 b45900159d06..ff33f15f9eac 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1135,11 +1135,11 @@ int ImgUDevice::configureInput(const Size &size,
> >  		.width = inputFormat->size.width,
> >  		.height = 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 5a479a96b795..432e89eacbd3 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -134,27 +134,42 @@ int V4L2Subdevice::open()
> >   */
> >
> >  /**
> > - * \brief Set a crop rectangle on one of the V4L2 subdevice pads
> > + * \brief Set selection rectangle \a rect for \a target
> >   * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > - * \param[inout] rect The rectangle describing crop target area
> > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > + * \param[inout] rect The selection rectangle to be applied
>
> Maybe you could add
>
>   * \todo Define a V4L2SelectionTarget enum for the selection target
>
> here to remember the comment above.

We could, yes, not something I would concern, as if one calls a
function with paramters in a different order than expected, there's
not much we can do. But this is tricky as pad and target could be
valid for the kernel driver as well, so it's worth recording this

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> (and no need to submit a new version just for this, you can fix when
> pushing if you agree with the comment.

Thanks
   j

>
> >   * \return 0 on success or a negative error code otherwise
> >   */
> > -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
> > +int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > +				Rectangle *rect)
> >  {
> > -	return setSelection(pad, V4L2_SEL_TGT_CROP, rect);
> > -}
> > +	struct v4l2_subdev_selection sel = {};
> >
> > -/**
> > - * \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
> > - * \return 0 on success or a negative error code otherwise
> > - */
> > -int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> > -{
> > -	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> > -}
> > +	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->width;
> > +	sel.r.height = rect->height;
> > +
> > +	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->width = sel.r.width;
> > +	rect->height = sel.r.height;
> > +
> > +	return 0;
> > +}
> >  /**
> >   * \brief Enumerate all media bus codes and frame sizes on a \a pad
> >   * \param[in] pad The 0-indexed pad number to enumerate formats on
> > @@ -343,35 +358,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->width;
> > -	sel.r.height = rect->height;
> > -
> > -	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->width = sel.r.width;
> > -	rect->height = sel.r.height;
> > -
> > -	return 0;
> > -}
> > -
> >  } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart April 28, 2020, 10:51 a.m. UTC | #4 
Hi Jacopo,

On Tue, Apr 28, 2020 at 09:10:00AM +0200, Jacopo Mondi wrote:
> On Tue, Apr 28, 2020 at 04:36:43AM +0300, Laurent Pinchart wrote:
> > On Mon, Apr 27, 2020 at 11:32:30PM +0200, Jacopo Mondi wrote:
> > > Expose V4L2Subdevice::setSelection() method and drop
> > > V4L2Subdevice::setCrop() and V4L2Subdevice::setComopse() as wrapping
> > > each target with a single function does not provide any benefit.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/libcamera/include/v4l2_subdevice.h |  7 +--
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp   |  4 +-
> > >  src/libcamera/v4l2_subdevice.cpp       | 76 ++++++++++----------------
> > >  3 files changed, 34 insertions(+), 53 deletions(-)
> > >
> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > > index 4a04eadfb1f9..9a5c812db9b6 100644
> > > --- a/src/libcamera/include/v4l2_subdevice.h
> > > +++ b/src/libcamera/include/v4l2_subdevice.h
> > > @@ -46,8 +46,8 @@ public:
> > >
> > >  	const MediaEntity *entity() const { return entity_; }
> > >
> > > -	int setCrop(unsigned int pad, Rectangle *rect);
> > > -	int setCompose(unsigned int pad, Rectangle *rect);
> > > +	int setSelection(unsigned int pad, unsigned int target,
> > > +			 Rectangle *rect);
> >
> > A note on API design, not intended to be applied to this patch: with two
> > integer parameters for pad and target, there's a risk that the caller
> > would use them in the wrong order. If we defined
> >
> > 	enum V4L2SelectionTarget {
> > 		V4L2SelectionTargetCrop = V4L2_SEL_TGT_CROP,
> > 		...
> > 	};
> >
> > and turned the function into
> >
> > 	int setSelection(unsigned int pad, V4L2SelectionTarget target,
> > 			 Rectangle *rect);
> >
> > then the compiler would tell us if we called
> >
> > 	subdev->setSelection(V4L2SelectionTargetCrop, 0, &rect);
> >
> > >
> > >  	ImageFormats formats(unsigned int pad);
> > >
> > > @@ -67,9 +67,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 b45900159d06..ff33f15f9eac 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -1135,11 +1135,11 @@ int ImgUDevice::configureInput(const Size &size,
> > >  		.width = inputFormat->size.width,
> > >  		.height = 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 5a479a96b795..432e89eacbd3 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -134,27 +134,42 @@ int V4L2Subdevice::open()
> > >   */
> > >
> > >  /**
> > > - * \brief Set a crop rectangle on one of the V4L2 subdevice pads
> > > + * \brief Set selection rectangle \a rect for \a target
> > >   * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > > - * \param[inout] rect The rectangle describing crop target area
> > > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > > + * \param[inout] rect The selection rectangle to be applied
> >
> > Maybe you could add
> >
> >   * \todo Define a V4L2SelectionTarget enum for the selection target
> >
> > here to remember the comment above.
> 
> We could, yes, not something I would concern, as if one calls a
> function with paramters in a different order than expected, there's
> not much we can do. But this is tricky as pad and target could be
> valid for the kernel driver as well, so it's worth recording this

That's where I don't agree :-) By using an enum, we would ensure the
compiler catches this error. Not something we have to do now, but it's a
matter of API design. A great API is an API that is not possible to
misuse :-)

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > (and no need to submit a new version just for this, you can fix when
> > pushing if you agree with the comment.
> >
> > >   * \return 0 on success or a negative error code otherwise
> > >   */
> > > -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
> > > +int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > > +				Rectangle *rect)
> > >  {
> > > -	return setSelection(pad, V4L2_SEL_TGT_CROP, rect);
> > > -}
> > > +	struct v4l2_subdev_selection sel = {};
> > >
> > > -/**
> > > - * \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
> > > - * \return 0 on success or a negative error code otherwise
> > > - */
> > > -int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> > > -{
> > > -	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> > > -}
> > > +	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->width;
> > > +	sel.r.height = rect->height;
> > > +
> > > +	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->width = sel.r.width;
> > > +	rect->height = sel.r.height;
> > > +
> > > +	return 0;
> > > +}
> > >  /**
> > >   * \brief Enumerate all media bus codes and frame sizes on a \a pad
> > >   * \param[in] pad The 0-indexed pad number to enumerate formats on
> > > @@ -343,35 +358,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->width;
> > > -	sel.r.height = rect->height;
> > > -
> > > -	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->width = sel.r.width;
> > > -	rect->height = sel.r.height;
> > > -
> > > -	return 0;
> > > -}
> > > -
> > >  } /* namespace libcamera */

Patch
diff mbox series 

diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
index 4a04eadfb1f9..9a5c812db9b6 100644
--- a/src/libcamera/include/v4l2_subdevice.h
+++ b/src/libcamera/include/v4l2_subdevice.h
@@ -46,8 +46,8 @@  public:
 
 	const MediaEntity *entity() const { return entity_; }
 
-	int setCrop(unsigned int pad, Rectangle *rect);
-	int setCompose(unsigned int pad, Rectangle *rect);
+	int setSelection(unsigned int pad, unsigned int target,
+			 Rectangle *rect);
 
 	ImageFormats formats(unsigned int pad);
 
@@ -67,9 +67,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 b45900159d06..ff33f15f9eac 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1135,11 +1135,11 @@  int ImgUDevice::configureInput(const Size &size,
 		.width = inputFormat->size.width,
 		.height = 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 5a479a96b795..432e89eacbd3 100644
--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -134,27 +134,42 @@  int V4L2Subdevice::open()
  */
 
 /**
- * \brief Set a crop rectangle on one of the V4L2 subdevice pads
+ * \brief Set selection rectangle \a rect for \a target
  * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
- * \param[inout] rect The rectangle describing crop target area
+ * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
+ * \param[inout] rect The selection rectangle to be applied
  * \return 0 on success or a negative error code otherwise
  */
-int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
+int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
+				Rectangle *rect)
 {
-	return setSelection(pad, V4L2_SEL_TGT_CROP, rect);
-}
+	struct v4l2_subdev_selection sel = {};
 
-/**
- * \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
- * \return 0 on success or a negative error code otherwise
- */
-int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
-{
-	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
-}
+	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->width;
+	sel.r.height = rect->height;
+
+	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->width = sel.r.width;
+	rect->height = sel.r.height;
+
+	return 0;
+}
 /**
  * \brief Enumerate all media bus codes and frame sizes on a \a pad
  * \param[in] pad The 0-indexed pad number to enumerate formats on
@@ -343,35 +358,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->width;
-	sel.r.height = rect->height;
-
-	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->width = sel.r.width;
-	rect->height = sel.r.height;
-
-	return 0;
-}
-
 } /* namespace libcamera */