[libcamera-devel] [PATCH 4/5] libcamera: v4l2_subdevice: Add G_SELECTION ioctl support

[Laurent Pinchart]

Hi Jacopo,

On Mon, Aug 19, 2019 at 09:37:26AM +0200, Jacopo Mondi wrote:
> On Sat, Aug 17, 2019 at 07:25:47PM +0300, Laurent Pinchart wrote:
> > On Sat, Aug 17, 2019 at 07:19:43PM +0300, Laurent Pinchart wrote:
> >> On Sat, Aug 17, 2019 at 12:59:36PM +0200, Jacopo Mondi wrote:
> >>> Add a private operation to perform G_SELECTION ioctl on the v4l2
> >>> subdevice and create two public methods to retrieve the crop bound
> >>> rectangle and the subdevice native area size.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>> ---
> >>>  src/libcamera/include/v4l2_subdevice.h |  4 +++
> >>>  src/libcamera/v4l2_subdevice.cpp       | 48 ++++++++++++++++++++++++++
> >>>  2 files changed, 52 insertions(+)
> >>>
> >>> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> >>> index 9c077674f997..5316617c6873 100644
> >>> --- a/src/libcamera/include/v4l2_subdevice.h
> >>> +++ b/src/libcamera/include/v4l2_subdevice.h
> >>> @@ -43,6 +43,8 @@ public:
> >>>
> >>>  	int setCrop(unsigned int pad, Rectangle *rect);
> >>>  	int setCompose(unsigned int pad, Rectangle *rect);
> >>> +	int getCropBounds(unsigned int pad, Rectangle *rect);
> >>> +	int getNativeSize(unsigned int pad, Rectangle *rect);
> >>
> >> Should these two methods be named cropBounds() and nativeSize() ?
> >> They don't map directly to a VIDIOC_SUBDEV_G_... ioctl. And should they
> >> return a Rectangle instance instead of taking it through an argument ?
> >> Finally, should we remove the pad argument ? The native size is the
> >> native size of the sensor, not the size of a pad.
> >
> > My bad, for some reason I though I was reviewing the CameraSensor class.
> > Please ignore all comments about the getCropBounds and getNativeSize
> > methods. I wonder, however, if we shouldn't make the get/set selection
> > methods public (and remove setCrop() and setCompose()), otherwise we'll
> > keep adding accessors for all targets, and I'm not sure it's worth it.
> > If you really think dedicated accessors are best I would at least make
> > them inline and move the get methods before the set methods.
> 
> I had the same concern.. We'll keep adding wrappers and wrappers.
> However this is not too bad, the other way around would be what you
> said, but this will then require user of the subdev API to use V4L2
> defined flags (which is not a big issue, as they're already using a
> -v4l2-_subdevice instance after all)

Exactly, this class is specific to V4L2 subdevs, so I think we can use
the V4L2 API :-)

> >>>
> >>>  	ImageFormats formats(unsigned int pad);
> >>>
> >>> @@ -62,6 +64,8 @@ private:
> >>>
> >>>  	int setSelection(unsigned int pad, unsigned int target,
> >>>  			 Rectangle *rect);
> >>> +	int getSelection(unsigned int pad, unsigned int target,
> >>> +			 Rectangle *rect);
> >>
> >> I would move getSelection() above setSelection().
> 
> OK
> 
> >>>
> >>>  	const MediaEntity *entity_;
> >>>  };
> >>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> >>> index a188298de34c..5e661e2ef1fc 100644
> >>> --- a/src/libcamera/v4l2_subdevice.cpp
> >>> +++ b/src/libcamera/v4l2_subdevice.cpp
> >>> @@ -148,6 +148,28 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> >>>  	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> >>>  }
> >>>
> >>> +/**
> >>> + * \brief Get the crop bounds rectangle on one of the V4L2 subdevice pads
> >>
> >> I think this method shouldn't use V4L2 terminology, but explain in
> >> clearer terms what it retrieves. It should thus likely be renamed.
> >>
> >>> + * \param[in] pad The 0-indexed pad number the rectangle is retrived from
> >>> + * \param[out] rect The rectangle describing the crop bounds area
> >>> + * \return 0 on success or a negative error code otherwise
> >>> + */
> >>> +int V4L2Subdevice::getCropBounds(unsigned int pad, Rectangle *rect)
> >>> +{
> >>> +	return getSelection(pad, V4L2_SEL_TGT_CROP_BOUNDS, rect);
> >>> +}
> >>> +
> >>> +/**
> >>> + * \brief Get the native area size on one of the V4L2 subdevice pads
> >>
> >> If you rename the method to nativeSize(), I would write the brief as
> >> "Retrieve the ..." as we usually do.
> 
> Ignoring the renaming part, but I should probably use the usual
> "Retrieve" here.

If you turn these functions into getSelection and setSelection, I think
"get" and "set" are good for the documentation.

> >>> + * \param[in] pad The 0-indexed pad number the native area is retrieved from
> >>> + * \param[out] rect The rectangle describing the native size area
> >>> + * \return 0 on success or a negative error code otherwise
> >>> + */
> >>> +int V4L2Subdevice::getNativeSize(unsigned int pad, Rectangle *rect)
> >>
> >> Should this be renamted to pixelArraySize() and return a Size ?
> >>
> >>> +{
> >>> +	return getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, rect);
> >>> +}
> >>> +
> >>>  /**
> >>>   * \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
> >>> @@ -360,4 +382,30 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> >>>  	return 0;
> >>>  }
> >>>
> >>> +int V4L2Subdevice::getSelection(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;
> >>
> >> You can drop this as you initialise sel to 0.
> >>
> >>> +
> >>> +	int ret = ioctl(VIDIOC_SUBDEV_G_SELECTION, &sel);
> >>> +	if (ret < 0) {
> >>> +		LOG(V4L2, Error)
> >>> +			<< "Unable to get 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