[libcamera-devel] [PATCH v3 04/13] libcamera: v4l2_subdevice: Implement get selection

[Laurent Pinchart]

Hi Jacopo,

On Sat, Apr 25, 2020 at 03:38:35PM +0200, Jacopo Mondi wrote:
> On Sat, Apr 25, 2020 at 03:42:43PM +0300, Laurent Pinchart wrote:
> > On Fri, Apr 24, 2020 at 11:52:55PM +0200, Jacopo Mondi wrote:
> > > Implement get selection for the V4L2Subdevice class to support
> > > retrieving three targets: the subdevice native size, the analog crop and
> > > the active pixel area.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/libcamera/include/v4l2_subdevice.h |  6 ++
> > >  src/libcamera/v4l2_subdevice.cpp       | 92 ++++++++++++++++++++++++++
> > >  2 files changed, 98 insertions(+)
> > >
> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > > index 4a04eadfb1f9..763ffadb61fb 100644
> > > --- a/src/libcamera/include/v4l2_subdevice.h
> > > +++ b/src/libcamera/include/v4l2_subdevice.h
> > > @@ -46,6 +46,10 @@ public:
> > >
> > >  	const MediaEntity *entity() const { return entity_; }
> > >
> > > +	int getNativeSize(unsigned int pad, Size *size);
> > > +	int getActiveArea(unsigned int pad, Rectangle *rect);
> >
> > I think we're mixing two layers here. The V4L2 API defines selection
> > targets. The fact the crop bounds or crop default targets map to the
> > full size and active area is specific to camera sensors. I would thus
> > move these two functions to the CameraSensor class. As this will require
> > accessing different selection targets, we could add a target argument to
> > getCrop(), but I think it may just be simpler to drop setCrop() and
> > setCompose() and make getSelection() and setSelection() public.
> 
> That' a thin distinction imho. My prefere here was to minimize the
> amount of V4L2 dependencies in CameraSensor, so that it will still use
> a 'getNativeSize' if we have to support an API != V4L2.

I think getNativeSize() should be exposed by CameraSensor, so that
pipeline handlers don't need to care how it's retrieved. Internally,
CameraSensor can depend on V4L2 for now, and we could have a different
implementation later if we need to support a different API. We will
already have different implementations today, even just based on V4L2,
to support camera sensors exposed through one, two or three subdevs.

> I get your point though, and as you can read from function
> documentation
> "if the sub-device represents a camera sensor then..." I agree the
> v4l2 subdev should not expose sensor-specific concepts maybe.
> 
> Although, it might require more work later to change this. I'm fine
> changin this, but be aware that it implies polluting the library with
> the V4L2_SEL_TGT_* flags.

I think that's fine, the users of the class use V4L2 concepts
explicitly, so adding V4L2_SEL_TGT_* isn't an issue in my opinion.

> > > +	int getCropRectangle(unsigned int pad, Rectangle *rect);
> >
> > As the counterpart function is called setCrop(), shouldn't this be
> > called getCrop() ?
> 
> ack
> 
> > > +
> > >  	int setCrop(unsigned int pad, Rectangle *rect);
> > >  	int setCompose(unsigned int pad, Rectangle *rect);
> > >
> > > @@ -67,6 +71,8 @@ private:
> > >  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > >  					    unsigned int code);
> > >
> > > +	int getSelection(unsigned int pad, unsigned int target,
> > > +			 Rectangle *rect);
> > >  	int setSelection(unsigned int pad, unsigned int target,
> > >  			 Rectangle *rect);
> > >
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 5a479a96b795..4dcf8ce48754 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -133,6 +133,68 @@ int V4L2Subdevice::open()
> > >   * \return The subdevice's associated media entity.
> > >   */
> > >
> > > +/**
> > > + * \brief Get the sub-device native size
> > > + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > > + * \param[out] size The sub-device native area size
> > > + *
> > > + * Retrieve the size of the sub-device native area.
> > > + * If the sub-device represent an image sensor, the native area describes
> > > + * the pixel array dimensions, including inactive and calibration pixels.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + * \retval -ENOTTY The native size information is not available
> > > + */
> > > +int V4L2Subdevice::getNativeSize(unsigned int pad, Size *size)
> > > +{
> > > +	Rectangle rect = {};
> > > +	int ret = getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, &rect);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	size->width = rect.width;
> > > +	size->height = rect.height;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \brief Get the active area rectangle
> > > + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > > + * \param[out] rect The rectangle describing the sub-device active area
> > > + *
> > > + * Retrieve the rectangle describing the sub-device active area.
> > > + * If the sub-device represent an image sensor, the active area describes
> > > + * the pixel array active portion.
> > > + *
> > > + * The returned \a rect 'x' and 'y' fields represent the displacement from the
> > > + * native size rectangle top-left corner.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + * \retval -ENOTTY The active areas size information is not available
> > > + */
> > > +int V4L2Subdevice::getActiveArea(unsigned int pad, Rectangle *rect)
> > > +{
> > > +	return getSelection(pad, V4L2_SEL_TGT_CROP_DEFAULT, rect);
> > > +}
> > > +
> > > +/**
> > > + * \brief Get the crop rectangle
> > > + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > > + * \param[out] rect The rectangle describing the cropped area rectangle
> > > + *
> > > + * Retrieve the rectangle representing the cropped area. If the sub-device
> > > + * represents an image sensor, the cropped area describes the pixel array
> > > + * portion from which the final image is produced from.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + * \retval -ENOTTY The crop rectangle size information is not available
> > > + */
> > > +int V4L2Subdevice::getCropRectangle(unsigned int pad, Rectangle *rect)
> > > +{
> > > +	return getSelection(pad, V4L2_SEL_TGT_CROP, rect);
> > > +}
> > > +
> > >  /**
> > >   * \brief Set a crop rectangle on one of the V4L2 subdevice pads
> > >   * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> > > @@ -343,6 +405,36 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > >  	return sizes;
> > >  }
> > >
> > > +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;
> > > +
> > > +	int ret = ioctl(VIDIOC_SUBDEV_G_SELECTION, &sel);
> > > +	if (ret < 0 && ret != -ENOTTY) {
> > > +		LOG(V4L2, Error)
> > > +			<< "Unable to get rectangle " << target << " on pad "
> > > +			<< pad << ": " << strerror(-ret);
> > > +		return ret;
> > > +	}
> > > +	if (ret == -ENOTTY) {
> > > +		LOG(V4L2, Debug) << "Selection API not available";
> > > +		return ret;
> > > +	}
> >
> > Do you expect valid users of this function when the subdev doesn't
> > implement the selection API ? I can imagine that a caller would check
> > for -ENOTTY and use some fallback mechanism to make this error
> > non-fatal, in which case a debug message could be better than an error
> > message, but do we have any practical use case for that ? Is it only
> > meant to cover a transition period until sensor drivers get converted ?
> 
> I see you have found that out by yourself
> 
> > > +
> > > +	rect->x = sel.r.left;
> > > +	rect->y = sel.r.top;
> > > +	rect->width = sel.r.width;
> > > +	rect->height = sel.r.height;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > >  				Rectangle *rect)
> > >  {

-- 
Regards,

Laurent Pinchart