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

[Jean-Michel Hautbois]

Hi Laurent,

On 25/05/2021 11:04, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Tue, May 25, 2021 at 11:55:28AM +0300, 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.
> 
> I see a potential fix on the list but for now let's unblock users fast
> Acked-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> Thanks
>    j
> 
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
This is indeed a blocker on master, so:
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois 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++;
>>  	}
>>  }
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>