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

[Laurent Pinchart]

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

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 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.
> >
> > >   * \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