[libcamera-devel] [PATCH v4 1/7] libcamera: v4l2_subdevice: Expose setSelection()

[Jacopo Mondi]

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 at 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 at 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