[libcamera-devel] [PATCH 07/11] libcamera: v4l2_device: Support writing array U8 controls

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 09, 2020 at 05:24:10PM +0100, Jacopo Mondi wrote:
> Add support to write array controls of type V4L2_CTRL_TYPE_U8.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/v4l2_device.cpp | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 65e97f92b01f..950e6286b84d 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -267,11 +267,25 @@ int V4L2Device::setControls(ControlList *ctrls)
>  		case ControlTypeInteger64:
>  			v4l2Ctrls[i].value64 = value.get<int64_t>();
>  			break;
> +		case ControlTypeByte: {
> +			if (!value.isArray())
> +				/*
> +				 * \todo This happens if a V4L2_CTRL_TYPE_U8
> +				 * control is set with a non-array ControlValue.
> +				 *
> +				 * Should we fail loudly here ?
> +				 */
> +				break;

Yes we should fail loudly, it's an error.

> +
> +			auto values = value.get<Span<const uint8_t>>();

			Span<const uint8_t> data = value.get<Span<const uint8_t>>();

> +			v4l2Ctrls[i].p_u8 = const_cast<uint8_t *>(values.data());

The VIDIOC_S_EXT_CTRLS call below will potentially update the value of
the control in the memory pointed by p_u8, so the const_cast here is
really not nice.

> +			v4l2Ctrls[i].size = values.size_bytes();
> +
> +			break;
> +		}
> +
>  		default:
> -			/*
> -			 * \todo To be changed when support for string and
> -			 * compound controls will be added.
> -			 */
> +			/* \todo To be changed to support strings. */
>  			v4l2Ctrls[i].value = value.get<int32_t>();
>  			break;
>  		}
> @@ -413,6 +427,16 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  		case ControlTypeInteger64:
>  			value.set<int64_t>(v4l2Ctrl->value64);
>  			break;
> +
> +		case ControlTypeByte: {
> +			std::vector<uint8_t> data = {
> +				v4l2Ctrl->p_u8, v4l2Ctrl->p_u8 + v4l2Ctrl->size
> +			};

You're creating a copy of the data, you should instead create the span
directly.

			Span<const uint8_t> data(v4l2Ctrl->p_u8, v4l2Ctrl->size);
			value.set(data);

> +			Span<const uint8_t> values(data);
> +			value.set<Span<const uint8_t>>(values);
> +			break;

But in any case, all this isn't needed as the ioctl hs already updated
the value, as explained above.

> +		}
> +
>  		default:
>  			/*
>  			 * \todo To be changed when support for string and

-- 
Regards,

Laurent Pinchart