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

[Jacopo Mondi]

Hi Laurent,

On Sat, Aug 17, 2019 at 07:25:47PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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)

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

> > > + * \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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190819/469ccb0b/attachment-0001.sig>