[libcamera-devel] [PATCH v4 1/4] libcamera: V4L2Device: Remove the controls order assumption in updateControls()

[Jacopo Mondi]

Hi Hiro,

On Thu, Apr 22, 2021 at 11:18:06AM +0900, Hirokazu Honda wrote:
> The original updateControls() has the assumption that ctrls and
> v4l2Ctrls lists in the same order. It is dependent on the caller

s/in the same order/are in the same order/

> implementation though. This changes updateControls()
> implementation so that it works without the assumption.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/libcamera/v4l2_device.cpp | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index decd19ef..3c32d682 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -179,12 +179,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>
>  	ControlList ctrls{ controls_ };
>
> -	/*
> -	 * Start by filling the ControlList. This can't be combined with filling
> -	 * v4l2Ctrls, as updateControls() relies on both containers having the
> -	 * same order, and the control list is based on a map, which is not
> -	 * sorted by insertion order.
> -	 */
>  	for (uint32_t id : ids) {
>  		const auto iter = controls_.find(id);
>  		if (iter == controls_.end()) {
> @@ -525,19 +519,19 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  				const struct v4l2_ext_control *v4l2Ctrls,
>  				unsigned int count)
>  {
> -	unsigned int i = 0;
> -	for (auto &ctrl : *ctrls) {
> -		if (i == count)
> -			break;
> -
> -		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> -		unsigned int id = ctrl.first;
> -		ControlValue &value = ctrl.second;
> +	for (unsigned int i = 0; i < count; i++) {
> +		const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];
> +		const unsigned int id = v4l2Ctrl.id;
> +		if (!ctrls->contains(id)) {
> +			LOG(V4L2, Error) << "ControlList doesn't contain id: "
> +					 << id;
> +			return;
> +		}

Can this happen ?

>
> -		const auto iter = controls_.find(id);
> -		switch (iter->first->type()) {
> +		ControlValue value = ctrls->get(id);

This should be &value (or better *value, if you want to modify it) ? see [1]

> +		switch (value.type()) {

The type information recorded in controls_ (which is a
ControlInfoMap) and in the ControlValue should be the same, hence
there should be no functional changes, right ?

>  		case ControlTypeInteger64:
> -			value.set<int64_t>(v4l2Ctrl->value64);
> +			value.set<int64_t>(v4l2Ctrl.value64);
>  			break;
>
>  		case ControlTypeByte:
> @@ -552,11 +546,11 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  			 * \todo To be changed when support for string controls
>  			 * will be added.
>  			 */
> -			value.set<int32_t>(v4l2Ctrl->value);
> +			value.set<int32_t>(v4l2Ctrl.value);
>  			break;
>  		}
>
> -		i++;
> +		ctrls->set(id, value);

[1] so you can avoid this ?

I still think we pay a little performance loss, for the additional
crls->get() but the code is nicer to ead indeed

Can you specify you've run all tests and they're all good in the cover
letter of the next iteration for out peace of mind ? :)

Thanks
   j
>  	}
>  }
>
> --
> 2.31.1.368.gbe11c130af-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel