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

[Laurent Pinchart]

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 at 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