[libcamera-devel] libcamera: V4L2Device: Return the requested Controls in getContrls()

11933 diff mbox series

Originally V4L2Device::getControls() returns all the available
controls while requested control values are acquired by
VIDIOC_G_EXT_CTRLS. V4L2Device::getControls() should rather
return the request controls only. This fixes the bug.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/libcamera/v4l2_device.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Hiro,

Thank you for the patch.

On Thu, Apr 15, 2021 at 12:07:30PM +0900, Hirokazu Honda wrote:
> Originally V4L2Device::getControls() returns all the available
> controls while requested control values are acquired by
> VIDIOC_G_EXT_CTRLS. V4L2Device::getControls() should rather
> return the request controls only. This fixes the bug.

These types of internal bugs should be caught by unit tests. If the
issue hasn't been noticed before, it means a test is missing, and our
policy is to submit the additional test (or a patch extending an
existing test) to catch the problem in the same series as the fix.

This being said, this patch actually breaks a unit test:

30/61 libcamera:v4l2_videodevice / controls                      FAIL           0.02s (exit status 255 or signal 127 SIGinvalid)

I'll thus take this as an opportunity to say that unit tests should be
run before submitting patches :-) You can simply run "ninja test". The
vivid, vim2m and vimc drivers need to be loaded.

The reason is that the controls_ parameter passed to the ControlList
constructor isn't a ControlList, but a ControlInfoMap. The code doesn't
copy controls_ in ctrls, it references the ControlInfoMap which contains
information about all supported controls by the device.

> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/libcamera/v4l2_device.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index decd19ef..d4a9bb75 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -177,7 +177,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  	if (count == 0)
>  		return {};
>  
> -	ControlList ctrls{ controls_ };
> +	ControlList ctrls;
>  
>  	/*
>  	 * Start by filling the ControlList. This can't be combined with filling

Thanks Laurent for reviewing.

On Tue, Apr 20, 2021 at 9:33 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Thu, Apr 15, 2021 at 12:07:30PM +0900, Hirokazu Honda wrote:
> > Originally V4L2Device::getControls() returns all the available
> > controls while requested control values are acquired by
> > VIDIOC_G_EXT_CTRLS. V4L2Device::getControls() should rather
> > return the request controls only. This fixes the bug.
>
> These types of internal bugs should be caught by unit tests. If the
> issue hasn't been noticed before, it means a test is missing, and our
> policy is to submit the additional test (or a patch extending an
> existing test) to catch the problem in the same series as the fix.
>
> This being said, this patch actually breaks a unit test:
>
> 30/61 libcamera:v4l2_videodevice / controls                      FAIL           0.02s (exit status 255 or signal 127 SIGinvalid)
>
> I'll thus take this as an opportunity to say that unit tests should be
> run before submitting patches :-) You can simply run "ninja test". The
> vivid, vim2m and vimc drivers need to be loaded.
>
> The reason is that the controls_ parameter passed to the ControlList
> constructor isn't a ControlList, but a ControlInfoMap. The code doesn't
> copy controls_ in ctrls, it references the ControlInfoMap which contains
> information about all supported controls by the device.
>

I read ControlList code and it doesn't generate control maps.
This patch should be abandoned.


> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/libcamera/v4l2_device.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index decd19ef..d4a9bb75 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -177,7 +177,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
> >       if (count == 0)
> >               return {};
> >
> > -     ControlList ctrls{ controls_ };
> > +     ControlList ctrls;
> >
> >       /*
> >        * Start by filling the ControlList. This can't be combined with filling
>
> --
> Regards,
>
> Laurent Pinchart