[libcamera-devel] libcamera: v4l2_device: Fix control enumeration bug
diff mbox series

Message ID 20190626101308.19099-1-jacopo@jmondi.org
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: v4l2_device: Fix control enumeration bug
Related show

Commit Message

Jacopo Mondi June 26, 2019, 10:13 a.m. UTC 
When enumerating the available V4L2 controls at video device open
time set the V4L2_CTRL_FLAG_NEXT_CTRL flag when hitting an unsupported
control type not to loop forever.

While at it, downgrade the message reporting the control type in not
supported to Debug, as it is not an error worth being reported
unconditionally.

Fixes: 030ce6491ed3 ("libcamera: v4l2_device: List valid controls at open")
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/v4l2_device.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Kieran Bingham June 26, 2019, 10:17 a.m. UTC | #1 
Hi Jacopo,

On 26/06/2019 11:13, Jacopo Mondi wrote:
> When enumerating the available V4L2 controls at video device open
> time set the V4L2_CTRL_FLAG_NEXT_CTRL flag when hitting an unsupported
> control type not to loop forever.

"set the V4L2_CTRL_FLAG_NEXT_CTRL flag if an unsupported control type is
encountered to prevent infinite loops."


> While at it, downgrade the message reporting the control type in not

"reporting the unsupported control type to Debug"


> supported to Debug, as it is not an error worth being reported
> unconditionally.
> 
> Fixes: 030ce6491ed3 ("libcamera: v4l2_device: List valid controls at open")
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/libcamera/v4l2_device.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 84758a811c27..13d4bce5c013 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -341,8 +341,9 @@ void V4L2Device::listControls()
>  			break;
>  		/* \todo Support compound controls. */
>  		default:
> -			LOG(V4L2, Error) << "Control type '" << info.type()
> +			LOG(V4L2, Debug) << "Control type '" << info.type()
>  					 << "' not supported";
> +			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
>  			continue;
>  		}
>  
>
Laurent Pinchart June 26, 2019, 1:44 p.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Wed, Jun 26, 2019 at 11:17:23AM +0100, Kieran Bingham wrote:
> On 26/06/2019 11:13, Jacopo Mondi wrote:
> > When enumerating the available V4L2 controls at video device open

s/video device/V4L2 device/

> > time set the V4L2_CTRL_FLAG_NEXT_CTRL flag when hitting an unsupported
> > control type not to loop forever.
> 
> "set the V4L2_CTRL_FLAG_NEXT_CTRL flag if an unsupported control type is
> encountered to prevent infinite loops."
> 
> > While at it, downgrade the message reporting the control type in not
> 
> "reporting the unsupported control type to Debug"
> 
> 
> > supported to Debug, as it is not an error worth being reported
> > unconditionally.
> > 
> > Fixes: 030ce6491ed3 ("libcamera: v4l2_device: List valid controls at open")
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > ---
> >  src/libcamera/v4l2_device.cpp | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 84758a811c27..13d4bce5c013 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -341,8 +341,9 @@ void V4L2Device::listControls()
> >  			break;
> >  		/* \todo Support compound controls. */
> >  		default:
> > -			LOG(V4L2, Error) << "Control type '" << info.type()
> > +			LOG(V4L2, Debug) << "Control type '" << info.type()
> >  					 << "' not supported";
> > +			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> >  			continue;
> >  		}
> >  

Would the following (untested) change be a better solution, to avoid
or'ing V4L2_CTRL_FLAG_NEXT_CTRL in multiple places ?

diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 84758a811c27..f74c5b2aff02 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -321,13 +321,15 @@ void V4L2Device::listControls()
 	struct v4l2_query_ext_ctrl ctrl = {};

 	/* \todo Add support for menu and compound controls. */
-	ctrl.id = V4L2_CTRL_FLAG_NEXT_CTRL;
-	while (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl) == 0) {
+	while (1) {
+		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
+		int ret = ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl);
+		if (ret < 0)
+			break;
+
 		if (ctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS ||
-		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED) {
-			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
+		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
 			continue;
-		}

 		V4L2ControlInfo info(ctrl);
 		switch (info.type()) {
@@ -347,7 +349,6 @@ void V4L2Device::listControls()
 		}

 		controls_.emplace(ctrl.id, info);
-		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
 	}
 }
Jacopo Mondi June 27, 2019, 7:51 a.m. UTC | #3 
Hi Laurent,

On Wed, Jun 26, 2019 at 04:44:57PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jun 26, 2019 at 11:17:23AM +0100, Kieran Bingham wrote:
> > On 26/06/2019 11:13, Jacopo Mondi wrote:
> > > When enumerating the available V4L2 controls at video device open
>
> s/video device/V4L2 device/
>
> > > time set the V4L2_CTRL_FLAG_NEXT_CTRL flag when hitting an unsupported
> > > control type not to loop forever.
> >
> > "set the V4L2_CTRL_FLAG_NEXT_CTRL flag if an unsupported control type is
> > encountered to prevent infinite loops."
> >
> > > While at it, downgrade the message reporting the control type in not
> >
> > "reporting the unsupported control type to Debug"
> >
> >
> > > supported to Debug, as it is not an error worth being reported
> > > unconditionally.
> > >
> > > Fixes: 030ce6491ed3 ("libcamera: v4l2_device: List valid controls at open")
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > > ---
> > >  src/libcamera/v4l2_device.cpp | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > index 84758a811c27..13d4bce5c013 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -341,8 +341,9 @@ void V4L2Device::listControls()
> > >  			break;
> > >  		/* \todo Support compound controls. */
> > >  		default:
> > > -			LOG(V4L2, Error) << "Control type '" << info.type()
> > > +			LOG(V4L2, Debug) << "Control type '" << info.type()
> > >  					 << "' not supported";
> > > +			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > >  			continue;
> > >  		}
> > >
>
> Would the following (untested) change be a better solution, to avoid
> or'ing V4L2_CTRL_FLAG_NEXT_CTRL in multiple places ?

It is. I'll apply and merge. I assume I can ad your tag.

Thanks
  j

>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 84758a811c27..f74c5b2aff02 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -321,13 +321,15 @@ void V4L2Device::listControls()
>  	struct v4l2_query_ext_ctrl ctrl = {};
>
>  	/* \todo Add support for menu and compound controls. */
> -	ctrl.id = V4L2_CTRL_FLAG_NEXT_CTRL;
> -	while (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl) == 0) {
> +	while (1) {
> +		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> +		int ret = ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl);
> +		if (ret < 0)
> +			break;
> +
>  		if (ctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS ||
> -		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED) {
> -			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> +		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
>  			continue;
> -		}
>
>  		V4L2ControlInfo info(ctrl);
>  		switch (info.type()) {
> @@ -347,7 +349,6 @@ void V4L2Device::listControls()
>  		}
>
>  		controls_.emplace(ctrl.id, info);
> -		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
>  	}
>  }
>
>
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart June 27, 2019, 8:08 a.m. UTC | #4 
Hi Jacopo,

On Thu, Jun 27, 2019 at 09:51:28AM +0200, Jacopo Mondi wrote:
> On Wed, Jun 26, 2019 at 04:44:57PM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 26, 2019 at 11:17:23AM +0100, Kieran Bingham wrote:
> > > On 26/06/2019 11:13, Jacopo Mondi wrote:
> > > > When enumerating the available V4L2 controls at video device open
> >
> > s/video device/V4L2 device/
> >
> > > > time set the V4L2_CTRL_FLAG_NEXT_CTRL flag when hitting an unsupported
> > > > control type not to loop forever.
> > >
> > > "set the V4L2_CTRL_FLAG_NEXT_CTRL flag if an unsupported control type is
> > > encountered to prevent infinite loops."
> > >
> > > > While at it, downgrade the message reporting the control type in not
> > >
> > > "reporting the unsupported control type to Debug"
> > >
> > >
> > > > supported to Debug, as it is not an error worth being reported
> > > > unconditionally.
> > > >
> > > > Fixes: 030ce6491ed3 ("libcamera: v4l2_device: List valid controls at open")
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > > ---
> > > >  src/libcamera/v4l2_device.cpp | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > > index 84758a811c27..13d4bce5c013 100644
> > > > --- a/src/libcamera/v4l2_device.cpp
> > > > +++ b/src/libcamera/v4l2_device.cpp
> > > > @@ -341,8 +341,9 @@ void V4L2Device::listControls()
> > > >  			break;
> > > >  		/* \todo Support compound controls. */
> > > >  		default:
> > > > -			LOG(V4L2, Error) << "Control type '" << info.type()
> > > > +			LOG(V4L2, Debug) << "Control type '" << info.type()
> > > >  					 << "' not supported";
> > > > +			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > > >  			continue;
> > > >  		}
> > > >
> >
> > Would the following (untested) change be a better solution, to avoid
> > or'ing V4L2_CTRL_FLAG_NEXT_CTRL in multiple places ?
> 
> It is. I'll apply and merge. I assume I can ad your tag.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 84758a811c27..f74c5b2aff02 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -321,13 +321,15 @@ void V4L2Device::listControls()
> >  	struct v4l2_query_ext_ctrl ctrl = {};
> >
> >  	/* \todo Add support for menu and compound controls. */
> > -	ctrl.id = V4L2_CTRL_FLAG_NEXT_CTRL;
> > -	while (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl) == 0) {
> > +	while (1) {
> > +		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > +		int ret = ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl);
> > +		if (ret < 0)
> > +			break;
> > +
> >  		if (ctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS ||
> > -		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED) {
> > -			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > +		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> >  			continue;
> > -		}
> >
> >  		V4L2ControlInfo info(ctrl);
> >  		switch (info.type()) {
> > @@ -347,7 +349,6 @@ void V4L2Device::listControls()
> >  		}
> >
> >  		controls_.emplace(ctrl.id, info);
> > -		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> >  	}
> >  }
> >
Jacopo Mondi June 27, 2019, 8:17 a.m. UTC | #5 
Hello,

On Thu, Jun 27, 2019 at 11:08:18AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thu, Jun 27, 2019 at 09:51:28AM +0200, Jacopo Mondi wrote:
> > On Wed, Jun 26, 2019 at 04:44:57PM +0300, Laurent Pinchart wrote:
> > > On Wed, Jun 26, 2019 at 11:17:23AM +0100, Kieran Bingham wrote:
> > > > On 26/06/2019 11:13, Jacopo Mondi wrote:
> > > > > When enumerating the available V4L2 controls at video device open
> > >
> > > s/video device/V4L2 device/
> > >
> > > > > time set the V4L2_CTRL_FLAG_NEXT_CTRL flag when hitting an unsupported
> > > > > control type not to loop forever.
> > > >
> > > > "set the V4L2_CTRL_FLAG_NEXT_CTRL flag if an unsupported control type is
> > > > encountered to prevent infinite loops."
> > > >
> > > > > While at it, downgrade the message reporting the control type in not
> > > >
> > > > "reporting the unsupported control type to Debug"
> > > >
> > > >
> > > > > supported to Debug, as it is not an error worth being reported
> > > > > unconditionally.
> > > > >
> > > > > Fixes: 030ce6491ed3 ("libcamera: v4l2_device: List valid controls at open")
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > >
> > > > > ---
> > > > >  src/libcamera/v4l2_device.cpp | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > > > index 84758a811c27..13d4bce5c013 100644
> > > > > --- a/src/libcamera/v4l2_device.cpp
> > > > > +++ b/src/libcamera/v4l2_device.cpp
> > > > > @@ -341,8 +341,9 @@ void V4L2Device::listControls()
> > > > >  			break;
> > > > >  		/* \todo Support compound controls. */
> > > > >  		default:
> > > > > -			LOG(V4L2, Error) << "Control type '" << info.type()
> > > > > +			LOG(V4L2, Debug) << "Control type '" << info.type()
> > > > >  					 << "' not supported";
> > > > > +			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > > > >  			continue;
> > > > >  		}
> > > > >
> > >
> > > Would the following (untested) change be a better solution, to avoid
> > > or'ing V4L2_CTRL_FLAG_NEXT_CTRL in multiple places ?
> >
> > It is. I'll apply and merge. I assume I can ad your tag.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>

Thanks, now pushed to master

> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > index 84758a811c27..f74c5b2aff02 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -321,13 +321,15 @@ void V4L2Device::listControls()
> > >  	struct v4l2_query_ext_ctrl ctrl = {};
> > >
> > >  	/* \todo Add support for menu and compound controls. */
> > > -	ctrl.id = V4L2_CTRL_FLAG_NEXT_CTRL;
> > > -	while (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl) == 0) {
> > > +	while (1) {
> > > +		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > > +		int ret = ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl);
> > > +		if (ret < 0)
> > > +			break;
> > > +
> > >  		if (ctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS ||
> > > -		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED) {
> > > -			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > > +		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> > >  			continue;
> > > -		}
> > >
> > >  		V4L2ControlInfo info(ctrl);
> > >  		switch (info.type()) {
> > > @@ -347,7 +349,6 @@ void V4L2Device::listControls()
> > >  		}
> > >
> > >  		controls_.emplace(ctrl.id, info);
> > > -		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > >  	}
> > >  }
> > >
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 84758a811c27..13d4bce5c013 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -341,8 +341,9 @@  void V4L2Device::listControls()
 			break;
 		/* \todo Support compound controls. */
 		default:
-			LOG(V4L2, Error) << "Control type '" << info.type()
+			LOG(V4L2, Debug) << "Control type '" << info.type()
 					 << "' not supported";
+			ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;
 			continue;
 		}