libcamera: v4l2_videodevice: Add getSelection() function
diff mbox series

Message ID 20240925180713.27476-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 4cb3380f4d5615f041ed5698eb1fd12b2e46a720
Headers show
Series
  • libcamera: v4l2_videodevice: Add getSelection() function
Related show

Commit Message

Laurent Pinchart Sept. 25, 2024, 6:07 p.m. UTC 
The V4L2VideoDevice class implements setSelection() but not
getSelection(). The latter is useful for instance to query crop bounds.
Implement the function.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/v4l2_videodevice.h |  1 +
 src/libcamera/v4l2_videodevice.cpp            | 32 +++++++++++++++++++
 2 files changed, 33 insertions(+)


base-commit: 8bcec687344e5cc2ccef1361c03b87f0fd2cc59b

Comments

Umang Jain Sept. 26, 2024, 4:28 a.m. UTC | #1 
Hi Laurent,

On 25/09/24 11:37 pm, Laurent Pinchart wrote:
> The V4L2VideoDevice class implements setSelection() but not
> getSelection(). The latter is useful for instance to query crop bounds.
> Implement the function.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I was already in favour of this a while ago ;-)

https://patchwork.libcamera.org/patch/20432/

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   include/libcamera/internal/v4l2_videodevice.h |  1 +
>   src/libcamera/v4l2_videodevice.cpp            | 32 +++++++++++++++++++
>   2 files changed, 33 insertions(+)
>
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 9057be08f18a..f021c2a0177b 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -208,6 +208,7 @@ public:
>   	int setFormat(V4L2DeviceFormat *format);
>   	Formats formats(uint32_t code = 0);
>   
> +	int getSelection(unsigned int target, Rectangle *rect);
>   	int setSelection(unsigned int target, Rectangle *rect);
>   
>   	int allocateBuffers(unsigned int count,
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 93cb1697935e..14eba0561d6a 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1214,6 +1214,38 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat)
>   	return sizes;
>   }
>   
> +/**
> + * \brief Get the selection rectangle for \a target
> + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> + * \param[out] rect The selection rectangle to retrieve
> + *
> + * \todo Define a V4L2SelectionTarget enum for the selection target
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2VideoDevice::getSelection(unsigned int target, Rectangle *rect)
> +{
> +	struct v4l2_selection sel = {};
> +
> +	sel.type = bufferType_;
> +	sel.target = target;
> +	sel.flags = 0;
> +
> +	int ret = ioctl(VIDIOC_G_SELECTION, &sel);
> +	if (ret < 0) {
> +		LOG(V4L2, Error) << "Unable to get rectangle " << target
> +				 << ": " << 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 Set a selection rectangle \a rect for \a target
>    * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
>
> base-commit: 8bcec687344e5cc2ccef1361c03b87f0fd2cc59b
Jacopo Mondi Sept. 26, 2024, 8:06 a.m. UTC | #2 
Hi Laurent

On Wed, Sep 25, 2024 at 09:07:13PM GMT, Laurent Pinchart wrote:
> The V4L2VideoDevice class implements setSelection() but not
> getSelection(). The latter is useful for instance to query crop bounds.
> Implement the function.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  1 +
>  src/libcamera/v4l2_videodevice.cpp            | 32 +++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 9057be08f18a..f021c2a0177b 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -208,6 +208,7 @@ public:
>  	int setFormat(V4L2DeviceFormat *format);
>  	Formats formats(uint32_t code = 0);
>
> +	int getSelection(unsigned int target, Rectangle *rect);
>  	int setSelection(unsigned int target, Rectangle *rect);
>
>  	int allocateBuffers(unsigned int count,
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 93cb1697935e..14eba0561d6a 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1214,6 +1214,38 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat)
>  	return sizes;
>  }
>
> +/**
> + * \brief Get the selection rectangle for \a target
> + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> + * \param[out] rect The selection rectangle to retrieve
> + *
> + * \todo Define a V4L2SelectionTarget enum for the selection target
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2VideoDevice::getSelection(unsigned int target, Rectangle *rect)
> +{
> +	struct v4l2_selection sel = {};
> +
> +	sel.type = bufferType_;
> +	sel.target = target;
> +	sel.flags = 0;
> +
> +	int ret = ioctl(VIDIOC_G_SELECTION, &sel);
> +	if (ret < 0) {
> +		LOG(V4L2, Error) << "Unable to get rectangle " << target
> +				 << ": " << strerror(-ret);
> +		return ret;

Just noticed in this class we don't

		ret = -errno;
		LOG(V4L2, Error) <<
				 << strerror(-ret);
		return ret;

Not related to this patch though
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> +	}
> +
> +	rect->x = sel.r.left;
> +	rect->y = sel.r.top;
> +	rect->width = sel.r.width;
> +	rect->height = sel.r.height;
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Set a selection rectangle \a rect for \a target
>   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
>
> base-commit: 8bcec687344e5cc2ccef1361c03b87f0fd2cc59b
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Sept. 26, 2024, 9:33 a.m. UTC | #3 
On Thu, Sep 26, 2024 at 10:06:14AM +0200, Jacopo Mondi wrote:
> Hi Laurent
> 
> On Wed, Sep 25, 2024 at 09:07:13PM GMT, Laurent Pinchart wrote:
> > The V4L2VideoDevice class implements setSelection() but not
> > getSelection(). The latter is useful for instance to query crop bounds.
> > Implement the function.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/internal/v4l2_videodevice.h |  1 +
> >  src/libcamera/v4l2_videodevice.cpp            | 32 +++++++++++++++++++
> >  2 files changed, 33 insertions(+)
> >
> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > index 9057be08f18a..f021c2a0177b 100644
> > --- a/include/libcamera/internal/v4l2_videodevice.h
> > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > @@ -208,6 +208,7 @@ public:
> >  	int setFormat(V4L2DeviceFormat *format);
> >  	Formats formats(uint32_t code = 0);
> >
> > +	int getSelection(unsigned int target, Rectangle *rect);
> >  	int setSelection(unsigned int target, Rectangle *rect);
> >
> >  	int allocateBuffers(unsigned int count,
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 93cb1697935e..14eba0561d6a 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1214,6 +1214,38 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat)
> >  	return sizes;
> >  }
> >
> > +/**
> > + * \brief Get the selection rectangle for \a target
> > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > + * \param[out] rect The selection rectangle to retrieve
> > + *
> > + * \todo Define a V4L2SelectionTarget enum for the selection target
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int V4L2VideoDevice::getSelection(unsigned int target, Rectangle *rect)
> > +{
> > +	struct v4l2_selection sel = {};
> > +
> > +	sel.type = bufferType_;
> > +	sel.target = target;
> > +	sel.flags = 0;
> > +
> > +	int ret = ioctl(VIDIOC_G_SELECTION, &sel);
> > +	if (ret < 0) {
> > +		LOG(V4L2, Error) << "Unable to get rectangle " << target
> > +				 << ": " << strerror(-ret);
> > +		return ret;
> 
> Just noticed in this class we don't
> 
> 		ret = -errno;
> 		LOG(V4L2, Error) <<
> 				 << strerror(-ret);
> 		return ret;

That's because we're calling V4L2Device::ioctl(), which returns -errno.

> Not related to this patch though
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> > +	}
> > +
> > +	rect->x = sel.r.left;
> > +	rect->y = sel.r.top;
> > +	rect->width = sel.r.width;
> > +	rect->height = sel.r.height;
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \brief Set a selection rectangle \a rect for \a target
> >   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> >
> > base-commit: 8bcec687344e5cc2ccef1361c03b87f0fd2cc59b
Jacopo Mondi Sept. 26, 2024, 9:39 a.m. UTC | #4 
On Thu, Sep 26, 2024 at 12:33:27PM GMT, Laurent Pinchart wrote:
> On Thu, Sep 26, 2024 at 10:06:14AM +0200, Jacopo Mondi wrote:
> > Hi Laurent
> >
> > On Wed, Sep 25, 2024 at 09:07:13PM GMT, Laurent Pinchart wrote:
> > > The V4L2VideoDevice class implements setSelection() but not
> > > getSelection(). The latter is useful for instance to query crop bounds.
> > > Implement the function.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  include/libcamera/internal/v4l2_videodevice.h |  1 +
> > >  src/libcamera/v4l2_videodevice.cpp            | 32 +++++++++++++++++++
> > >  2 files changed, 33 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > index 9057be08f18a..f021c2a0177b 100644
> > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > @@ -208,6 +208,7 @@ public:
> > >  	int setFormat(V4L2DeviceFormat *format);
> > >  	Formats formats(uint32_t code = 0);
> > >
> > > +	int getSelection(unsigned int target, Rectangle *rect);
> > >  	int setSelection(unsigned int target, Rectangle *rect);
> > >
> > >  	int allocateBuffers(unsigned int count,
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 93cb1697935e..14eba0561d6a 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -1214,6 +1214,38 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat)
> > >  	return sizes;
> > >  }
> > >
> > > +/**
> > > + * \brief Get the selection rectangle for \a target
> > > + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > > + * \param[out] rect The selection rectangle to retrieve
> > > + *
> > > + * \todo Define a V4L2SelectionTarget enum for the selection target
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int V4L2VideoDevice::getSelection(unsigned int target, Rectangle *rect)
> > > +{
> > > +	struct v4l2_selection sel = {};
> > > +
> > > +	sel.type = bufferType_;
> > > +	sel.target = target;
> > > +	sel.flags = 0;
> > > +
> > > +	int ret = ioctl(VIDIOC_G_SELECTION, &sel);
> > > +	if (ret < 0) {
> > > +		LOG(V4L2, Error) << "Unable to get rectangle " << target
> > > +				 << ": " << strerror(-ret);
> > > +		return ret;
> >
> > Just noticed in this class we don't
> >
> > 		ret = -errno;
> > 		LOG(V4L2, Error) <<
> > 				 << strerror(-ret);
> > 		return ret;
>
> That's because we're calling V4L2Device::ioctl(), which returns -errno.

I thought the whole point of the above churn was because writing to an
iostream like LOG() << does overwrites errno, but indeed in this case
errno is already returned by the ioctl() function. Sorry for the noise!

>
> > Not related to this patch though
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > > +	}
> > > +
> > > +	rect->x = sel.r.left;
> > > +	rect->y = sel.r.top;
> > > +	rect->width = sel.r.width;
> > > +	rect->height = sel.r.height;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * \brief Set a selection rectangle \a rect for \a target
> > >   * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> > >
> > > base-commit: 8bcec687344e5cc2ccef1361c03b87f0fd2cc59b
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index 9057be08f18a..f021c2a0177b 100644
--- a/include/libcamera/internal/v4l2_videodevice.h
+++ b/include/libcamera/internal/v4l2_videodevice.h
@@ -208,6 +208,7 @@  public:
 	int setFormat(V4L2DeviceFormat *format);
 	Formats formats(uint32_t code = 0);
 
+	int getSelection(unsigned int target, Rectangle *rect);
 	int setSelection(unsigned int target, Rectangle *rect);
 
 	int allocateBuffers(unsigned int count,
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 93cb1697935e..14eba0561d6a 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -1214,6 +1214,38 @@  std::vector<SizeRange> V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat)
 	return sizes;
 }
 
+/**
+ * \brief Get the selection rectangle for \a target
+ * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
+ * \param[out] rect The selection rectangle to retrieve
+ *
+ * \todo Define a V4L2SelectionTarget enum for the selection target
+ *
+ * \return 0 on success or a negative error code otherwise
+ */
+int V4L2VideoDevice::getSelection(unsigned int target, Rectangle *rect)
+{
+	struct v4l2_selection sel = {};
+
+	sel.type = bufferType_;
+	sel.target = target;
+	sel.flags = 0;
+
+	int ret = ioctl(VIDIOC_G_SELECTION, &sel);
+	if (ret < 0) {
+		LOG(V4L2, Error) << "Unable to get rectangle " << target
+				 << ": " << 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 Set a selection rectangle \a rect for \a target
  * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags