[libcamera-devel] [PATCH] Revert "libcamera: V4L2Device: Remove the controls order assumption in updateControls()"

[Umang Jain]

Hi Laurent,

On 5/25/21 2:25 PM, Laurent Pinchart wrote:
> This reverts commit 34bee5e84ecba01e0ded5cacbc46c277c5a0edba.
>
> The commit introduced a breakage in the master branch, reported by
> linux-surface users already. Let's revert it while discussing the
> propert fix.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
I started facing

cam: 
../../../../../tmp/portage/media-libs/libcamera-9999/work/libcamera-9999/include/libcamera/controls.h:150: 
T libcamera::ControlValue::get() const [T = long]: Assertion `type_ == 
details::control_type<std::remove_cv_t<T>>::value' failed.
Aborted (core dumped)

today morning, under the impression it was due to my local changes for 
IPA controls on nautilus. So, applying this revert solves the issue on 
my end.

Tested-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>   src/libcamera/v4l2_device.cpp | 26 +++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 7f7e5b8fdf09..caafbc2d16bb 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -244,6 +244,12 @@ 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()) {
> @@ -617,13 +623,19 @@ void V4L2Device::updateControlInfo()
>   void V4L2Device::updateControls(ControlList *ctrls,
>   				Span<const v4l2_ext_control> v4l2Ctrls)
>   {
> -	for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {
> -		const unsigned int id = v4l2Ctrl.id;
> +	unsigned int i = 0;
> +	for (auto &ctrl : *ctrls) {
> +		if (i == v4l2Ctrls.size())
> +			break;
>   
> -		ControlValue value = ctrls->get(id);
> -		switch (value.type()) {
> +		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> +		unsigned int id = ctrl.first;
> +		ControlValue &value = ctrl.second;
> +
> +		const auto iter = controls_.find(id);
> +		switch (iter->first->type()) {
>   		case ControlTypeInteger64:
> -			value.set<int64_t>(v4l2Ctrl.value64);
> +			value.set<int64_t>(v4l2Ctrl->value64);
>   			break;
>   
>   		case ControlTypeByte:
> @@ -638,11 +650,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;
>   		}
>   
> -		ctrls->set(id, value);
> +		i++;
>   	}
>   }
>