[libcamera-devel] [PATCH v3] libcamera: v4l2_device: Workaround faulty control menus

[Kieran Bingham]

On 24/11/2022 12:31, Jacopo Mondi wrote:
> Hi Kieran
> 
>     this seems  a better fix indeed: not registering the
> control at all if faulty. Users might be wonder why the control is not
> exposed, but the warning message should warn them.. speaking of which
> 
> 
> On Wed, Nov 23, 2022 at 01:42:32PM +0000, Kieran Bingham via libcamera-devel wrote:
>> Some UVC cameras have been identified that can provide V4L2 menu
>> controls without any menu items.
>>
>> This leads to a segfault where we try to construct a
>> ControlInfo(Span<>,default) with an empty span.
>>
>> Convert the v4l2ControlInfo and v4l2MenuControlInfo helper functions to
>> return std::optional<ControlInfo> to be able to account in the caller if
>> the control is valid, and only add acceptable controls to the supported
>> control list.
>>
>> Menu controls without a list of menu items are no longer added as a
>> valid control and a warning is logged.
>>
>> This also fixes a potential crash that would have occured in the
>> unlikely event that a ctrl.minimum was set to less than 0.
>>
>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=167
>> Reported-by: Marian Buschsieweke <marian.buschsieweke at ovgu.de>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>   include/libcamera/internal/v4l2_device.h |  4 ++--
>>   src/libcamera/v4l2_device.cpp            | 23 ++++++++++++++++++-----
>>   2 files changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
>> index 75304be11f77..50d4adbc5f2b 100644
>> --- a/include/libcamera/internal/v4l2_device.h
>> +++ b/include/libcamera/internal/v4l2_device.h
>> @@ -70,8 +70,8 @@ protected:
>>   private:
>>   	static ControlType v4l2CtrlType(uint32_t ctrlType);
>>   	static std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);
>> -	ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);
>> -	ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);
>> +	std::optional<ControlInfo> v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);
>> +	std::optional<ControlInfo> v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);
>>
>>   	void listControls();
>>   	void updateControls(ControlList *ctrls,
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index c17b323f8af0..c2e21bdf33b7 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -529,7 +529,7 @@ std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &
>>    * \param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control
>>    * \return A ControlInfo that represents \a ctrl
>>    */
>> -ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
>> +std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
>>   {
>>   	switch (ctrl.type) {
>>   	case V4L2_CTRL_TYPE_U8:
>> @@ -566,14 +566,14 @@ ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
>>    *
>>    * \return A ControlInfo that represents \a ctrl
>>    */
>> -ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
>> +std::optional<ControlInfo> V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
>>   {
>>   	std::vector<ControlValue> indices;
>>   	struct v4l2_querymenu menu = {};
>>   	menu.id = ctrl.id;
>>
>>   	if (ctrl.minimum < 0)
>> -		return ControlInfo();
>> +		return std::nullopt;
>>
>>   	for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) {
>>   		menu.index = index;
>> @@ -583,6 +583,17 @@ ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ct
>>   		indices.push_back(index);
>>   	}
>>
>> +	/*
>> +	 * Some faulty UVC drivers are known to return an empty menu control.
>> +	 * Controls without a menu option can not be set, or read, so they are
>> +	 * not exposed.
>> +	 */
>> +	if (indices.size() == 0) {
>> +		LOG(V4L2, Warning)
>> +			<< "Menu control '" << ctrl.name << "' has no entries";
> 
> I would explicitely say "Cannot register control..."
> 

As there are two error paths here that could fail to add a control, I'll 
update the higher layer as follows:


                 std::optional<ControlInfo> info = v4l2ControlInfo(ctrl);

                 if (!info) {
                         LOG(V4L2, Error)
                                 << "Control " << ctrl.name
                                 << " cannot be registered."

                         continue;
                 }

                 ctrls.emplace(controlIds_.back().get(), *info);


> Apart from that
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks

> 
> Thanks
>    j
> 
>> +		return std::nullopt;
>> +	}
>> +
>>   	return ControlInfo(indices,
>>   			   ControlValue(static_cast<int32_t>(ctrl.default_value)));
>>   }
>> @@ -631,7 +642,9 @@ void V4L2Device::listControls()
>>   		controlIdMap_[ctrl.id] = controlIds_.back().get();
>>   		controlInfo_.emplace(ctrl.id, ctrl);
>>
>> -		ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
>> +		std::optional<ControlInfo> info = v4l2ControlInfo(ctrl);
>> +		if (info)
>> +			ctrls.emplace(controlIds_.back().get(), *info);
>>   	}
>>
>>   	controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
>> @@ -670,7 +683,7 @@ void V4L2Device::updateControlInfo()
>>   			continue;
>>   		}
>>
>> -		info = v4l2ControlInfo(ctrl);
>> +		info = *v4l2ControlInfo(ctrl);
>>   	}
>>   }
>>
>> --
>> 2.34.1
>>