[libcamera-devel,1/5] libcamera: v4l2_subdevice: Make crop/compose rectangle a reference
diff mbox series

Message ID 20190220131757.14004-2-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: IPU3: create CIO2 and IMGU devices
Related show

Commit Message

Jacopo Mondi Feb. 20, 2019, 1:17 p.m. UTC 
As the crop/compose rectangle provided to setCrop and setCompose methods
is never modified, make it a const reference.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/v4l2_subdevice.h |  6 +++---
 src/libcamera/v4l2_subdevice.cpp       | 18 +++++++++---------
 2 files changed, 12 insertions(+), 12 deletions(-)

Comments

Niklas Söderlund Feb. 21, 2019, 3:54 p.m. UTC | #1 
Hi Jacopo,

Thanks for your patch.

On 2019-02-20 14:17:53 +0100, Jacopo Mondi wrote:
> As the crop/compose rectangle provided to setCrop and setCompose methods
> is never modified, make it a const reference.

I think this is the wrong thing to do. Instead I think you should change 
this patch to update the rectangle after VIDIOC_SUBDEV_S_SELECTION.

The kernel can update the requested selection rectangle and the caller 
should be made aware of this. Look at V4L2Subdevice::setFormat() which 
have a similar pattern but for VIDIOC_SUBDEV_S_FMT.

> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_subdevice.h |  6 +++---
>  src/libcamera/v4l2_subdevice.cpp       | 18 +++++++++---------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 40becd0ca99b..18000d6ed06b 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -44,8 +44,8 @@ public:
>  	std::string deviceNode() const { return deviceNode_; }
>  	std::string deviceName() const { return deviceName_; }
>  
> -	int setCrop(unsigned int pad, Rectangle *rect);
> -	int setCompose(unsigned int pad, Rectangle *rect);
> +	int setCrop(unsigned int pad, const Rectangle &rect);
> +	int setCompose(unsigned int pad, const Rectangle &rect);
>  
>  	int enumFormat(unsigned int pad, V4L2SubdeviceFormatEnum *formatEnum);
>  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> @@ -53,7 +53,7 @@ public:
>  
>  private:
>  	int setSelection(unsigned int pad, unsigned int target,
> -			 Rectangle *rect);
> +			 const Rectangle &rect);
>  
>  	std::string deviceNode_;
>  	std::string deviceName_;
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 4411ffa51460..2e73edee68c9 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -203,11 +203,11 @@ void V4L2Subdevice::close()
>  /**
>   * \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
> - * \param[inout] rect The rectangle describing crop target area
> + * \param[in] rect The rectangle describing crop target area
>   *
>   * \return 0 on success, or a negative error code otherwise
>   */
> -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
> +int V4L2Subdevice::setCrop(unsigned int pad, const Rectangle &rect)
>  {
>  	return setSelection(pad, V4L2_SEL_TGT_CROP, rect);
>  }
> @@ -215,11 +215,11 @@ int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
>  /**
>   * \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
> + * \param[in] 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)
> +int V4L2Subdevice::setCompose(unsigned int pad, const Rectangle &rect)
>  {
>  	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
>  }
> @@ -333,7 +333,7 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
>  }
>  
>  int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> -				Rectangle *rect)
> +				const Rectangle &rect)
>  {
>  	struct v4l2_subdev_selection sel = {};
>  
> @@ -342,10 +342,10 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
>  	sel.target = target;
>  	sel.flags = 0;
>  
> -	sel.r.left = rect->y;
> -	sel.r.top = rect->x;
> -	sel.r.width = rect->w;
> -	sel.r.height = rect->h;
> +	sel.r.left = rect.y;
> +	sel.r.top = rect.x;
> +	sel.r.width = rect.w;
> +	sel.r.height = rect.h;
>  
>  	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
>  	if (ret < 0) {
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Feb. 21, 2019, 11:31 p.m. UTC | #2 
On Thu, Feb 21, 2019 at 04:54:05PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
> 
> Thanks for your patch.
> 
> On 2019-02-20 14:17:53 +0100, Jacopo Mondi wrote:
> > As the crop/compose rectangle provided to setCrop and setCompose methods
> > is never modified, make it a const reference.
> 
> I think this is the wrong thing to do. Instead I think you should change 
> this patch to update the rectangle after VIDIOC_SUBDEV_S_SELECTION.
> 
> The kernel can update the requested selection rectangle and the caller 
> should be made aware of this. Look at V4L2Subdevice::setFormat() which 
> have a similar pattern but for VIDIOC_SUBDEV_S_FMT.

Agreed, I was about to say the same.

> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_subdevice.h |  6 +++---
> >  src/libcamera/v4l2_subdevice.cpp       | 18 +++++++++---------
> >  2 files changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index 40becd0ca99b..18000d6ed06b 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -44,8 +44,8 @@ public:
> >  	std::string deviceNode() const { return deviceNode_; }
> >  	std::string deviceName() const { return deviceName_; }
> >  
> > -	int setCrop(unsigned int pad, Rectangle *rect);
> > -	int setCompose(unsigned int pad, Rectangle *rect);
> > +	int setCrop(unsigned int pad, const Rectangle &rect);
> > +	int setCompose(unsigned int pad, const Rectangle &rect);
> >  
> >  	int enumFormat(unsigned int pad, V4L2SubdeviceFormatEnum *formatEnum);
> >  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> > @@ -53,7 +53,7 @@ public:
> >  
> >  private:
> >  	int setSelection(unsigned int pad, unsigned int target,
> > -			 Rectangle *rect);
> > +			 const Rectangle &rect);
> >  
> >  	std::string deviceNode_;
> >  	std::string deviceName_;
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 4411ffa51460..2e73edee68c9 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -203,11 +203,11 @@ void V4L2Subdevice::close()
> >  /**
> >   * \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
> > - * \param[inout] rect The rectangle describing crop target area
> > + * \param[in] rect The rectangle describing crop target area
> >   *
> >   * \return 0 on success, or a negative error code otherwise
> >   */
> > -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
> > +int V4L2Subdevice::setCrop(unsigned int pad, const Rectangle &rect)
> >  {
> >  	return setSelection(pad, V4L2_SEL_TGT_CROP, rect);
> >  }
> > @@ -215,11 +215,11 @@ int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
> >  /**
> >   * \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
> > + * \param[in] 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)
> > +int V4L2Subdevice::setCompose(unsigned int pad, const Rectangle &rect)
> >  {
> >  	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> >  }
> > @@ -333,7 +333,7 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> >  }
> >  
> >  int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > -				Rectangle *rect)
> > +				const Rectangle &rect)
> >  {
> >  	struct v4l2_subdev_selection sel = {};
> >  
> > @@ -342,10 +342,10 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> >  	sel.target = target;
> >  	sel.flags = 0;
> >  
> > -	sel.r.left = rect->y;
> > -	sel.r.top = rect->x;
> > -	sel.r.width = rect->w;
> > -	sel.r.height = rect->h;
> > +	sel.r.left = rect.y;
> > +	sel.r.top = rect.x;
> > +	sel.r.width = rect.w;
> > +	sel.r.height = rect.h;
> >  
> >  	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
> >  	if (ret < 0) {
Jacopo Mondi Feb. 22, 2019, 8:23 a.m. UTC | #3 
Hi Laurent, Niklas,

On Fri, Feb 22, 2019 at 01:31:54AM +0200, Laurent Pinchart wrote:
> On Thu, Feb 21, 2019 at 04:54:05PM +0100, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your patch.
> >
> > On 2019-02-20 14:17:53 +0100, Jacopo Mondi wrote:
> > > As the crop/compose rectangle provided to setCrop and setCompose methods
> > > is never modified, make it a const reference.
> >
> > I think this is the wrong thing to do. Instead I think you should change
> > this patch to update the rectangle after VIDIOC_SUBDEV_S_SELECTION.
> >
> > The kernel can update the requested selection rectangle and the caller
> > should be made aware of this. Look at V4L2Subdevice::setFormat() which
> > have a similar pattern but for VIDIOC_SUBDEV_S_FMT.
>
> Agreed, I was about to say the same.
>

Thanks, I overlooked this and assumed the crop/compose rectangles were
immutable. I'll make this behave like setFormat then

Thanks
   j

> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/libcamera/include/v4l2_subdevice.h |  6 +++---
> > >  src/libcamera/v4l2_subdevice.cpp       | 18 +++++++++---------
> > >  2 files changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > > index 40becd0ca99b..18000d6ed06b 100644
> > > --- a/src/libcamera/include/v4l2_subdevice.h
> > > +++ b/src/libcamera/include/v4l2_subdevice.h
> > > @@ -44,8 +44,8 @@ public:
> > >  	std::string deviceNode() const { return deviceNode_; }
> > >  	std::string deviceName() const { return deviceName_; }
> > >
> > > -	int setCrop(unsigned int pad, Rectangle *rect);
> > > -	int setCompose(unsigned int pad, Rectangle *rect);
> > > +	int setCrop(unsigned int pad, const Rectangle &rect);
> > > +	int setCompose(unsigned int pad, const Rectangle &rect);
> > >
> > >  	int enumFormat(unsigned int pad, V4L2SubdeviceFormatEnum *formatEnum);
> > >  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> > > @@ -53,7 +53,7 @@ public:
> > >
> > >  private:
> > >  	int setSelection(unsigned int pad, unsigned int target,
> > > -			 Rectangle *rect);
> > > +			 const Rectangle &rect);
> > >
> > >  	std::string deviceNode_;
> > >  	std::string deviceName_;
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 4411ffa51460..2e73edee68c9 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -203,11 +203,11 @@ void V4L2Subdevice::close()
> > >  /**
> > >   * \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
> > > - * \param[inout] rect The rectangle describing crop target area
> > > + * \param[in] rect The rectangle describing crop target area
> > >   *
> > >   * \return 0 on success, or a negative error code otherwise
> > >   */
> > > -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
> > > +int V4L2Subdevice::setCrop(unsigned int pad, const Rectangle &rect)
> > >  {
> > >  	return setSelection(pad, V4L2_SEL_TGT_CROP, rect);
> > >  }
> > > @@ -215,11 +215,11 @@ int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
> > >  /**
> > >   * \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
> > > + * \param[in] 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)
> > > +int V4L2Subdevice::setCompose(unsigned int pad, const Rectangle &rect)
> > >  {
> > >  	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> > >  }
> > > @@ -333,7 +333,7 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> > >  }
> > >
> > >  int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > > -				Rectangle *rect)
> > > +				const Rectangle &rect)
> > >  {
> > >  	struct v4l2_subdev_selection sel = {};
> > >
> > > @@ -342,10 +342,10 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > >  	sel.target = target;
> > >  	sel.flags = 0;
> > >
> > > -	sel.r.left = rect->y;
> > > -	sel.r.top = rect->x;
> > > -	sel.r.width = rect->w;
> > > -	sel.r.height = rect->h;
> > > +	sel.r.left = rect.y;
> > > +	sel.r.top = rect.x;
> > > +	sel.r.width = rect.w;
> > > +	sel.r.height = rect.h;
> > >
> > >  	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
> > >  	if (ret < 0) {
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
index 40becd0ca99b..18000d6ed06b 100644
--- a/src/libcamera/include/v4l2_subdevice.h
+++ b/src/libcamera/include/v4l2_subdevice.h
@@ -44,8 +44,8 @@  public:
 	std::string deviceNode() const { return deviceNode_; }
 	std::string deviceName() const { return deviceName_; }
 
-	int setCrop(unsigned int pad, Rectangle *rect);
-	int setCompose(unsigned int pad, Rectangle *rect);
+	int setCrop(unsigned int pad, const Rectangle &rect);
+	int setCompose(unsigned int pad, const Rectangle &rect);
 
 	int enumFormat(unsigned int pad, V4L2SubdeviceFormatEnum *formatEnum);
 	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
@@ -53,7 +53,7 @@  public:
 
 private:
 	int setSelection(unsigned int pad, unsigned int target,
-			 Rectangle *rect);
+			 const Rectangle &rect);
 
 	std::string deviceNode_;
 	std::string deviceName_;
diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
index 4411ffa51460..2e73edee68c9 100644
--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -203,11 +203,11 @@  void V4L2Subdevice::close()
 /**
  * \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
- * \param[inout] rect The rectangle describing crop target area
+ * \param[in] rect The rectangle describing crop target area
  *
  * \return 0 on success, or a negative error code otherwise
  */
-int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
+int V4L2Subdevice::setCrop(unsigned int pad, const Rectangle &rect)
 {
 	return setSelection(pad, V4L2_SEL_TGT_CROP, rect);
 }
@@ -215,11 +215,11 @@  int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect)
 /**
  * \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
+ * \param[in] 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)
+int V4L2Subdevice::setCompose(unsigned int pad, const Rectangle &rect)
 {
 	return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
 }
@@ -333,7 +333,7 @@  int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
 }
 
 int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
-				Rectangle *rect)
+				const Rectangle &rect)
 {
 	struct v4l2_subdev_selection sel = {};
 
@@ -342,10 +342,10 @@  int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
 	sel.target = target;
 	sel.flags = 0;
 
-	sel.r.left = rect->y;
-	sel.r.top = rect->x;
-	sel.r.width = rect->w;
-	sel.r.height = rect->h;
+	sel.r.left = rect.y;
+	sel.r.top = rect.x;
+	sel.r.width = rect.w;
+	sel.r.height = rect.h;
 
 	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
 	if (ret < 0) {