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

[Jacopo Mondi]

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..."

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

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
>